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?
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.
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.
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
>
