Re: [PATCH v2 04/13] drivers/ata: Stop using 32-bit MSR interfaces

[David Laight]

On Wed, 19 Aug 2026 12:23:05 +0200
Juergen Gross <jgross@xxxxxxxx> wrote:

> The 32-bit MSR interfaces rdmsr() and wrmsr() are planned to be
> removed. Use the related 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
> a simple #ifdef.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V2:
> - avoid sign extension when using wrmsrq() (Sashiko bot)
> ---
>  drivers/ata/pata_cs5535.c | 20 ++++++++++----------
>  drivers/ata/pata_cs5536.c | 17 ++++++++---------
>  2 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ata/pata_cs5535.c b/drivers/ata/pata_cs5535.c
> index d793fc441b46..e16c7f4c7c27 100644
> --- a/drivers/ata/pata_cs5535.c
> +++ b/drivers/ata/pata_cs5535.c
> @@ -90,7 +90,7 @@ static void cs5535_set_piomode(struct ata_port *ap, struct ata_device *adev)
>  	static const u16 pio_cmd_timings[5] = {
>  		0xF7F4, 0x53F3, 0x13F1, 0x5131, 0x1131
>  	};
> -	u32 reg, __maybe_unused dummy;
> +	u32 reg;
>  	struct ata_device *pair = ata_dev_pair(adev);
>  
>  	int mode = adev->pio_mode - XFER_PIO_0;
> @@ -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,
> +				(u32)pio_cmd_timings[cmdmode] << 16 | pio_timings[pairmode]);

That cast is horribly subtle.
Perhaps change the array to be u32 and shift the values in the array initialiser?

David

>  	}
>  	/* 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,
> +		(u32)pio_cmd_timings[cmdmode] << 16 | pio_timings[mode]);
>  
>  	/* Set the PIO "format 1" bit in the DMA timing register */
> -	rdmsr(ATAC_CH0D0_DMA + 2 * adev->devno, reg, dummy);
> -	wrmsr(ATAC_CH0D0_DMA + 2 * adev->devno, reg | 0x80000000UL, 0);
> +	rdmsrq(ATAC_CH0D0_DMA + 2 * adev->devno, reg);
> +	wrmsrq(ATAC_CH0D0_DMA + 2 * adev->devno, reg | 0x80000000UL);
>  }
>  
>  /**
> @@ -129,16 +129,16 @@ static void cs5535_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>  	static const u32 mwdma_timings[3] = {
>  		0x7F0FFFF3, 0x7F035352, 0x7F024241
>  	};
> -	u32 reg, __maybe_unused dummy;
> +	u32 reg;
>  	int mode = adev->dma_mode;
>  
> -	rdmsr(ATAC_CH0D0_DMA + 2 * adev->devno, reg, dummy);
> +	rdmsrq(ATAC_CH0D0_DMA + 2 * adev->devno, reg);
>  	reg &= 0x80000000UL;
>  	if (mode >= XFER_UDMA_0)
>  		reg |= udma_timings[mode - XFER_UDMA_0];
>  	else
>  		reg |= mwdma_timings[mode - XFER_MW_DMA_0];
> -	wrmsr(ATAC_CH0D0_DMA + 2 * adev->devno, reg, 0);
> +	wrmsrq(ATAC_CH0D0_DMA + 2 * adev->devno, reg);
>  }
>  
>  static const struct scsi_host_template cs5535_sht = {
> diff --git a/drivers/ata/pata_cs5536.c b/drivers/ata/pata_cs5536.c
> index 73e81e160c91..61d232a82b5e 100644
> --- a/drivers/ata/pata_cs5536.c
> +++ b/drivers/ata/pata_cs5536.c
> @@ -32,11 +32,8 @@
>  static int use_msr;
>  module_param_named(msr, use_msr, int, 0644);
>  MODULE_PARM_DESC(msr, "Force using MSR to configure IDE function (Default: 0)");
> +#define MAYBE_USE_MSR
>  #else
> -#undef rdmsr	/* avoid accidental MSR usage on, e.g. x86-64 */
> -#undef wrmsr
> -#define rdmsr(x, y, z) do { } while (0)
> -#define wrmsr(x, y, z) do { } while (0)
>  #define use_msr 0
>  #endif
>  
> @@ -85,22 +82,24 @@ static const struct dmi_system_id udma_quirk_dmi_table[] = {
>  
>  static int cs5536_read(struct pci_dev *pdev, int reg, u32 *val)
>  {
> +#ifdef MAYBE_USE_MSR
>  	if (unlikely(use_msr)) {
> -		u32 dummy __maybe_unused;
> -
> -		rdmsr(MSR_IDE_CFG + reg, *val, dummy);
> +		rdmsrq(MSR_IDE_CFG + reg, *val);
>  		return 0;
>  	}
> +#endif
>  
>  	return pci_read_config_dword(pdev, PCI_IDE_CFG + reg * 4, val);
>  }
>  
> -static int cs5536_write(struct pci_dev *pdev, int reg, int val)
> +static int cs5536_write(struct pci_dev *pdev, int reg, u32 val)
>  {
> +#ifdef MAYBE_USE_MSR
>  	if (unlikely(use_msr)) {
> -		wrmsr(MSR_IDE_CFG + reg, val, 0);
> +		wrmsrq(MSR_IDE_CFG + reg, val);
>  		return 0;
>  	}
> +#endif
>  
>  	return pci_write_config_dword(pdev, PCI_IDE_CFG + reg * 4, val);
>  }