[PATCH v6 0/7] Improve libata support for FUA

[PATCH v6 0/7] Improve libata support for FUA

[Damien Le Moal]

These patches cleanup and improve libata support for ATA devices
supporting the FUA feature.

The first patch modifies the block layer to prevent the use of REQ_FUA
with read requests. This is necessary as the block layer code expect
REQ_FUA to be used with write requests (the flush machinery cannot
enforce access to the media for FUA read commands) and FUA is not
supported with ATA devices when NCQ is not enabled (device queue depth
set to 1).

Patch 2 and 3 are libata cleanup preparatory patches. Patch 4 cleans up
the detection for FUA support. Patch 5 fixes building a taskfile for FUA
write requests. Patch 6 prevents the use of FUA with known bad drives.

Finally, patch 7 enables FUA support by default in libata for devices
supporting this features.

Changes from v5:
 - Removed WARN for FUA reads in patch 5.
 - Added reviewed-by tags.

Changes from v4:
 - Changed patch 1 to the one suggested by Christoph.
 - Added Hannes review tag.

Changes from v3:
 - Added patch 1 to prevent any block device user from issuing a
   REQ_FUA read.
 - Changed patch 5 to remove the check for REQ_FUA read and also remove 
   support for ATA_CMD_WRITE_MULTI_FUA_EXT as this command is obsolete
   in recent ACS specifications.

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.

Christoph Hellwig (1):
  block: add a sanity check for non-write flush/fua bios

Damien Le Moal (6):
  ata: libata: Introduce ata_ncq_supported()
  ata: libata: Rename and cleanup ata_rwcmd_protocol()
  ata: libata: cleanup fua support detection
  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 +
 block/blk-core.c                              | 13 ++--
 drivers/ata/libata-core.c                     | 73 ++++++++++++++-----
 drivers/ata/libata-scsi.c                     | 30 +-------
 include/linux/libata.h                        | 34 ++++++---
 5 files changed, 92 insertions(+), 61 deletions(-)

-- 
2.38.1

[PATCH v6 1/7] block: add a sanity check for non-write flush/fua bios

[Damien Le Moal]

From: Christoph Hellwig <hch@infradead.org>

Check that the PREFUSH and FUA flags are only set on write bios,
given that the flush state machine expects that.

Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 block/blk-core.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 17667159482e..d3446d38ba77 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -730,12 +730,15 @@ void submit_bio_noacct(struct bio *bio)
 	 * Filter flush bio's early so that bio based drivers without flush
 	 * support don't have to worry about them.
 	 */
-	if (op_is_flush(bio->bi_opf) &&
-	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
-		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
-		if (!bio_sectors(bio)) {
-			status = BLK_STS_OK;
+	if (op_is_flush(bio->bi_opf)) {
+		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE))
 			goto end_io;
+		if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
+			bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
+			if (!bio_sectors(bio)) {
+				status = BLK_STS_OK;
+				goto end_io;
+			}
 		}
 	}
 
-- 
2.38.1

[PATCH v6 2/7] 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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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.38.1

[PATCH v6 3/7] 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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libata-core.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 884ae73b11ea..6ee1cbac3ab0 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
- *	@tf: command to examine and configure
- *	@dev: device tf belongs to
+ *	ata_set_rwcmd_protocol - set taskfile r/w command and protocol
+ *	@dev: target device for the taskfile
+ *	@tf: taskfile to examine and configure
  *
- *	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 for @tf.
  *
  *	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.38.1

[PATCH v6 4/7] ata: libata: cleanup fua support detection

[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 if a device
supports FUA 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 supports 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).

Enabling or diabling libata FUA support for all devices can now also be
done using the "force=[no]fua" module parameter when libata.fua is set
to 1.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 .../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 6ee1cbac3ab0..30adae16efff 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.38.1

[PATCH v6 5/7] 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 tests if LBA 28 can be used
based on the write command sector and number of blocks. 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.

Given that the WRITE MULTI FUA EXT command is marked as obsolete in the
ATA specification since ACS-3 (published in 2013), remove the
ATA_CMD_WRITE_MULTI_FUA_EXT command from the ata_rw_cmds array.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 30adae16efff..2b66fe529d81 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -552,7 +552,7 @@ static const u8 ata_rw_cmds[] = {
 	0,
 	0,
 	0,
-	ATA_CMD_WRITE_MULTI_FUA_EXT,
+	0,
 	/* pio */
 	ATA_CMD_PIO_READ,
 	ATA_CMD_PIO_WRITE,
@@ -727,7 +727,8 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
 	} else if (dev->flags & ATA_DFLAG_LBA) {
 		tf->flags |= ATA_TFLAG_LBA;
 
-		if (lba_28_ok(block, n_block)) {
+		/* We need LBA48 for FUA writes */
+		if (!(tf->flags & ATA_TFLAG_FUA) && lba_28_ok(block, n_block)) {
 			/* use LBA28 */
 			tf->device |= (block >> 24) & 0xf;
 		} else if (lba_48_ok(block, n_block)) {
@@ -742,9 +743,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.38.1

[PATCH v6 6/7] 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>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 2b66fe529d81..97ade977b830 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4133,6 +4133,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.38.1

[PATCH v6 7/7] 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>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.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 97ade977b830..2967671131d2 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.38.1

Re: [PATCH v6 5/7] ata: libata: Fix FUA handling in ata_build_rw_tf()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v6 1/7] block: add a sanity check for non-write flush/fua bios

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v6 2/7] ata: libata: Introduce ata_ncq_supported()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v6 3/7] ata: libata: Rename and cleanup ata_rwcmd_protocol()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v6 4/7] ata: libata: cleanup fua support detection

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v6 5/7] ata: libata: Fix FUA handling in ata_build_rw_tf()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v6 6/7] ata: libata: blacklist FUA support for known buggy drives

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v6 7/7] ata: libata: Enable fua support by default

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v6 0/7] Improve libata support for FUA

[Maciej S. Szmigiero]

On 8.11.2022 06:55, Damien Le Moal wrote:
> These patches cleanup and improve libata support for ATA devices
> supporting the FUA feature.
> 
> The first patch modifies the block layer to prevent the use of REQ_FUA
> with read requests. This is necessary as the block layer code expect
> REQ_FUA to be used with write requests (the flush machinery cannot
> enforce access to the media for FUA read commands) and FUA is not
> supported with ATA devices when NCQ is not enabled (device queue depth
> set to 1).
> 
> Patch 2 and 3 are libata cleanup preparatory patches. Patch 4 cleans up
> the detection for FUA support. Patch 5 fixes building a taskfile for FUA
> write requests. Patch 6 prevents the use of FUA with known bad drives.
> 
> Finally, patch 7 enables FUA support by default in libata for devices
> supporting this features.
> 
> Changes from v5:
>   - Removed WARN for FUA reads in patch 5.
>   - Added reviewed-by tags.
> 
> Changes from v4:
>   - Changed patch 1 to the one suggested by Christoph.
>   - Added Hannes review tag.
> 
> Changes from v3:
>   - Added patch 1 to prevent any block device user from issuing a
>     REQ_FUA read.
>   - Changed patch 5 to remove the check for REQ_FUA read and also remove
>     support for ATA_CMD_WRITE_MULTI_FUA_EXT as this command is obsolete
>     in recent ACS specifications.
> 
> 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.
> 
> Christoph Hellwig (1):
>    block: add a sanity check for non-write flush/fua bios
> 
> Damien Le Moal (6):
>    ata: libata: Introduce ata_ncq_supported()
>    ata: libata: Rename and cleanup ata_rwcmd_protocol()
>    ata: libata: cleanup fua support detection
>    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
> 

Now that 6.2-rc1 is out can this patch set be merged?

Thanks,
Maciej

Re: [PATCH v6 1/7] block: add a sanity check for non-write flush/fua bios

[Niklas Cassel]

On Tue, Nov 08, 2022 at 02:55:38PM +0900, Damien Le Moal wrote:
> From: Christoph Hellwig <hch@infradead.org>
>
> Check that the PREFUSH and FUA flags are only set on write bios,
> given that the flush state machine expects that.
>
> Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
>  block/blk-core.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 17667159482e..d3446d38ba77 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -730,12 +730,15 @@ void submit_bio_noacct(struct bio *bio)
>	 * Filter flush bio's early so that bio based drivers without flush
>	 * support don't have to worry about them.
>	 */
> -	if (op_is_flush(bio->bi_opf) &&
> -	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
> -		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
> -		if (!bio_sectors(bio)) {
> -			status = BLK_STS_OK;
> +	if (op_is_flush(bio->bi_opf)) {
> +		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE))
>			goto end_io;

Considering that e.g. zonefs does:

	bio = bio_alloc(bdev, nr_pages,
			REQ_OP_ZONE_APPEND | REQ_SYNC | REQ_IDLE, GFP_NOFS);
	...
	if (iocb_is_dsync(iocb))
		bio->bi_opf |= REQ_FUA;

It seems that we need to either allow REQ_OP_ZONE_APPEND explicitly here,
or perhaps make use of something like op_is_write().


Kind regards,
Niklas

Re: [PATCH v6 2/7] ata: libata: Introduce ata_ncq_supported()

[Niklas Cassel]

On Tue, Nov 08, 2022 at 02:55:39PM +0900, 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>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  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;

Since you changed the return type to bool, and the function comment says
that you should return "false otherwise", perhaps change "return 0" to
"return false".

(Yes, they are technically the same, but it still makes me double check the
function's return type every time I see a "return 0" where I expected a bool,
since perhaps I was reading too quickly and overlooked something.)

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

With the small nit:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v6 3/7] ata: libata: Rename and cleanup ata_rwcmd_protocol()

[Niklas Cassel]

On Tue, Nov 08, 2022 at 02:55:40PM +0900, 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>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/ata/libata-core.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 884ae73b11ea..6ee1cbac3ab0 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
> - *	@tf: command to examine and configure
> - *	@dev: device tf belongs to
> + *	ata_set_rwcmd_protocol - set taskfile r/w command and protocol
> + *	@dev: target device for the taskfile
> + *	@tf: taskfile to examine and configure
>   *
> - *	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 for @tf.
>   *
>   *	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.38.1
> 

With or without small typo fixed:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v6 4/7] ata: libata: cleanup fua support detection

[Niklas Cassel]

On Tue, Nov 08, 2022 at 02:55:41PM +0900, 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 if a device
> supports FUA 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 supports 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).
> 
> Enabling or diabling libata FUA support for all devices can now also be
> done using the "force=[no]fua" module parameter when libata.fua is set
> to 1.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  .../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 6ee1cbac3ab0..30adae16efff 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.38.1
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v6 5/7] ata: libata: Fix FUA handling in ata_build_rw_tf()

[Niklas Cassel]

On Tue, Nov 08, 2022 at 02:55:42PM +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 tests if LBA 28 can be used
> based on the write command sector and number of blocks. 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.
> 
> Given that the WRITE MULTI FUA EXT command is marked as obsolete in the
> ATA specification since ACS-3 (published in 2013), remove the
> ATA_CMD_WRITE_MULTI_FUA_EXT command from the ata_rw_cmds array.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/ata/libata-core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 30adae16efff..2b66fe529d81 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -552,7 +552,7 @@ static const u8 ata_rw_cmds[] = {
>  	0,
>  	0,
>  	0,
> -	ATA_CMD_WRITE_MULTI_FUA_EXT,
> +	0,
>  	/* pio */
>  	ATA_CMD_PIO_READ,
>  	ATA_CMD_PIO_WRITE,
> @@ -727,7 +727,8 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
>  	} else if (dev->flags & ATA_DFLAG_LBA) {
>  		tf->flags |= ATA_TFLAG_LBA;
>  
> -		if (lba_28_ok(block, n_block)) {
> +		/* We need LBA48 for FUA writes */
> +		if (!(tf->flags & ATA_TFLAG_FUA) && lba_28_ok(block, n_block)) {
>  			/* use LBA28 */
>  			tf->device |= (block >> 24) & 0xf;
>  		} else if (lba_48_ok(block, n_block)) {
> @@ -742,9 +743,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.38.1
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v6 6/7] ata: libata: blacklist FUA support for known buggy drives

[Niklas Cassel]

On Tue, Nov 08, 2022 at 02:55:43PM +0900, Damien Le Moal wrote:
> 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>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  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 2b66fe529d81..97ade977b830 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4133,6 +4133,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.38.1
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v6 7/7] ata: libata: Enable fua support by default

[Niklas Cassel]

On Tue, Nov 08, 2022 at 02:55:44PM +0900, Damien Le Moal wrote:
> 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>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.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 97ade977b830..2967671131d2 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.38.1
> 

Before this commit:
-For a SCSI mode sense:
 FUA bit will not get set in the simulated SCSI cmd output buffer,
 since ATA_DFLAG_FUA is not be set, since libata_fua == 0.
-For a SCSI WRITE_16 / WRITE_16 command:
 ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
 is set or not. ata_set_rwcmd_protocol() will return a FUA command
 as long as ATA_TFLAG_FUA is set.

After this commit:
-For a SCSI mode sense:
 FUA bit will get set in the simulated SCSI cmd output buffer,
 since ATA_DFLAG_FUA is set, since libata_fua == 1.
-For a SCSI WRITE_16 / WRITE_16 command:
 ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
 is set or not. ata_set_rwcmd_protocol() will return a FUA command
 as long as ATA_TFLAG_FUA is set.


Perhaps this commit should more clearly say that this commit only affects
the simulated output for a SCSI mode sense command?

Currently, the commit message is a bit misleading, since it makes the
reader think that a SCSI write command with the FUA bit was not propagated
to the device before this commit, which AFAICT, is not true.

If the intention of this series was to only send a ATA write command to
the device, if the device has the ATA_DFLAG_FUA flag set, then perhaps the
series should include a new commit which either:
-Adds a check to ata_scsi_rw_xlat() so that it only sets ATA_TFLAG_FUA if
ATA_DFLAG_FUA is set;
or
-Adds a check to ata_scsi_rw_xlat() which returns an error if the SCSI
command has the FUA bit set, but ATA_DFLAG_FUA is not set.


Kind regards,
Niklas

Re: [PATCH v6 7/7] ata: libata: Enable fua support by default

[Damien Le Moal]

On 12/30/22 19:54, Niklas Cassel wrote:
> On Tue, Nov 08, 2022 at 02:55:44PM +0900, Damien Le Moal wrote:
>> 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>
>> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.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 97ade977b830..2967671131d2 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.38.1
>>
> 
> Before this commit:
> -For a SCSI mode sense:
>  FUA bit will not get set in the simulated SCSI cmd output buffer,
>  since ATA_DFLAG_FUA is not be set, since libata_fua == 0.
> -For a SCSI WRITE_16 / WRITE_16 command:
>  ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
>  is set or not. ata_set_rwcmd_protocol() will return a FUA command
>  as long as ATA_TFLAG_FUA is set.
> 
> After this commit:
> -For a SCSI mode sense:
>  FUA bit will get set in the simulated SCSI cmd output buffer,
>  since ATA_DFLAG_FUA is set, since libata_fua == 1.
> -For a SCSI WRITE_16 / WRITE_16 command:
>  ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
>  is set or not. ata_set_rwcmd_protocol() will return a FUA command
>  as long as ATA_TFLAG_FUA is set.
> 
> 
> Perhaps this commit should more clearly say that this commit only affects
> the simulated output for a SCSI mode sense command?
> 
> Currently, the commit message is a bit misleading, since it makes the
> reader think that a SCSI write command with the FUA bit was not propagated
> to the device before this commit, which AFAICT, is not true.

It was not with the default libata.fua = 0, since the drive would never be
exposed as having FUA support. See ata_dev_supports_fua() and its test for
"!libata_fua" and how that function was used in ata_scsiop_mode_sense().

The confusing thing here is that indeed there is no code preventing
propagating FUA bit to the device in libata-core/libata-scsi for any
REQ_FUA request. But the fact that devices would never report FUA support
means that the block layer would never issue a request with REQ_FUA set.

> 
> If the intention of this series was to only send a ATA write command to
> the device, if the device has the ATA_DFLAG_FUA flag set, then perhaps the
> series should include a new commit which either:
> -Adds a check to ata_scsi_rw_xlat() so that it only sets ATA_TFLAG_FUA if
> ATA_DFLAG_FUA is set;

See above. I do not think that is needed since the block layer will never
ever send a REQ_FUA request when ATA_DFLAG_FUA is not set.

> or
> -Adds a check to ata_scsi_rw_xlat() which returns an error if the SCSI
> command has the FUA bit set, but ATA_DFLAG_FUA is not set.

That would be possible for passthrough commands only. What is going to
happen is that we'll now get an error for that write if the drive does not
support NCQ or has NCQ disabled.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v6 1/7] block: add a sanity check for non-write flush/fua bios

[Niklas Cassel]

On Tue, Nov 08, 2022 at 02:55:38PM +0900, Damien Le Moal wrote:
> From: Christoph Hellwig <hch@infradead.org>
>
> Check that the PREFUSH and FUA flags are only set on write bios,
> given that the flush state machine expects that.
>
> Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
>  block/blk-core.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 17667159482e..d3446d38ba77 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -730,12 +730,15 @@ void submit_bio_noacct(struct bio *bio)
>	 * Filter flush bio's early so that bio based drivers without flush
>	 * support don't have to worry about them.
>	 */
> -	if (op_is_flush(bio->bi_opf) &&
> -	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
> -		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
> -		if (!bio_sectors(bio)) {
> -			status = BLK_STS_OK;
> +	if (op_is_flush(bio->bi_opf)) {
> +		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE))
>			goto end_io;
> +		if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {

Considering that blk_queue_write_cache() looks like this:

void blk_queue_write_cache(struct request_queue *q, bool wc, bool fua)
{
	if (wc)
		blk_queue_flag_set(QUEUE_FLAG_WC, q);
	else
		blk_queue_flag_clear(QUEUE_FLAG_WC, q);
	if (fua)
		blk_queue_flag_set(QUEUE_FLAG_FUA, q);
	else
		blk_queue_flag_clear(QUEUE_FLAG_FUA, q);

	wbt_set_write_cache(q, test_bit(QUEUE_FLAG_WC, &q->queue_flags));
}

It seems that you can have flag WC set, without having flag FUA set.

So should perhaps the line:

> +             if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {

instead be:

if (!test_bit(QUEUE_FLAG_FUA, &q->queue_flags)) {


Kind regards,
Niklas

Re: [PATCH v6 1/7] block: add a sanity check for non-write flush/fua bios

[Damien Le Moal]

On 12/30/22 20:54, Niklas Cassel wrote:
> On Tue, Nov 08, 2022 at 02:55:38PM +0900, Damien Le Moal wrote:
>> From: Christoph Hellwig <hch@infradead.org>
>>
>> Check that the PREFUSH and FUA flags are only set on write bios,
>> given that the flush state machine expects that.
>>
>> Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  block/blk-core.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 17667159482e..d3446d38ba77 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -730,12 +730,15 @@ void submit_bio_noacct(struct bio *bio)
>> 	 * Filter flush bio's early so that bio based drivers without flush
>> 	 * support don't have to worry about them.
>> 	 */
>> -	if (op_is_flush(bio->bi_opf) &&
>> -	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
>> -		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
>> -		if (!bio_sectors(bio)) {
>> -			status = BLK_STS_OK;
>> +	if (op_is_flush(bio->bi_opf)) {
>> +		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE))
>> 			goto end_io;
>> +		if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
> 
> Considering that blk_queue_write_cache() looks like this:
> 
> void blk_queue_write_cache(struct request_queue *q, bool wc, bool fua)
> {
> 	if (wc)
> 		blk_queue_flag_set(QUEUE_FLAG_WC, q);
> 	else
> 		blk_queue_flag_clear(QUEUE_FLAG_WC, q);
> 	if (fua)
> 		blk_queue_flag_set(QUEUE_FLAG_FUA, q);
> 	else
> 		blk_queue_flag_clear(QUEUE_FLAG_FUA, q);
> 
> 	wbt_set_write_cache(q, test_bit(QUEUE_FLAG_WC, &q->queue_flags));
> }
> 
> It seems that you can have flag WC set, without having flag FUA set.
> 
> So should perhaps the line:
> 
>> +             if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
> 
> instead be:
> 
> if (!test_bit(QUEUE_FLAG_FUA, &q->queue_flags)) {

Need both. If there is no write cache or write cache is off, FUA is
implied and is useless.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v6 7/7] ata: libata: Enable fua support by default

[Niklas Cassel]

On Fri, Dec 30, 2022 at 08:21:59PM +0900, Damien Le Moal wrote:
> On 12/30/22 19:54, Niklas Cassel wrote:
> >
> > Perhaps this commit should more clearly say that this commit only affects
> > the simulated output for a SCSI mode sense command?
> >
> > Currently, the commit message is a bit misleading, since it makes the
> > reader think that a SCSI write command with the FUA bit was not propagated
> > to the device before this commit, which AFAICT, is not true.
>
> It was not with the default libata.fua = 0, since the drive would never be
> exposed as having FUA support. See ata_dev_supports_fua() and its test for
> "!libata_fua" and how that function was used in ata_scsiop_mode_sense().

I see, drivers/scsi/sd.c does a SCSI mode sense to check if FUA is reported
as being supported, and then calls the block layer setter for this.

So changing the simulated SCSI cmd output is sufficient to tell the block
layer that it can send FUA requests to us.

>
> The confusing thing here is that indeed there is no code preventing
> propagating FUA bit to the device in libata-core/libata-scsi for any
> REQ_FUA request. But the fact that devices would never report FUA support
> means that the block layer would never issue a request with REQ_FUA set.

Well, ata_scsi_rw_xlat() is used both for passthrough commands and commands
issued by the block layer.

Just like how ata_scsiop_mode_sense() will be called regardless if it was a
passthrough command or a command issued by the block layer.

ata_scsiop_mode_sense() will not behave differently if it was a command
issued by the block layer or if it was e.g. a SG command.

I would argue that it makes sense (pun intended) for ata_scsi_rw_xlat() to
also behave the same, regardless if it was a command issued by the block
layer of if it was e.g. a SG command, but I will leave that to you to decide.


> That would be possible for passthrough commands only. What is going to
> happen is that we'll now get an error for that write if the drive does not
> support NCQ or has NCQ disabled.

Why would it give an error on a drive that has NCQ disabled?

Your new code looks like this:

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;
}

So it should only give an error for a drive where NCQ is not supported.
If NCQ is supported, but disabled, you will still send a ATA_CMD_WRITE_FUA_EXT
command.


Kind regards,
Niklas

Re: [PATCH v6 7/7] ata: libata: Enable fua support by default

[Damien Le Moal]

On 12/30/22 21:41, Niklas Cassel wrote:
> On Fri, Dec 30, 2022 at 08:21:59PM +0900, Damien Le Moal wrote:
>> On 12/30/22 19:54, Niklas Cassel wrote:
>>>
>>> Perhaps this commit should more clearly say that this commit only affects
>>> the simulated output for a SCSI mode sense command?
>>>
>>> Currently, the commit message is a bit misleading, since it makes the
>>> reader think that a SCSI write command with the FUA bit was not propagated
>>> to the device before this commit, which AFAICT, is not true.
>>
>> It was not with the default libata.fua = 0, since the drive would never be
>> exposed as having FUA support. See ata_dev_supports_fua() and its test for
>> "!libata_fua" and how that function was used in ata_scsiop_mode_sense().
> 
> I see, drivers/scsi/sd.c does a SCSI mode sense to check if FUA is reported
> as being supported, and then calls the block layer setter for this.
> 
> So changing the simulated SCSI cmd output is sufficient to tell the block
> layer that it can send FUA requests to us.
> 
>>
>> The confusing thing here is that indeed there is no code preventing
>> propagating FUA bit to the device in libata-core/libata-scsi for any
>> REQ_FUA request. But the fact that devices would never report FUA support
>> means that the block layer would never issue a request with REQ_FUA set.
> 
> Well, ata_scsi_rw_xlat() is used both for passthrough commands and commands
> issued by the block layer.
> 
> Just like how ata_scsiop_mode_sense() will be called regardless if it was a
> passthrough command or a command issued by the block layer.
> 
> ata_scsiop_mode_sense() will not behave differently if it was a command
> issued by the block layer or if it was e.g. a SG command.
> 
> I would argue that it makes sense (pun intended) for ata_scsi_rw_xlat() to
> also behave the same, regardless if it was a command issued by the block
> layer of if it was e.g. a SG command, but I will leave that to you to decide.

Yes, we can improve that. The current series does not change the
relatively (and really old) bad handling of passthrough FUA commands. So
this is something we can do on top of the fua series.

> 
> 
>> That would be possible for passthrough commands only. What is going to
>> happen is that we'll now get an error for that write if the drive does not
>> support NCQ or has NCQ disabled.
> 
> Why would it give an error on a drive that has NCQ disabled?

My bad. It will not. In that case, WRITE FUA EXT will be used for an FUA
write.

> 
> Your new code looks like this:
> 
> 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;
> }
> 
> So it should only give an error for a drive where NCQ is not supported.
> If NCQ is supported, but disabled, you will still send a ATA_CMD_WRITE_FUA_EXT
> command.

Yes.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v6 7/7] ata: libata: Enable fua support by default

[Niklas Cassel]

On Tue, Nov 08, 2022 at 02:55:44PM +0900, Damien Le Moal wrote:
> Change the default value of the fua module parameter to 1 to enable fua
> support by default for all devices supporting it.

Perhaps add the following to the commit message:

With this change, ata_dev_config_fua() will now set the flag
ATA_DFLAG_FUA by default for devices that support FUA.

This will cause ata_scsiop_mode_sense() to set the DPOFUA bit in
the DEVICE-SPECIFIC PARAMETER field in the mode parameter header.

The SCSI disk driver performs a MODE SENSE and looks at the
DPOFUA bit, it then calls blk_queue_write_cache() with the
value of the DPOFUA bit, to inform the the block layer if FUA
is supported or not.

The block layer will not issue REQ_FUA requests if it hasn't
been informed that the lower levels actually support FUA.

> 
> 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>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.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 97ade977b830..2967671131d2 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.38.1
> 

With a less spartan commit message:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v6 1/7] block: add a sanity check for non-write flush/fua bios

[Jens Axboe]

On 11/7/22 10:55 PM, Damien Le Moal wrote:
> From: Christoph Hellwig <hch@infradead.org>
> 
> Check that the PREFUSH and FUA flags are only set on write bios,
> given that the flush state machine expects that.
> 
> Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe