Re: [PATCH v2.6.38-rc7] Revert "libata: ahci_start_engine compliant to AHCI spec"

[Jian Peng <jipeng2005@xxxxxxxxx>]

Hi, Tejun/Valdis,

 

Since this is an interoperability issue of SATA host controller, the first step I want to try it to make sure the tweak that MAKE my controller WORK does not break other controllers.

You are both right that adding this majic 5ms delay at this place should not be the final solution.

 

If this magic 5ms delay works on other affected systems, I plan to post a new patch that inside ahci_start_engine(), still perform same check, and show warning message if failed, but will set a flag, then still set START bit, and if there is such failure flag, add 5ms delay.

 

Valdis, could you apply the following patch and retest it?

 

Tejun, please review it.

 

--- a/drivers/ata/libahci.c 2011-05-18 14:23:36.564665643 -0700
+++ c/drivers/ata/libahci.c 2011-05-20 09:48:06.194663506 -0700
@@ -540,6 +540,7 @@
  void __iomem *port_mmio = ahci_port_base(ap);
  u32 tmp;
  u8 status;
+ int err = 0;
 
  status = readl(port_mmio + PORT_TFDATA) & 0xFF;
 
@@ -553,12 +554,12 @@
   * specific controller will fail under this condition
   */
  if (status & (ATA_BUSY | ATA_DRQ))
-  return;
+  err = 1;
  else {
   ahci_scr_read(&ap->link, SCR_STATUS, &tmp);
 
   if ((tmp & 0xf) != 0x3)
-   return;
+   err = 1;
  }
 
  /* start DMA */
@@ -566,6 +567,13 @@
  tmp |= PORT_CMD_START;
  writel(tmp, port_mmio + PORT_CMD);
  readl(port_mmio + PORT_CMD); /* flush */
+
+ /* Some controllers need longer time to be ready */
+ if(err) {
+  printk(KERN_WARNING
+   "Controller in wrong state when setting START bit\n");
+  msleep(5);
+ } 
 }
 EXPORT_SYMBOL_GPL(ahci_start_engine);
 

 

 

On Fri, May 20, 2011 at 8:43 AM, Tejun Heo <tj@xxxxxxxxxx> wrote:

On Fri, May 20, 2011 at 11:40:20AM -0400, Valdis.Kletnieks@xxxxxx wrote:
> On Wed, 18 May 2011 17:14:56 PDT, Jian Peng said:
>
> > > @@ -1353,6 +1332,8 @@
> > >
> > >
> > >   ahci_start_engine(ap);
> > >
> > > + msleep(5);
> > > +
> > >   if (online)
> > >
> > >    *class = ahci_dev_classify(ap);
> > >
>
> It may very well be that adding a magic msleep(5) here just Makes It Work, but
> I have a gut feeling that it's in the wrong place (for starters, 'online' can't change
> during the msleep() unless somebody *else* sets it - in which case the locking
> is screwed up as we're not forcing a re-read of the value).  The msleep() probably
> needs to be before something else further down in the code (but I have no idea
> exactly what).

At this point, I think it would be better to simply add a flag and
enable the check for the affected controller.

Thanks.

--
tejun

_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm