Re: [PATCH] ata: libata-core: Ignore forced transfer modes the host does not support

[Damien Le Moal <dlemoal@kernel.org>]

On 2026/10/14 14:59, Rand Deeb wrote:
> The libata.force parameter can force a transfer mode (pio0-6, mwdma0-4,
> udma0-7) for a port or device. ata_force_xfermask() applies it in
> ata_do_set_mode() right after ata_dev_xfermask() has limited the device
> masks to what the host supports. It replaces those masks with the forced
> mode without checking the host masks, so a mode the controller does not
> support is passed to the driver's ->set_piomode() or ->set_dmamode().
> 
> Several PATA drivers rely on the host masks to keep the mode number in
> range when indexing their timing tables. ata_piix, pata_efar,
> pata_it8213, pata_oldpiix, pata_radisys and pata_rdc declare ATA_PIO4
> and ATA_MWDMA12_ONLY, index a five-entry PIO timing table with
> pio_mode - XFER_PIO_0, and index a three-entry needed_pio[] table with
> dma_mode - XFER_MW_DMA_0. Booting with libata.force=pio5, pio6, mwdma3
> or mwdma4 makes them read past those tables, use the values read as
> further indices into the timing table, and write the result to the
> controller's timing registers.
> 
> Make ata_force_xfermask() ignore a forced transfer mode that is not in
> the port's PIO, MWDMA or UDMA mask, and warn about it. Forcing a mode the
> host supports still works as before, including a mode that
> ata_dev_xfermask() removed for the device or cable.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 332673257056 ("libata: implement libata.force module parameter")
> Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
> ---
>  drivers/ata/libata-core.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 92233fb25..324aaa81c 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -434,6 +434,7 @@ static void ata_force_link_limits(struct ata_link *link)
>   */
>  static void ata_force_xfermask(struct ata_device *dev)
>  {
> +	struct ata_port *ap = dev->link->ap;
>  	int devno = dev->link->pmp + dev->devno;
>  	int alt_devno = devno;
>  	int i;
> @@ -458,6 +459,17 @@ static void ata_force_xfermask(struct ata_device *dev)
>  
>  		ata_unpack_xfermask(fe->param.xfer_mask,
>  				    &pio_mask, &mwdma_mask, &udma_mask);
> +
> +		/* Never force a transfer mode the host does not support */
> +		if ((pio_mask & ~ap->pio_mask) ||
> +		    (mwdma_mask & ~ap->mwdma_mask) ||
> +		    (udma_mask & ~ap->udma_mask)) {
> +			ata_dev_warn(dev,
> +				     "FORCE: %s not supported by the host, ignored\n",
> +				     fe->param.name);
> +			return;
> +		}

I would rewrite this as:

		/* Make sure that the port supports the specified xfer modes. */
		if (!(pio_mask & ap->pio_mask) &&
		    !(mwdma_mask & ap->mwdma_mask) &&
		    !(udma_mask & ap->udma_mask)) {
			ata_dev_warn(dev,
				"FORCE: ignoring unsupported xfer_mask %s\n",
				fe->param.name);
			continue;
		}

as that is easier to understand. The "continue" here should address Sashiko comment.

> +
>  		if (udma_mask)
>  			dev->udma_mask = udma_mask;
>  		else if (mwdma_mask) {
> 
> base-commit: 4056b3c9d3c92ea45955641dbafeb9b960e146ef


-- 
Damien Le Moal
Western Digital Research