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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 10/27/22 17:21, Niklas Cassel wrote:
> On Thu, Oct 27, 2022 at 04:50:24PM +0900, 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;
> 
> Hello Damien,
> 
> I'm a bit confused.
> Didn't you write in the other thread that you wanted to force the use of
> NCQ commands, for a drive that supports NCQ, regardless of queue depth?
> Did you change your mind?

Yes. Because the user can always manually set the QD to 1, which turns
off NCQ. I do not want to change that since there may be users out there
relying on this (e.g. with a udev rule) to work around drives with buggy
NCQ support.

> Because as far as I understand, the code after this patch, for a drive
> that has NCQ support, with QD set to > 1, will accept and send down a
> read command with the FUA bit set to the drive.

Yes.

> But the same drive, with QD set to 1, will reject a read command with
> the FUA bit set and propagate that error back to user-space.

Correct. But given that we had fua disabled by default since forever,
the non-ncq read FUA part of the story keeps *not* working, as it did
before. Given that there are no in-kernel FUA read issuer that I can
find, I do not think we are breaking anything, nor are we breaking any
userspace (since that was not working before).

> 
> 
> Kind regards,
> Niklas
> 
>> +			/* 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)) {
>> @@ -742,9 +754,10 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
>>  			tf->hob_lbah = (block >> 40) & 0xff;
>>  			tf->hob_lbam = (block >> 32) & 0xff;
>>  			tf->hob_lbal = (block >> 24) & 0xff;
>> -		} else
>> +		} else {
>>  			/* request too large even for LBA48 */
>>  			return -ERANGE;
>> +		}
>>  
>>  		if (unlikely(!ata_set_rwcmd_protocol(dev, tf)))
>>  			return -EINVAL;
>> -- 
>> 2.37.3

-- 
Damien Le Moal
Western Digital Research