| Message ID | BANLkTi=iNjWsaYCJqP057FkhAXtab5nS=Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, 20 May 2011 10:02:56 PDT, Jian Peng said: > --20cf307f38f6d842a904a3b81730 > 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? I should be able to do that this weekend. To clarify - should this be with the problem commit 270dac35c26433d06a89150c51e75ca0181ca7e4 applied, or reverted?
HI, Valdis, This patch is on top of reverted patch 81ca7e4. So you should not revert 81ca7e4 before applying this new one. Best regards, Jian On Fri, May 20, 2011 at 11:25 AM, <Valdis.Kletnieks@vt.edu> wrote: > > On Fri, 20 May 2011 10:02:56 PDT, Jian Peng said: > > --20cf307f38f6d842a904a3b81730 > > > 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? > > I should be able to do that this weekend. To clarify - should this be with the > problem commit 270dac35c26433d06a89150c51e75ca0181ca7e4 applied, or reverted? >
Hello, On Fri, May 20, 2011 at 10:02:56AM -0700, Jian Peng wrote: > 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. Yeah, sounds like a plan but please add ample comment explaining what's going on for which controller including link to the mailing list threads. As we're basically adding a black magic, I would still like to enable it only for the affected controllers so that we don't have to worry whether there are controllers which are affected by this problem but we don't know about. > --- 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; I think bool stat_failed = false; would be more in line with recent coding style. > 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); ata_port_printk()? Thanks.
Hi, Tejun, Defintely. I will clarify it as much as possible and meanwhile, wait for it to be tested by reporters. Thanks, Jian On Mon, May 23, 2011 at 5:13 AM, Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Fri, May 20, 2011 at 10:02:56AM -0700, Jian Peng wrote: >> 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. > > Yeah, sounds like a plan but please add ample comment explaining > what's going on for which controller including link to the mailing > list threads. As we're basically adding a black magic, I would still > like to enable it only for the affected controllers so that we don't > have to worry whether there are controllers which are affected by this > problem but we don't know about. > >> --- 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; > > I think > > bool stat_failed = false; > > would be more in line with recent coding style. > >> 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); > > ata_port_printk()? > > Thanks. > > -- > tejun >
--- 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);