Re: [PATCH 2.6.25-rc6] sata_promise: fix hardreset hotplug quirk

[Mikael Pettersson]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/