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

[sashiko-bot@kernel.org]

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