[RFC PATCH #upstream-fixes] libata: don't use 32bit PIO for small transfers

[RFC PATCH #upstream-fixes] libata: don't use 32bit PIO for small transfers

[Tejun Heo]

Some configurations have problems with using 32bit PIO on ATAPI cdb
transfers.  The most likely cause is different division of payload
across FISes but hasn't been confirmed.  There isn't much to be gained
by using 32bit PIO for small transfers.  Play it safe and use 16bit
PIO for transfers smaller than 512 bytes.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-and-Tested-by: Lei Ming <tom.leiming@gmail.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=40592
Cc: Alan Cox <alan@linux.intel.com>
Cc: stable@kernel.org
---
 drivers/ata/libata-sff.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

This fixes the problem but I'm slightly uneasy about it as I don't
know what's going on for sure.  However, given that we've been using
16bit PIO for far longer time and haven't seen much problem, this
seems like a sensible thing to do.  Alan, Jeff, what do you guys
think?

Thanks.

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index c24127d..60e3bac 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -617,7 +617,15 @@ unsigned int ata_sff_data_xfer32(struct ata_device *dev, unsigned char *buf,
 	unsigned int words = buflen >> 2;
 	int slop = buflen & 3;
 
-	if (!(ap->pflags & ATA_PFLAG_PIO32))
+	/*
+	 * Some configurations have problem with 32bit PIO on ATAPI cdb
+	 * transfers and there isn't much to be gained by using 32bit PIO
+	 * for small transfers anyway.  Fall back to 16bit if 32bit isn't
+	 * enabled or transfer size is smaller than ATA_SECT_SIZE.
+	 *
+	 * https://bugzilla.kernel.org/show_bug.cgi?id=40592
+	 */
+	if (!(ap->pflags & ATA_PFLAG_PIO32) || buflen < ATA_SECT_SIZE)
 		return ata_sff_data_xfer(dev, buf, buflen, rw);
 
 	/* Transfer multiple of 4 bytes */

Re: [RFC PATCH #upstream-fixes] libata: don't use 32bit PIO for small transfers

[Alan Cox]

On Tue, 6 Sep 2011 13:21:01 +0900
Tejun Heo <tj@kernel.org> wrote:

> Some configurations have problems with using 32bit PIO on ATAPI cdb
> transfers.  The most likely cause is different division of payload
> across FISes but hasn't been confirmed.  There isn't much to be gained
> by using 32bit PIO for small transfers.  Play it safe and use 16bit
> PIO for transfers smaller than 512 bytes.

This seems to me to be the wrong approach as it hurts lots of other
commands unnecessarily and may cause regressions on other setups
because their behaviour is unnecessarily changed and performance
reduced.

The problem report is specific to Sandybridge in IDE mode, which is a
recent controller so it should be chased up with the Intel folks who
submitted the device ID as an apparent hardware problem - that way if
it is a chipset problem (and I have no idea if it is) it might get a
proper workaround, or fixed in future.

Secondly it's a SATA specific problem.

Thirdly we've been doing 32bit PIO on old IDE since it was invented.

This seems an extreme reaction for a report of a single specific
problem on a single specific device and controller.

So NACK.

Add a piix_sata_data_xfer32_maybe() function to the ata_piix driver
specifically for this chipset.


I've added Seth who submitted the relevant PCI identifers to the email.
I'm a bit at a loss to understand why none of the previous discussion
or patch was Cc'd in his direction ?

Seth - the bug report is
https://bugzilla.kernel.org/show_bug.cgi?id=40592

and I guess the underlying question is "Are there any known 32bit PIO
errata that would explain this ?"

Before the changes go in it might also be worth testing CD burning and
audio ripping etc to see if the problem is one dependent on transfer
size or the fact the transfer is the cdb on a SATA transfer, so that
any fixing is done right.

Alan

Re: [RFC PATCH #upstream-fixes] libata: don't use 32bit PIO for small transfers

[Alan Cox]

On Tue, 6 Sep 2011 13:21:01 +0900
Tejun Heo <tj@kernel.org> wrote:

> https://bugzilla.kernel.org/show_bug.cgi?id=40592

Bit more digging on this. Dug out other reports - the common link is
Sandybridge/Cougar Point chipsets & ATAPI in IDE mode.

Alan

Re: [RFC PATCH #upstream-fixes] libata: don't use 32bit PIO for small transfers

[Ming Lei]

Hi Tejun and Alan,

Thanks for your spending time on the bug.

On Tue, Sep 6, 2011 at 5:49 PM, Alan Cox <alan@linux.intel.com> wrote:

>
> So NACK.
>
> Add a piix_sata_data_xfer32_maybe() function to the ata_piix driver
> specifically for this chipset.

Could you accept the quirk patch below to make these controllers working
at least now?

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 43107e9..eb7ea56 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -341,11 +341,12 @@ static struct ata_port_operations piix_sata_ops = {
 };

 static struct ata_port_operations piix_pata_ops = {
-	.inherits		= &piix_sata_ops,
+	.inherits		= &ata_bmdma32_port_ops,
 	.cable_detect		= ata_cable_40wire,
 	.set_piomode		= piix_set_piomode,
 	.set_dmamode		= piix_set_dmamode,
 	.prereset		= piix_pata_prereset,
+	.sff_irq_check		= piix_irq_check,
 };

 static struct ata_port_operations piix_vmw_ops = {
@@ -1585,6 +1586,15 @@ static int __devinit piix_init_one(struct pci_dev *pdev,
 				"on poweroff and hibernation\n");
 	}

+	/*
+	 * Sandybridge chipset H61/P67/H67 have broken 32 mode up to now
+	 * see https://bugzilla.kernel.org/show_bug.cgi?id=40592
+	 */
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device == 0x1c00)
+		piix_sata_ops.inherits = &ata_bmdma_port_ops;
+	else
+		piix_sata_ops.inherits = &ata_bmdma32_port_ops;
+
 	port_info[0] = piix_port_info[ent->driver_data];
 	port_info[1] = piix_port_info[ent->driver_data];



thanks,
-- 
Ming Lei

Re: [RFC PATCH #upstream-fixes] libata: don't use 32bit PIO for small transfers

[Alan Cox]

On Wed, 14 Sep 2011 15:44:43 +0800
Ming Lei <tom.leiming@gmail.com> wrote:

> Hi Tejun and Alan,
> 
> Thanks for your spending time on the bug.
> 
> On Tue, Sep 6, 2011 at 5:49 PM, Alan Cox <alan@linux.intel.com> wrote:
> 
> >
> > So NACK.
> >
> > Add a piix_sata_data_xfer32_maybe() function to the ata_piix driver
> > specifically for this chipset.
> 
> Could you accept the quirk patch below to make these controllers
> working at least now?

To me that looks like the right fix. It affects the minimum amount of
hardware. It uses the standard operations in full and it documents
clearly what it is doing.

Alan