Re: [PATCH v3 4/6] ata: libata: Fix FUA handling in ata_build_rw_tf()

["Maciej S. Szmigiero" <mail@maciej.szmigiero.name>]

On 27.10.2022 09:50, Damien Le Moal wrote:
> If a user issues a write command with the FUA bit set for a device with
> NCQ support disabled (that is, the device queue depth was set to 1), the
> LBA 48 command WRITE DMA FUA EXT must be used. However,
> ata_build_rw_tf() ignores this and first test if LBA 28 can be used.
> That is, for small FUA writes at low LBAs, ata_rwcmd_protocol() will
> cause the write to fail.
> 
> Fix this by preventing the use of LBA 28 for any FUA write request.
> While at it, also early test if the request is a FUA read and fail these
> requests for the NCQ-disabled case instead of relying on
> ata_rwcmd_protocol() returning an error.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/ata/libata-core.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 81b20ffb1554..fea06f41f371 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -725,9 +725,21 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
>   		    class == IOPRIO_CLASS_RT)
>   			tf->hob_nsect |= ATA_PRIO_HIGH << ATA_SHIFT_PRIO;
>   	} else if (dev->flags & ATA_DFLAG_LBA) {
> +		bool lba28_ok;
> +
> +		if (tf->flags & ATA_TFLAG_FUA) {
> +			/* FUA reads are not defined */
> +			if (!(tf->flags & ATA_TFLAG_WRITE))
> +				return -EINVAL;
> +			/* We need LBA48 / WRITE DMA FUA EXT for FUA writes */
> +			lba28_ok = false;
> +		} else {
> +			lba28_ok = lba_28_ok(block, n_block);
> +		}
> +

If we are supporting FUA even in the non-NCQ case (at least for writes)
we do *not* need to limit the FUA support to NCQ-supporting drives in
ata_dev_config_fua() anymore.

Having support for ATA_DFLAG_LBA48 and ata_id_has_fua() is enough in
this case.

Limiting the FUA support to NCQ-supporting drives would make sense if NCQ
commands were always used to implement FUA (even with QD set to 1), but
this patch set version does not do that.

Thanks,
Maciej