linux-kernel - RE: questions regarding possible violation of AHCI spec in AHCI driver

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <E18F441196CA634DB8E1F1C56A50A874319FA38CD3@IRVEXCHCCR01.corp.ad.broadcom.com>
Date:	Wed, 8 Dec 2010 12:09:32 -0800
From:	"Jian Peng" <jipeng@...adcom.com>
To:	"Tejun Heo" <tj@...nel.org>
cc:	"Robert Hancock" <hancockrwd@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"jgarzik@...ox.com" <jgarzik@...ox.com>,
	ide <linux-ide@...r.kernel.org>
Subject: RE: questions regarding possible violation of AHCI spec in AHCI
 driver

The controller may take much longer time to recover in this case, and leads to wrong HW state after stop_engine() inside ahci_hardreset() and cause device type checking failure due to unfinished HW state change and missing D2H FIS after start_engine() again inside ahci_hardreset(). I guess this is the reason why AHCI spec try to emphasize.

Yes, without this change, Broadcom controller will fail due to above reason.

Thanks,
Jian

-----Original Message-----
From: Tejun Heo [mailto:tj@...nel.org] 
Sent: Wednesday, December 08, 2010 11:55 AM
To: Jian Peng
Cc: Robert Hancock; linux-kernel@...r.kernel.org; jgarzik@...ox.com; ide
Subject: Re: questions regarding possible violation of AHCI spec in AHCI driver

Hello,

On 12/08/2010 08:49 PM, Jian Peng wrote:
> I agree. I have AHCI based PCI card using HBA from Marvell, Via and
> Silicon Image, and am going to test my patch.  Before this patch can
> be applied universally, I like to use it for specific PCI_VENDOR_ID
> first. Here is my new patch to limit it to Broadcom's AHCI core

Hmmm... is the change actually necessary for broadcom controllers?  As
I wrote before, any ahci controller should just work without the above
checks because,

> +	/* avoid race condition per spec (end of section 10.1.2) */
> +	if (pdev->vendor == PCI_VENDOR_ID_BROADCOM) {
> +		u8 status = readl(port_mmio + PORT_TFDATA) & 0xFF;
> +
> +		if (status & (ATA_BUSY | ATA_DRQ) ||
> +		    ahci_scr_read(&ap->link, SCR_STATUS, &tmp) ||
> +		    (tmp & 0x0f) != 0x03)
> +			return;

PHY event can occur here which causes the device to send D2H Reg FIS
w/ BSY set.

1. So, the controller _MUST NOT_ fail in irrecoverable way even if the
   driver sets ST while BSY is set.

2. The driver guarantees the final ST setting before entering normal
   operation is done when the prerequisites are met.

If you combine 1 and 2, the current behavior is perfectly fine.  Do
broadcom controllers actually fail without the above change?

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/