Re: [PATCH 06/14] libata: improve ATAPI draining

[Albert Lee <albertcc@tw.ibm.com>]

Tejun Heo wrote:
> For misc ATAPI commands which transfer variable length data to the
> host, overflow can occur due to application or hardware bug.  Such
> overflows can be ignored safely as long as overflow data is properly
> drained.  libata HSM implementation has this implemented in
> __atapi_pio_bytes() but it isn't enough.  Improve drain logic such
> that...
> 
> * Multiple PIO data phases are allowed.  Not allowing this used to be
>   okay when transfer chunk size was set to 8k unconditionally but with
>   transfer hcunk size set to allocation size, treating extra PIO data
>   phases as HSM violations cause a lot of trouble.
> 
> * Limit the amount of draining to ATAPI_MAX_DRAIN (16k currently).
> 
> * Don't whine if overflow is allowed and safe.  When unexpected
>   overflow occurs, trigger HSM violation and report the problem using
>   ehi error description.
> 
> * Properly calculate the number of bytes to be drained considering
>   actual number of consumed bytes for partial draining.
> 
> * Add and use ata_drain_page for draining.  This change fixes the
>   problem where LLDs which do 32bit IOs consumes 4 bytes on each 2
>   byte draining resulting in draining twice more data than requested.
> 
> This patch fixes ATAPI regressions introduced by setting transfer
> chunk size to allocation size.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Albert Lee <albertcc@tw.ibm.com>

Sorry for the late reply. The new patch looks good.

However, I am a little worried about the following code segment in 
__atapi_pio_bytes():

next_sg:
	if (!bytes)
		return 0;


Maybe we should add an additional check to atapi_pio_bytes() such
that "DRQ asserted with bytes = 0" is considered as AC_ERR_HSM?
(patch attached below.) Otherwise the device might keep interruping us
with DRQ asserted + zero byte count, and we are in infinite loop...

--
albert


diff -Nrup 00_libata-dev/drivers/ata/libata-core.c 07_more_check/drivers/ata/libata-core.c
--- 00_libata-dev/drivers/ata/libata-core.c	2007-11-14 10:08:36.000000000 +0800
+++ 07_more_check/drivers/ata/libata-core.c	2007-12-04 18:03:48.000000000 +0800
@@ -5341,6 +5341,9 @@ static void atapi_pio_bytes(struct ata_q
 	if (do_write != i_write)
 		goto err_out;
 
+	if (!bytes)
+		goto err_out;
+
 	VPRINTK("ata%u: xfering %d bytes\n", ap->print_id, bytes);
 
 	__atapi_pio_bytes(qc, bytes);