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

[Hannes Reinecke <hare@suse.de>]

On 10/27/22 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);
> +		}
> +
>   		tf->flags |= ATA_TFLAG_LBA;
>   
> -		if (lba_28_ok(block, n_block)) {
> +		if (lba28_ok) {
>   			/* use LBA28 */
>   			tf->device |= (block >> 24) & 0xf;
>   		} else if (lba_48_ok(block, n_block)) {

I am still skeptical about this change.
Having checked the code I don't think that we ever issue a 
REQ_READ|REQ_FUA; but at the same time there doesn't seem to be a strict 
rule. I wonder if we shouldn't move that check into the block layer, and 
error out any attempts to issue such?

Otherwise we would error out an otherwise fine I/O (which we _could_ 
have handled via PREFLUSH etc semantics), which I don't think is a good 
idea.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman