Re: [PATCH] scsi: arm: Convert acornscsi-io.S to C

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] scsi: arm: PTR_ALIGN() rounds up in __acornscsi_in() and __acornscsi_out()
- [High] scsi: arm: wrong MMIO pointer stride in __acornscsi_out() for trailing bytes
--

commit a11c03319840bf581ce6e4031f7b0b04ae3afccc
Author: Breno Rodrigues Alves <breno3011alves@gmail.com>

scsi: arm: Convert acornscsi-io.S to C

This commit replaces the hand-written ARM assembly routines __acornscsi_in()
and __acornscsi_out() in drivers/scsi/arm/acornscsi-io.S with C
implementations in drivers/scsi/arm/acornscsi-io.c.

> diff --git a/drivers/scsi/arm/acornscsi-io.c b/drivers/scsi/arm/acornscsi-io.c
> new file mode 100644
> index 0000000000000..ad16d984d513a
> --- /dev/null
> +++ b/drivers/scsi/arm/acornscsi-io.c
> @@ -0,0 +1,168 @@

[ ... ]

> +/*
> + * Transfer a block of data from the Acorn SCSI card into memory.
> + *
> + * The card address is rounded down to a 32-bit boundary.  Each
> + * 32-bit read from the card yields one valid 16-bit halfword in
> + * the low half; pairs of card reads are packed into single 32-bit
> + * memory words.
> + */
> +void __acornscsi_in(void __iomem *addr, char *buffer, int length)
> +{
> +	const u32 __iomem *card = PTR_ALIGN((const u32 __iomem *)addr, 4);

[Severity: Critical]
Should __acornscsi_in() and __acornscsi_out() use PTR_ALIGN_DOWN() instead of
PTR_ALIGN() here?

In the removed assembly in drivers/scsi/arm/acornscsi-io.S, both routines
started with:

    bic r0, r0, #3

which clears the lowest two bits of addr and rounds it down to the containing
4-byte boundary, matching the comment above __acornscsi_in().

PTR_ALIGN(addr, 4) expands to ALIGN() and rounds an unaligned address up to
the next 4-byte boundary. In drivers/scsi/arm/acornscsi.c,
acornscsi_data_read() and acornscsi_data_write() pass:

    host->base + (offset << 1)

Whenever start_addr (and therefore offset) in on-card RAM is odd,
(offset << 1) is only 2-byte aligned ((addr & 3) == 2). Rounding up with
PTR_ALIGN() advances card by 2 bytes to the next 32-bit card word instead of
the containing 32-bit card word.

Won't this shift the entire transfer by one 16-bit halfword in card RAM, and
cause a transfer reaching the end of the 4 KiB page window
(offset + this_len == (1 << 12)) to access past the RAM window into
host->base + 0x2000 (SBIC_REGIDX)?

> +	u8 *buf = (u8 *)buffer;

[ ... ]

> +void __acornscsi_out(void __iomem *addr, char *buffer, int length)
> +{
> +	u32 __iomem *card = PTR_ALIGN((u32 __iomem *)addr, 4);
> +	const u8 *buf = (const u8 *)buffer;

[ ... ]

> +	while (length >= 2) {
> +		u16 v = *(const u16 *)buf;
> +		void __iomem *cp = (void __iomem *)card;
> +
> +		writeb(v & 0xff, cp);
> +		writeb(v >> 8, cp + 1);
> +
> +		card = (u32 __iomem *)(cp + 2);
> +		buf += 2;
> +		length -= 2;
> +	}
> +
> +	if (length >= 1)
> +		writeb(*buf, (void __iomem *)card);
> +}

[Severity: High]
When acornscsi_data_write() calls __acornscsi_out() with an odd transfer
length where length % 4 == 3, does the trailing byte get written to the wrong
MMIO offset?

Each 16-bit SCSI data halfword is exposed in the low 16 bits (offsets +0 and
+1) of a 32-bit card word, while the upper 16 bits (offsets +2 and +3) are
not sampled by the hardware. In __acornscsi_in(), the while (length >= 2)
loop advances card++ (4 bytes) so that if (length >= 1) reads the final odd
byte from the next 32-bit card word.

Here in __acornscsi_out(), setting:

    card = (u32 __iomem *)(cp + 2);

only advances card by 2 bytes into the unused upper half of the current
32-bit card word. If length % 4 == 3, the while (length >= 2) loop runs once
and the following if (length >= 1) statement writes the final byte to cp + 2
instead of the low byte of the next 32-bit card word at cp + 4. Should this
advance card by a full 32-bit word (card++) instead?

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