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@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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
