[PATCH #upstream-fixes 1/2] libata: don't check whether to use DMA or not for no data commands

[PATCH #upstream-fixes 1/2] libata: don't check whether to use DMA or not for no data commands

[Tejun Heo]

There's no reason to check whether to use DMA or not for no data
commands.  Don't do it.  While at it, make local variable using_pio in
atapi_xlat() set iff ATAPI_PROT_PIO is going to be used and rename
ata_check_atapi_dma() to atapi_check_dma() for consistency.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
It's late in the -rc cycle but these two patches are to work around
specific hardware and low risk.  Thanks.

 drivers/ata/libata-core.c |    4 ++--
 drivers/ata/libata-scsi.c |   16 +++++++---------
 drivers/ata/libata.h      |    2 +-
 3 files changed, 10 insertions(+), 12 deletions(-)

Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -2343,8 +2343,8 @@ static unsigned int atapi_xlat(struct at
 {
 	struct scsi_cmnd *scmd = qc->scsicmd;
 	struct ata_device *dev = qc->dev;
-	int using_pio = (dev->flags & ATA_DFLAG_PIO);
 	int nodata = (scmd->sc_data_direction == DMA_NONE);
+	int using_pio = !nodata && (dev->flags & ATA_DFLAG_PIO);
 	unsigned int nbytes;
 
 	memset(qc->cdb, 0, dev->cdb_len);
@@ -2362,7 +2362,7 @@ static unsigned int atapi_xlat(struct at
 	ata_qc_set_pc_nbytes(qc);
 
 	/* check whether ATAPI DMA is safe */
-	if (!using_pio && ata_check_atapi_dma(qc))
+	if (!nodata && !using_pio && atapi_check_dma(qc))
 		using_pio = 1;
 
 	/* Some controller variants snoop this value for Packet
@@ -2402,13 +2402,11 @@ static unsigned int atapi_xlat(struct at
 	qc->tf.lbam = (nbytes & 0xFF);
 	qc->tf.lbah = (nbytes >> 8);
 
-	if (using_pio || nodata) {
-		/* no data, or PIO data xfer */
-		if (nodata)
-			qc->tf.protocol = ATAPI_PROT_NODATA;
-		else
-			qc->tf.protocol = ATAPI_PROT_PIO;
-	} else {
+	if (nodata)
+		qc->tf.protocol = ATAPI_PROT_NODATA;
+	else if (using_pio)
+		qc->tf.protocol = ATAPI_PROT_PIO;
+	else {
 		/* DMA data xfer */
 		qc->tf.protocol = ATAPI_PROT_DMA;
 		qc->tf.feature |= ATAPI_PKT_DMA;
Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -4297,7 +4297,7 @@ void ata_sg_clean(struct ata_queued_cmd 
 }
 
 /**
- *	ata_check_atapi_dma - Check whether ATAPI DMA can be supported
+ *	atapi_check_dma - Check whether ATAPI DMA can be supported
  *	@qc: Metadata associated with taskfile to check
  *
  *	Allow low-level driver to filter ATA PACKET commands, returning
@@ -4310,7 +4310,7 @@ void ata_sg_clean(struct ata_queued_cmd 
  *	RETURNS: 0 when ATAPI DMA can be used
  *               nonzero otherwise
  */
-int ata_check_atapi_dma(struct ata_queued_cmd *qc)
+int atapi_check_dma(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 
Index: work/drivers/ata/libata.h
===================================================================
--- work.orig/drivers/ata/libata.h
+++ work/drivers/ata/libata.h
@@ -106,7 +106,7 @@ extern void ata_sg_clean(struct ata_queu
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern void ata_qc_issue(struct ata_queued_cmd *qc);
 extern void __ata_qc_complete(struct ata_queued_cmd *qc);
-extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
+extern int atapi_check_dma(struct ata_queued_cmd *qc);
 extern void swap_buf_le16(u16 *buf, unsigned int buf_words);
 extern void ata_dev_init(struct ata_device *dev);
 extern void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp);

[PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Tejun Heo]

LG blueray drive GGW-H10N can't do ATAPI PIO.  Implement
ATAPI_HORKAGE_NOPIO and apply it to the drive.  If the horkage is
active, atapi_check_dma() always returns 0 and warns if the controller
needs ATAPI PIO for certain commands.

Reported by Luke Ross in bz 10091.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Luke Ross <luke@lukeross.name>
---
 drivers/ata/libata-core.c |   30 +++++++++++++++++++++++++-----
 drivers/ata/libata-eh.c   |    1 +
 include/linux/libata.h    |    2 ++
 3 files changed, 28 insertions(+), 5 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -2266,6 +2266,7 @@ int ata_dev_configure(struct ata_device 
 		const char *cdb_intr_string = "";
 		const char *atapi_an_string = "";
 		const char *dma_dir_string = "";
+		const char *nopio_string = "";
 		u32 sntf;
 
 		rc = atapi_cdb_len(id);
@@ -2311,14 +2312,17 @@ int ata_dev_configure(struct ata_device 
 			dma_dir_string = ", DMADIR";
 		}
 
+		if (dev->horkage & ATAPI_HORKAGE_NOPIO)
+			nopio_string = ", no PIO";
+
 		/* print device info to dmesg */
 		if (ata_msg_drv(ap) && print_info)
 			ata_dev_printk(dev, KERN_INFO,
-				       "ATAPI: %s, %s, max %s%s%s%s\n",
+				       "ATAPI: %s, %s, max %s%s%s%s%s\n",
 				       modelbuf, fwrevbuf,
 				       ata_mode_string(xfer_mask),
 				       cdb_intr_string, atapi_an_string,
-				       dma_dir_string);
+				       dma_dir_string, nopio_string);
 	}
 
 	/* determine max_sectors */
@@ -3946,6 +3950,9 @@ static const struct ata_blacklist_entry 
 	{ "TSSTcorp CDDVDW SH-S202N", "SB00",	  ATA_HORKAGE_IVB, },
 	{ "TSSTcorp CDDVDW SH-S202N", "SB01",	  ATA_HORKAGE_IVB, },
 
+	/* AAAARGH... this one can't do ATAPI PIO */
+	{ "HL-DT-ST BD-RE  GGW-H10N", NULL,	ATAPI_HORKAGE_NOPIO },
+
 	/* End Marker */
 	{ }
 };
@@ -4313,17 +4320,30 @@ void ata_sg_clean(struct ata_queued_cmd 
 int atapi_check_dma(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
+	struct ata_device *dev = qc->dev;
+	bool nopio = dev->horkage & ATAPI_HORKAGE_NOPIO;
+	int rc = 0;
 
 	/* Don't allow DMA if it isn't multiple of 16 bytes.  Quite a
 	 * few ATAPI devices choke on such DMA requests.
 	 */
-	if (unlikely(qc->nbytes & 15))
+	if (unlikely(qc->nbytes & 15) && !nopio)
 		return 1;
 
 	if (ap->ops->check_atapi_dma)
-		return ap->ops->check_atapi_dma(qc);
+		rc = ap->ops->check_atapi_dma(qc);
 
-	return 0;
+	if (unlikely(rc) && nopio) {
+		if (!(dev->flags & ATAPI_DFLAG_WARNED_NOPIO)) {
+			ata_dev_printk(dev, KERN_ERR,
+			       "device can't handle ATAPI PIO but "
+			       "controller needs it, expect problem\n");
+			dev->flags |= ATAPI_DFLAG_WARNED_NOPIO;
+		}
+		rc = 0;	/* we're screwed, be optimistic while being screwed */
+	}
+
+	return rc;
 }
 
 /**
Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -1693,6 +1693,7 @@ static unsigned int ata_eh_speed_down(st
 	 */
 	if ((verdict & ATA_EH_SPDN_FALLBACK_TO_PIO) && (dev->spdn_cnt >= 2) &&
 	    (link->ap->cbl != ATA_CBL_SATA || dev->class == ATA_DEV_ATAPI) &&
+	    !(dev->horkage & ATAPI_HORKAGE_NOPIO) &&
 	    (dev->xfer_shift != ATA_SHIFT_PIO)) {
 		if (ata_down_xfermask_limit(dev, ATA_DNXFER_FORCE_PIO) == 0) {
 			dev->spdn_cnt = 0;
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -145,6 +145,7 @@ enum {
 	ATA_DFLAG_SPUNDOWN	= (1 << 14), /* XXX: for spindown_compat */
 	ATA_DFLAG_SLEEPING	= (1 << 15), /* device is sleeping */
 	ATA_DFLAG_DUBIOUS_XFER	= (1 << 16), /* data transfer not verified */
+	ATAPI_DFLAG_WARNED_NOPIO = (1 << 17), /* warned about ATAPI NOPIO */
 	ATA_DFLAG_INIT_MASK	= (1 << 24) - 1,
 
 	ATA_DFLAG_DETACH	= (1 << 24),
@@ -353,6 +354,7 @@ enum {
 	ATA_HORKAGE_IPM		= (1 << 7),	/* Link PM problems */
 	ATA_HORKAGE_IVB		= (1 << 8),	/* cbl det validity bit bugs */
 	ATA_HORKAGE_STUCK_ERR	= (1 << 9),	/* stuck ERR on next PACKET */
+	ATAPI_HORKAGE_NOPIO	= (1 << 10),	/* don't use ATAPI PIO */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Alan Cox]

On Tue, 17 Jun 2008 12:37:24 +0900
Tejun Heo <tj@kernel.org> wrote:

> LG blueray drive GGW-H10N can't do ATAPI PIO.  Implement
> ATAPI_HORKAGE_NOPIO and apply it to the drive.  If the horkage is
> active, atapi_check_dma() always returns 0 and warns if the controller
> needs ATAPI PIO for certain commands.

This all seems wildly improbable as a drive with such a bug would not
appear to be going to work in Windows. I'd like to know more and see more
than one bug report for that drive before it goes in

You might even be looking at a single drive with a one off wiring flaw or
chip fault at this point.

So in the absence of multiple reports of this drive failing, and
of no other drive being tested on the same controller after the bug was
seen I think this is a *very bad idea* as a patch.

If after 3 months we see a series of such reports then maybe.

Alan

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Tejun Heo]

Alan Cox wrote:
> On Tue, 17 Jun 2008 12:37:24 +0900
> Tejun Heo <tj@kernel.org> wrote:
> 
>> LG blueray drive GGW-H10N can't do ATAPI PIO.  Implement
>> ATAPI_HORKAGE_NOPIO and apply it to the drive.  If the horkage is
>> active, atapi_check_dma() always returns 0 and warns if the controller
>> needs ATAPI PIO for certain commands.
> 
> This all seems wildly improbable as a drive with such a bug would not
> appear to be going to work in Windows. I'd like to know more and see more
> than one bug report for that drive before it goes in
> 
> You might even be looking at a single drive with a one off wiring flaw or
> chip fault at this point.

I thought it was improbable too but it's a brand new relatively cheap
blueray writer so it's quite possible that they screwed up the firmware
on this one as are often the cases for first gen devices.

Relatively cheap it may be, it's still around $300, so I don't think
I'll get one for testing.  The high price could probably explain low
error report rate.

Luke, what happens if you use the drive on windows?  Does it just work?

> So in the absence of multiple reports of this drive failing, and
> of no other drive being tested on the same controller after the bug was
> seen I think this is a *very bad idea* as a patch.

Well, if the diagnose is correct, the patch itself is just implementing
what should be done.  I agree the problem itself is very nasty but don't
think the patch is too bad given the problem.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Tejun Heo]

Tejun Heo wrote:
> Alan Cox wrote:
>> On Tue, 17 Jun 2008 12:37:24 +0900
>> Tejun Heo <tj@kernel.org> wrote:
>>
>>> LG blueray drive GGW-H10N can't do ATAPI PIO.  Implement
>>> ATAPI_HORKAGE_NOPIO and apply it to the drive.  If the horkage is
>>> active, atapi_check_dma() always returns 0 and warns if the controller
>>> needs ATAPI PIO for certain commands.
>> This all seems wildly improbable as a drive with such a bug would not
>> appear to be going to work in Windows. I'd like to know more and see more
>> than one bug report for that drive before it goes in
>>
>> You might even be looking at a single drive with a one off wiring flaw or
>> chip fault at this point.
> 
> I thought it was improbable too but it's a brand new relatively cheap
> blueray writer so it's quite possible that they screwed up the firmware
> on this one as are often the cases for first gen devices.
> 
> Relatively cheap it may be, it's still around $300, so I don't think
> I'll get one for testing.  The high price could probably explain low
> error report rate.

Ah.. one more thing to note.  The drive doesn't choke on all PIO
commands, so it's quite possible that windows just luckily dodged bullet
and we're taking the fall instead && the reporter says that the
controller worked fine with a dvd writer.

Is there anyone who is willing to shell out $300 in the name of penguin?

-- 
tejun

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Luke Ross]

Hi,

On Tue, Jun 17, 2008 at 06:14:40PM +0900, Tejun Heo wrote:
> Alan Cox wrote:
> > On Tue, 17 Jun 2008 12:37:24 +0900
> > Tejun Heo <tj@kernel.org> wrote:
> > 
> >> LG blueray drive GGW-H10N can't do ATAPI PIO.  Implement
> >> ATAPI_HORKAGE_NOPIO and apply it to the drive.  If the horkage is
> >> active, atapi_check_dma() always returns 0 and warns if the controller
> >> needs ATAPI PIO for certain commands.
> > 
> > This all seems wildly improbable as a drive with such a bug would not
> > appear to be going to work in Windows. I'd like to know more and see more
> > than one bug report for that drive before it goes in
> > 
> > You might even be looking at a single drive with a one off wiring flaw or
> > chip fault at this point.
> 
> I thought it was improbable too but it's a brand new relatively cheap
> blueray writer so it's quite possible that they screwed up the firmware
> on this one as are often the cases for first gen devices.
> 
> Relatively cheap it may be, it's still around $300, so I don't think
> I'll get one for testing.  The high price could probably explain low
> error report rate.
> 
> Luke, what happens if you use the drive on windows?  Does it just work?

Sorry, I've a Windows-free household and the machines a self-build so 
doesn't come with a license. I'll see if I can find a solution but it 
may take a little while.

It could be a drive fault or it could be a funky issue with the
drive/controller combination. nvidia claimed they weren't aware of any
problems with the device on the nForce chipset (LG didn't bother
responding to my email). It is LG's first Blu-Ray/HD-DVD combo drive and
I did buy it shortly after UK release so it could be an early build.
I've run it with firmwares 1.01 to 1.04.

Originally the drive was bought for a different PC with a Silicon Image
3114 controller. The drive worked fine with this (sata_sil). Then I
changed to the nForce motherboard and it never worked with the mobo
(AHCI driver/AHCI mode in BIOS). However it does work on a Silicon Image
3512 PCI card (sata_sil again) in the same machine, and a Lite-On DVD
rewriter works on the nForce SATA just fine (ahci).

To muddy the waters further the LG drive on the nForce motherboard works
fine for reading. It'll even write a Blu-Ray disc happily*. It just
won't accept commands like "wodim -toc", "wodim <filename>", "growisofs
-Z" without Tejun's patch.

Luke

* I use the term loosely, it's a bit sulky about BD-R media.
-- 
``Doing linear scans over an associative array is like trying to club
  someone to death with a loaded Uzi.'' -- Larry Wall

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Alan Cox]

> Originally the drive was bought for a different PC with a Silicon Image
> 3114 controller. The drive worked fine with this (sata_sil). Then I

Ok that proves the point quite nicely. If there is a problem it is not
the one suggested.

Alan

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Tejun Heo]

Alan Cox wrote:
>> Originally the drive was bought for a different PC with a Silicon Image
>> 3114 controller. The drive worked fine with this (sata_sil). Then I
> 
> Ok that proves the point quite nicely. If there is a problem it is not
> the one suggested.

Yeah, indeed.  It's still weird tho.  The difference between
ATAPI_PROT_PIO and ATAPI_PROT_DMA is very small especially on ahci.  The
data are still transferred using the same FIS.  Only the completion
mechanism is different.  It would be great to try the drive on another
ahci controller preferably intel one.  Luke, do you have access to any
machine which has intel ahci?

-- 
tejun

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Luke Ross]

Hi,

On Tue, Jun 17, 2008 at 09:27:47PM +0900, Tejun Heo wrote:
> Alan Cox wrote:
> >> Originally the drive was bought for a different PC with a Silicon Image
> >> 3114 controller. The drive worked fine with this (sata_sil). Then I
> > 
> > Ok that proves the point quite nicely. If there is a problem it is not
> > the one suggested.
> 
> Yeah, indeed.  It's still weird tho.  The difference between
> ATAPI_PROT_PIO and ATAPI_PROT_DMA is very small especially on ahci.  The
> data are still transferred using the same FIS.  Only the completion
> mechanism is different.  It would be great to try the drive on another
> ahci controller preferably intel one.  Luke, do you have access to any
> machine which has intel ahci?

I tried it out on a machine with an Intel ICH7 controller using the AHCI
driver (using a Fedora 9 LiveCD). On the ICH7 the drive wrote a CD
without issue, so possibly it's the drive/controller pair that causes
the problem. I was careful to ensure I was using the ahci driver and not 
the ata_piix driver.

Luke

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Jeff Garzik]

Luke Ross wrote:
> Hi,
> 
> On Tue, Jun 17, 2008 at 09:27:47PM +0900, Tejun Heo wrote:
>> Alan Cox wrote:
>>>> Originally the drive was bought for a different PC with a Silicon Image
>>>> 3114 controller. The drive worked fine with this (sata_sil). Then I
>>> Ok that proves the point quite nicely. If there is a problem it is not
>>> the one suggested.
>> Yeah, indeed.  It's still weird tho.  The difference between
>> ATAPI_PROT_PIO and ATAPI_PROT_DMA is very small especially on ahci.  The
>> data are still transferred using the same FIS.  Only the completion
>> mechanism is different.  It would be great to try the drive on another
>> ahci controller preferably intel one.  Luke, do you have access to any
>> machine which has intel ahci?
> 
> I tried it out on a machine with an Intel ICH7 controller using the AHCI
> driver (using a Fedora 9 LiveCD). On the ICH7 the drive wrote a CD
> without issue, so possibly it's the drive/controller pair that causes
> the problem. I was careful to ensure I was using the ahci driver and not 
> the ata_piix driver.

I really think it's more generalized ATAPI brokenness, perhaps specific 
to AHCI.

I'm going to allocate some time to dig into this, if Tejun doesn't find 
the problem, because it's not just limited to a few models.  We've been 
getting _tons_ of timeout bug reports, and in fact, my own AHCI-based 
workstation cannot rip audio CDs at all:


ahci 0000:00:1f.2: AHCI 0001.0100 32 slots 4 ports 3 Gbps 0xf impl SATA mode
ahci 0000:00:1f.2: flags: 64bit ncq led clo pio slum part

	...

ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata4.00: ATAPI: PLEXTOR DVDR   PX-755A, 1.03, max UDMA/66
ata4.00: configured for UDMA/66

	...

scsi 3:0:0:0: CD-ROM            PLEXTOR  DVDR   PX-755A   1.03 PQ: 0 ANSI: 5

	...

ata4.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2 frozen
ata4.00: cmd a0/01:00:00:30:09/00:00:00:00:00/a0 tag 0 dma 2352 in
          cdb be 00 00 00 0d 6d 00 00  01 f8 00 00 00 00 00 00
          res 40/00:03:00:00:00/00:00:00:00:00/a0 Emask 0x4 (timeout)
ata4.00: status: { DRDY }
ata4: soft resetting link
ata4: port is slow to respond, please be patient (Status 0xd0)
ata4: softreset failed (device not ready)
ata4: hard resetting link
ata4: port is slow to respond, please be patient (Status 0x80)
ata4: COMRESET failed (errno=-16)
ata4: hard resetting link
ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata4.00: configured for UDMA/66
ata4: EH complete
scsi: unknown opcode 0xd4
scsi: unknown opcode 0xd5
scsi: unknown opcode 0xd8

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Dan Williams]

On Wed, Jun 18, 2008 at 4:59 AM, Jeff Garzik <jeff@garzik.org> wrote:
> I really think it's more generalized ATAPI brokenness, perhaps specific to
> AHCI.
>
> I'm going to allocate some time to dig into this, if Tejun doesn't find the
> problem, because it's not just limited to a few models.  We've been getting
> _tons_ of timeout bug reports, and in fact, my own AHCI-based workstation
> cannot rip audio CDs at all:
>

...maybe the following is related?

I have a problem report against an ICH9R platform in ahci mode.  The
customer is trying to send 'set streaming' (0xb6) ATAPI commands to an
optical drive.  They report that it works fine with sata_mv, but the
same test fails on ahci.

ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata2.00: ATAPI: HL-DT-ST BD-RE  GBW-H20L, 8.B8, max UDMA/100
ata2.00: configured for UDMA/100
scsi 1:0:0:0: CD-ROM            HL-DT-ST BD-RE  GBW-H20L  8.B8 PQ: 0 ANSI: 5
sr0: scsi3-mmc drive: 40x/40x writer dvd-ram cd/rw xa/form2 cdda tray
Uniform CD-ROM driver Revision: 3.20
sr 1:0:0:0: Attached scsi CD-ROM sr0
sr 1:0:0:0: Attached scsi generic sg1 type 5
[..]
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
ata2.00: cmd a0/00:00:00:1c:00/00:00:00:00:00/a0 tag 0 pio 28 out
         cdb b6 00 00 00 00 1c 00 00  00 00 00 00 00 00 00 00
         res 40/00:03:00:00:00/00:00:00:00:00/a0 Emask 0x4 (timeout)
ata2.00: status: { DRDY }
ata2: hard resetting link
ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
ata2.00: configured for UDMA/100
ata2: EH complete

I'll try to get a reproducer, in the meantime any ideas?

Thanks,
Dan

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Robert Hancock]

On 06/18/2008 05:59 AM, Jeff Garzik wrote:
> Luke Ross wrote:
>> Hi,
>>
>> On Tue, Jun 17, 2008 at 09:27:47PM +0900, Tejun Heo wrote:
>>> Alan Cox wrote:
>>>>> Originally the drive was bought for a different PC with a Silicon
>>>>> Image
>>>>> 3114 controller. The drive worked fine with this (sata_sil). Then I
>>>> Ok that proves the point quite nicely. If there is a problem it is not
>>>> the one suggested.
>>> Yeah, indeed. It's still weird tho. The difference between
>>> ATAPI_PROT_PIO and ATAPI_PROT_DMA is very small especially on ahci. The
>>> data are still transferred using the same FIS. Only the completion
>>> mechanism is different. It would be great to try the drive on another
>>> ahci controller preferably intel one. Luke, do you have access to any
>>> machine which has intel ahci?
>>
>> I tried it out on a machine with an Intel ICH7 controller using the AHCI
>> driver (using a Fedora 9 LiveCD). On the ICH7 the drive wrote a CD
>> without issue, so possibly it's the drive/controller pair that causes
>> the problem. I was careful to ensure I was using the ahci driver and
>> not the ata_piix driver.
>
> I really think it's more generalized ATAPI brokenness, perhaps specific
> to AHCI.
>
> I'm going to allocate some time to dig into this, if Tejun doesn't find
> the problem, because it's not just limited to a few models. We've been
> getting _tons_ of timeout bug reports, and in fact, my own AHCI-based
> workstation cannot rip audio CDs at all:
>
>
> ahci 0000:00:1f.2: AHCI 0001.0100 32 slots 4 ports 3 Gbps 0xf impl SATA
> mode
> ahci 0000:00:1f.2: flags: 64bit ncq led clo pio slum part
>
> ...
>
> ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> ata4.00: ATAPI: PLEXTOR DVDR PX-755A, 1.03, max UDMA/66
> ata4.00: configured for UDMA/66
>
> ...
>
> scsi 3:0:0:0: CD-ROM PLEXTOR DVDR PX-755A 1.03 PQ: 0 ANSI: 5
>
> ...
>
> ata4.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2 frozen
> ata4.00: cmd a0/01:00:00:30:09/00:00:00:00:00/a0 tag 0 dma 2352 in
> cdb be 00 00 00 0d 6d 00 00 01 f8 00 00 00 00 00 00
> res 40/00:03:00:00:00/00:00:00:00:00/a0 Emask 0x4 (timeout)
> ata4.00: status: { DRDY }
> ata4: soft resetting link
> ata4: port is slow to respond, please be patient (Status 0xd0)
> ata4: softreset failed (device not ready)
> ata4: hard resetting link
> ata4: port is slow to respond, please be patient (Status 0x80)
> ata4: COMRESET failed (errno=-16)
> ata4: hard resetting link
> ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> ata4.00: configured for UDMA/66
> ata4: EH complete

Thing is, ATAPI isn't really very special from the driver point of view 
on AHCI. The only thing unusual I can think of (at least in this case 
with the audio READ CD command) is that the transfer size is a little 
bit odd (2352 bytes). It's possible that there's some kind of mismatch 
between the total PRD byte count given to AHCI and the actual transfer size?

Luke/Tejun, do you have some error output from those wodim or growisofs 
errors? It would be useful to know what commands those were that failed 
and whether they were also an unusual data size.

It might be a useful debugging exercise to dump out a few things in the 
error handler here:

-PORT_IRQ_STAT state (particularly PORT_IRQ_OVERFLOW) to see if the 
controller is unhappy with something we don't enable an IRQ for

-the Physical Region Descriptor Byte Count (PRDBC) field (the status 
field in the command structure in memory) to see how much data it thinks 
it transferred

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Tejun Heo]

Hello,

11/14/2009 09:11 AM, Robert Hancock wrote:
> Luke/Tejun, do you have some error output from those wodim or growisofs
> errors? It would be useful to know what commands those were that failed
> and whether they were also an unusual data size.

I don't have anything.  For some reason, ahci works fine on the setups
here.  :-(

I'm waiting for Dan's further report.  I still am not sure whether
we're actually seeing a lot of ahci ATAPI failures or it's just that a
lot of reports are on ahci because it's the most often used driver
now.  I don't think anything fundamental is broken.  The reports are
too infrequent for that to be true.  It might have something to do
with the buffer padding / draining we do or how the controller and
driver react.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Robert Hancock]

On Sun, Nov 15, 2009 at 2:16 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> 11/14/2009 09:11 AM, Robert Hancock wrote:
>> Luke/Tejun, do you have some error output from those wodim or growisofs
>> errors? It would be useful to know what commands those were that failed
>> and whether they were also an unusual data size.
>
> I don't have anything.  For some reason, ahci works fine on the setups
> here.  :-(
>
> I'm waiting for Dan's further report.  I still am not sure whether
> we're actually seeing a lot of ahci ATAPI failures or it's just that a
> lot of reports are on ahci because it's the most often used driver
> now.  I don't think anything fundamental is broken.  The reports are
> too infrequent for that to be true.  It might have something to do
> with the buffer padding / draining we do or how the controller and
> driver react.

From the bug 10091 it looks like the failing command was a MODE SELECT
with data transfer length of 66 bytes which is definitely odd..

My suspicion is that we're either padding out the data buffer and the
indicated PRD length to a longer length than the drive actually wants
to transfer, and the controller isn't happy with that, OR maybe
vice-versa (i.e. we don't pad out the length and it wants us to). It's
not clear if this and Jeff's issue are actually the same thing as the
response is fairly different (interface fatal error and SError
indications on the NV vs. a timeout on whatever chipset Jeff's using),
but that may just be a chipset difference.

If Jeff has a repeatable test case then that's likely the most
promising place to start poking at things..

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Jeff Garzik]

On 11/15/2009 01:22 PM, Robert Hancock wrote:
> On Sun, Nov 15, 2009 at 2:16 AM, Tejun Heo<tj@kernel.org>  wrote:
>> Hello,
>>
>> 11/14/2009 09:11 AM, Robert Hancock wrote:
>>> Luke/Tejun, do you have some error output from those wodim or growisofs
>>> errors? It would be useful to know what commands those were that failed
>>> and whether they were also an unusual data size.
>>
>> I don't have anything.  For some reason, ahci works fine on the setups
>> here.  :-(
>>
>> I'm waiting for Dan's further report.  I still am not sure whether
>> we're actually seeing a lot of ahci ATAPI failures or it's just that a
>> lot of reports are on ahci because it's the most often used driver
>> now.  I don't think anything fundamental is broken.  The reports are
>> too infrequent for that to be true.  It might have something to do
>> with the buffer padding / draining we do or how the controller and
>> driver react.
>
>> From the bug 10091 it looks like the failing command was a MODE SELECT
> with data transfer length of 66 bytes which is definitely odd..
>
> My suspicion is that we're either padding out the data buffer and the
> indicated PRD length to a longer length than the drive actually wants
> to transfer, and the controller isn't happy with that, OR maybe
> vice-versa (i.e. we don't pad out the length and it wants us to). It's
> not clear if this and Jeff's issue are actually the same thing as the
> response is fairly different (interface fatal error and SError
> indications on the NV vs. a timeout on whatever chipset Jeff's using),
> but that may just be a chipset difference.
>
> If Jeff has a repeatable test case then that's likely the most
> promising place to start poking at things..

Nope, I replied to that original thread, well after the fact, noting my 
hardware likely flaky.  Unable to reproduce the problem anymore, 
unfortunately.

	Jeff

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Dan Williams]

On Sun, Nov 15, 2009 at 1:16 AM, Tejun Heo <tj@kernel.org> wrote:
> I'm waiting for Dan's further report.

After pressing a bit more for details, the customer reports that they
have resolved the issue.  I was hoping to get details on the malformed
request that was being sent so that maybe the kernel could report an
error rather than silently dropping the initial command and timing out
a subsequent command... no such luck.

Thanks for the attention.

Regards,
Dan

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Luke Ross]

Hi,

On Fri, Nov 13, 2009 at 06:11:20PM -0600, Robert Hancock wrote:
>
> Luke/Tejun, do you have some error output from those wodim or growisofs  
> errors? It would be useful to know what commands those were that failed  
> and whether they were also an unusual data size.

The only information I still have is on the bug:

http://bugzilla.kernel.org/show_bug.cgi?id=10091

It felt like it must have been something very edge-case, as only 
nVidia_chipset+LG_drive+Linux seemed to show the issue. Linux on the 
nVidia with a SATA Lite-On drive worked fine; Linux on an Intel ICH with 
AHCI and the LG drive worked fine; Windows XP on the nVidia with the LG 
drive worked fine.

I've since upgraded the motherboard to a newer nVidia chipset (8300) and 
the drive to a newer LG (GGW-H20L) and these both play fine with AHCI - 
so I no longer have the original hardware to test with.

Luke

Re: [PATCH #upstream-fixes 2/2] libata: implement ATAPI_HORKAGE_NOPIO and apply it to GGW-H10N

[Alan Cox]

On Tue, 17 Jun 2008 12:37:24 +0900
Tejun Heo <tj@kernel.org> wrote:

> LG blueray drive GGW-H10N can't do ATAPI PIO.  Implement
> ATAPI_HORKAGE_NOPIO and apply it to the drive.  If the horkage is
> active, atapi_check_dma() always returns 0 and warns if the controller
> needs ATAPI PIO for certain commands.

A further sweep of google shows no other bug reports but this one anywhere
and nothing from LG about compatiblity as would be expected for such a
problem

So NACK this patch.

Alan

Re: [PATCH #upstream-fixes 1/2] libata: don't check whether to use DMA or not for no data commands

[Jeff Garzik]

Tejun Heo wrote:
> There's no reason to check whether to use DMA or not for no data
> commands.  Don't do it.  While at it, make local variable using_pio in
> atapi_xlat() set iff ATAPI_PROT_PIO is going to be used and rename
> ata_check_atapi_dma() to atapi_check_dma() for consistency.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
> It's late in the -rc cycle but these two patches are to work around
> specific hardware and low risk.  Thanks.
> 
>  drivers/ata/libata-core.c |    4 ++--
>  drivers/ata/libata-scsi.c |   16 +++++++---------
>  drivers/ata/libata.h      |    2 +-
>  3 files changed, 10 insertions(+), 12 deletions(-)

applied this (patch #1)

dropped patch #2 per discussion