Tejun Heo writes:
> Hello, Mikael.
>
> Mikael Pettersson wrote:
> > There is an undocumented hardware quirk in Promise's SATA controllers,
> > where a SATA COMRESET causes the controller to signal hotplug events.
> > These unexpected interrupts confuse libata's error handler and cause
> > a sequence of failed reset attempts until EH finally gives up.
>
> Actually, this is common to many SATA controllers. Lots of them raise
> PHY event or hotplug interrupt during COMRESET and they all plug PHY
> events from ->freeze.
Hmm, no I didn't know that. It's still undocumented, but perhaps
shouldn't be called a "quirk" if it's as common as you say.
> > Although SATA hotplug status and control is per-port, it resides in
> > a single register shared by all ports. Therefore accesses to it must
> > be serialised: the controller's host->lock is used for that. The
> > interrupt handler is also adjusted so its hotplug register accesses
> > are inside the region protected by host->lock.
>
> Hmmm... This is supposed to be handled by setting ap->lock appropriately
> and ap->lock already is initialized to &host->lock, so sticking with
> ap->lock is the right thing to do.
I'll check this. The code relies on the lock being shared by all ports,
so at a minimum it will need a comment stating that requirement.
> > +static void pdc_sata_disable_hotplug(const struct ata_port *ap)
> > +{
> > + struct ata_host *host = ap->host;
> > + void __iomem *host_mmio = host->iomap[PDC_MMIO_BAR];
> > + unsigned int hotplug_offset = pdc_sata_hotplug_offset(ap);
> > + unsigned int ata_no = pdc_sata_ata_port_to_ata_no(ap);
> > + u32 hotplug_status;
> > +
> > + spin_lock(&host->lock);
> > +
> > + hotplug_status = readl(host_mmio + hotplug_offset);
> > + hotplug_status |= 0x11 << (ata_no + 16);
> > + writel(hotplug_status, host_mmio + hotplug_offset);
> > + readl(host_mmio + hotplug_offset); /* flush */
> > +
> > + spin_unlock(&host->lock);
> > +}
> > +
> > +static void pdc_sata_enable_hotplug(const struct ata_port *ap)
> > +{
> > + struct ata_host *host = ap->host;
> > + void __iomem *host_mmio = host->iomap[PDC_MMIO_BAR];
> > + unsigned int hotplug_offset = pdc_sata_hotplug_offset(ap);
> > + unsigned int ata_no = pdc_sata_ata_port_to_ata_no(ap);
> > + u32 hotplug_status;
> > +
> > + spin_lock(&host->lock);
> > +
> > + hotplug_status = readl(host_mmio + hotplug_offset);
> > + hotplug_status |= 0x11 << ata_no;
> > + hotplug_status &= ~(0x11 << (ata_no + 16));
> > + writel(hotplug_status, host_mmio + hotplug_offset);
> > + readl(host_mmio + hotplug_offset); /* flush */
> > +
> > + spin_unlock(&host->lock);
> > +}
> > +
> > static void pdc_freeze(struct ata_port *ap)
> > {
> > void __iomem *mmio = ap->ioaddr.cmd_addr;
> > @@ -643,6 +717,12 @@ static void pdc_freeze(struct ata_port *
> > readl(mmio + PDC_CTLSTAT); /* flush */
> > }
> >
> > +static void pdc_sata_freeze(struct ata_port *ap)
> > +{
> > + pdc_sata_disable_hotplug(ap);
> > + pdc_freeze(ap);
> > +}
>
> ->freeze() is called with ap->lock held, trying to lock host->lock
> inside pdc_sata_enable/disable_hotplug() will result in deadlock. Have
> you tested w/ SMP configuration or spinlock debugging turned on?
No, I'll do that and fix whatever damage occurs.
/Mikael
--
