linux-kernel - Re: [PATCH v2.6.38-rc7] Revert "libata: ahci_start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 May 2011 14:13:10 +0200
From:	Tejun Heo <tj@...nel.org>
To:	Jian Peng <jipeng2005@...il.com>
Cc:	Valdis.Kletnieks@...edu, "Rafael J. Wysocki" <rjw@...k.pl>,
	Jeff Garzik <jgarzik@...ox.com>,
	Michael Leun <lkml20100708@...ton.leun.net>,
	linux-ide@...r.kernel.org,
	Linux PM mailing list <linux-pm@...ts.linux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2.6.38-rc7] Revert "libata: ahci_start_engine
 compliant to AHCI spec"

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