[PATCH 0/5] fetch sense data for ATA devices supporting sense reporting

[PATCH 0/5] fetch sense data for ATA devices supporting sense reporting

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

NOTE TO MAINTAINERS:
Perhaps the SCSI patch could go into both the scsi tree and the libata
tree to avoid any merge conflicts during the merge window.


Hello,

Currently, the sense data reporting feature set is enabled for all ATA
devices which supports the feature set (ata_id_has_sense_reporting()),
see ata_dev_config_sense_reporting().

However, even if sense data reporting is enabled, and the device
indicates that sense data is available, the sense data is only fetched
for ATA ZAC devices. For regular ATA devices, the available sense data
is never fetched, it is simply ignored. Instead, libata will use the
ERROR + STATUS fields and map them to a very generic and reduced set
of sense data, see ata_gen_ata_sense() and ata_to_sense_error().

When sense data reporting was first implemented, regular ATA devices
did fetch the sense data from the device. However, this was restricted
to only ATA ZAC devices in commit ca156e006add ("libata: don't request
sense data on !ZAC ATA devices").

With the recent fixes surrounding sense data and NCQ autosense:
https://lore.kernel.org/linux-ide/20220916122838.1190628-1-niklas.cassel@wdc.com/T/#u
together with the patches in this series, we want to, once again,
fetch the sense data for all ATA devices supporting sense reporting.
ata_gen_ata_sense() should only be used for devices that don't support
the sense data reporting feature set.

If we encounter a device that is misbehaving because the sense data is
actually fetched, then that device should be quirked such that it
never enables the sense data reporting feature set in the first place,
since such a device is obviously not compliant with the specification.


Kind regards,
Niklas

Damien Le Moal (1):
  scsi: Define the COMPLETED sense key

Niklas Cassel (4):
  ata: libata: fix NCQ autosense logic
  ata: libata: clarify when ata_eh_request_sense() will be called
  ata: libata: only set sense valid flag if sense data is valid
  ata: libata: fetch sense data for ATA devices supporting sense
    reporting

 drivers/ata/libata-eh.c   | 18 +++++++++++++-----
 drivers/ata/libata-sata.c | 21 ++++++++++++++-------
 drivers/ata/libata-scsi.c | 16 ++++++++++++++++
 drivers/ata/libata.h      |  1 +
 include/scsi/scsi_proto.h |  4 ++--
 5 files changed, 46 insertions(+), 14 deletions(-)

-- 
2.37.3

[PATCH 1/5] scsi: Define the COMPLETED sense key

[Niklas Cassel]

From: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Add the definition for the COMPLETED sense key in scsi_proto.h. While at
it, cleanup the white lines around the sense keys macro definitions.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 include/scsi/scsi_proto.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index c03e35fc382c..919ed4137f9a 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -205,10 +205,10 @@ enum sam_status {
 };
 
 #define STATUS_MASK         0xfe
+
 /*
  *  SENSE KEYS
  */
-
 #define NO_SENSE            0x00
 #define RECOVERED_ERROR     0x01
 #define NOT_READY           0x02
@@ -223,7 +223,7 @@ enum sam_status {
 #define ABORTED_COMMAND     0x0b
 #define VOLUME_OVERFLOW     0x0d
 #define MISCOMPARE          0x0e
-
+#define COMPLETED	    0x0f
 
 /*
  *  DEVICE TYPES
-- 
2.37.3

[PATCH 3/5] ata: libata: clarify when ata_eh_request_sense() will be called

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

ata_eh_request_sense() returns early when flag ATA_QCFLAG_SENSE_VALID is
set. However, since the call to ata_eh_request_sense() is guarded by a
ATA_SENSE bit conditional, the logical conclusion for the reader is that
all checks are performed at the call site.

Highlight the fact that the sense data will not be fetched if flag
ATA_QCFLAG_SENSE_VALID is already set by adding an additional check to
the existing guarding conditional. No functional change.

Additionally, add a comment explaining that ata_eh_analyze_tf() will
only fetch the sense data if:
-It was a non-NCQ command that failed, or
-It was a NCQ command that failed, but the sense data was not included
 in the NCQ command error log (i.e. NCQ autosense is not supported).

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-eh.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 89ddc15235b7..c6964fd2bc42 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1578,7 +1578,14 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc)
 
 	switch (qc->dev->class) {
 	case ATA_DEV_ZAC:
-		if (stat & ATA_SENSE)
+		/*
+		 * Fetch the sense data explicitly if:
+		 * -It was a non-NCQ command that failed, or
+		 * -It was a NCQ command that failed, but the sense data
+		 *  was not included in the NCQ command error log
+		 *  (i.e. NCQ autosense is not supported by the device).
+		 */
+		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID) && (stat & ATA_SENSE))
 			ata_eh_request_sense(qc);
 		fallthrough;
 	case ATA_DEV_ATA:
-- 
2.37.3

[PATCH 2/5] ata: libata: fix NCQ autosense logic

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Currently, the logic if we should call ata_scsi_set_sense()
(and set flag ATA_QCFLAG_SENSE_VALID to indicate that we have
successfully added sense data to the struct ata_queued_cmd)
looks like this:

if (dev->class == ATA_DEV_ZAC &&
    ((qc->result_tf.status & ATA_SENSE) || qc->result_tf.auxiliary))

The problem with this is that a drive can support the NCQ command
error log without supporting NCQ autosense.

On such a drive, if the failing command has sense data, the status
field in the NCQ command error log will have the ATA_SENSE bit set.

It is just that this sense data is not included in the NCQ command
error log when NCQ autosense is not supported. Instead the sense
data has to be fetched using the REQUEST SENSE DATA EXT command.

Therefore, we should only add the sense data if the drive supports
NCQ autosense AND the ATA_SENSE bit is set in the status field.

Fix this, and at the same time, remove the duplicated ATA_DEV_ZAC
check. The struct ata_taskfile supplied to ata_eh_read_log_10h()
is memset:ed before calling the function, so simply checking if
qc->result_tf.auxiliary is set is sufficient to tell us that the
log actually contained sense data.

Fixes: d238ffd59d3c ("libata: do not attempt to retrieve sense code twice")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-sata.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index eef57d101ed1..dfbc660651e7 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1392,7 +1392,8 @@ static int ata_eh_read_log_10h(struct ata_device *dev,
 	tf->hob_lbah = buf[10];
 	tf->nsect = buf[12];
 	tf->hob_nsect = buf[13];
-	if (dev->class == ATA_DEV_ZAC && ata_id_has_ncq_autosense(dev->id))
+	if (dev->class == ATA_DEV_ZAC && ata_id_has_ncq_autosense(dev->id) &&
+	    (tf->status & ATA_SENSE))
 		tf->auxiliary = buf[14] << 16 | buf[15] << 8 | buf[16];
 
 	return 0;
@@ -1456,8 +1457,12 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 	memcpy(&qc->result_tf, &tf, sizeof(tf));
 	qc->result_tf.flags = ATA_TFLAG_ISADDR | ATA_TFLAG_LBA | ATA_TFLAG_LBA48;
 	qc->err_mask |= AC_ERR_DEV | AC_ERR_NCQ;
-	if (dev->class == ATA_DEV_ZAC &&
-	    ((qc->result_tf.status & ATA_SENSE) || qc->result_tf.auxiliary)) {
+
+	/*
+	 * If the device supports NCQ autosense, ata_eh_read_log_10h() will have
+	 * stored the sense data in qc->result_tf.auxiliary.
+	 */
+	if (qc->result_tf.auxiliary) {
 		char sense_key, asc, ascq;
 
 		sense_key = (qc->result_tf.auxiliary >> 16) & 0xff;
-- 
2.37.3

[PATCH 4/5] ata: libata: only set sense valid flag if sense data is valid

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

While this shouldn't be needed if all devices that claim that they
support NCQ autosense (ata_id_has_ncq_autosense()) and/or the sense
data reporting feature (ata_id_has_sense_reporting()), actually
supported those features.

However, there might be some old ATA devices that either have these
bits set, even when they don't support those features, or they simply
return malformed data when using those features.

These devices should be quirked, but in order to try to minimize the
impact for the users of these such devices, it was suggested by Damien
Le Moal that it might be a good idea to sanity check the sense data
received from the device. If the sense data looks bogus, then the
sense data is never added to the scsi_cmnd command.

Introduce a new function, ata_scsi_sense_is_valid(), and use it in all
places where sense data is received from the device.

Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-eh.c   |  6 ++++--
 drivers/ata/libata-sata.c | 11 +++++++----
 drivers/ata/libata-scsi.c | 16 ++++++++++++++++
 drivers/ata/libata.h      |  1 +
 4 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index c6964fd2bc42..922e6c37ea9b 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1431,8 +1431,10 @@ static void ata_eh_request_sense(struct ata_queued_cmd *qc)
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
 	/* Ignore err_mask; ATA_ERR might be set */
 	if (tf.status & ATA_SENSE) {
-		ata_scsi_set_sense(dev, cmd, tf.lbah, tf.lbam, tf.lbal);
-		qc->flags |= ATA_QCFLAG_SENSE_VALID;
+		if (ata_scsi_sense_is_valid(tf.lbah, tf.lbam, tf.lbal)) {
+			ata_scsi_set_sense(dev, cmd, tf.lbah, tf.lbam, tf.lbal);
+			qc->flags |= ATA_QCFLAG_SENSE_VALID;
+		}
 	} else {
 		ata_dev_warn(dev, "request sense failed stat %02x emask %x\n",
 			     tf.status, err_mask);
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index dfbc660651e7..5d75e62f27b5 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1468,10 +1468,13 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 		sense_key = (qc->result_tf.auxiliary >> 16) & 0xff;
 		asc = (qc->result_tf.auxiliary >> 8) & 0xff;
 		ascq = qc->result_tf.auxiliary & 0xff;
-		ata_scsi_set_sense(dev, qc->scsicmd, sense_key, asc, ascq);
-		ata_scsi_set_sense_information(dev, qc->scsicmd,
-					       &qc->result_tf);
-		qc->flags |= ATA_QCFLAG_SENSE_VALID;
+		if (ata_scsi_sense_is_valid(sense_key, asc, ascq)) {
+			ata_scsi_set_sense(dev, qc->scsicmd, sense_key, asc,
+					   ascq);
+			ata_scsi_set_sense_information(dev, qc->scsicmd,
+						       &qc->result_tf);
+			qc->flags |= ATA_QCFLAG_SENSE_VALID;
+		}
 	}
 
 	ehc->i.err_mask &= ~AC_ERR_DEV;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index f3c64e796423..1222aaa62aff 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -188,6 +188,22 @@ DEVICE_ATTR(unload_heads, S_IRUGO | S_IWUSR,
 	    ata_scsi_park_show, ata_scsi_park_store);
 EXPORT_SYMBOL_GPL(dev_attr_unload_heads);
 
+bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq)
+{
+	/*
+	 * If sk == NO_SENSE, and asc + ascq == NO ADDITIONAL SENSE INFORMATION,
+	 * then there is no sense data to add.
+	 */
+	if (sk == 0 && asc == 0 && ascq == 0)
+		return false;
+
+	/* If sk > COMPLETED, sense data is bogus. */
+	if (sk > COMPLETED)
+		return false;
+
+	return true;
+}
+
 void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
 			u8 sk, u8 asc, u8 ascq)
 {
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 2c5c8273af01..2cd6124a01e8 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -114,6 +114,7 @@ extern int ata_scsi_add_hosts(struct ata_host *host,
 			      struct scsi_host_template *sht);
 extern void ata_scsi_scan_host(struct ata_port *ap, int sync);
 extern int ata_scsi_offline_dev(struct ata_device *dev);
+extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq);
 extern void ata_scsi_set_sense(struct ata_device *dev,
 			       struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq);
 extern void ata_scsi_set_sense_information(struct ata_device *dev,
-- 
2.37.3

[PATCH 5/5] ata: libata: fetch sense data for ATA devices supporting sense reporting

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Currently, the sense data reporting feature set is enabled for all ATA
devices which supports the feature set (ata_id_has_sense_reporting()),
see ata_dev_config_sense_reporting().

However, even if sense data reporting is enabled, and the device
indicates that sense data is available, the sense data is only fetched
for ATA ZAC devices. For regular ATA devices, the available sense data
is never fetched, it is simply ignored. Instead, libata will use the
ERROR + STATUS fields and map them to a very generic and reduced set
of sense data, see ata_gen_ata_sense() and ata_to_sense_error().

When sense data reporting was first implemented, regular ATA devices
did fetch the sense data from the device. However, this was restricted
to only ATA ZAC devices in commit ca156e006add ("libata: don't request
sense data on !ZAC ATA devices").

With recent changes related to sense data and NCQ autosense, we want
to, once again, fetch the sense data for all ATA devices supporting
sense reporting.
ata_gen_ata_sense() should only be used for devices that don't support
the sense data reporting feature set.
hopefully the features will be more robust this time around.

It is not just ZAC, many new ATA features, e.g. Command Duration
Limits, relies on working NCQ autosense and sense data. Therefore,
it is not really an option to avoid fetching the sense data forever.

If we encounter a device that is misbehaving because the sense data is
actually fetched, then that device should be quirked such that it
never enables the sense data reporting feature set in the first place,
since such a device is obviously not compliant with the specification.

The order in which we will try to add sense data to a scsi_cmnd:
1) NCQ autosense (if supported) - ata_eh_analyze_ncq_error()
2) REQUEST SENSE DATA EXT (if supported) - ata_eh_request_sense()
3) error + status field translation - ata_gen_ata_sense(), called
   by ata_scsi_qc_complete() if neither 1) or 2) is supported.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-eh.c   | 3 +--
 drivers/ata/libata-sata.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 922e6c37ea9b..8610756d7d0a 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1579,6 +1579,7 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc)
 	}
 
 	switch (qc->dev->class) {
+	case ATA_DEV_ATA:
 	case ATA_DEV_ZAC:
 		/*
 		 * Fetch the sense data explicitly if:
@@ -1589,8 +1590,6 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc)
 		 */
 		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID) && (stat & ATA_SENSE))
 			ata_eh_request_sense(qc);
-		fallthrough;
-	case ATA_DEV_ATA:
 		if (err & ATA_ICRC)
 			qc->err_mask |= AC_ERR_ATA_BUS;
 		if (err & (ATA_UNC | ATA_AMNF))
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 5d75e62f27b5..464e85d5cd83 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1392,8 +1392,7 @@ static int ata_eh_read_log_10h(struct ata_device *dev,
 	tf->hob_lbah = buf[10];
 	tf->nsect = buf[12];
 	tf->hob_nsect = buf[13];
-	if (dev->class == ATA_DEV_ZAC && ata_id_has_ncq_autosense(dev->id) &&
-	    (tf->status & ATA_SENSE))
+	if (ata_id_has_ncq_autosense(dev->id) && (tf->status & ATA_SENSE))
 		tf->auxiliary = buf[14] << 16 | buf[15] << 8 | buf[16];
 
 	return 0;
-- 
2.37.3

Re: [PATCH 1/5] scsi: Define the COMPLETED sense key

[Martin K. Petersen]

Niklas,

I'll just ACK this change so it can go through ATA with the rest of the
series.

> Add the definition for the COMPLETED sense key in scsi_proto.h. While
> at it, cleanup the white lines around the sense keys macro
> definitions.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/5] fetch sense data for ATA devices supporting sense reporting

[Damien Le Moal]

On 9/27/22 05:53, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> NOTE TO MAINTAINERS:
> Perhaps the SCSI patch could go into both the scsi tree and the libata
> tree to avoid any merge conflicts during the merge window.
> 
> 
> Hello,
> 
> Currently, the sense data reporting feature set is enabled for all ATA
> devices which supports the feature set (ata_id_has_sense_reporting()),
> see ata_dev_config_sense_reporting().
> 
> However, even if sense data reporting is enabled, and the device
> indicates that sense data is available, the sense data is only fetched
> for ATA ZAC devices. For regular ATA devices, the available sense data
> is never fetched, it is simply ignored. Instead, libata will use the
> ERROR + STATUS fields and map them to a very generic and reduced set
> of sense data, see ata_gen_ata_sense() and ata_to_sense_error().
> 
> When sense data reporting was first implemented, regular ATA devices
> did fetch the sense data from the device. However, this was restricted
> to only ATA ZAC devices in commit ca156e006add ("libata: don't request
> sense data on !ZAC ATA devices").
> 
> With the recent fixes surrounding sense data and NCQ autosense:
> https://lore.kernel.org/linux-ide/20220916122838.1190628-1-niklas.cassel@wdc.com/T/#u
> together with the patches in this series, we want to, once again,
> fetch the sense data for all ATA devices supporting sense reporting.
> ata_gen_ata_sense() should only be used for devices that don't support
> the sense data reporting feature set.
> 
> If we encounter a device that is misbehaving because the sense data is
> actually fetched, then that device should be quirked such that it
> never enables the sense data reporting feature set in the first place,
> since such a device is obviously not compliant with the specification.

Applied to for-6.2. Thanks !

> 
> 
> Kind regards,
> Niklas
> 
> Damien Le Moal (1):
>   scsi: Define the COMPLETED sense key
> 
> Niklas Cassel (4):
>   ata: libata: fix NCQ autosense logic
>   ata: libata: clarify when ata_eh_request_sense() will be called
>   ata: libata: only set sense valid flag if sense data is valid
>   ata: libata: fetch sense data for ATA devices supporting sense
>     reporting
> 
>  drivers/ata/libata-eh.c   | 18 +++++++++++++-----
>  drivers/ata/libata-sata.c | 21 ++++++++++++++-------
>  drivers/ata/libata-scsi.c | 16 ++++++++++++++++
>  drivers/ata/libata.h      |  1 +
>  include/scsi/scsi_proto.h |  4 ++--
>  5 files changed, 46 insertions(+), 14 deletions(-)
> 

-- 
Damien Le Moal
Western Digital Research