Re: [PATCH 20/32] drivers/ata: Stop using 32-bit MSR interfaces

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The change to `wrmsrq()` in `cs5535_set_piomode` introduces a critical sign-extension bug, writing `0xFFFFFFFF` to the upper 32 bits of the MSR.
- [Low] The commit message is incomplete and truncated mid-sentence.
--

commit 9a4c6821b4ef60316ec2b36e0f505f4148809966
Author: Juergen Gross <jgross@suse.com>

drivers/ata: Stop using 32-bit MSR interfaces

The 32-bit MSR interfaces rdmsr() and wrmsr() are planned to be removed.
This patch switches the pata_cs5535 and pata_cs5536 drivers to use the
64-bit variants instead.

>     In drivers/ata/pata_cs5536.c don't redefine rdmsr() and wrmsr() as
>     empty macros for avoiding misuse, but guard the affected code via

[Severity: Low]
This isn't a bug, but the commit message appears to be truncated mid-sentence.

> diff --git a/drivers/ata/pata_cs5535.c b/drivers/ata/pata_cs5535.c
> index d793fc441b467..da98390cc49e8 100644
> --- a/drivers/ata/pata_cs5535.c
> +++ b/drivers/ata/pata_cs5535.c
> @@ -102,16 +102,16 @@ static void cs5535_set_piomode(struct ata_port *ap, struct ata_device *adev)
>  		cmdmode = min(mode, pairmode);
>  		/* Write the other drive timing register if it changed */
>  		if (cmdmode < pairmode)
> -			wrmsr(ATAC_CH0D0_PIO + 2 * pair->devno,
> -				pio_cmd_timings[cmdmode] << 16 | pio_timings[pairmode], 0);
> +			wrmsrq(ATAC_CH0D0_PIO + 2 * pair->devno,
> +				pio_cmd_timings[cmdmode] << 16 | pio_timings[pairmode]);
>  	}
>  	/* Write the drive timing register */
> -	wrmsr(ATAC_CH0D0_PIO + 2 * adev->devno,
> -		pio_cmd_timings[cmdmode] << 16 | pio_timings[mode], 0);
> +	wrmsrq(ATAC_CH0D0_PIO + 2 * adev->devno,
> +		pio_cmd_timings[cmdmode] << 16 | pio_timings[mode]);

[Severity: Critical]
Will these wrmsrq() calls write 0xFFFFFFFF into the upper 32 bits of the
MSR for PIO mode 0?

Because pio_cmd_timings contains u16 values, C integer promotion rules will
promote pio_cmd_timings[cmdmode] to a signed 32-bit int before the shift.

For PIO mode 0 (0xF7F4), the shift results in a negative 32-bit integer
(0xF7F40000).

Since wrmsrq() takes a u64 parameter, this negative 32-bit int is implicitly
sign-extended to 64 bits (0xFFFFFFFFF7F4XXXX). This corrupts the upper
32 bits of the timing registers, which could trigger a General Protection
Fault.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629060526.3638272-1-jgross@suse.com?part=20