[PATCH v2 0/9] libata cleanups and improvements

[PATCH v2 0/9] libata cleanups and improvements

[Damien Le Moal]

The first two patches of this series fix potential NULL pointer
dereference problems.
The following three patches cleanup libata-core code in the area of
device configuration (ata_dev_configure() function).
Patch r64 improves ata_read_log_page() to avoid unnecessary warning
messages and patch 7 adds an informational message on device scan to
advertize the features supported by a device.

Path 8 adds the new sysfs ahci device attribute ncq_prio_supported to
indicate that a disk supports NCQ priority. Patch 9 does the same for
the mpt3sas driver, adding the sas_ncq_prio_supported device attribute.

Changes from v1:
* Added patch 1 and 2 to fix problems reported by the kernel test robot
* Use strscpy() instead of strcpy in patch 4
* Use sysfs_emit in patch 8 and 9 as suggested by Bart
* Fix typos in comments of the new sas_ncq_prio_supported attribute in
  patch 9

Damien Le Moal (9):
  libata: fix ata_host_alloc_pinfo()
  libata: fix ata_host_start()
  libata: cleanup device sleep capability detection
  libata: cleanup ata_dev_configure()
  libata: cleanup NCQ priority handling
  libata: fix ata_read_log_page() warning
  libata: print feature list on device scan
  libahci: Introduce ncq_prio_supported sysfs sttribute
  scsi: mpt3sas: Introduce sas_ncq_prio_supported sysfs sttribute

 drivers/ata/libahci.c              |   1 +
 drivers/ata/libata-core.c          | 253 +++++++++++++++--------------
 drivers/ata/libata-sata.c          |  61 ++++---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c |  19 +++
 include/linux/libata.h             |   5 +
 5 files changed, 192 insertions(+), 147 deletions(-)

-- 
2.31.1

[PATCH v2 1/9] libata: fix ata_host_alloc_pinfo()

[Damien Le Moal]

Avoid a potential NULL pointer dereference by testing that the ATA port
info variable "pi".

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/ata/libata-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 61c762961ca8..ea8b91297f12 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5458,7 +5458,7 @@ struct ata_host *ata_host_alloc_pinfo(struct device *dev,
 		ap->link.flags |= pi->link_flags;
 		ap->ops = pi->port_ops;
 
-		if (!host->ops && (pi->port_ops != &ata_dummy_port_ops))
+		if (!host->ops && pi && pi->port_ops != &ata_dummy_port_ops)
 			host->ops = pi->port_ops;
 	}
 
-- 
2.31.1

[PATCH v2 2/9] libata: fix ata_host_start()

[Damien Le Moal]

The loop on entry of ata_host_start() may not initialize host->ops to a
non NULL value. The test on the host_stop field of host->ops must then
be preceded by a check that host->ops is not NULL.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/ata/libata-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ea8b91297f12..fe49197caf99 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5573,7 +5573,7 @@ int ata_host_start(struct ata_host *host)
 			have_stop = 1;
 	}
 
-	if (host->ops->host_stop)
+	if (host->ops && host->ops->host_stop)
 		have_stop = 1;
 
 	if (have_stop) {
-- 
2.31.1

[PATCH v2 3/9] libata: cleanup device sleep capability detection

[Damien Le Moal]

Move the code to retrieve the device sleep capability and timings out of
ata_dev_configure() into the helper function ata_dev_config_devslp().
While at it, mark the device as supporting the device sleep capability
only if the sata settings page was retrieved successfully to ensure that
the timing information is correctly initialized.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index fe49197caf99..b13194432e5a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2363,6 +2363,37 @@ static void ata_dev_config_trusted(struct ata_device *dev)
 		dev->flags |= ATA_DFLAG_TRUSTED;
 }
 
+static void ata_dev_config_devslp(struct ata_device *dev)
+{
+	u8 *sata_setting = dev->link->ap->sector_buf;
+	unsigned int err_mask;
+	int i, j;
+
+	/*
+	 * Check device sleep capability. Get DevSlp timing variables
+	 * from SATA Settings page of Identify Device Data Log.
+	 */
+	if (!ata_id_has_devslp(dev->id))
+		return;
+
+	err_mask = ata_read_log_page(dev,
+				     ATA_LOG_IDENTIFY_DEVICE,
+				     ATA_LOG_SATA_SETTINGS,
+				     sata_setting, 1);
+	if (err_mask) {
+		ata_dev_dbg(dev,
+			    "failed to get SATA Settings Log, Emask 0x%x\n",
+			    err_mask);
+		return;
+	}
+
+	dev->flags |= ATA_DFLAG_DEVSLP;
+	for (i = 0; i < ATA_LOG_DEVSLP_SIZE; i++) {
+		j = ATA_LOG_DEVSLP_OFFSET + i;
+		dev->devslp_timing[i] = sata_setting[j];
+	}
+}
+
 /**
  *	ata_dev_configure - Configure the specified ATA/ATAPI device
  *	@dev: Target device to configure
@@ -2565,29 +2596,7 @@ int ata_dev_configure(struct ata_device *dev)
 			}
 		}
 
-		/* Check and mark DevSlp capability. Get DevSlp timing variables
-		 * from SATA Settings page of Identify Device Data Log.
-		 */
-		if (ata_id_has_devslp(dev->id)) {
-			u8 *sata_setting = ap->sector_buf;
-			int i, j;
-
-			dev->flags |= ATA_DFLAG_DEVSLP;
-			err_mask = ata_read_log_page(dev,
-						     ATA_LOG_IDENTIFY_DEVICE,
-						     ATA_LOG_SATA_SETTINGS,
-						     sata_setting,
-						     1);
-			if (err_mask)
-				ata_dev_dbg(dev,
-					    "failed to get Identify Device Data, Emask 0x%x\n",
-					    err_mask);
-			else
-				for (i = 0; i < ATA_LOG_DEVSLP_SIZE; i++) {
-					j = ATA_LOG_DEVSLP_OFFSET + i;
-					dev->devslp_timing[i] = sata_setting[j];
-				}
-		}
+		ata_dev_config_devslp(dev);
 		ata_dev_config_sense_reporting(dev);
 		ata_dev_config_zac(dev);
 		ata_dev_config_trusted(dev);
-- 
2.31.1

[PATCH v2 4/9] libata: cleanup ata_dev_configure()

[Damien Le Moal]

Introduce the helper functions ata_dev_config_lba() and
ata_dev_config_chs() to configure the addressing capabilities of a
device. Each helper takes a string as argument for the addressing
information printed after these helpers execution completes.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index b13194432e5a..2b6054cdd8fc 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2363,6 +2363,52 @@ static void ata_dev_config_trusted(struct ata_device *dev)
 		dev->flags |= ATA_DFLAG_TRUSTED;
 }
 
+static int ata_dev_config_lba(struct ata_device *dev,
+			      char *info, size_t infosz)
+{
+	const u16 *id = dev->id;
+	int info_ofst;
+
+	dev->flags |= ATA_DFLAG_LBA;
+
+	if (ata_id_has_lba48(id)) {
+		dev->flags |= ATA_DFLAG_LBA48;
+		strscpy(info, "LBA48 ", infosz);
+
+		if (dev->n_sectors >= (1UL << 28) &&
+		    ata_id_has_flush_ext(id))
+			dev->flags |= ATA_DFLAG_FLUSH_EXT;
+	} else {
+		strscpy(info, "LBA ", infosz);
+	}
+	info_ofst = strlen(info);
+
+	/* config NCQ */
+	return ata_dev_config_ncq(dev, info + info_ofst,
+				  infosz - info_ofst);
+}
+
+static void ata_dev_config_chs(struct ata_device *dev,
+			       char *info, size_t infosz)
+{
+	const u16 *id = dev->id;
+
+	/* Default translation */
+	dev->cylinders	= id[1];
+	dev->heads	= id[3];
+	dev->sectors	= id[6];
+
+	if (ata_id_current_chs_valid(id)) {
+		/* Current CHS translation is valid. */
+		dev->cylinders = id[54];
+		dev->heads     = id[55];
+		dev->sectors   = id[56];
+	}
+
+	snprintf(info, infosz, "CHS %u/%u/%u",
+		 dev->cylinders, dev->heads, dev->sectors);
+}
+
 static void ata_dev_config_devslp(struct ata_device *dev)
 {
 	u8 *sata_setting = dev->link->ap->sector_buf;
@@ -2418,6 +2464,7 @@ int ata_dev_configure(struct ata_device *dev)
 	char revbuf[7];		/* XYZ-99\0 */
 	char fwrevbuf[ATA_ID_FW_REV_LEN+1];
 	char modelbuf[ATA_ID_PROD_LEN+1];
+	char lba_info[40];
 	int rc;
 
 	if (!ata_dev_enabled(dev) && ata_msg_info(ap)) {
@@ -2539,61 +2586,22 @@ int ata_dev_configure(struct ata_device *dev)
 		}
 
 		if (ata_id_has_lba(id)) {
-			const char *lba_desc;
-			char ncq_desc[24];
-
-			lba_desc = "LBA";
-			dev->flags |= ATA_DFLAG_LBA;
-			if (ata_id_has_lba48(id)) {
-				dev->flags |= ATA_DFLAG_LBA48;
-				lba_desc = "LBA48";
-
-				if (dev->n_sectors >= (1UL << 28) &&
-				    ata_id_has_flush_ext(id))
-					dev->flags |= ATA_DFLAG_FLUSH_EXT;
-			}
-
-			/* config NCQ */
-			rc = ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
+			rc = ata_dev_config_lba(dev, lba_info, sizeof(lba_info));
 			if (rc)
 				return rc;
-
-			/* print device info to dmesg */
-			if (ata_msg_drv(ap) && print_info) {
-				ata_dev_info(dev, "%s: %s, %s, max %s\n",
-					     revbuf, modelbuf, fwrevbuf,
-					     ata_mode_string(xfer_mask));
-				ata_dev_info(dev,
-					     "%llu sectors, multi %u: %s %s\n",
-					(unsigned long long)dev->n_sectors,
-					dev->multi_count, lba_desc, ncq_desc);
-			}
 		} else {
-			/* CHS */
-
-			/* Default translation */
-			dev->cylinders	= id[1];
-			dev->heads	= id[3];
-			dev->sectors	= id[6];
-
-			if (ata_id_current_chs_valid(id)) {
-				/* Current CHS translation is valid. */
-				dev->cylinders = id[54];
-				dev->heads     = id[55];
-				dev->sectors   = id[56];
-			}
+			ata_dev_config_chs(dev, lba_info, sizeof(lba_info));
+		}
 
-			/* print device info to dmesg */
-			if (ata_msg_drv(ap) && print_info) {
-				ata_dev_info(dev, "%s: %s, %s, max %s\n",
-					     revbuf,	modelbuf, fwrevbuf,
-					     ata_mode_string(xfer_mask));
-				ata_dev_info(dev,
-					     "%llu sectors, multi %u, CHS %u/%u/%u\n",
-					     (unsigned long long)dev->n_sectors,
-					     dev->multi_count, dev->cylinders,
-					     dev->heads, dev->sectors);
-			}
+		/* print device info to dmesg */
+		if (ata_msg_drv(ap) && print_info) {
+			ata_dev_info(dev, "%s: %s, %s, max %s\n",
+				     revbuf, modelbuf, fwrevbuf,
+				     ata_mode_string(xfer_mask));
+			ata_dev_info(dev,
+				     "%llu sectors, multi %u, %s\n",
+				     (unsigned long long)dev->n_sectors,
+				     dev->multi_count, lba_info);
 		}
 
 		ata_dev_config_devslp(dev);
-- 
2.31.1

[PATCH v2 5/9] libata: cleanup NCQ priority handling

[Damien Le Moal]

The ata device flag ATA_DFLAG_NCQ_PRIO indicates if a device supports
the NCQ Priority feature while the ATA_DFLAG_NCQ_PRIO_ENABLE device
flag indicates if the feature is enabled. Enabling NCQ priority use is
controlled by the user through the device sysfs attribute
ncq_prio_enable. As a result, the ATA_DFLAG_NCQ_PRIO flag should not be
cleared when ATA_DFLAG_NCQ_PRIO_ENABLE is not set as the device still
supports the feature even after the user disables it. This leads to the
following cleanups:
- In ata_build_rw_tf(), set a command high priority bit based on the
  ATA_DFLAG_NCQ_PRIO_ENABLE flag, not on the ATA_DFLAG_NCQ flag. That
  is, set a command high priority only if the user enabled NCQ priority
  use.
- In ata_dev_config_ncq_prio(), ATA_DFLAG_NCQ_PRIO should not be cleared
  if ATA_DFLAG_NCQ_PRIO_ENABLE is not set. If the device does not
  support NCQ priority, both ATA_DFLAG_NCQ_PRIO and
  ATA_DFLAG_NCQ_PRIO_ENABLE must be cleared.

With the above ata_dev_config_ncq_prio() change, ATA_DFLAG_NCQ_PRIO flag
is set on device scan and revalidation. There is no need to trigger a
device revalidation in ata_ncq_prio_enable_store() when the user enables
the use of NCQ priority. Remove the revalidation code from that funciton
to simplify it. Also change the return value from -EIO to -EINVAL when a
user tries to enable NCQ priority for a device that does not support
this feature.  While at it, also simplify ata_ncq_prio_enable_show().

Overall, there is no functional change introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/ata/libata-core.c | 32 ++++++++++++++------------------
 drivers/ata/libata-sata.c | 37 ++++++++++++-------------------------
 2 files changed, 26 insertions(+), 43 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2b6054cdd8fc..b556401cc110 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -706,11 +706,9 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 		if (tf->flags & ATA_TFLAG_FUA)
 			tf->device |= 1 << 7;
 
-		if (dev->flags & ATA_DFLAG_NCQ_PRIO) {
-			if (class == IOPRIO_CLASS_RT)
-				tf->hob_nsect |= ATA_PRIO_HIGH <<
-						 ATA_SHIFT_PRIO;
-		}
+		if (dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLE &&
+		    class == IOPRIO_CLASS_RT)
+			tf->hob_nsect |= ATA_PRIO_HIGH << ATA_SHIFT_PRIO;
 	} else if (dev->flags & ATA_DFLAG_LBA) {
 		tf->flags |= ATA_TFLAG_LBA;
 
@@ -2173,11 +2171,6 @@ static void ata_dev_config_ncq_prio(struct ata_device *dev)
 	struct ata_port *ap = dev->link->ap;
 	unsigned int err_mask;
 
-	if (!(dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLE)) {
-		dev->flags &= ~ATA_DFLAG_NCQ_PRIO;
-		return;
-	}
-
 	err_mask = ata_read_log_page(dev,
 				     ATA_LOG_IDENTIFY_DEVICE,
 				     ATA_LOG_SATA_SETTINGS,
@@ -2185,18 +2178,21 @@ static void ata_dev_config_ncq_prio(struct ata_device *dev)
 				     1);
 	if (err_mask) {
 		ata_dev_dbg(dev,
-			    "failed to get Identify Device data, Emask 0x%x\n",
+			    "failed to get SATA settings log, Emask 0x%x\n",
 			    err_mask);
-		return;
+		goto not_supported;
 	}
 
-	if (ap->sector_buf[ATA_LOG_NCQ_PRIO_OFFSET] & BIT(3)) {
-		dev->flags |= ATA_DFLAG_NCQ_PRIO;
-	} else {
-		dev->flags &= ~ATA_DFLAG_NCQ_PRIO;
-		ata_dev_dbg(dev, "SATA page does not support priority\n");
-	}
+	if (!(ap->sector_buf[ATA_LOG_NCQ_PRIO_OFFSET] & BIT(3)))
+		goto not_supported;
+
+	dev->flags |= ATA_DFLAG_NCQ_PRIO;
+
+	return;
 
+not_supported:
+	dev->flags &= ~ATA_DFLAG_NCQ_PRIO_ENABLE;
+	dev->flags &= ~ATA_DFLAG_NCQ_PRIO;
 }
 
 static int ata_dev_config_ncq(struct ata_device *dev,
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 8adeab76dd38..dc397ebda089 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -839,23 +839,17 @@ static ssize_t ata_ncq_prio_enable_show(struct device *device,
 					char *buf)
 {
 	struct scsi_device *sdev = to_scsi_device(device);
-	struct ata_port *ap;
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
 	struct ata_device *dev;
 	bool ncq_prio_enable;
 	int rc = 0;
 
-	ap = ata_shost_to_port(sdev->host);
-
 	spin_lock_irq(ap->lock);
 	dev = ata_scsi_find_dev(ap, sdev);
-	if (!dev) {
+	if (!dev)
 		rc = -ENODEV;
-		goto unlock;
-	}
-
-	ncq_prio_enable = dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLE;
-
-unlock:
+	else
+		ncq_prio_enable = dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLE;
 	spin_unlock_irq(ap->lock);
 
 	return rc ? rc : snprintf(buf, 20, "%u\n", ncq_prio_enable);
@@ -869,7 +863,7 @@ static ssize_t ata_ncq_prio_enable_store(struct device *device,
 	struct ata_port *ap;
 	struct ata_device *dev;
 	long int input;
-	int rc;
+	int rc = 0;
 
 	rc = kstrtol(buf, 10, &input);
 	if (rc)
@@ -883,27 +877,20 @@ static ssize_t ata_ncq_prio_enable_store(struct device *device,
 		return  -ENODEV;
 
 	spin_lock_irq(ap->lock);
+
+	if (!(dev->flags & ATA_DFLAG_NCQ_PRIO)) {
+		rc = -EINVAL;
+		goto unlock;
+	}
+
 	if (input)
 		dev->flags |= ATA_DFLAG_NCQ_PRIO_ENABLE;
 	else
 		dev->flags &= ~ATA_DFLAG_NCQ_PRIO_ENABLE;
 
-	dev->link->eh_info.action |= ATA_EH_REVALIDATE;
-	dev->link->eh_info.flags |= ATA_EHI_QUIET;
-	ata_port_schedule_eh(ap);
+unlock:
 	spin_unlock_irq(ap->lock);
 
-	ata_port_wait_eh(ap);
-
-	if (input) {
-		spin_lock_irq(ap->lock);
-		if (!(dev->flags & ATA_DFLAG_NCQ_PRIO)) {
-			dev->flags &= ~ATA_DFLAG_NCQ_PRIO_ENABLE;
-			rc = -EIO;
-		}
-		spin_unlock_irq(ap->lock);
-	}
-
 	return rc ? rc : len;
 }
 
-- 
2.31.1

[PATCH v2 6/9] libata: fix ata_read_log_page() warning

[Damien Le Moal]

Support for the READ LOG PAGE DMA EXT command is indicated by words 119
and 120 of a device identify data. This is tested in
ata_read_log_page() with ata_id_has_read_log_dma_ext() and the
READ LOG PAGE DMA command used if the device reports supports for it.

However, some devices lie about this support and using the DMA version
of the command fails, generating the warning message "READ LOG DMA EXT
failed, trying PIO". Since READ LOG PAGE DMA EXT is an optional command,
this warning is not at all important but may be scary for the user.
Change ata_read_log_page() to suppres this warning and to print an
error message if both DMA and PIO attempts failed.

With this change, there is no need to print again an error message when
ata_read_log_page() returns an error. So simplify the users of this
function.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index b556401cc110..3c213d125e95 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2021,13 +2021,15 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
 				     buf, sectors * ATA_SECT_SIZE, 0);
 
-	if (err_mask && dma) {
-		dev->horkage |= ATA_HORKAGE_NO_DMA_LOG;
-		ata_dev_warn(dev, "READ LOG DMA EXT failed, trying PIO\n");
-		goto retry;
+	if (err_mask) {
+		if (dma) {
+			dev->horkage |= ATA_HORKAGE_NO_DMA_LOG;
+			goto retry;
+		}
+		ata_dev_err(dev, "Read log page 0x%02x failed, Emask 0x%x\n",
+			    (unsigned int)page, err_mask);
 	}
 
-	DPRINTK("EXIT, err_mask=%x\n", err_mask);
 	return err_mask;
 }
 
@@ -2056,12 +2058,8 @@ static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
 	 */
 	err = ata_read_log_page(dev, ATA_LOG_IDENTIFY_DEVICE, 0, ap->sector_buf,
 				1);
-	if (err) {
-		ata_dev_info(dev,
-			     "failed to get Device Identify Log Emask 0x%x\n",
-			     err);
+	if (err)
 		return false;
-	}
 
 	for (i = 0; i < ap->sector_buf[8]; i++) {
 		if (ap->sector_buf[9 + i] == page)
@@ -2125,11 +2123,7 @@ static void ata_dev_config_ncq_send_recv(struct ata_device *dev)
 	}
 	err_mask = ata_read_log_page(dev, ATA_LOG_NCQ_SEND_RECV,
 				     0, ap->sector_buf, 1);
-	if (err_mask) {
-		ata_dev_dbg(dev,
-			    "failed to get NCQ Send/Recv Log Emask 0x%x\n",
-			    err_mask);
-	} else {
+	if (!err_mask) {
 		u8 *cmds = dev->ncq_send_recv_cmds;
 
 		dev->flags |= ATA_DFLAG_NCQ_SEND_RECV;
@@ -2155,11 +2149,7 @@ static void ata_dev_config_ncq_non_data(struct ata_device *dev)
 	}
 	err_mask = ata_read_log_page(dev, ATA_LOG_NCQ_NON_DATA,
 				     0, ap->sector_buf, 1);
-	if (err_mask) {
-		ata_dev_dbg(dev,
-			    "failed to get NCQ Non-Data Log Emask 0x%x\n",
-			    err_mask);
-	} else {
+	if (!err_mask) {
 		u8 *cmds = dev->ncq_non_data_cmds;
 
 		memcpy(cmds, ap->sector_buf, ATA_LOG_NCQ_NON_DATA_SIZE);
@@ -2176,12 +2166,8 @@ static void ata_dev_config_ncq_prio(struct ata_device *dev)
 				     ATA_LOG_SATA_SETTINGS,
 				     ap->sector_buf,
 				     1);
-	if (err_mask) {
-		ata_dev_dbg(dev,
-			    "failed to get SATA settings log, Emask 0x%x\n",
-			    err_mask);
+	if (err_mask)
 		goto not_supported;
-	}
 
 	if (!(ap->sector_buf[ATA_LOG_NCQ_PRIO_OFFSET] & BIT(3)))
 		goto not_supported;
@@ -2342,11 +2328,8 @@ static void ata_dev_config_trusted(struct ata_device *dev)
 
 	err = ata_read_log_page(dev, ATA_LOG_IDENTIFY_DEVICE, ATA_LOG_SECURITY,
 			ap->sector_buf, 1);
-	if (err) {
-		ata_dev_dbg(dev,
-			    "failed to read Security Log, Emask 0x%x\n", err);
+	if (err)
 		return;
-	}
 
 	trusted_cap = get_unaligned_le64(&ap->sector_buf[40]);
 	if (!(trusted_cap & (1ULL << 63))) {
@@ -2422,12 +2405,8 @@ static void ata_dev_config_devslp(struct ata_device *dev)
 				     ATA_LOG_IDENTIFY_DEVICE,
 				     ATA_LOG_SATA_SETTINGS,
 				     sata_setting, 1);
-	if (err_mask) {
-		ata_dev_dbg(dev,
-			    "failed to get SATA Settings Log, Emask 0x%x\n",
-			    err_mask);
+	if (err_mask)
 		return;
-	}
 
 	dev->flags |= ATA_DFLAG_DEVSLP;
 	for (i = 0; i < ATA_LOG_DEVSLP_SIZE; i++) {
-- 
2.31.1

[PATCH v2 7/9] libata: print feature list on device scan

[Damien Le Moal]

Print a list of features supported by a drive when it is configured in
ata_dev_configure() using the new function ata_dev_print_features().
The features printed are not already advertized and are: trusted
send-recev support, device attention support, device sleep support,
NCQ send-recv support and NCQ priority support.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/ata/libata-core.c | 17 +++++++++++++++++
 include/linux/libata.h    |  4 ++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3c213d125e95..cef6d516b4d8 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2415,6 +2415,20 @@ static void ata_dev_config_devslp(struct ata_device *dev)
 	}
 }
 
+static void ata_dev_print_features(struct ata_device *dev)
+{
+	if (!(dev->flags & ATA_DFLAG_FEATURES_MASK))
+		return;
+
+	ata_dev_info(dev,
+		     "Features:%s%s%s%s%s\n",
+		     dev->flags & ATA_DFLAG_TRUSTED ? " Trust" : "",
+		     dev->flags & ATA_DFLAG_DA ? " Dev-Attention" : "",
+		     dev->flags & ATA_DFLAG_DEVSLP ? " Dev-Sleep" : "",
+		     dev->flags & ATA_DFLAG_NCQ_SEND_RECV ? " NCQ-sndrcv" : "",
+		     dev->flags & ATA_DFLAG_NCQ_PRIO ? " NCQ-prio" : "");
+}
+
 /**
  *	ata_dev_configure - Configure the specified ATA/ATAPI device
  *	@dev: Target device to configure
@@ -2584,6 +2598,9 @@ int ata_dev_configure(struct ata_device *dev)
 		ata_dev_config_zac(dev);
 		ata_dev_config_trusted(dev);
 		dev->cdb_len = 32;
+
+		if (ata_msg_drv(ap) && print_info)
+			ata_dev_print_features(dev);
 	}
 
 	/* ATAPI-specific feature tests */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3fcd24236793..b23f28cfc8e0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -161,6 +161,10 @@ 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_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
 	ATA_DEV_ATA_UNSUP	= 2,	/* ATA device (unsupported) */
-- 
2.31.1

[PATCH v2 8/9] libahci: Introduce ncq_prio_supported sysfs sttribute

[Damien Le Moal]

Currently, the only way a user can determine if a SATA device supports
NCQ priority is to try to enable the use of this feature using the
ncq_prio_enable sysfs device attribute. If enabling the feature fails,
it is because the device does not support NCQ priority. Otherwise, the
feature is enabled and indicates that the device supports NCQ priority.

Improve this odd interface by introducing the read-only
ncq_prio_supported sysfs device attribute to indicate if a SATA device
supports NCQ priority. The value of this attribute reflects if the
device flag ATA_DFLAG_NCQ_PRIO is set or cleared.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/ata/libahci.c     |  1 +
 drivers/ata/libata-sata.c | 24 ++++++++++++++++++++++++
 include/linux/libata.h    |  1 +
 3 files changed, 26 insertions(+)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index fec2e9754aed..5b3fa2cbe722 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -125,6 +125,7 @@ EXPORT_SYMBOL_GPL(ahci_shost_attrs);
 struct device_attribute *ahci_sdev_attrs[] = {
 	&dev_attr_sw_activity,
 	&dev_attr_unload_heads,
+	&dev_attr_ncq_prio_supported,
 	&dev_attr_ncq_prio_enable,
 	NULL
 };
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index dc397ebda089..5566fd4bb38f 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -834,6 +834,30 @@ DEVICE_ATTR(link_power_management_policy, S_IRUGO | S_IWUSR,
 	    ata_scsi_lpm_show, ata_scsi_lpm_store);
 EXPORT_SYMBOL_GPL(dev_attr_link_power_management_policy);
 
+static ssize_t ata_ncq_prio_supported_show(struct device *device,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(device);
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
+	struct ata_device *dev;
+	bool ncq_prio_supported;
+	int rc = 0;
+
+	spin_lock_irq(ap->lock);
+	dev = ata_scsi_find_dev(ap, sdev);
+	if (!dev)
+		rc = -ENODEV;
+	else
+		ncq_prio_supported = dev->flags & ATA_DFLAG_NCQ_PRIO;
+	spin_unlock_irq(ap->lock);
+
+	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_supported);
+}
+
+DEVICE_ATTR(ncq_prio_supported, S_IRUGO, ata_ncq_prio_supported_show, NULL);
+EXPORT_SYMBOL_GPL(dev_attr_ncq_prio_supported);
+
 static ssize_t ata_ncq_prio_enable_show(struct device *device,
 					struct device_attribute *attr,
 					char *buf)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b23f28cfc8e0..a2d1bae7900b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -539,6 +539,7 @@ typedef void (*ata_postreset_fn_t)(struct ata_link *link, unsigned int *classes)
 extern struct device_attribute dev_attr_unload_heads;
 #ifdef CONFIG_SATA_HOST
 extern struct device_attribute dev_attr_link_power_management_policy;
+extern struct device_attribute dev_attr_ncq_prio_supported;
 extern struct device_attribute dev_attr_ncq_prio_enable;
 extern struct device_attribute dev_attr_em_message_type;
 extern struct device_attribute dev_attr_em_message;
-- 
2.31.1

[PATCH v2 9/9] scsi: mpt3sas: Introduce sas_ncq_prio_supported sysfs sttribute

[Damien Le Moal]

Similarly to AHCI, introduce the device sysfs attribute
sas_ncq_prio_supported to advertize if a SATA device supports the NCQ
priority feature. Without this new attribute, the user can only
discover if a SATA device supports NCQ priority by trying to enable
the feature use with the sas_ncq_prio_enable sysfs device attribute,
which fails when the device does not support high priroity commands.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index b66140e4c370..f83d4d32d459 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -3918,6 +3918,24 @@ sas_device_handle_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(sas_device_handle);
 
+/**
+ * sas_ncq_prio_supported_show - Indicate if device supports NCQ priority
+ * @dev: pointer to embedded device
+ * @attr: sas_ncq_prio_supported attribute descriptor
+ * @buf: the buffer returned
+ *
+ * A sysfs 'read-only' sdev attribute, only works with SATA
+ */
+static ssize_t
+sas_ncq_prio_supported_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+
+	return sysfs_emit(buf, "%d\n", scsih_ncq_prio_supp(sdev));
+}
+static DEVICE_ATTR_RO(sas_ncq_prio_supported);
+
 /**
  * sas_ncq_prio_enable_show - send prioritized io commands to device
  * @dev: pointer to embedded device
@@ -3960,6 +3978,7 @@ static DEVICE_ATTR_RW(sas_ncq_prio_enable);
 struct device_attribute *mpt3sas_dev_attrs[] = {
 	&dev_attr_sas_address,
 	&dev_attr_sas_device_handle,
+	&dev_attr_sas_ncq_prio_supported,
 	&dev_attr_sas_ncq_prio_enable,
 	NULL,
 };
-- 
2.31.1

Re: [PATCH v2 1/9] libata: fix ata_host_alloc_pinfo()

[Hannes Reinecke]

On 8/6/21 9:42 AM, Damien Le Moal wrote:
> Avoid a potential NULL pointer dereference by testing that the ATA port
> info variable "pi".
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/ata/libata-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 61c762961ca8..ea8b91297f12 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5458,7 +5458,7 @@ struct ata_host *ata_host_alloc_pinfo(struct device *dev,
>  		ap->link.flags |= pi->link_flags;
>  		ap->ops = pi->port_ops;
>  
> -		if (!host->ops && (pi->port_ops != &ata_dummy_port_ops))
> +		if (!host->ops && pi && pi->port_ops != &ata_dummy_port_ops)
>  			host->ops = pi->port_ops;
>  	}
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

Re: [PATCH v2 2/9] libata: fix ata_host_start()

[Hannes Reinecke]

On 8/6/21 9:42 AM, Damien Le Moal wrote:
> The loop on entry of ata_host_start() may not initialize host->ops to a
> non NULL value. The test on the host_stop field of host->ops must then
> be preceded by a check that host->ops is not NULL.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/ata/libata-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index ea8b91297f12..fe49197caf99 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5573,7 +5573,7 @@ int ata_host_start(struct ata_host *host)
>  			have_stop = 1;
>  	}
>  
> -	if (host->ops->host_stop)
> +	if (host->ops && host->ops->host_stop)
>  		have_stop = 1;
>  
>  	if (have_stop) {
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

Re: [PATCH v2 3/9] libata: cleanup device sleep capability detection

[Hannes Reinecke]

On 8/6/21 9:42 AM, Damien Le Moal wrote:
> Move the code to retrieve the device sleep capability and timings out of
> ata_dev_configure() into the helper function ata_dev_config_devslp().
> While at it, mark the device as supporting the device sleep capability
> only if the sata settings page was retrieved successfully to ensure that
> the timing information is correctly initialized.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/ata/libata-core.c | 55 +++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 23 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

Re: [PATCH v2 4/9] libata: cleanup ata_dev_configure()

[Hannes Reinecke]

On 8/6/21 9:42 AM, Damien Le Moal wrote:
> Introduce the helper functions ata_dev_config_lba() and
> ata_dev_config_chs() to configure the addressing capabilities of a
> device. Each helper takes a string as argument for the addressing
> information printed after these helpers execution completes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/ata/libata-core.c | 110 ++++++++++++++++++++------------------
>  1 file changed, 59 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index b13194432e5a..2b6054cdd8fc 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2363,6 +2363,52 @@ static void ata_dev_config_trusted(struct ata_device *dev)
>  		dev->flags |= ATA_DFLAG_TRUSTED;
>  }
>  
> +static int ata_dev_config_lba(struct ata_device *dev,
> +			      char *info, size_t infosz)
> +{
> +	const u16 *id = dev->id;
> +	int info_ofst;
> +
> +	dev->flags |= ATA_DFLAG_LBA;
> +
> +	if (ata_id_has_lba48(id)) {
> +		dev->flags |= ATA_DFLAG_LBA48;
> +		strscpy(info, "LBA48 ", infosz);
> +
> +		if (dev->n_sectors >= (1UL << 28) &&
> +		    ata_id_has_flush_ext(id))
> +			dev->flags |= ATA_DFLAG_FLUSH_EXT;
> +	} else {
> +		strscpy(info, "LBA ", infosz);
> +	}
> +	info_ofst = strlen(info);
> +
> +	/* config NCQ */
> +	return ata_dev_config_ncq(dev, info + info_ofst,
> +				  infosz - info_ofst);
> +}
> +
> +static void ata_dev_config_chs(struct ata_device *dev,
> +			       char *info, size_t infosz)
> +{
> +	const u16 *id = dev->id;
> +
> +	/* Default translation */
> +	dev->cylinders	= id[1];
> +	dev->heads	= id[3];
> +	dev->sectors	= id[6];
> +
> +	if (ata_id_current_chs_valid(id)) {
> +		/* Current CHS translation is valid. */
> +		dev->cylinders = id[54];
> +		dev->heads     = id[55];
> +		dev->sectors   = id[56];
> +	}
> +
> +	snprintf(info, infosz, "CHS %u/%u/%u",
> +		 dev->cylinders, dev->heads, dev->sectors);
> +}
> +
>  static void ata_dev_config_devslp(struct ata_device *dev)
>  {
>  	u8 *sata_setting = dev->link->ap->sector_buf;
> @@ -2418,6 +2464,7 @@ int ata_dev_configure(struct ata_device *dev)
>  	char revbuf[7];		/* XYZ-99\0 */
>  	char fwrevbuf[ATA_ID_FW_REV_LEN+1];
>  	char modelbuf[ATA_ID_PROD_LEN+1];
> +	char lba_info[40];
>  	int rc;
>  
>  	if (!ata_dev_enabled(dev) && ata_msg_info(ap)) {
> @@ -2539,61 +2586,22 @@ int ata_dev_configure(struct ata_device *dev)
>  		}
>  
>  		if (ata_id_has_lba(id)) {
> -			const char *lba_desc;
> -			char ncq_desc[24];
> -
> -			lba_desc = "LBA";
> -			dev->flags |= ATA_DFLAG_LBA;
> -			if (ata_id_has_lba48(id)) {
> -				dev->flags |= ATA_DFLAG_LBA48;
> -				lba_desc = "LBA48";
> -
> -				if (dev->n_sectors >= (1UL << 28) &&
> -				    ata_id_has_flush_ext(id))
> -					dev->flags |= ATA_DFLAG_FLUSH_EXT;
> -			}
> -
> -			/* config NCQ */
> -			rc = ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
> +			rc = ata_dev_config_lba(dev, lba_info, sizeof(lba_info));
>  			if (rc)
>  				return rc;
> -
> -			/* print device info to dmesg */
> -			if (ata_msg_drv(ap) && print_info) {
> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
> -					     revbuf, modelbuf, fwrevbuf,
> -					     ata_mode_string(xfer_mask));
> -				ata_dev_info(dev,
> -					     "%llu sectors, multi %u: %s %s\n",
> -					(unsigned long long)dev->n_sectors,
> -					dev->multi_count, lba_desc, ncq_desc);
> -			}
>  		} else {
> -			/* CHS */
> -
> -			/* Default translation */
> -			dev->cylinders	= id[1];
> -			dev->heads	= id[3];
> -			dev->sectors	= id[6];
> -
> -			if (ata_id_current_chs_valid(id)) {
> -				/* Current CHS translation is valid. */
> -				dev->cylinders = id[54];
> -				dev->heads     = id[55];
> -				dev->sectors   = id[56];
> -			}
> +			ata_dev_config_chs(dev, lba_info, sizeof(lba_info));
> +		}
>  
> -			/* print device info to dmesg */
> -			if (ata_msg_drv(ap) && print_info) {
> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
> -					     revbuf,	modelbuf, fwrevbuf,
> -					     ata_mode_string(xfer_mask));
> -				ata_dev_info(dev,
> -					     "%llu sectors, multi %u, CHS %u/%u/%u\n",
> -					     (unsigned long long)dev->n_sectors,
> -					     dev->multi_count, dev->cylinders,
> -					     dev->heads, dev->sectors);
> -			}
> +		/* print device info to dmesg */
> +		if (ata_msg_drv(ap) && print_info) {
> +			ata_dev_info(dev, "%s: %s, %s, max %s\n",
> +				     revbuf, modelbuf, fwrevbuf,
> +				     ata_mode_string(xfer_mask));
> +			ata_dev_info(dev,
> +				     "%llu sectors, multi %u, %s\n",
> +				     (unsigned long long)dev->n_sectors,
> +				     dev->multi_count, lba_info);
>  		}
>  
>  		ata_dev_config_devslp(dev);
> 
Hmm. Can't say I like it.
Can't you move the second 'ata_dev_info()' call into the respective
functions, and kill the temporary buffer?

Cheers,

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

Re: [PATCH v2 5/9] libata: cleanup NCQ priority handling

[Hannes Reinecke]

On 8/6/21 9:42 AM, Damien Le Moal wrote:
> The ata device flag ATA_DFLAG_NCQ_PRIO indicates if a device supports
> the NCQ Priority feature while the ATA_DFLAG_NCQ_PRIO_ENABLE device
> flag indicates if the feature is enabled. Enabling NCQ priority use is
> controlled by the user through the device sysfs attribute
> ncq_prio_enable. As a result, the ATA_DFLAG_NCQ_PRIO flag should not be
> cleared when ATA_DFLAG_NCQ_PRIO_ENABLE is not set as the device still
> supports the feature even after the user disables it. This leads to the
> following cleanups:
> - In ata_build_rw_tf(), set a command high priority bit based on the
>   ATA_DFLAG_NCQ_PRIO_ENABLE flag, not on the ATA_DFLAG_NCQ flag. That
>   is, set a command high priority only if the user enabled NCQ priority
>   use.
> - In ata_dev_config_ncq_prio(), ATA_DFLAG_NCQ_PRIO should not be cleared
>   if ATA_DFLAG_NCQ_PRIO_ENABLE is not set. If the device does not
>   support NCQ priority, both ATA_DFLAG_NCQ_PRIO and
>   ATA_DFLAG_NCQ_PRIO_ENABLE must be cleared.
> 
> With the above ata_dev_config_ncq_prio() change, ATA_DFLAG_NCQ_PRIO flag
> is set on device scan and revalidation. There is no need to trigger a
> device revalidation in ata_ncq_prio_enable_store() when the user enables
> the use of NCQ priority. Remove the revalidation code from that funciton
> to simplify it. Also change the return value from -EIO to -EINVAL when a
> user tries to enable NCQ priority for a device that does not support
> this feature.  While at it, also simplify ata_ncq_prio_enable_show().
> 
> Overall, there is no functional change introduced by this patch.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/ata/libata-core.c | 32 ++++++++++++++------------------
>  drivers/ata/libata-sata.c | 37 ++++++++++++-------------------------
>  2 files changed, 26 insertions(+), 43 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

Re: [PATCH v2 6/9] libata: fix ata_read_log_page() warning

[Hannes Reinecke]

On 8/6/21 9:42 AM, Damien Le Moal wrote:
> Support for the READ LOG PAGE DMA EXT command is indicated by words 119
> and 120 of a device identify data. This is tested in
> ata_read_log_page() with ata_id_has_read_log_dma_ext() and the
> READ LOG PAGE DMA command used if the device reports supports for it.
> 
> However, some devices lie about this support and using the DMA version
> of the command fails, generating the warning message "READ LOG DMA EXT
> failed, trying PIO". Since READ LOG PAGE DMA EXT is an optional command,
> this warning is not at all important but may be scary for the user.
> Change ata_read_log_page() to suppres this warning and to print an
> error message if both DMA and PIO attempts failed.
> 
> With this change, there is no need to print again an error message when
> ata_read_log_page() returns an error. So simplify the users of this
> function.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/ata/libata-core.c | 47 +++++++++++----------------------------
>  1 file changed, 13 insertions(+), 34 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

Re: [PATCH v2 7/9] libata: print feature list on device scan

[Hannes Reinecke]

On 8/6/21 9:42 AM, Damien Le Moal wrote:
> Print a list of features supported by a drive when it is configured in
> ata_dev_configure() using the new function ata_dev_print_features().
> The features printed are not already advertized and are: trusted
> send-recev support, device attention support, device sleep support,
> NCQ send-recv support and NCQ priority support.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/ata/libata-core.c | 17 +++++++++++++++++
>  include/linux/libata.h    |  4 ++++
>  2 files changed, 21 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@use.de>

Cheers,

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

Re: [PATCH v2 4/9] libata: cleanup ata_dev_configure()

[Damien Le Moal]

On 2021/08/06 18:07, Hannes Reinecke wrote:
> On 8/6/21 9:42 AM, Damien Le Moal wrote:
>> Introduce the helper functions ata_dev_config_lba() and
>> ata_dev_config_chs() to configure the addressing capabilities of a
>> device. Each helper takes a string as argument for the addressing
>> information printed after these helpers execution completes.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  drivers/ata/libata-core.c | 110 ++++++++++++++++++++------------------
>>  1 file changed, 59 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index b13194432e5a..2b6054cdd8fc 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2363,6 +2363,52 @@ static void ata_dev_config_trusted(struct ata_device *dev)
>>  		dev->flags |= ATA_DFLAG_TRUSTED;
>>  }
>>  
>> +static int ata_dev_config_lba(struct ata_device *dev,
>> +			      char *info, size_t infosz)
>> +{
>> +	const u16 *id = dev->id;
>> +	int info_ofst;
>> +
>> +	dev->flags |= ATA_DFLAG_LBA;
>> +
>> +	if (ata_id_has_lba48(id)) {
>> +		dev->flags |= ATA_DFLAG_LBA48;
>> +		strscpy(info, "LBA48 ", infosz);
>> +
>> +		if (dev->n_sectors >= (1UL << 28) &&
>> +		    ata_id_has_flush_ext(id))
>> +			dev->flags |= ATA_DFLAG_FLUSH_EXT;
>> +	} else {
>> +		strscpy(info, "LBA ", infosz);
>> +	}
>> +	info_ofst = strlen(info);
>> +
>> +	/* config NCQ */
>> +	return ata_dev_config_ncq(dev, info + info_ofst,
>> +				  infosz - info_ofst);
>> +}
>> +
>> +static void ata_dev_config_chs(struct ata_device *dev,
>> +			       char *info, size_t infosz)
>> +{
>> +	const u16 *id = dev->id;
>> +
>> +	/* Default translation */
>> +	dev->cylinders	= id[1];
>> +	dev->heads	= id[3];
>> +	dev->sectors	= id[6];
>> +
>> +	if (ata_id_current_chs_valid(id)) {
>> +		/* Current CHS translation is valid. */
>> +		dev->cylinders = id[54];
>> +		dev->heads     = id[55];
>> +		dev->sectors   = id[56];
>> +	}
>> +
>> +	snprintf(info, infosz, "CHS %u/%u/%u",
>> +		 dev->cylinders, dev->heads, dev->sectors);
>> +}
>> +
>>  static void ata_dev_config_devslp(struct ata_device *dev)
>>  {
>>  	u8 *sata_setting = dev->link->ap->sector_buf;
>> @@ -2418,6 +2464,7 @@ int ata_dev_configure(struct ata_device *dev)
>>  	char revbuf[7];		/* XYZ-99\0 */
>>  	char fwrevbuf[ATA_ID_FW_REV_LEN+1];
>>  	char modelbuf[ATA_ID_PROD_LEN+1];
>> +	char lba_info[40];
>>  	int rc;
>>  
>>  	if (!ata_dev_enabled(dev) && ata_msg_info(ap)) {
>> @@ -2539,61 +2586,22 @@ int ata_dev_configure(struct ata_device *dev)
>>  		}
>>  
>>  		if (ata_id_has_lba(id)) {
>> -			const char *lba_desc;
>> -			char ncq_desc[24];
>> -
>> -			lba_desc = "LBA";
>> -			dev->flags |= ATA_DFLAG_LBA;
>> -			if (ata_id_has_lba48(id)) {
>> -				dev->flags |= ATA_DFLAG_LBA48;
>> -				lba_desc = "LBA48";
>> -
>> -				if (dev->n_sectors >= (1UL << 28) &&
>> -				    ata_id_has_flush_ext(id))
>> -					dev->flags |= ATA_DFLAG_FLUSH_EXT;
>> -			}
>> -
>> -			/* config NCQ */
>> -			rc = ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
>> +			rc = ata_dev_config_lba(dev, lba_info, sizeof(lba_info));
>>  			if (rc)
>>  				return rc;
>> -
>> -			/* print device info to dmesg */
>> -			if (ata_msg_drv(ap) && print_info) {
>> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
>> -					     revbuf, modelbuf, fwrevbuf,
>> -					     ata_mode_string(xfer_mask));
>> -				ata_dev_info(dev,
>> -					     "%llu sectors, multi %u: %s %s\n",
>> -					(unsigned long long)dev->n_sectors,
>> -					dev->multi_count, lba_desc, ncq_desc);
>> -			}
>>  		} else {
>> -			/* CHS */
>> -
>> -			/* Default translation */
>> -			dev->cylinders	= id[1];
>> -			dev->heads	= id[3];
>> -			dev->sectors	= id[6];
>> -
>> -			if (ata_id_current_chs_valid(id)) {
>> -				/* Current CHS translation is valid. */
>> -				dev->cylinders = id[54];
>> -				dev->heads     = id[55];
>> -				dev->sectors   = id[56];
>> -			}
>> +			ata_dev_config_chs(dev, lba_info, sizeof(lba_info));
>> +		}
>>  
>> -			/* print device info to dmesg */
>> -			if (ata_msg_drv(ap) && print_info) {
>> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
>> -					     revbuf,	modelbuf, fwrevbuf,
>> -					     ata_mode_string(xfer_mask));
>> -				ata_dev_info(dev,
>> -					     "%llu sectors, multi %u, CHS %u/%u/%u\n",
>> -					     (unsigned long long)dev->n_sectors,
>> -					     dev->multi_count, dev->cylinders,
>> -					     dev->heads, dev->sectors);
>> -			}
>> +		/* print device info to dmesg */
>> +		if (ata_msg_drv(ap) && print_info) {
>> +			ata_dev_info(dev, "%s: %s, %s, max %s\n",
>> +				     revbuf, modelbuf, fwrevbuf,
>> +				     ata_mode_string(xfer_mask));
>> +			ata_dev_info(dev,
>> +				     "%llu sectors, multi %u, %s\n",
>> +				     (unsigned long long)dev->n_sectors,
>> +				     dev->multi_count, lba_info);
>>  		}
>>  
>>  		ata_dev_config_devslp(dev);
>>
> Hmm. Can't say I like it.
> Can't you move the second 'ata_dev_info()' call into the respective
> functions, and kill the temporary buffer?

That would reverse the order of the messages... And I wanted to avoid having an
"if (ata_id_has_lba(id))" again just for the print. Moving the 2 ata_dev_info()
calls into the respective functions was the other solution I tried, but then the
functions require *a lot* more arguments (revbuf, modelbuf, fwrevbuf, ...) wich
was not super nice.

This one is the least ugly I thought... Any other idea ?

> 
> Cheers,
> 
> Hannes
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 9/9] scsi: mpt3sas: Introduce sas_ncq_prio_supported sysfs sttribute

[Hannes Reinecke]

On 8/6/21 9:42 AM, Damien Le Moal wrote:
> Similarly to AHCI, introduce the device sysfs attribute
> sas_ncq_prio_supported to advertize if a SATA device supports the NCQ
> priority feature. Without this new attribute, the user can only
> discover if a SATA device supports NCQ priority by trying to enable
> the feature use with the sas_ncq_prio_enable sysfs device attribute,
> which fails when the device does not support high priroity commands.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index b66140e4c370..f83d4d32d459 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -3918,6 +3918,24 @@ sas_device_handle_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(sas_device_handle);
>  
> +/**
> + * sas_ncq_prio_supported_show - Indicate if device supports NCQ priority
> + * @dev: pointer to embedded device
> + * @attr: sas_ncq_prio_supported attribute descriptor
> + * @buf: the buffer returned
> + *
> + * A sysfs 'read-only' sdev attribute, only works with SATA
> + */
> +static ssize_t
> +sas_ncq_prio_supported_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +
> +	return sysfs_emit(buf, "%d\n", scsih_ncq_prio_supp(sdev));
> +}
> +static DEVICE_ATTR_RO(sas_ncq_prio_supported);
> +
>  /**
>   * sas_ncq_prio_enable_show - send prioritized io commands to device
>   * @dev: pointer to embedded device
> @@ -3960,6 +3978,7 @@ static DEVICE_ATTR_RW(sas_ncq_prio_enable);
>  struct device_attribute *mpt3sas_dev_attrs[] = {
>  	&dev_attr_sas_address,
>  	&dev_attr_sas_device_handle,
> +	&dev_attr_sas_ncq_prio_supported,
>  	&dev_attr_sas_ncq_prio_enable,
>  	NULL,
>  };
> 
One wonders where the relationship to the previous patches are, but hey:

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

Cheers,

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

Re: [PATCH v2 9/9] scsi: mpt3sas: Introduce sas_ncq_prio_supported sysfs sttribute

[Damien Le Moal]

On 2021/08/06 18:12, Hannes Reinecke wrote:
> On 8/6/21 9:42 AM, Damien Le Moal wrote:
>> Similarly to AHCI, introduce the device sysfs attribute
>> sas_ncq_prio_supported to advertize if a SATA device supports the NCQ
>> priority feature. Without this new attribute, the user can only
>> discover if a SATA device supports NCQ priority by trying to enable
>> the feature use with the sas_ncq_prio_enable sysfs device attribute,
>> which fails when the device does not support high priroity commands.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_ctl.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> index b66140e4c370..f83d4d32d459 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> @@ -3918,6 +3918,24 @@ sas_device_handle_show(struct device *dev, struct device_attribute *attr,
>>  }
>>  static DEVICE_ATTR_RO(sas_device_handle);
>>  
>> +/**
>> + * sas_ncq_prio_supported_show - Indicate if device supports NCQ priority
>> + * @dev: pointer to embedded device
>> + * @attr: sas_ncq_prio_supported attribute descriptor
>> + * @buf: the buffer returned
>> + *
>> + * A sysfs 'read-only' sdev attribute, only works with SATA
>> + */
>> +static ssize_t
>> +sas_ncq_prio_supported_show(struct device *dev,
>> +			    struct device_attribute *attr, char *buf)
>> +{
>> +	struct scsi_device *sdev = to_scsi_device(dev);
>> +
>> +	return sysfs_emit(buf, "%d\n", scsih_ncq_prio_supp(sdev));
>> +}
>> +static DEVICE_ATTR_RO(sas_ncq_prio_supported);
>> +
>>  /**
>>   * sas_ncq_prio_enable_show - send prioritized io commands to device
>>   * @dev: pointer to embedded device
>> @@ -3960,6 +3978,7 @@ static DEVICE_ATTR_RW(sas_ncq_prio_enable);
>>  struct device_attribute *mpt3sas_dev_attrs[] = {
>>  	&dev_attr_sas_address,
>>  	&dev_attr_sas_device_handle,
>> +	&dev_attr_sas_ncq_prio_supported,
>>  	&dev_attr_sas_ncq_prio_enable,
>>  	NULL,
>>  };
>>
> One wonders where the relationship to the previous patches are, but hey:

Yes, hard to see... mpt3sas is the only SAS HBA that can actually carry the
"high prio" bit for the command to the HBA FW SATL using the prio bit of the SAS
frame for the SCSI command that is to be translated into a high prio SATA command.

This driver supports NCQ priority since the first support series in 4.10 kernel.
So I added the same change as for libahci to keep it in sync with the pure ATA
side of things.

But sure, it could be a patch on its own outside this series.

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


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 4/9] libata: cleanup ata_dev_configure()

[Hannes Reinecke]

On 8/6/21 11:12 AM, Damien Le Moal wrote:
> On 2021/08/06 18:07, Hannes Reinecke wrote:
>> On 8/6/21 9:42 AM, Damien Le Moal wrote:
>>> Introduce the helper functions ata_dev_config_lba() and
>>> ata_dev_config_chs() to configure the addressing capabilities of a
>>> device. Each helper takes a string as argument for the addressing
>>> information printed after these helpers execution completes.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>> ---
>>>  drivers/ata/libata-core.c | 110 ++++++++++++++++++++------------------
>>>  1 file changed, 59 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index b13194432e5a..2b6054cdd8fc 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
[ .. ]
>>>  				return rc;
>>> -
>>> -			/* print device info to dmesg */
>>> -			if (ata_msg_drv(ap) && print_info) {
>>> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
>>> -					     revbuf, modelbuf, fwrevbuf,
>>> -					     ata_mode_string(xfer_mask));
>>> -				ata_dev_info(dev,
>>> -					     "%llu sectors, multi %u: %s %s\n",
>>> -					(unsigned long long)dev->n_sectors,
>>> -					dev->multi_count, lba_desc, ncq_desc);
>>> -			}
>>>  		} else {
>>> -			/* CHS */
>>> -
>>> -			/* Default translation */
>>> -			dev->cylinders	= id[1];
>>> -			dev->heads	= id[3];
>>> -			dev->sectors	= id[6];
>>> -
>>> -			if (ata_id_current_chs_valid(id)) {
>>> -				/* Current CHS translation is valid. */
>>> -				dev->cylinders = id[54];
>>> -				dev->heads     = id[55];
>>> -				dev->sectors   = id[56];
>>> -			}
>>> +			ata_dev_config_chs(dev, lba_info, sizeof(lba_info));
>>> +		}
>>>  
>>> -			/* print device info to dmesg */
>>> -			if (ata_msg_drv(ap) && print_info) {
>>> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
>>> -					     revbuf,	modelbuf, fwrevbuf,
>>> -					     ata_mode_string(xfer_mask));
>>> -				ata_dev_info(dev,
>>> -					     "%llu sectors, multi %u, CHS %u/%u/%u\n",
>>> -					     (unsigned long long)dev->n_sectors,
>>> -					     dev->multi_count, dev->cylinders,
>>> -					     dev->heads, dev->sectors);
>>> -			}
>>> +		/* print device info to dmesg */
>>> +		if (ata_msg_drv(ap) && print_info) {
>>> +			ata_dev_info(dev, "%s: %s, %s, max %s\n",
>>> +				     revbuf, modelbuf, fwrevbuf,
>>> +				     ata_mode_string(xfer_mask));
>>> +			ata_dev_info(dev,
>>> +				     "%llu sectors, multi %u, %s\n",
>>> +				     (unsigned long long)dev->n_sectors,
>>> +				     dev->multi_count, lba_info);
>>>  		}
>>>  
>>>  		ata_dev_config_devslp(dev);
>>>
>> Hmm. Can't say I like it.
>> Can't you move the second 'ata_dev_info()' call into the respective
>> functions, and kill the temporary buffer?
> 
> That would reverse the order of the messages... And I wanted to avoid having an
> "if (ata_id_has_lba(id))" again just for the print. Moving the 2 ata_dev_info()
> calls into the respective functions was the other solution I tried, but then the
> functions require *a lot* more arguments (revbuf, modelbuf, fwrevbuf, ...) wich
> was not super nice.
> 
> This one is the least ugly I thought... Any other idea ?
> 
Well, it should be possible to move the first 'ata_dev_info()' call
_prior_ to the if(ata_id_has_lba(id)) condition, and then we can move
the second call into the respective functions, no?

Cheers,

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

Re: [PATCH v2 4/9] libata: cleanup ata_dev_configure()

[Damien Le Moal]

On 2021/08/06 18:28, Hannes Reinecke wrote:
> On 8/6/21 11:12 AM, Damien Le Moal wrote:
>> On 2021/08/06 18:07, Hannes Reinecke wrote:
>>> On 8/6/21 9:42 AM, Damien Le Moal wrote:
>>>> Introduce the helper functions ata_dev_config_lba() and
>>>> ata_dev_config_chs() to configure the addressing capabilities of a
>>>> device. Each helper takes a string as argument for the addressing
>>>> information printed after these helpers execution completes.
>>>>
>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>>> ---
>>>>  drivers/ata/libata-core.c | 110 ++++++++++++++++++++------------------
>>>>  1 file changed, 59 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>>> index b13194432e5a..2b6054cdd8fc 100644
>>>> --- a/drivers/ata/libata-core.c
>>>> +++ b/drivers/ata/libata-core.c
> [ .. ]
>>>>  				return rc;
>>>> -
>>>> -			/* print device info to dmesg */
>>>> -			if (ata_msg_drv(ap) && print_info) {
>>>> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
>>>> -					     revbuf, modelbuf, fwrevbuf,
>>>> -					     ata_mode_string(xfer_mask));
>>>> -				ata_dev_info(dev,
>>>> -					     "%llu sectors, multi %u: %s %s\n",
>>>> -					(unsigned long long)dev->n_sectors,
>>>> -					dev->multi_count, lba_desc, ncq_desc);
>>>> -			}
>>>>  		} else {
>>>> -			/* CHS */
>>>> -
>>>> -			/* Default translation */
>>>> -			dev->cylinders	= id[1];
>>>> -			dev->heads	= id[3];
>>>> -			dev->sectors	= id[6];
>>>> -
>>>> -			if (ata_id_current_chs_valid(id)) {
>>>> -				/* Current CHS translation is valid. */
>>>> -				dev->cylinders = id[54];
>>>> -				dev->heads     = id[55];
>>>> -				dev->sectors   = id[56];
>>>> -			}
>>>> +			ata_dev_config_chs(dev, lba_info, sizeof(lba_info));
>>>> +		}
>>>>  
>>>> -			/* print device info to dmesg */
>>>> -			if (ata_msg_drv(ap) && print_info) {
>>>> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
>>>> -					     revbuf,	modelbuf, fwrevbuf,
>>>> -					     ata_mode_string(xfer_mask));
>>>> -				ata_dev_info(dev,
>>>> -					     "%llu sectors, multi %u, CHS %u/%u/%u\n",
>>>> -					     (unsigned long long)dev->n_sectors,
>>>> -					     dev->multi_count, dev->cylinders,
>>>> -					     dev->heads, dev->sectors);
>>>> -			}
>>>> +		/* print device info to dmesg */
>>>> +		if (ata_msg_drv(ap) && print_info) {
>>>> +			ata_dev_info(dev, "%s: %s, %s, max %s\n",
>>>> +				     revbuf, modelbuf, fwrevbuf,
>>>> +				     ata_mode_string(xfer_mask));
>>>> +			ata_dev_info(dev,
>>>> +				     "%llu sectors, multi %u, %s\n",
>>>> +				     (unsigned long long)dev->n_sectors,
>>>> +				     dev->multi_count, lba_info);
>>>>  		}
>>>>  
>>>>  		ata_dev_config_devslp(dev);
>>>>
>>> Hmm. Can't say I like it.
>>> Can't you move the second 'ata_dev_info()' call into the respective
>>> functions, and kill the temporary buffer?
>>
>> That would reverse the order of the messages... And I wanted to avoid having an
>> "if (ata_id_has_lba(id))" again just for the print. Moving the 2 ata_dev_info()
>> calls into the respective functions was the other solution I tried, but then the
>> functions require *a lot* more arguments (revbuf, modelbuf, fwrevbuf, ...) wich
>> was not super nice.
>>
>> This one is the least ugly I thought... Any other idea ?
>>
> Well, it should be possible to move the first 'ata_dev_info()' call
> _prior_ to the if(ata_id_has_lba(id)) condition, and then we can move
> the second call into the respective functions, no?

Indeed, it looks like that would work. Let me try that.

> 
> Cheers,
> 
> Hannes
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/9] libata: fix ata_host_alloc_pinfo()

[James Bottomley]

On Fri, 2021-08-06 at 16:42 +0900, Damien Le Moal wrote:
> Avoid a potential NULL pointer dereference by testing that the ATA
> port
> info variable "pi".
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/ata/libata-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 61c762961ca8..ea8b91297f12 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5458,7 +5458,7 @@ struct ata_host *ata_host_alloc_pinfo(struct
> device *dev,
>  		ap->link.flags |= pi->link_flags;
>  		ap->ops = pi->port_ops;

Hey, pi is used here

>  
> -		if (!host->ops && (pi->port_ops !=
> &ata_dummy_port_ops))
> +		if (!host->ops && pi && pi->port_ops !=
> &ata_dummy_port_ops)

So checking it here is just going to get us a load of static checker
reports.

James

Re: [PATCH v2 2/9] libata: fix ata_host_start()

[James Bottomley]

On Fri, 2021-08-06 at 16:42 +0900, Damien Le Moal wrote:
> The loop on entry of ata_host_start() may not initialize host->ops to
> a non NULL value. The test on the host_stop field of host->ops must
> then be preceded by a check that host->ops is not NULL.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/ata/libata-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index ea8b91297f12..fe49197caf99 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5573,7 +5573,7 @@ int ata_host_start(struct ata_host *host)
>  			have_stop = 1;
>  	}
>  
> -	if (host->ops->host_stop)
> +	if (host->ops && host->ops->host_stop)

since have_stop was already set by the port ops, surely this entire if
is redundant?

James

Re: [PATCH v2 1/9] libata: fix ata_host_alloc_pinfo()

[Damien Le Moal]

On 2021/08/06 23:25, James Bottomley wrote:
> On Fri, 2021-08-06 at 16:42 +0900, Damien Le Moal wrote:
>> Avoid a potential NULL pointer dereference by testing that the ATA
>> port
>> info variable "pi".
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  drivers/ata/libata-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 61c762961ca8..ea8b91297f12 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -5458,7 +5458,7 @@ struct ata_host *ata_host_alloc_pinfo(struct
>> device *dev,
>>  		ap->link.flags |= pi->link_flags;
>>  		ap->ops = pi->port_ops;
> 
> Hey, pi is used here
> 
>>  
>> -		if (!host->ops && (pi->port_ops !=
>> &ata_dummy_port_ops))
>> +		if (!host->ops && pi && pi->port_ops !=
>> &ata_dummy_port_ops)
> 
> So checking it here is just going to get us a load of static checker
> reports.

I got a load of static checker warnings already before sending this :)
And I got lost in that load obviously. Will check again.

> 
> James
> 
> 
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/9] libata: fix ata_host_start()

[Damien Le Moal]

On 2021/08/06 23:31, James Bottomley wrote:
> On Fri, 2021-08-06 at 16:42 +0900, Damien Le Moal wrote:
>> The loop on entry of ata_host_start() may not initialize host->ops to
>> a non NULL value. The test on the host_stop field of host->ops must
>> then be preceded by a check that host->ops is not NULL.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  drivers/ata/libata-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index ea8b91297f12..fe49197caf99 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -5573,7 +5573,7 @@ int ata_host_start(struct ata_host *host)
>>  			have_stop = 1;
>>  	}
>>  
>> -	if (host->ops->host_stop)
>> +	if (host->ops && host->ops->host_stop)
> 
> since have_stop was already set by the port ops, surely this entire if
> is redundant?

Will check.

> 
> James
> 
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/9] libata: fix ata_host_start()

[Damien Le Moal]

On Fri, 2021-08-06 at 07:31 -0700, James Bottomley wrote:
> On Fri, 2021-08-06 at 16:42 +0900, Damien Le Moal wrote:
> > The loop on entry of ata_host_start() may not initialize host->ops to
> > a non NULL value. The test on the host_stop field of host->ops must
> > then be preceded by a check that host->ops is not NULL.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> > ---
> >  drivers/ata/libata-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index ea8b91297f12..fe49197caf99 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5573,7 +5573,7 @@ int ata_host_start(struct ata_host *host)
> >  			have_stop = 1;
> >  	}
> >  
> > -	if (host->ops->host_stop)
> > +	if (host->ops && host->ops->host_stop)
> 
> since have_stop was already set by the port ops, surely this entire if
> is redundant?

Having another look at this, I do not think so.
The loop preceding the if is:

	for (i = 0; i < host->n_ports; i++) {
		struct ata_port *ap = host->ports[i];

		ata_finalize_port_ops(ap->ops);

		if (!host->ops && !ata_port_is_dummy(ap))
			host->ops = ap->ops;

		if (ap->ops->port_stop)
			have_stop = 1;
	}

So have_stop is set based on the port_stop operation of the port. The "if"
tests the host operation host_stop, so not the same thing.

The kernel bot warnings I got complain about the fact that the host ops may not
be set by the loop and can be null if the host ops are not already set and all
ports are dummy ones. In practice, I do not think that can happen. The patch
does not change anything and will only silence static checkers.

> 
> James
> 
> 

-- 
Damien Le Moal
Western Digital Research