diff mbox series

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

Message ID 20221108055544.1481583-5-damien.lemoal@opensource.wdc.com (mailing list archive)
State New, archived
Headers show
Series Improve libata support for FUA | expand

Commit Message

Damien Le Moal Nov. 8, 2022, 5:55 a.m. UTC 
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(-)

Comments

Johannes Thumshirn Nov. 8, 2022, 7:42 a.m. UTC | #1 
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Niklas Cassel Dec. 30, 2022, 8:01 a.m. UTC | #2 
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>
diff mbox series

Patch

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 */