[PATCH #upstream-fixes] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size

[PATCH #upstream-fixes] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size

[Tejun Heo]

While updating lbam/h for ATAPI commands, atapi_eh_request_sense() was
left out.  Update it.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
This patch is from improve-ATAPI-data-xfer patchset pending for
#upstream.

 drivers/ata/libata-eh.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -1264,8 +1264,8 @@ static unsigned int atapi_eh_request_sen
 		tf.feature |= ATAPI_PKT_DMA;
 	} else {
 		tf.protocol = ATA_PROT_ATAPI;
-		tf.lbam = (8 * 1024) & 0xff;
-		tf.lbah = (8 * 1024) >> 8;
+		tf.lbam = SCSI_SENSE_BUFFERSIZE;
+		tf.lbah = 0;
 	}
 
 	return ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,

[PATCH #upstream-fixes] libata: fix ATAPI draining

[Tejun Heo]

With ATAPI transfer chunk size properly programmed, libata PIO HSM
should be able to handle full spurious data chunks.  Also, it's a good
idea to suppress trailing data warning for misc ATAPI commands as
there can be many of them per command - for example, if the chunk size
is 16 and the drive tries to transfer 510 bytes, there can be 31
trailing data messages.

This patch makes the following updates to libata ATAPI PIO HSM
implementation.

* Make it drain full spurious chunks.

* Suppress trailing data warning message for misc commands.

* Put limit on how many bytes can be drained.

* If odd, round up consumed bytes and the number of bytes to be
  drained.  This gets the number of bytes to drain right for drivers
  which do 16bit PIO.

This patch is partial backport of improve-ATAPI-data-xfer patchset
pending for #upstream.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
This combined with the previous patch fixes bug 9346.

 drivers/ata/libata-core.c |   87 ++++++++++++++++++++++++++++++++++------------
 include/linux/libata.h    |    2 +
 2 files changed, 67 insertions(+), 22 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -64,6 +64,7 @@
 #include <linux/libata.h>
 #include <asm/semaphore.h>
 #include <asm/byteorder.h>
+#include <linux/cdrom.h>
 
 #include "libata.h"
 
@@ -4649,6 +4650,43 @@ int ata_check_atapi_dma(struct ata_queue
 }
 
 /**
+ *	atapi_qc_may_overflow - Check whether data transfer may overflow
+ *	@qc: ATA command in question
+ *
+ *	ATAPI commands which transfer variable length data to host
+ *	might overflow due to application error or hardare bug.  This
+ *	function checks whether overflow should be drained and ignored
+ *	for @qc.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	1 if @qc may overflow; otherwise, 0.
+ */
+static int atapi_qc_may_overflow(struct ata_queued_cmd *qc)
+{
+	if (qc->tf.protocol != ATA_PROT_ATAPI &&
+	    qc->tf.protocol != ATA_PROT_ATAPI_DMA)
+		return 0;
+
+	if (qc->tf.flags & ATA_TFLAG_WRITE)
+		return 0;
+
+	switch (qc->cdb[0]) {
+	case READ_10:
+	case READ_12:
+	case WRITE_10:
+	case WRITE_12:
+	case GPCMD_READ_CD:
+	case GPCMD_READ_CD_MSF:
+		return 0;
+	}
+
+	return 1;
+}
+
+/**
  *	ata_std_qc_defer - Check whether a qc needs to be deferred
  *	@qc: ATA command in question
  *
@@ -5136,23 +5174,19 @@ static void atapi_send_cdb(struct ata_po
  *	Inherited from caller.
  *
  */
-
-static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
+static int __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
-	struct scatterlist *sg = qc->__sg;
-	struct scatterlist *lsg = sg_last(qc->__sg, qc->n_elem);
 	struct ata_port *ap = qc->ap;
+	struct ata_eh_info *ehi = &qc->dev->link->eh_info;
+	struct scatterlist *sg;
 	struct page *page;
 	unsigned char *buf;
 	unsigned int offset, count;
-	int no_more_sg = 0;
-
-	if (qc->curbytes + bytes >= qc->nbytes)
-		ap->hsm_task_state = HSM_ST_LAST;
 
 next_sg:
-	if (unlikely(no_more_sg)) {
+	sg = qc->cursg;
+	if (unlikely(!sg)) {
 		/*
 		 * The end of qc->sg is reached and the device expects
 		 * more data to transfer. In order not to overrun qc->sg
@@ -5161,21 +5195,28 @@ next_sg:
 		 *    - for write case, padding zero data to the device
 		 */
 		u16 pad_buf[1] = { 0 };
-		unsigned int words = bytes >> 1;
 		unsigned int i;
 
-		if (words) /* warning if bytes > 1 */
-			ata_dev_printk(qc->dev, KERN_WARNING,
-				       "%u bytes trailing data\n", bytes);
+		if (bytes > qc->curbytes - qc->nbytes + ATAPI_MAX_DRAIN) {
+			ata_ehi_push_desc(ehi, "too much trailing data "
+					  "buf=%u cur=%u bytes=%u",
+					  qc->nbytes, qc->curbytes, bytes);
+			return -1;
+		}
+
+		 /* overflow is exptected for misc ATAPI commands */
+		if (bytes && !atapi_qc_may_overflow(qc))
+			ata_dev_printk(qc->dev, KERN_WARNING, "ATAPI %u bytes "
+				       "trailing data (cdb=%02x nbytes=%u)\n",
+				       bytes, qc->cdb[0], qc->nbytes);
 
-		for (i = 0; i < words; i++)
+		for (i = 0; i < (bytes + 1) / 2; i++)
 			ap->ops->data_xfer(qc->dev, (unsigned char *)pad_buf, 2, do_write);
 
-		ap->hsm_task_state = HSM_ST_LAST;
-		return;
-	}
+		qc->curbytes += bytes;
 
-	sg = qc->cursg;
+		return 0;
+	}
 
 	page = sg_page(sg);
 	offset = sg->offset + qc->cursg_ofs;
@@ -5210,19 +5251,20 @@ next_sg:
 	}
 
 	bytes -= count;
+	if ((count & 1) && bytes)
+		bytes--;
 	qc->curbytes += count;
 	qc->cursg_ofs += count;
 
 	if (qc->cursg_ofs == sg->length) {
-		if (qc->cursg == lsg)
-			no_more_sg = 1;
-
 		qc->cursg = sg_next(qc->cursg);
 		qc->cursg_ofs = 0;
 	}
 
 	if (bytes)
 		goto next_sg;
+
+	return 0;
 }
 
 /**
@@ -5265,7 +5307,8 @@ static void atapi_pio_bytes(struct ata_q
 
 	VPRINTK("ata%u: xfering %d bytes\n", ap->print_id, bytes);
 
-	__atapi_pio_bytes(qc, bytes);
+	if (__atapi_pio_bytes(qc, bytes))
+		goto err_out;
 	ata_altstatus(ap); /* flush */
 
 	return;
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -119,6 +119,8 @@ enum {
 	ATA_DEF_BUSY_WAIT	= 10000,
 	ATA_SHORT_PAUSE		= (HZ >> 6) + 1,
 
+	ATAPI_MAX_DRAIN		= 16 << 10,
+
 	ATA_SHT_EMULATED	= 1,
 	ATA_SHT_CMD_PER_LUN	= 1,
 	ATA_SHT_THIS_ID		= -1,

Re: [PATCH #upstream-fixes] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size

[Will Trives]

Hello guys,

Yannick does it work ok for you when you try to blank a dvd+rw disc ?
Mine no longer jams up the drive now but it does say that the media or
drive does not support blanking. I'm thinking its either because these
discs just do multisession so this is what was meant to happen all along
or there's still something going on. 


wodim -blank=fast dev=/dev/sr0
wodim: Operation not permitted. Warning: Cannot raise RLIMIT_MEMLOCK
limits.Device type    : Removable CD-ROM
Version        : 5
Response Format: 2
Capabilities   :
Vendor_info    : 'BENQ    '
Identification : 'DVD DD DW1640   '
Revision       : 'BSRB'
Device seems to be: Generic mmc2 DVD-R/DVD-RW.
Using generic SCSI-3/mmc DVD-R(W) driver (mmc_mdvd).
Driver flags   : SWABAUDIO BURNFREE
Supported modes: PACKET SAO
HINT: use dvd+rw-mediainfo from dvd+rw-tools for information extraction.
Speed set to 5540 KB/s
Starting to write CD/DVD at speed   4.0 in real BLANK mode for single
session.
Last chance to quit, starting real write i   0 seconds. Operation
starts.
Error: this media does not support blanking, ignoring.
This drive or media does not support the 'BLANK media' command
wodim: Cannot blank disk, aborting.
wodim: Some drives do not support all blank types.
wodim: Try again with wodim blank=all.


Regards,

Will Trives

On Wed, 2007-12-12 at 12:12 +0900, Tejun Heo wrote:
> While updating lbam/h for ATAPI commands, atapi_eh_request_sense() was
> left out.  Update it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> This patch is from improve-ATAPI-data-xfer patchset pending for
> #upstream.
> 
>  drivers/ata/libata-eh.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: work/drivers/ata/libata-eh.c
> ===================================================================
> --- work.orig/drivers/ata/libata-eh.c
> +++ work/drivers/ata/libata-eh.c
> @@ -1264,8 +1264,8 @@ static unsigned int atapi_eh_request_sen
>  		tf.feature |= ATAPI_PKT_DMA;
>  	} else {
>  		tf.protocol = ATA_PROT_ATAPI;
> -		tf.lbam = (8 * 1024) & 0xff;
> -		tf.lbah = (8 * 1024) >> 8;
> +		tf.lbam = SCSI_SENSE_BUFFERSIZE;
> +		tf.lbah = 0;
>  	}
>  
>  	return ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,

Re: [PATCH #upstream-fixes] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size

[Will Trives]

On Wed, 2007-12-12 at 17:16 +0900, Tejun Heo wrote:
Will Trives wrote:
> > Hello guys,
> > 
> > Yannick does it work ok for you when you try to blank a dvd+rw disc ?
> > Mine no longer jams up the drive now but it does say that the media or
> > drive does not support blanking. I'm thinking its either because these
> > discs just do multisession so this is what was meant to happen all along
> > or there's still something going on. 
> 
> Does the kernel complain about anything?
> 
Nope.

> This drive or media does not support the 'BLANK media' command
> > wodim: Cannot blank disk, aborting.
> > wodim: Some drives do not support all blank types.
> > wodim: Try again with wodim blank=all.
> 
> wodim suggests "blank=all", does it work?
> 
Nope same thing happens.

"dvd+rw-format /dev/sr0 -force"  Works ok though.

This is the media info after the format

dvd+rw-mediainfo /dev/sr0

INQUIRY:                [BENQ    ][DVD DD DW1640   ][BSRB]
GET [CURRENT] CONFIGURATION:
 Mounted Media:         1Ah, DVD+RW
 Current Write Speed:   4.0x1385=5540KB/s
 Write Speed #0:        4.0x1385=5540KB/s
 Write Speed #1:        2.4x1385=3324KB/s
GET [CURRENT] PERFORMANCE:
 Write Performance:     4.0x1385=5540KB/s@[0 -> 264671]
 Speed Descriptor#0:    00/264671 R@5.2x1385=7181KB/s W@4.0x1385=5540KB/s
 Speed Descriptor#1:    00/264671 R@5.2x1385=7181KB/s W@2.4x1385=3324KB/s
READ DVD STRUCTURE[#0h]:
 Media Book Type:       00h, DVD-ROM book [revision 0]
 Media ID:              MKM/A02
 Legacy lead-out at:    264672*2KB=542048256
READ DISC INFORMATION:
 Disc status:           complete
 Number of Sessions:    1
 State of Last Session: complete
 Number of Tracks:      1
 BG Format Status:      suspended
READ FORMAT CAPACITIES:
 formatted:             264672*2048=542048256
 26h(0):                2295104*2048=4700372992
READ TRACK INFORMATION[#1]:
 Track State:           complete
 Track Start Address:   0*2KB
 Free Blocks:           0*2KB
 Track Size:            2295104*2KB
FABRICATED TOC:
 Track#1  :             14@0
 Track#AA :             17@2295104
 Multi-session Info:    #1@0
READ CAPACITY:          2295104*2048=4700372992

Re: [PATCH #upstream-fixes] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size

[Tejun Heo]

Will Trives wrote:
> Hello guys,
> 
> Yannick does it work ok for you when you try to blank a dvd+rw disc ?
> Mine no longer jams up the drive now but it does say that the media or
> drive does not support blanking. I'm thinking its either because these
> discs just do multisession so this is what was meant to happen all along
> or there's still something going on. 

Does the kernel complain about anything?

> This drive or media does not support the 'BLANK media' command
> wodim: Cannot blank disk, aborting.
> wodim: Some drives do not support all blank types.
> wodim: Try again with wodim blank=all.

wodim suggests "blank=all", does it work?

-- 
tejun

Re: [PATCH #upstream-fixes] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size

[Tejun Heo]

Hello,

Will Trives wrote:
>> This drive or media does not support the 'BLANK media' command
>>> wodim: Cannot blank disk, aborting.
>>> wodim: Some drives do not support all blank types.
>>> wodim: Try again with wodim blank=all.
>> wodim suggests "blank=all", does it work?
>>
> Nope same thing happens.
> 
> "dvd+rw-format /dev/sr0 -force"  Works ok though.

This looks like a wodim or drive/media problem and for some reason wodim
thinks that the media can't be blanked.  Hmmm.. why do you need -force
to dvd+rw-format?  What happens if you don't use -force?

-- 
tejun

Re: [PATCH #upstream-fixes] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size

[Yannick Dirou]

Will Trives a écrit :
> Hello guys,
>
> Yannick does it work ok for you when you try to blank a dvd+rw disc ?
> Mine no longer jams up the drive now but it does say that the media or
> drive does not support blanking. 
i will try as soon as i get home

-- 
____________________________________

A      X      E      T      I      C
____________________________________
QUIMPER  - 390 route de ROSPORDEN
Tél. :   0  810 812 278 (N° Azur)
Fax  :   02 98 10 09 26
contact@axetic.com   www.axetic.com

Re: [PATCH #upstream-fixes] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size

[Will Trives]

I'm no expert but it might have something to do with the disc supporting
that incremental stuff, so you don't need to blank the disc the
traditional way.

On Wed, 2007-12-12 at 17:38 +0900, Tejun Heo wrote:
> Hello,
> 
> Will Trives wrote:
> >> This drive or media does not support the 'BLANK media' command
> >>> wodim: Cannot blank disk, aborting.
> >>> wodim: Some drives do not support all blank types.
> >>> wodim: Try again with wodim blank=all.
> >> wodim suggests "blank=all", does it work?
> >>
> > Nope same thing happens.
> > 
> > "dvd+rw-format /dev/sr0 -force"  Works ok though.
> 
> This looks like a wodim or drive/media problem and for some reason wodim
> thinks that the media can't be blanked.  Hmmm.. why do you need -force
> to dvd+rw-format?  What happens if you don't use -force?
> 

Re: [PATCH #upstream-fixes] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size

[Jeff Garzik]

Tejun Heo wrote:
> While updating lbam/h for ATAPI commands, atapi_eh_request_sense() was
> left out.  Update it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> This patch is from improve-ATAPI-data-xfer patchset pending for
> #upstream.
> 
>  drivers/ata/libata-eh.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

applied #upstream-fixes

Re: [PATCH #upstream-fixes] libata: fix ATAPI draining

[Jeff Garzik]

Tejun Heo wrote:
> With ATAPI transfer chunk size properly programmed, libata PIO HSM
> should be able to handle full spurious data chunks.  Also, it's a good
> idea to suppress trailing data warning for misc ATAPI commands as
> there can be many of them per command - for example, if the chunk size
> is 16 and the drive tries to transfer 510 bytes, there can be 31
> trailing data messages.
> 
> This patch makes the following updates to libata ATAPI PIO HSM
> implementation.
> 
> * Make it drain full spurious chunks.
> 
> * Suppress trailing data warning message for misc commands.
> 
> * Put limit on how many bytes can be drained.
> 
> * If odd, round up consumed bytes and the number of bytes to be
>   drained.  This gets the number of bytes to drain right for drivers
>   which do 16bit PIO.
> 
> This patch is partial backport of improve-ATAPI-data-xfer patchset
> pending for #upstream.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> This combined with the previous patch fixes bug 9346.
> 
>  drivers/ata/libata-core.c |   87 ++++++++++++++++++++++++++++++++++------------
>  include/linux/libata.h    |    2 +
>  2 files changed, 67 insertions(+), 22 deletions(-)
> 
> Index: work/drivers/ata/libata-core.c
> ===================================================================
> --- work.orig/drivers/ata/libata-core.c
> +++ work/drivers/ata/libata-core.c
> @@ -64,6 +64,7 @@
>  #include <linux/libata.h>
>  #include <asm/semaphore.h>
>  #include <asm/byteorder.h>
> +#include <linux/cdrom.h>
>  
>  #include "libata.h"
>  
> @@ -4649,6 +4650,43 @@ int ata_check_atapi_dma(struct ata_queue
>  }
>  
>  /**
> + *	atapi_qc_may_overflow - Check whether data transfer may overflow
> + *	@qc: ATA command in question
> + *
> + *	ATAPI commands which transfer variable length data to host
> + *	might overflow due to application error or hardare bug.  This
> + *	function checks whether overflow should be drained and ignored
> + *	for @qc.
> + *
> + *	LOCKING:
> + *	None.
> + *
> + *	RETURNS:
> + *	1 if @qc may overflow; otherwise, 0.
> + */
> +static int atapi_qc_may_overflow(struct ata_queued_cmd *qc)
> +{
> +	if (qc->tf.protocol != ATA_PROT_ATAPI &&
> +	    qc->tf.protocol != ATA_PROT_ATAPI_DMA)
> +		return 0;
> +
> +	if (qc->tf.flags & ATA_TFLAG_WRITE)
> +		return 0;
> +
> +	switch (qc->cdb[0]) {
> +	case READ_10:
> +	case READ_12:
> +	case WRITE_10:
> +	case WRITE_12:
> +	case GPCMD_READ_CD:
> +	case GPCMD_READ_CD_MSF:
> +		return 0;
> +	}
> +
> +	return 1;

applied, though I hope we can eventually find a better solution...