[PATCH] ata: pata_ep93xx: fix incorrect return value in data_xfer

[PATCH] ata: pata_ep93xx: fix incorrect return value in data_xfer

[Rosen Penev]

The while (words--) loop post-decrements words, so it underflows from
0 to UINT_MAX on exit.  The function then returns "words << 1", which
is either 0 (for odd-length transfers after words++) or 0xFFFFFFFE
(for even-length transfers).  Callers like __atapi_pio_bytes() depend
on the correct byte count to manage scatter-gather progress; a wrong
value causes ATAPI PIO transfer errors.

Fix by returning buflen instead of the corrupted words counter, matching
what ata_sff_data_xfer() does.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_ep93xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
index 192f75636d18..5b79a7a019ce 100644
--- a/drivers/ata/pata_ep93xx.c
+++ b/drivers/ata/pata_ep93xx.c
@@ -506,7 +506,7 @@ static unsigned int ep93xx_pata_data_xfer(struct ata_queued_cmd *qc,
 		words++;
 	}
 
-	return words << 1;
+	return buflen;
 }
 
 /* Note: original code is ata_devchk */
-- 
2.54.0

Re: [PATCH] ata: pata_ep93xx: fix incorrect return value in data_xfer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The patch returns `buflen` instead of the actual number of bytes consumed from the device hardware, causing ATAPI PIO transfers with odd byte counts to fail.
- [Low] The `words++` statement is left behind as dead code.

Pre-existing issues:
- [High] The fallback path for odd-length trailing bytes incorrectly assigns 16-bit values to an 8-bit pointer, breaking big-endian systems due to endianness swapping and data truncation.
--

commit a4f8c7ea905bbd7b345d74a2225d1a93448d03c1
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: fix incorrect return value in data_xfer

This patch modifies ep93xx_pata_data_xfer() to return buflen instead of the
shifted words counter. It aims to fix an underflow from 0 to UINT_MAX on exit
when managing scatter-gather progress for ATAPI PIO transfers.

> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
[ ... ]
>  	/* Transfer trailing 1 byte, if any. */
>  	if (unlikely(buflen & 0x01)) {
>  		unsigned char pad[2] = { };
>  
>  		buf += buflen - 1;
>  
>  		if (rw == READ) {
>  			*pad = cpu_to_le16(
>  				ep93xx_pata_read_data(
>  					drv_data, IDECTRL_ADDR_DATA));

[Severity: High]
This is a pre-existing issue, but does assigning the 16-bit result of
cpu_to_le16() to the 8-bit *pad cause data truncation and endianness bugs
on big-endian systems? 

It looks like the fallback path for odd-length trailing bytes might need a
cast to extract the correct byte.

>  			*buf = pad[0];
>  		} else {
>  			pad[0] = *buf;
>  			ep93xx_pata_write_data(drv_data, le16_to_cpu(*pad),
>  					  IDECTRL_ADDR_DATA);
>  		}
>  		words++;

[Severity: Low]
This isn't a regression, but since the return statement below no longer uses
the words variable, does this words++ statement become dead code?

>  	}
>  
> -	return words << 1;
> +	return buflen;

[Severity: High]
Does returning the unrounded buflen instead of the actual number of bytes
consumed from the hardware cause a regression where __atapi_pio_bytes()
under-decrements its remaining byte count?

If buflen is an odd number, returning buflen instead of the rounded hardware
byte count could trigger an extra unintended read cycle and subsequent HSM
errors for ATAPI PIO transfers. Should this return the consumed bytes rounded
to the hardware transfer size instead?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601192109.21262-1-rosenp@gmail.com?part=1