[PATCH v3 0/6] Improve libata support for FUA

[PATCH v3 0/6] Improve libata support for FUA

[Damien Le Moal]

These patches cleanup and improve libata support for the FUA device
feature. Patch 6 enables FUA support by default for any drive that
reports supporting the feature as well as NCQ.

Changes from v2:
 - Added patch 1 and 2 as preparatory patches
 - Added patch 4 to fix FUA writes handling for the non-ncq case. Note
   that it is possible that the drives blacklisted in patch 5 are
   actually OK since the code back in 2012 had the issue with the wrong
   use of LBA 28 commands for FUA writes.

Changes from v1:
 - Removed Maciej's patch 2. Instead, blacklist drives which are known
   to have a buggy FUA support.

Damien Le Moal (6):
  ata: libata: Introduce ata_ncq_supported()
  ata: libata: Rename and cleanup ata_rwcmd_protocol()
  ata: libata: cleanup fua handling
  ata: libata: Fix FUA handling in ata_build_rw_tf()
  ata: libata: blacklist FUA support for known buggy drives
  ata: libata: Enable fua support by default

 .../admin-guide/kernel-parameters.txt         |  3 +
 drivers/ata/libata-core.c                     | 80 +++++++++++++++----
 drivers/ata/libata-scsi.c                     | 30 +------
 include/linux/libata.h                        | 34 +++++---
 4 files changed, 93 insertions(+), 54 deletions(-)

-- 
2.37.3

[PATCH v3 1/6] ata: libata: Introduce ata_ncq_supported()

[Damien Le Moal]

Introduce the inline helper function ata_ncq_supported() to test if a
device supports NCQ commands. The function ata_ncq_enabled() is also
rewritten using this new helper function.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 include/linux/libata.h | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index af4953b95f76..58651f565b36 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1690,21 +1690,35 @@ extern struct ata_device *ata_dev_next(struct ata_device *dev,
 	     (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))
 
 /**
- *	ata_ncq_enabled - Test whether NCQ is enabled
- *	@dev: ATA device to test for
+ *	ata_ncq_supported - Test whether NCQ is supported
+ *	@dev: ATA device to test
  *
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
  *
  *	RETURNS:
- *	1 if NCQ is enabled for @dev, 0 otherwise.
+ *	true if @dev supports NCQ, false otherwise.
  */
-static inline int ata_ncq_enabled(struct ata_device *dev)
+static inline bool ata_ncq_supported(struct ata_device *dev)
 {
 	if (!IS_ENABLED(CONFIG_SATA_HOST))
 		return 0;
-	return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
-			      ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ;
+	return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ;
+}
+
+/**
+ *	ata_ncq_enabled - Test whether NCQ is enabled
+ *	@dev: ATA device to test
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host lock)
+ *
+ *	RETURNS:
+ *	true if NCQ is enabled for @dev, false otherwise.
+ */
+static inline bool ata_ncq_enabled(struct ata_device *dev)
+{
+	return ata_ncq_supported(dev) && !(dev->flags & ATA_DFLAG_NCQ_OFF);
 }
 
 static inline bool ata_fpdma_dsm_supported(struct ata_device *dev)
-- 
2.37.3

[PATCH v3 2/6] ata: libata: Rename and cleanup ata_rwcmd_protocol()

[Damien Le Moal]

Rename ata_rwcmd_protocol() to ata_set_rwcmd_protocol() to better
reflect the fact that this function sets a task file command and
protocol. The arguments order is also reversed and the function return
type changed to a bool to indicate if the command and protocol were set
corretly (instead of returning a completely arbitrary "-1" value.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/ata/libata-core.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 884ae73b11ea..56ddcbaa0c6f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -574,17 +574,18 @@ static const u8 ata_rw_cmds[] = {
 };
 
 /**
- *	ata_rwcmd_protocol - set taskfile r/w commands and protocol
+ *	ata_set_rwcmd_protocol - set taskfile r/w command and protocol
+ *	@dev: target device for the tf
  *	@tf: command to examine and configure
- *	@dev: device tf belongs to
  *
- *	Examine the device configuration and tf->flags to calculate
- *	the proper read/write commands and protocol to use.
+ *	Examine the device configuration and tf->flags to determine
+ *	the proper read/write command and protocol to use.
  *
  *	LOCKING:
  *	caller.
  */
-static int ata_rwcmd_protocol(struct ata_taskfile *tf, struct ata_device *dev)
+static bool ata_set_rwcmd_protocol(struct ata_device *dev,
+				   struct ata_taskfile *tf)
 {
 	u8 cmd;
 
@@ -607,11 +608,12 @@ static int ata_rwcmd_protocol(struct ata_taskfile *tf, struct ata_device *dev)
 	}
 
 	cmd = ata_rw_cmds[index + fua + lba48 + write];
-	if (cmd) {
-		tf->command = cmd;
-		return 0;
-	}
-	return -1;
+	if (!cmd)
+		return false;
+
+	tf->command = cmd;
+
+	return true;
 }
 
 /**
@@ -744,7 +746,7 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
 			/* request too large even for LBA48 */
 			return -ERANGE;
 
-		if (unlikely(ata_rwcmd_protocol(tf, dev) < 0))
+		if (unlikely(!ata_set_rwcmd_protocol(dev, tf)))
 			return -EINVAL;
 
 		tf->nsect = n_block & 0xff;
@@ -762,7 +764,7 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
 		if (!lba_28_ok(block, n_block))
 			return -ERANGE;
 
-		if (unlikely(ata_rwcmd_protocol(tf, dev) < 0))
+		if (unlikely(!ata_set_rwcmd_protocol(dev, tf)))
 			return -EINVAL;
 
 		/* Convert LBA to CHS */
-- 
2.37.3

[PATCH v3 3/6] ata: libata: cleanup fua handling

[Damien Le Moal]

Move the detection of a device FUA support from
ata_scsiop_mode_sense()/ata_dev_supports_fua() to device scan time in
ata_dev_configure().

The function ata_dev_config_fua() is introduced to detect a device FUA
support and this support is indicated using the new device flag
ATA_DFLAG_FUA. In order to blacklist known buggy devices, the horkage
flag ATA_HORKAGE_NO_FUA is introduced. Similarly to other horkage flags,
the libata.force= arguments "fua" and "nofua" are also introduced to
allow a user to control this horkage flag through the "force" libata
module parameter.

The ATA_DFLAG_FUA device flag is set only and only if all the following
conditions are met:
* libata.fua module parameter is set to 1
* The device advertizes support for the WRITE DMA FUA EXT command,
* The device is not marked with the ATA_HORKAGE_NO_FUA flag, either from
  the blacklist or set by the user with libata.force=nofua
* The device supports NCQ (while this is not mandated by the standards,
  this restriction is introduced to avoid problems with older non-NCQ
  devices).

Note: Enabling or diabling libata fua support for all devices by default
can now by done using either the "fua" module parameter or the
"force=[port[.device]][no]fua" module parameter when libata.fua==1.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 .../admin-guide/kernel-parameters.txt         |  3 ++
 drivers/ata/libata-core.c                     | 30 ++++++++++++++++++-
 drivers/ata/libata-scsi.c                     | 30 ++-----------------
 include/linux/libata.h                        |  8 +++--
 4 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a465d5242774..f9724642c703 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2786,6 +2786,9 @@
 			* [no]setxfer: Indicate if transfer speed mode setting
 			  should be skipped.
 
+			* [no]fua: Disable or enable FUA (Force Unit Access)
+			  support for devices supporting this feature.
+
 			* dump_id: Dump IDENTIFY data.
 
 			* disable: Disable this device.
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 56ddcbaa0c6f..81b20ffb1554 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2422,6 +2422,28 @@ static void ata_dev_config_chs(struct ata_device *dev)
 			     dev->heads, dev->sectors);
 }
 
+static void ata_dev_config_fua(struct ata_device *dev)
+{
+	/* Ignore FUA support if its use is disabled globally */
+	if (!libata_fua)
+		goto nofua;
+
+	/* Ignore devices without support for WRITE DMA FUA EXT */
+	if (!(dev->flags & ATA_DFLAG_LBA48) || !ata_id_has_fua(dev->id))
+		goto nofua;
+
+	/* Ignore known bad devices and devices that lack NCQ support */
+	if (!ata_ncq_supported(dev) || (dev->horkage & ATA_HORKAGE_NO_FUA))
+		goto nofua;
+
+	dev->flags |= ATA_DFLAG_FUA;
+
+	return;
+
+nofua:
+	dev->flags &= ~ATA_DFLAG_FUA;
+}
+
 static void ata_dev_config_devslp(struct ata_device *dev)
 {
 	u8 *sata_setting = dev->link->ap->sector_buf;
@@ -2510,7 +2532,8 @@ static void ata_dev_print_features(struct ata_device *dev)
 		return;
 
 	ata_dev_info(dev,
-		     "Features:%s%s%s%s%s%s\n",
+		     "Features:%s%s%s%s%s%s%s\n",
+		     dev->flags & ATA_DFLAG_FUA ? " FUA" : "",
 		     dev->flags & ATA_DFLAG_TRUSTED ? " Trust" : "",
 		     dev->flags & ATA_DFLAG_DA ? " Dev-Attention" : "",
 		     dev->flags & ATA_DFLAG_DEVSLP ? " Dev-Sleep" : "",
@@ -2671,6 +2694,7 @@ int ata_dev_configure(struct ata_device *dev)
 			ata_dev_config_chs(dev);
 		}
 
+		ata_dev_config_fua(dev);
 		ata_dev_config_devslp(dev);
 		ata_dev_config_sense_reporting(dev);
 		ata_dev_config_zac(dev);
@@ -4105,6 +4129,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	 */
 	{ "SATADOM-ML 3ME",		NULL,	ATA_HORKAGE_NO_LOG_DIR },
 
+	/* Buggy FUA */
+	{ "Maxtor",		"BANC1G10",	ATA_HORKAGE_NO_FUA },
+
 	/* End Marker */
 	{ }
 };
@@ -6216,6 +6243,7 @@ static const struct ata_force_param force_tbl[] __initconst = {
 	force_horkage_onoff(lpm,	ATA_HORKAGE_NOLPM),
 	force_horkage_onoff(setxfer,	ATA_HORKAGE_NOSETXFER),
 	force_horkage_on(dump_id,	ATA_HORKAGE_DUMP_ID),
+	force_horkage_onoff(fua,	ATA_HORKAGE_NO_FUA),
 
 	force_horkage_on(disable,	ATA_HORKAGE_DISABLE),
 };
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4cb914103382..69948e2a8f6d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2240,30 +2240,6 @@ static unsigned int ata_msense_rw_recovery(u8 *buf, bool changeable)
 	return sizeof(def_rw_recovery_mpage);
 }
 
-/*
- * We can turn this into a real blacklist if it's needed, for now just
- * blacklist any Maxtor BANC1G10 revision firmware
- */
-static int ata_dev_supports_fua(u16 *id)
-{
-	unsigned char model[ATA_ID_PROD_LEN + 1], fw[ATA_ID_FW_REV_LEN + 1];
-
-	if (!libata_fua)
-		return 0;
-	if (!ata_id_has_fua(id))
-		return 0;
-
-	ata_id_c_string(id, model, ATA_ID_PROD, sizeof(model));
-	ata_id_c_string(id, fw, ATA_ID_FW_REV, sizeof(fw));
-
-	if (strcmp(model, "Maxtor"))
-		return 1;
-	if (strcmp(fw, "BANC1G10"))
-		return 1;
-
-	return 0; /* blacklisted */
-}
-
 /**
  *	ata_scsiop_mode_sense - Simulate MODE SENSE 6, 10 commands
  *	@args: device IDENTIFY data / SCSI command of interest.
@@ -2287,7 +2263,7 @@ static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf)
 	};
 	u8 pg, spg;
 	unsigned int ebd, page_control, six_byte;
-	u8 dpofua, bp = 0xff;
+	u8 dpofua = 0, bp = 0xff;
 	u16 fp;
 
 	six_byte = (scsicmd[0] == MODE_SENSE);
@@ -2350,9 +2326,7 @@ static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf)
 		goto invalid_fld;
 	}
 
-	dpofua = 0;
-	if (ata_dev_supports_fua(args->id) && (dev->flags & ATA_DFLAG_LBA48) &&
-	    (!(dev->flags & ATA_DFLAG_PIO) || dev->multi_count))
+	if (dev->flags & ATA_DFLAG_FUA)
 		dpofua = 1 << 4;
 
 	if (six_byte) {
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 58651f565b36..d30c1288504d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -91,6 +91,7 @@ enum {
 	ATA_DFLAG_AN		= (1 << 7), /* AN configured */
 	ATA_DFLAG_TRUSTED	= (1 << 8), /* device supports trusted send/recv */
 	ATA_DFLAG_DMADIR	= (1 << 10), /* device requires DMADIR */
+	ATA_DFLAG_FUA		= (1 << 11), /* device supports FUA */
 	ATA_DFLAG_CFG_MASK	= (1 << 12) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 12), /* device limited to PIO mode */
@@ -113,9 +114,9 @@ enum {
 	ATA_DFLAG_D_SENSE	= (1 << 29), /* Descriptor sense requested */
 	ATA_DFLAG_ZAC		= (1 << 30), /* ZAC device */
 
-	ATA_DFLAG_FEATURES_MASK	= ATA_DFLAG_TRUSTED | ATA_DFLAG_DA | \
-				  ATA_DFLAG_DEVSLP | ATA_DFLAG_NCQ_SEND_RECV | \
-				  ATA_DFLAG_NCQ_PRIO,
+	ATA_DFLAG_FEATURES_MASK	= (ATA_DFLAG_TRUSTED | ATA_DFLAG_DA |	\
+				   ATA_DFLAG_DEVSLP | ATA_DFLAG_NCQ_SEND_RECV | \
+				   ATA_DFLAG_NCQ_PRIO | ATA_DFLAG_FUA),
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
@@ -381,6 +382,7 @@ enum {
 	ATA_HORKAGE_NO_NCQ_ON_ATI = (1 << 27),	/* Disable NCQ on ATI chipset */
 	ATA_HORKAGE_NO_ID_DEV_LOG = (1 << 28),	/* Identify device log missing */
 	ATA_HORKAGE_NO_LOG_DIR	= (1 << 29),	/* Do not read log directory */
+	ATA_HORKAGE_NO_FUA	= (1 << 30),	/* Do not use FUA */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */
-- 
2.37.3

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

[Damien Le Moal]

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)) {
@@ -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

[PATCH v3 5/6] ata: libata: blacklist FUA support for known buggy drives

[Damien Le Moal]

Thread [1] reported back in 2012 problems with enabling FUA for 3
different drives. Add these drives to ata_device_blacklist[] to mark
them with the ATA_HORKAGE_NO_FUA flag. To be conservative and avoid
problems on old systems, the model number for the three new entries
are defined as to widely match all drives in the same product line.

[1]: https://lore.kernel.org/lkml/CA+6av4=uxu_q5U_46HtpUt=FSgbh3pZuAEY54J5_xK=MKWq-YQ@mail.gmail.com/

Suggested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 drivers/ata/libata-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index fea06f41f371..ec732c47b6fb 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4144,6 +4144,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 
 	/* Buggy FUA */
 	{ "Maxtor",		"BANC1G10",	ATA_HORKAGE_NO_FUA },
+	{ "WDC*WD2500J*",	NULL,		ATA_HORKAGE_NO_FUA },
+	{ "OCZ-VERTEX*",	NULL,		ATA_HORKAGE_NO_FUA },
+	{ "INTEL*SSDSC2CT*",	NULL,		ATA_HORKAGE_NO_FUA },
 
 	/* End Marker */
 	{ }
-- 
2.37.3

[PATCH v3 6/6] ata: libata: Enable fua support by default

[Damien Le Moal]

Change the default value of the fua module parameter to 1 to enable fua
support by default for all devices supporting it.

FUA support can be disabled for individual drives using the
force=[ID]nofua libata module argument.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 drivers/ata/libata-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ec732c47b6fb..77a7be74e65e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -127,9 +127,9 @@ int atapi_passthru16 = 1;
 module_param(atapi_passthru16, int, 0444);
 MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
 
-int libata_fua = 0;
+int libata_fua = 1;
 module_param_named(fua, libata_fua, int, 0444);
-MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
+MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");
 
 static int ata_ignore_hpa;
 module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);
-- 
2.37.3

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

[Niklas Cassel]

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
> 

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

[Damien Le Moal]

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

Re: [PATCH v3 1/6] ata: libata: Introduce ata_ncq_supported()

[Hannes Reinecke]

On 10/27/22 09:50, Damien Le Moal wrote:
> Introduce the inline helper function ata_ncq_supported() to test if a
> device supports NCQ commands. The function ata_ncq_enabled() is also
> rewritten using this new helper function.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   include/linux/libata.h | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH v3 2/6] ata: libata: Rename and cleanup ata_rwcmd_protocol()

[Hannes Reinecke]

On 10/27/22 09:50, Damien Le Moal wrote:
> Rename ata_rwcmd_protocol() to ata_set_rwcmd_protocol() to better
> reflect the fact that this function sets a task file command and
> protocol. The arguments order is also reversed and the function return
> type changed to a bool to indicate if the command and protocol were set
> corretly (instead of returning a completely arbitrary "-1" value.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/ata/libata-core.c | 26 ++++++++++++++------------
>   1 file changed, 14 insertions(+), 12 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH v3 3/6] ata: libata: cleanup fua handling

[Hannes Reinecke]

On 10/27/22 09:50, Damien Le Moal wrote:
> Move the detection of a device FUA support from
> ata_scsiop_mode_sense()/ata_dev_supports_fua() to device scan time in
> ata_dev_configure().
> 
> The function ata_dev_config_fua() is introduced to detect a device FUA
> support and this support is indicated using the new device flag

'detect a device FUA support'?
maybe 'to detect if a device supports FUA'?

> ATA_DFLAG_FUA. In order to blacklist known buggy devices, the horkage
> flag ATA_HORKAGE_NO_FUA is introduced. Similarly to other horkage flags,
> the libata.force= arguments "fua" and "nofua" are also introduced to
> allow a user to control this horkage flag through the "force" libata
> module parameter.
> 
> The ATA_DFLAG_FUA device flag is set only and only if all the following
> conditions are met:
> * libata.fua module parameter is set to 1
> * The device advertizes support for the WRITE DMA FUA EXT command,
> * The device is not marked with the ATA_HORKAGE_NO_FUA flag, either from
>    the blacklist or set by the user with libata.force=nofua
> * The device supports NCQ (while this is not mandated by the standards,
>    this restriction is introduced to avoid problems with older non-NCQ
>    devices).
> 
> Note: Enabling or diabling libata fua support for all devices by default
> can now by done using either the "fua" module parameter or the
> "force=[port[.device]][no]fua" module parameter when libata.fua==1.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   .../admin-guide/kernel-parameters.txt         |  3 ++
>   drivers/ata/libata-core.c                     | 30 ++++++++++++++++++-
>   drivers/ata/libata-scsi.c                     | 30 ++-----------------
>   include/linux/libata.h                        |  8 +++--
>   4 files changed, 39 insertions(+), 32 deletions(-)
> Other than that:

Reviewed-by: Hannes Reinecke <hare@suse.de>

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

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

[Hannes Reinecke]

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

Re: [PATCH v3 2/6] ata: libata: Rename and cleanup ata_rwcmd_protocol()

[Sergei Shtylyov]

Hello!

On 10/27/22 10:50 AM, Damien Le Moal wrote:

> Rename ata_rwcmd_protocol() to ata_set_rwcmd_protocol() to better
> reflect the fact that this function sets a task file command and
> protocol. The arguments order is also reversed and the function return
> type changed to a bool to indicate if the command and protocol were set
> corretly (instead of returning a completely arbitrary "-1" value.

   Correctly. :-)

> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/ata/libata-core.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 884ae73b11ea..56ddcbaa0c6f 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -574,17 +574,18 @@ static const u8 ata_rw_cmds[] = {
>  };
>  
>  /**
> - *	ata_rwcmd_protocol - set taskfile r/w commands and protocol
> + *	ata_set_rwcmd_protocol - set taskfile r/w command and protocol
> + *	@dev: target device for the tf

   s/tf/taskfile/?

>   *	@tf: command to examine and configure
> - *	@dev: device tf belongs to
[...]

MBR, Sergey

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

[Damien Le Moal]

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

-- 
Damien Le Moal
Western Digital Research

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

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

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

[Maciej S. Szmigiero]

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

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

[Maciej S. Szmigiero]

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