| Message ID | 20221027075026.240017-5-damien.lemoal@opensource.wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve libata support for FUA | expand |
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? 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 >
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
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
On 10/27/22 18:42, Hannes Reinecke wrote: > 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? That definitely would be good for ATA, but potentially restrictive for scsi ? Not sure about NVMe, I have not checked the specs. That said, the only reasonable reason to do an FUA read that I can think of would be a "scrub" like write-and-verify feature. And I do not think that any FS implement their scrub process using FUA. > 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. Well no. Given that there is no FUA read command for the non-ncq case, doing the same as for a write FUA in reverse order (synchronize cache first, then read) would still not necessarily force the drive to access the media because synchronize cache is *not* and "invalidate cache" operation. So we cannot use the block layer either as we potentially would end up lying about the media access part of "FUA". With that in mind, failing the FUA read is a much safer option I think. What we could do given that we now have FUA restricted to NCQ is this: diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 77a7be74e65e..61e449877d8d 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -693,7 +693,7 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block, tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; tf->flags |= tf_flags; - if (ata_ncq_enabled(dev)) { + if (ata_ncq_enabled(dev) || tf->flags & ATA_TFLAG_FUA) { /* yay, NCQ */ if (!lba_48_ok(block, n_block)) return -ERANGE; That is, ignore if NCQ is off and always use NCQ read/write for FUA. I am not a huge fan of this as that would lead to mixing NCQ and non-NCQ commands when the drive QD is set to 1. Not exactly nice, which is why I did not initially modify the patch to do that. However, with this change, we would be fully on par with scsi and can do both read and write FUA with the same semantic, as expected from the user if we declare that we support FUA. This hunk: 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) { Would then not really be needed. Thoughts ? > > Cheers, > > Hannes
On 10/28/22 00:22, Damien Le Moal wrote: > On 10/27/22 18:42, Hannes Reinecke wrote: >> 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? > > That definitely would be good for ATA, but potentially restrictive for > scsi ? Not sure about NVMe, I have not checked the specs. That said, the > only reasonable reason to do an FUA read that I can think of would be a > "scrub" like write-and-verify feature. And I do not think that any FS > implement their scrub process using FUA. > But that was the point. _If_ the block layer never issues a REQ_READ|REQ_FUA we could make this a restriction of the block layer, and would be perfectly fine to error that out in the libata stack, too. So we wouldn't need to worry on how to map that command, seeing that we'll never get it. Cheers, Hannes
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
On 28.10.2022 00:22, Damien Le Moal wrote: > On 10/27/22 18:42, Hannes Reinecke wrote: >> 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? > > That definitely would be good for ATA, but potentially restrictive for > scsi ? Not sure about NVMe, I have not checked the specs. That said, the > only reasonable reason to do an FUA read that I can think of would be a > "scrub" like write-and-verify feature. And I do not think that any FS > implement their scrub process using FUA. > >> 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. > > Well no. Given that there is no FUA read command for the non-ncq case, > doing the same as for a write FUA in reverse order (synchronize cache > first, then read) would still not necessarily force the drive to access > the media because synchronize cache is *not* and "invalidate cache" > operation. So we cannot use the block layer either as we potentially would > end up lying about the media access part of "FUA". With that in mind, > failing the FUA read is a much safer option I think. > > What we could do given that we now have FUA restricted to NCQ is this: > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 77a7be74e65e..61e449877d8d 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -693,7 +693,7 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 > block, u32 n_block, > tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; > tf->flags |= tf_flags; > > - if (ata_ncq_enabled(dev)) { > + if (ata_ncq_enabled(dev) || tf->flags & ATA_TFLAG_FUA) { > /* yay, NCQ */ > if (!lba_48_ok(block, n_block)) > return -ERANGE; > > That is, ignore if NCQ is off and always use NCQ read/write for FUA. > I am not a huge fan of this as that would lead to mixing NCQ and non-NCQ > commands when the drive QD is set to 1. Not exactly nice, which is why I > did not initially modify the patch to do that. > However, with this change, we would be fully on par with scsi and can do > both read and write FUA with the same semantic, as expected from the user > if we declare that we support FUA. > (..) > > Thoughts ? > To be honest, this seems like much cleaner solution to me - as you say, it makes libata consistent with SCSI. In this case, obviously the FUA support still needs to be dependent on NCQ support. Thanks, Maciej
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)) { @@ -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;
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(-)