[PATCH 0/3] NCQ Priority sysfs sttributes for libsas

[PATCH 0/3] NCQ Priority sysfs sttributes for libsas

[Igor Pylypiv]

This patch series adds sas_ncq_prio_supported and sas_ncq_prio_enable
sysfs sttributes for libsas managed SATA devices. Existing libata sysfs
attributes cannot be used directly because the ata_port location is
different for libsas.

Igor Pylypiv (3):
  ata: libata-sata: Factor out NCQ Priority configuration helpers
  scsi: libsas: Define NCQ Priority sysfs attributes for SATA devices
  scsi: pm80xx: Add libsas SATA sysfs attributes group

 drivers/ata/libata-sata.c         | 130 ++++++++++++++++++++----------
 drivers/scsi/libsas/sas_ata.c     |  87 ++++++++++++++++++++
 drivers/scsi/pm8001/pm8001_ctl.c  |   5 ++
 drivers/scsi/pm8001/pm8001_init.c |   1 +
 drivers/scsi/pm8001/pm8001_sas.h  |   1 +
 include/linux/libata.h            |   4 +
 include/scsi/sas_ata.h            |   6 ++
 7 files changed, 190 insertions(+), 44 deletions(-)

-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH 1/3] ata: libata-sata: Factor out NCQ Priority configuration helpers

[Igor Pylypiv]

Export libata NCQ Priority configuration helpers to be reused
for libsas managed SATA devices.

Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
Reviewed-by: TJ Adams <tadamsjr@google.com>
---
 drivers/ata/libata-sata.c | 130 +++++++++++++++++++++++++-------------
 include/linux/libata.h    |   4 ++
 2 files changed, 90 insertions(+), 44 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 0fb1934875f2..9c6c69d7feab 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -848,80 +848,104 @@ 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);
 
+/**
+ *	ata_ncq_prio_supported - Check if device supports NCQ Priority
+ *	@ap: ATA port of the target device
+ *	@sdev: SCSI device
+ *
+ *	Helper to check if device supports NCQ Priority feature,
+ *	usable with both libsas and libata.
+ */
+int ata_ncq_prio_supported(struct ata_port *ap, struct scsi_device *sdev)
+{
+	struct ata_device *dev;
+	unsigned long flags;
+	int rc;
+
+	spin_lock_irqsave(ap->lock, flags);
+	dev = ata_scsi_find_dev(ap, sdev);
+	rc = dev ? !!(dev->flags & ATA_DFLAG_NCQ_PRIO) : -ENODEV;
+	spin_unlock_irqrestore(ap->lock, flags);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ata_ncq_prio_supported);
+
 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);
+	int rc = ata_ncq_prio_supported(ap, sdev);
 
-	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_supported);
+	return (rc < 0) ? rc : sysfs_emit(buf, "%u\n", rc);
 }
-
 DEVICE_ATTR(ncq_prio_supported, S_IRUGO, ata_ncq_prio_supported_show, NULL);
 EXPORT_SYMBOL_GPL(dev_attr_ncq_prio_supported);
 
+/**
+ *	ata_ncq_prio_enabled - Check if NCQ Priority is enabled
+ *	@ap: ATA port of the target device
+ *	@sdev: SCSI device
+ *
+ *	Helper to check if NCQ Priority feature is enabled,
+ *	usable with both libsas and libata.
+ */
+int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev)
+{
+	struct ata_device *dev;
+	unsigned long flags;
+	int rc;
+
+	spin_lock_irqsave(ap->lock, flags);
+	dev = ata_scsi_find_dev(ap, sdev);
+	rc = dev ? !!(dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED) : -ENODEV;
+	spin_unlock_irqrestore(ap->lock, flags);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ata_ncq_prio_enabled);
+
 static ssize_t ata_ncq_prio_enable_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_enable;
-	int rc = 0;
-
-	spin_lock_irq(ap->lock);
-	dev = ata_scsi_find_dev(ap, sdev);
-	if (!dev)
-		rc = -ENODEV;
-	else
-		ncq_prio_enable = dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED;
-	spin_unlock_irq(ap->lock);
+	int rc = ata_ncq_prio_enabled(ap, sdev);
 
-	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_enable);
+	return (rc < 0) ? rc : sysfs_emit(buf, "%u\n", rc);
 }
 
-static ssize_t ata_ncq_prio_enable_store(struct device *device,
-					 struct device_attribute *attr,
-					 const char *buf, size_t len)
+/**
+ *	ata_ncq_prio_enable - Enable/disable NCQ Priority
+ *	@ap: ATA port of the target device
+ *	@sdev: SCSI device
+ *	@enable: true - enable NCQ Priority, false - disable NCQ Priority
+ *
+ *	Helper to enable/disable NCQ Priority feature, usable with both
+ *	libsas and libata.
+ */
+int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
+			bool enable)
 {
-	struct scsi_device *sdev = to_scsi_device(device);
-	struct ata_port *ap;
 	struct ata_device *dev;
-	long int input;
+	unsigned long flags;
 	int rc = 0;
 
-	rc = kstrtol(buf, 10, &input);
-	if (rc)
-		return rc;
-	if ((input < 0) || (input > 1))
-		return -EINVAL;
+	spin_lock_irqsave(ap->lock, flags);
 
-	ap = ata_shost_to_port(sdev->host);
 	dev = ata_scsi_find_dev(ap, sdev);
-	if (unlikely(!dev))
-		return  -ENODEV;
-
-	spin_lock_irq(ap->lock);
+	if (unlikely(!dev)) {
+		rc = -ENODEV;
+		goto unlock;
+	}
 
 	if (!(dev->flags & ATA_DFLAG_NCQ_PRIO)) {
 		rc = -EINVAL;
 		goto unlock;
 	}
 
-	if (input) {
+	if (enable) {
 		if (dev->flags & ATA_DFLAG_CDL_ENABLED) {
 			ata_dev_err(dev,
 				"CDL must be disabled to enable NCQ priority\n");
@@ -934,9 +958,27 @@ static ssize_t ata_ncq_prio_enable_store(struct device *device,
 	}
 
 unlock:
-	spin_unlock_irq(ap->lock);
+	spin_unlock_irqrestore(ap->lock, flags);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ata_ncq_prio_enable);
+
+static ssize_t ata_ncq_prio_enable_store(struct device *device,
+					 struct device_attribute *attr,
+					 const char *buf, size_t len)
+{
+	struct scsi_device *sdev = to_scsi_device(device);
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
+	long input;
+	int rc = 0;
+
+	rc = kstrtol(buf, 10, &input);
+	if (rc)
+		return rc;
+	if ((input < 0) || (input > 1))
+		return -EINVAL;
 
-	return rc ? rc : len;
+	return ata_ncq_prio_enable(ap, sdev, input) ? : len;
 }
 
 DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 26d68115afb8..f3ff2bf3ec6b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1157,6 +1157,10 @@ extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,
 				       int queue_depth);
 extern int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
 				  int queue_depth);
+extern int ata_ncq_prio_supported(struct ata_port *ap, struct scsi_device *sdev);
+extern int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev);
+extern int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
+			       bool enable);
 extern struct ata_device *ata_dev_pair(struct ata_device *adev);
 extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
 extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH 2/3] scsi: libsas: Define NCQ Priority sysfs attributes for SATA devices

[Igor Pylypiv]

Libata sysfs attributes cannot be used for libsas managed SATA devices
because the ata_port location is different for libsas.

Defined sysfs attributes (visible for SATA devices only):
- /sys/block/*/device/sas_ncq_prio_enable
- /sys/block/*/device/sas_ncq_prio_supported

The newly defined attributes will pass the correct ata_port to libata
helper functions.

Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
Reviewed-by: TJ Adams <tadamsjr@google.com>
---
 drivers/scsi/libsas/sas_ata.c | 87 +++++++++++++++++++++++++++++++++++
 include/scsi/sas_ata.h        |  6 +++
 2 files changed, 93 insertions(+)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 12e2653846e3..e0b19eee09b5 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -964,3 +964,90 @@ int sas_execute_ata_cmd(struct domain_device *device, u8 *fis, int force_phy_id)
 			       force_phy_id, &tmf_task);
 }
 EXPORT_SYMBOL_GPL(sas_execute_ata_cmd);
+
+static ssize_t sas_ncq_prio_supported_show(struct device *device,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(device);
+	struct domain_device *ddev = sdev_to_domain_dev(sdev);
+	int res;
+
+	// This attribute shall be visible for SATA devices only
+	if (WARN_ON(!dev_is_sata(ddev)))
+		return -EINVAL;
+
+	res = ata_ncq_prio_supported(ddev->sata_dev.ap, sdev);
+	if (res < 0)
+		return res;
+
+	return sysfs_emit(buf, "%u\n", res);
+}
+static DEVICE_ATTR_RO(sas_ncq_prio_supported);
+
+static ssize_t sas_ncq_prio_enable_show(struct device *device,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(device);
+	struct domain_device *ddev = sdev_to_domain_dev(sdev);
+	int res;
+
+	// This attribute shall be visible for SATA devices only
+	if (WARN_ON(!dev_is_sata(ddev)))
+		return -EINVAL;
+
+	res = ata_ncq_prio_enabled(ddev->sata_dev.ap, sdev);
+	if (res < 0)
+		return res;
+
+	return sysfs_emit(buf, "%u\n", res);
+}
+
+static ssize_t sas_ncq_prio_enable_store(struct device *device,
+					 struct device_attribute *attr,
+					 const char *buf, size_t len)
+{
+	struct scsi_device *sdev = to_scsi_device(device);
+	struct domain_device *ddev = sdev_to_domain_dev(sdev);
+	long input;
+	int res;
+
+	// This attribute shall be visible for SATA devices only
+	if (WARN_ON(!dev_is_sata(ddev)))
+		return -EINVAL;
+
+	res = kstrtol(buf, 10, &input);
+	if (res)
+		return res;
+	if ((input < 0) || (input > 1))
+		return -EINVAL;
+
+	return ata_ncq_prio_enable(ddev->sata_dev.ap, sdev, input) ? : len;
+}
+static DEVICE_ATTR_RW(sas_ncq_prio_enable);
+
+static struct attribute *sas_ata_sdev_attrs[] = {
+	&dev_attr_sas_ncq_prio_supported.attr,
+	&dev_attr_sas_ncq_prio_enable.attr,
+	NULL
+};
+
+static umode_t sas_ata_attr_is_visible(struct kobject *kobj,
+				       struct attribute *attr, int i)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct domain_device *ddev = sdev_to_domain_dev(sdev);
+
+	if (!dev_is_sata(ddev))
+		return 0;
+
+	return attr->mode;
+}
+
+const struct attribute_group sas_ata_sdev_attr_group = {
+	.attrs = sas_ata_sdev_attrs,
+	.is_visible = sas_ata_attr_is_visible,
+};
+EXPORT_SYMBOL_GPL(sas_ata_sdev_attr_group);
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 2f8c719840a6..cded782fdf33 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -39,6 +39,8 @@ int smp_ata_check_ready_type(struct ata_link *link);
 int sas_discover_sata(struct domain_device *dev);
 int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *phy,
 		    struct domain_device *child, int phy_id);
+
+extern const struct attribute_group sas_ata_sdev_attr_group;
 #else
 
 static inline void sas_ata_disabled_notice(void)
@@ -123,6 +125,10 @@ static inline int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *p
 	sas_ata_disabled_notice();
 	return -ENODEV;
 }
+
+static const struct attribute_group sas_ata_sdev_attr_group = {
+	.attrs = NULL,
+};
 #endif
 
 #endif /* _SAS_ATA_H_ */
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH 3/3] scsi: pm80xx: Add libsas SATA sysfs attributes group

[Igor Pylypiv]

The added sysfs attributes group enables the configuration of NCQ Priority
feature for HBAs that rely on libsas to manage SATA devices.

Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
Reviewed-by: TJ Adams <tadamsjr@google.com>
---
 drivers/scsi/pm8001/pm8001_ctl.c  | 5 +++++
 drivers/scsi/pm8001/pm8001_init.c | 1 +
 drivers/scsi/pm8001/pm8001_sas.h  | 1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
index 5c26a13ffbd2..9ffe1a868d0f 100644
--- a/drivers/scsi/pm8001/pm8001_ctl.c
+++ b/drivers/scsi/pm8001/pm8001_ctl.c
@@ -1039,3 +1039,8 @@ const struct attribute_group *pm8001_host_groups[] = {
 	&pm8001_host_attr_group,
 	NULL
 };
+
+const struct attribute_group *pm8001_sdev_groups[] = {
+	&sas_ata_sdev_attr_group,
+	NULL
+};
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index ed6b7d954dda..e6b1108f6117 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -134,6 +134,7 @@ static const struct scsi_host_template pm8001_sht = {
 	.compat_ioctl		= sas_ioctl,
 #endif
 	.shost_groups		= pm8001_host_groups,
+	.sdev_groups		= pm8001_sdev_groups,
 	.track_queue_depth	= 1,
 	.cmd_per_lun		= 32,
 	.map_queues		= pm8001_map_queues,
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 3ccb7371902f..ced6721380a8 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -717,6 +717,7 @@ int pm80xx_fatal_errors(struct pm8001_hba_info *pm8001_ha);
 void pm8001_free_dev(struct pm8001_device *pm8001_dev);
 /* ctl shared API */
 extern const struct attribute_group *pm8001_host_groups[];
+extern const struct attribute_group *pm8001_sdev_groups[];
 
 #define PM8001_INVALID_TAG	((u32)-1)
 
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH 2/3] scsi: libsas: Define NCQ Priority sysfs attributes for SATA devices

[John Garry]

On 01/03/2024 01:37, Igor Pylypiv wrote:
> Libata sysfs attributes cannot be used for libsas managed SATA devices
> because the ata_port location is different for libsas.
> 
> Defined sysfs attributes (visible for SATA devices only):
> - /sys/block/*/device/sas_ncq_prio_enable
> - /sys/block/*/device/sas_ncq_prio_supported

it would be good to show the full path

> 
> The newly defined attributes will pass the correct ata_port to libata
> helper functions.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> Reviewed-by: TJ Adams <tadamsjr@google.com>
> ---
>   drivers/scsi/libsas/sas_ata.c | 87 +++++++++++++++++++++++++++++++++++
>   include/scsi/sas_ata.h        |  6 +++
>   2 files changed, 93 insertions(+)
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 12e2653846e3..e0b19eee09b5 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -964,3 +964,90 @@ int sas_execute_ata_cmd(struct domain_device *device, u8 *fis, int force_phy_id)
>   			       force_phy_id, &tmf_task);
>   }
>   EXPORT_SYMBOL_GPL(sas_execute_ata_cmd);
> +
> +static ssize_t sas_ncq_prio_supported_show(struct device *device,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct scsi_device *sdev = to_scsi_device(device);
> +	struct domain_device *ddev = sdev_to_domain_dev(sdev);
> +	int res;
> +
> +	// This attribute shall be visible for SATA devices only

Please don't use c99 comment style

> +	if (WARN_ON(!dev_is_sata(ddev)))
> +		return -EINVAL;
> +
> +	res = ata_ncq_prio_supported(ddev->sata_dev.ap, sdev);
> +	if (res < 0)
> +		return res;
> +
> +	return sysfs_emit(buf, "%u\n", res);
> +}
> +static DEVICE_ATTR_RO(sas_ncq_prio_supported);
> +
> +static ssize_t sas_ncq_prio_enable_show(struct device *device,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct scsi_device *sdev = to_scsi_device(device);
> +	struct domain_device *ddev = sdev_to_domain_dev(sdev);
> +	int res;
> +
> +	// This attribute shall be visible for SATA devices only
> +	if (WARN_ON(!dev_is_sata(ddev)))
> +		return -EINVAL;
> +
> +	res = ata_ncq_prio_enabled(ddev->sata_dev.ap, sdev);
> +	if (res < 0)
> +		return res;
> +
> +	return sysfs_emit(buf, "%u\n", res);
> +}
> +
> +static ssize_t sas_ncq_prio_enable_store(struct device *device,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t len)
> +{
> +	struct scsi_device *sdev = to_scsi_device(device);
> +	struct domain_device *ddev = sdev_to_domain_dev(sdev);
> +	long input;
> +	int res;
> +
> +	// This attribute shall be visible for SATA devices only
> +	if (WARN_ON(!dev_is_sata(ddev)))
> +		return -EINVAL;
> +
> +	res = kstrtol(buf, 10, &input);

I think that kstrtobool_from_user() could be used. But 
kstrtobool_from_user() handles more than 0/1, so that would be a 
different behaviour, so maybe better not to use.

I would also suggest factor out some of ata_ncq_prio_enable_store() with 
this, but a public API which accepts char * would be a bit odd.


> +	if (res)
> +		return res;
> +	if ((input < 0) || (input > 1))
> +		return -EINVAL;
> +
> +	return ata_ncq_prio_enable(ddev->sata_dev.ap, sdev, input) ? : len;

Please don't use ternary operator like this. This seems more 
straightforfward:

res = ata_ncq_prio_enable();
if (res)
	return res;
return len;

> +}
> +static DEVICE_ATTR_RW(sas_ncq_prio_enable);
> +
> +static struct attribute *sas_ata_sdev_attrs[] = {
> +	&dev_attr_sas_ncq_prio_supported.attr,
> +	&dev_attr_sas_ncq_prio_enable.attr,
> +	NULL
> +};
> +
> +static umode_t sas_ata_attr_is_visible(struct kobject *kobj,
> +				       struct attribute *attr, int i)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +	struct domain_device *ddev = sdev_to_domain_dev(sdev);
> +
> +	if (!dev_is_sata(ddev))
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
> +const struct attribute_group sas_ata_sdev_attr_group = {
> +	.attrs = sas_ata_sdev_attrs,
> +	.is_visible = sas_ata_attr_is_visible,
> +};
> +EXPORT_SYMBOL_GPL(sas_ata_sdev_attr_group);
> diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
> index 2f8c719840a6..cded782fdf33 100644
> --- a/include/scsi/sas_ata.h
> +++ b/include/scsi/sas_ata.h
> @@ -39,6 +39,8 @@ int smp_ata_check_ready_type(struct ata_link *link);
>   int sas_discover_sata(struct domain_device *dev);
>   int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *phy,
>   		    struct domain_device *child, int phy_id);
> +
> +extern const struct attribute_group sas_ata_sdev_attr_group;
>   #else
>   
>   static inline void sas_ata_disabled_notice(void)
> @@ -123,6 +125,10 @@ static inline int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *p
>   	sas_ata_disabled_notice();
>   	return -ENODEV;
>   }
> +
> +static const struct attribute_group sas_ata_sdev_attr_group = {
> +	.attrs = NULL,
> +};
>   #endif
>   
>   #endif /* _SAS_ATA_H_ */

Re: [PATCH 3/3] scsi: pm80xx: Add libsas SATA sysfs attributes group

[John Garry]

On 01/03/2024 01:37, Igor Pylypiv wrote:
> The added sysfs attributes group enables the configuration of NCQ Priority
> feature for HBAs that rely on libsas to manage SATA devices.
> 
> Signed-off-by: Igor Pylypiv<ipylypiv@google.com>
> Reviewed-by: TJ Adams<tadamsjr@google.com>
> ---
>   drivers/scsi/pm8001/pm8001_ctl.c  | 5 +++++
>   drivers/scsi/pm8001/pm8001_init.c | 1 +
>   drivers/scsi/pm8001/pm8001_sas.h  | 1 +
>   3 files changed, 7 insertions(+)

More drivers than pm80xx use libsas and also handle SATA devices - 
please make similar changes to them also.

Thanks,
John

Re: [PATCH 1/3] ata: libata-sata: Factor out NCQ Priority configuration helpers

[Damien Le Moal]

On 2024/02/29 17:37, Igor Pylypiv wrote:
> Export libata NCQ Priority configuration helpers to be reused
> for libsas managed SATA devices.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> Reviewed-by: TJ Adams <tadamsjr@google.com>

Please drop this tag as the email signaling the review was not sent to the
list/in-reply to this email. The name of the reviewer should also be fully
spelled out. Same comment for the other 2 patches as they also have this review tag.

> ---
>  drivers/ata/libata-sata.c | 130 +++++++++++++++++++++++++-------------
>  include/linux/libata.h    |   4 ++
>  2 files changed, 90 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 0fb1934875f2..9c6c69d7feab 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -848,80 +848,104 @@ 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);
>  
> +/**
> + *	ata_ncq_prio_supported - Check if device supports NCQ Priority
> + *	@ap: ATA port of the target device
> + *	@sdev: SCSI device
> + *
> + *	Helper to check if device supports NCQ Priority feature,
> + *	usable with both libsas and libata.
> + */
> +int ata_ncq_prio_supported(struct ata_port *ap, struct scsi_device *sdev)
> +{
> +	struct ata_device *dev;
> +	unsigned long flags;
> +	int rc;
> +
> +	spin_lock_irqsave(ap->lock, flags);
> +	dev = ata_scsi_find_dev(ap, sdev);
> +	rc = dev ? !!(dev->flags & ATA_DFLAG_NCQ_PRIO) : -ENODEV;

Please expand this to make it more readable:

	if (!dev)
		rc = -ENODEV;
	else
		rc = !!(dev->flags & ATA_DFLAG_NCQ_PRIO);

> +	spin_unlock_irqrestore(ap->lock, flags);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(ata_ncq_prio_supported);
> +
>  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);
> +	int rc = ata_ncq_prio_supported(ap, sdev);
>  
> -	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_supported);
> +	return (rc < 0) ? rc : sysfs_emit(buf, "%u\n", rc);

Same here, please expand:

	if (rc < 0)
		return rc;
	return sysfs_emit(buf, "%d\n", rc);

And please not the change %u -> %d

>  }
> -

whiteline change. Please keep the white line.

>  DEVICE_ATTR(ncq_prio_supported, S_IRUGO, ata_ncq_prio_supported_show, NULL);
>  EXPORT_SYMBOL_GPL(dev_attr_ncq_prio_supported);
>  
> +/**
> + *	ata_ncq_prio_enabled - Check if NCQ Priority is enabled
> + *	@ap: ATA port of the target device
> + *	@sdev: SCSI device
> + *
> + *	Helper to check if NCQ Priority feature is enabled,
> + *	usable with both libsas and libata.
> + */
> +int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev)
> +{
> +	struct ata_device *dev;
> +	unsigned long flags;
> +	int rc;
> +
> +	spin_lock_irqsave(ap->lock, flags);
> +	dev = ata_scsi_find_dev(ap, sdev);
> +	rc = dev ? !!(dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED) : -ENODEV;

same comment as above. Please expand.

> +	spin_unlock_irqrestore(ap->lock, flags);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(ata_ncq_prio_enabled);
> +
>  static ssize_t ata_ncq_prio_enable_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_enable;
> -	int rc = 0;
> -
> -	spin_lock_irq(ap->lock);
> -	dev = ata_scsi_find_dev(ap, sdev);
> -	if (!dev)
> -		rc = -ENODEV;
> -	else
> -		ncq_prio_enable = dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED;
> -	spin_unlock_irq(ap->lock);
> +	int rc = ata_ncq_prio_enabled(ap, sdev);
>  
> -	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_enable);
> +	return (rc < 0) ? rc : sysfs_emit(buf, "%u\n", rc);

same comment as above.

>  }
>  
> -static ssize_t ata_ncq_prio_enable_store(struct device *device,
> -					 struct device_attribute *attr,
> -					 const char *buf, size_t len)
> +/**
> + *	ata_ncq_prio_enable - Enable/disable NCQ Priority
> + *	@ap: ATA port of the target device
> + *	@sdev: SCSI device
> + *	@enable: true - enable NCQ Priority, false - disable NCQ Priority
> + *
> + *	Helper to enable/disable NCQ Priority feature, usable with both
> + *	libsas and libata.
> + */
> +int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
> +			bool enable)
>  {
> -	struct scsi_device *sdev = to_scsi_device(device);
> -	struct ata_port *ap;
>  	struct ata_device *dev;
> -	long int input;
> +	unsigned long flags;
>  	int rc = 0;
>  
> -	rc = kstrtol(buf, 10, &input);
> -	if (rc)
> -		return rc;
> -	if ((input < 0) || (input > 1))
> -		return -EINVAL;
> +	spin_lock_irqsave(ap->lock, flags);

Any reason to not use spin_lock_irq() ?

>  
> -	ap = ata_shost_to_port(sdev->host);
>  	dev = ata_scsi_find_dev(ap, sdev);
> -	if (unlikely(!dev))
> -		return  -ENODEV;
> -
> -	spin_lock_irq(ap->lock);
> +	if (unlikely(!dev)) {
> +		rc = -ENODEV;
> +		goto unlock;
> +	}
>  
>  	if (!(dev->flags & ATA_DFLAG_NCQ_PRIO)) {
>  		rc = -EINVAL;
>  		goto unlock;
>  	}
>  
> -	if (input) {
> +	if (enable) {
>  		if (dev->flags & ATA_DFLAG_CDL_ENABLED) {
>  			ata_dev_err(dev,
>  				"CDL must be disabled to enable NCQ priority\n");
> @@ -934,9 +958,27 @@ static ssize_t ata_ncq_prio_enable_store(struct device *device,
>  	}
>  
>  unlock:
> -	spin_unlock_irq(ap->lock);
> +	spin_unlock_irqrestore(ap->lock, flags);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(ata_ncq_prio_enable);
> +
> +static ssize_t ata_ncq_prio_enable_store(struct device *device,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t len)
> +{
> +	struct scsi_device *sdev = to_scsi_device(device);
> +	struct ata_port *ap = ata_shost_to_port(sdev->host);
> +	long input;
> +	int rc = 0;
> +
> +	rc = kstrtol(buf, 10, &input);

Please use kstrtobool().

> +	if (rc)
> +		return rc;
> +	if ((input < 0) || (input > 1))
> +		return -EINVAL;
>  
> -	return rc ? rc : len;
> +	return ata_ncq_prio_enable(ap, sdev, input) ? : len;
>  }
>  
>  DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 26d68115afb8..f3ff2bf3ec6b 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1157,6 +1157,10 @@ extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,
>  				       int queue_depth);
>  extern int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
>  				  int queue_depth);
> +extern int ata_ncq_prio_supported(struct ata_port *ap, struct scsi_device *sdev);
> +extern int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev);
> +extern int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
> +			       bool enable);
>  extern struct ata_device *ata_dev_pair(struct ata_device *adev);
>  extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
>  extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] scsi: libsas: Define NCQ Priority sysfs attributes for SATA devices

[Damien Le Moal]

On 2024/03/01 3:57, John Garry wrote:
> On 01/03/2024 01:37, Igor Pylypiv wrote:
>> Libata sysfs attributes cannot be used for libsas managed SATA devices
>> because the ata_port location is different for libsas.
>>
>> Defined sysfs attributes (visible for SATA devices only):
>> - /sys/block/*/device/sas_ncq_prio_enable
>> - /sys/block/*/device/sas_ncq_prio_supported

Please no ! I know that the Broadcom mpt3sas driver has this attribute named
sas_ncq_prio_*, but I really think that it was a very unfortunate choice as that
makes it different from the attribute for libata, which does not have the sas_
prefix. That means that an attribute controlling a *device* feature has 2
different name depending on the hba used for the device. That is super annoying
to deal with in user space... So please, let's name this ncq_prio_*, same as for
AHCI. SAS does have a concept of priority, but that is for data frames and has
nothing to do with the actual command being transported. So mixing up sas and
ncq in the same name is only confusing...

For the Broadcom mpt3sas driver, we can define a link to have that same name for
the attributes to have everything consistent.

> 
> it would be good to show the full path
> 
>>
>> The newly defined attributes will pass the correct ata_port to libata
>> helper functions.
>>
>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
>> Reviewed-by: TJ Adams <tadamsjr@google.com>
>> ---
>>   drivers/scsi/libsas/sas_ata.c | 87 +++++++++++++++++++++++++++++++++++
>>   include/scsi/sas_ata.h        |  6 +++
>>   2 files changed, 93 insertions(+)
>>
>> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
>> index 12e2653846e3..e0b19eee09b5 100644
>> --- a/drivers/scsi/libsas/sas_ata.c
>> +++ b/drivers/scsi/libsas/sas_ata.c
>> @@ -964,3 +964,90 @@ int sas_execute_ata_cmd(struct domain_device *device, u8 *fis, int force_phy_id)
>>   			       force_phy_id, &tmf_task);
>>   }
>>   EXPORT_SYMBOL_GPL(sas_execute_ata_cmd);
>> +
>> +static ssize_t sas_ncq_prio_supported_show(struct device *device,
>> +					   struct device_attribute *attr,
>> +					   char *buf)
>> +{
>> +	struct scsi_device *sdev = to_scsi_device(device);
>> +	struct domain_device *ddev = sdev_to_domain_dev(sdev);
>> +	int res;
>> +
>> +	// This attribute shall be visible for SATA devices only
> 
> Please don't use c99 comment style
> 
>> +	if (WARN_ON(!dev_is_sata(ddev)))
>> +		return -EINVAL;
>> +
>> +	res = ata_ncq_prio_supported(ddev->sata_dev.ap, sdev);
>> +	if (res < 0)
>> +		return res;
>> +
>> +	return sysfs_emit(buf, "%u\n", res);
>> +}
>> +static DEVICE_ATTR_RO(sas_ncq_prio_supported);
>> +
>> +static ssize_t sas_ncq_prio_enable_show(struct device *device,
>> +					struct device_attribute *attr,
>> +					char *buf)
>> +{
>> +	struct scsi_device *sdev = to_scsi_device(device);
>> +	struct domain_device *ddev = sdev_to_domain_dev(sdev);
>> +	int res;
>> +
>> +	// This attribute shall be visible for SATA devices only
>> +	if (WARN_ON(!dev_is_sata(ddev)))
>> +		return -EINVAL;
>> +
>> +	res = ata_ncq_prio_enabled(ddev->sata_dev.ap, sdev);
>> +	if (res < 0)
>> +		return res;
>> +
>> +	return sysfs_emit(buf, "%u\n", res);
>> +}
>> +
>> +static ssize_t sas_ncq_prio_enable_store(struct device *device,
>> +					 struct device_attribute *attr,
>> +					 const char *buf, size_t len)
>> +{
>> +	struct scsi_device *sdev = to_scsi_device(device);
>> +	struct domain_device *ddev = sdev_to_domain_dev(sdev);
>> +	long input;
>> +	int res;
>> +
>> +	// This attribute shall be visible for SATA devices only
>> +	if (WARN_ON(!dev_is_sata(ddev)))
>> +		return -EINVAL;
>> +
>> +	res = kstrtol(buf, 10, &input);
> 
> I think that kstrtobool_from_user() could be used. But 
> kstrtobool_from_user() handles more than 0/1, so that would be a 
> different behaviour, so maybe better not to use.
> 
> I would also suggest factor out some of ata_ncq_prio_enable_store() with 
> this, but a public API which accepts char * would be a bit odd.

kstrtobool() is I think the standard way of parsing bool sysfs attributes.

> 
> 
>> +	if (res)
>> +		return res;
>> +	if ((input < 0) || (input > 1))
>> +		return -EINVAL;
>> +
>> +	return ata_ncq_prio_enable(ddev->sata_dev.ap, sdev, input) ? : len;
> 
> Please don't use ternary operator like this. This seems more 
> straightforfward:
> 
> res = ata_ncq_prio_enable();
> if (res)
> 	return res;
> return len;
> 
>> +}
>> +static DEVICE_ATTR_RW(sas_ncq_prio_enable);
>> +
>> +static struct attribute *sas_ata_sdev_attrs[] = {
>> +	&dev_attr_sas_ncq_prio_supported.attr,
>> +	&dev_attr_sas_ncq_prio_enable.attr,
>> +	NULL
>> +};
>> +
>> +static umode_t sas_ata_attr_is_visible(struct kobject *kobj,
>> +				       struct attribute *attr, int i)
>> +{
>> +	struct device *dev = kobj_to_dev(kobj);
>> +	struct scsi_device *sdev = to_scsi_device(dev);
>> +	struct domain_device *ddev = sdev_to_domain_dev(sdev);
>> +
>> +	if (!dev_is_sata(ddev))
>> +		return 0;
>> +
>> +	return attr->mode;
>> +}
>> +
>> +const struct attribute_group sas_ata_sdev_attr_group = {
>> +	.attrs = sas_ata_sdev_attrs,
>> +	.is_visible = sas_ata_attr_is_visible,
>> +};
>> +EXPORT_SYMBOL_GPL(sas_ata_sdev_attr_group);
>> diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
>> index 2f8c719840a6..cded782fdf33 100644
>> --- a/include/scsi/sas_ata.h
>> +++ b/include/scsi/sas_ata.h
>> @@ -39,6 +39,8 @@ int smp_ata_check_ready_type(struct ata_link *link);
>>   int sas_discover_sata(struct domain_device *dev);
>>   int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *phy,
>>   		    struct domain_device *child, int phy_id);
>> +
>> +extern const struct attribute_group sas_ata_sdev_attr_group;
>>   #else
>>   
>>   static inline void sas_ata_disabled_notice(void)
>> @@ -123,6 +125,10 @@ static inline int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *p
>>   	sas_ata_disabled_notice();
>>   	return -ENODEV;
>>   }
>> +
>> +static const struct attribute_group sas_ata_sdev_attr_group = {
>> +	.attrs = NULL,
>> +};
>>   #endif
>>   
>>   #endif /* _SAS_ATA_H_ */
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] ata: libata-sata: Factor out NCQ Priority configuration helpers

[Igor Pylypiv]

On Fri, Mar 01, 2024 at 04:16:46AM -0800, Damien Le Moal wrote:

Thank you for the review Damien!

> On 2024/02/29 17:37, Igor Pylypiv wrote:
> > Export libata NCQ Priority configuration helpers to be reused
> > for libsas managed SATA devices.
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > Reviewed-by: TJ Adams <tadamsjr@google.com>
> 
> Please drop this tag as the email signaling the review was not sent to the
> list/in-reply to this email. The name of the reviewer should also be fully
> spelled out. Same comment for the other 2 patches as they also have this review tag.
> 
> > ---
> >  drivers/ata/libata-sata.c | 130 +++++++++++++++++++++++++-------------
> >  include/linux/libata.h    |   4 ++
> >  2 files changed, 90 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > index 0fb1934875f2..9c6c69d7feab 100644
> > --- a/drivers/ata/libata-sata.c
> > +++ b/drivers/ata/libata-sata.c
> > @@ -848,80 +848,104 @@ 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);
> >  
> > +/**
> > + *	ata_ncq_prio_supported - Check if device supports NCQ Priority
> > + *	@ap: ATA port of the target device
> > + *	@sdev: SCSI device
> > + *
> > + *	Helper to check if device supports NCQ Priority feature,
> > + *	usable with both libsas and libata.
> > + */
> > +int ata_ncq_prio_supported(struct ata_port *ap, struct scsi_device *sdev)
> > +{
> > +	struct ata_device *dev;
> > +	unsigned long flags;
> > +	int rc;
> > +
> > +	spin_lock_irqsave(ap->lock, flags);
> > +	dev = ata_scsi_find_dev(ap, sdev);
> > +	rc = dev ? !!(dev->flags & ATA_DFLAG_NCQ_PRIO) : -ENODEV;
> 
> Please expand this to make it more readable:
> 
> 	if (!dev)
> 		rc = -ENODEV;
> 	else
> 		rc = !!(dev->flags & ATA_DFLAG_NCQ_PRIO);
> 
> > +	spin_unlock_irqrestore(ap->lock, flags);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(ata_ncq_prio_supported);
> > +
> >  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);
> > +	int rc = ata_ncq_prio_supported(ap, sdev);
> >  
> > -	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_supported);
> > +	return (rc < 0) ? rc : sysfs_emit(buf, "%u\n", rc);
> 
> Same here, please expand:
> 
> 	if (rc < 0)
> 		return rc;
> 	return sysfs_emit(buf, "%d\n", rc);
> 
> And please not the change %u -> %d
> 
> >  }
> > -
> 
> whiteline change. Please keep the white line.
> 
> >  DEVICE_ATTR(ncq_prio_supported, S_IRUGO, ata_ncq_prio_supported_show, NULL);
> >  EXPORT_SYMBOL_GPL(dev_attr_ncq_prio_supported);
> >  
> > +/**
> > + *	ata_ncq_prio_enabled - Check if NCQ Priority is enabled
> > + *	@ap: ATA port of the target device
> > + *	@sdev: SCSI device
> > + *
> > + *	Helper to check if NCQ Priority feature is enabled,
> > + *	usable with both libsas and libata.
> > + */
> > +int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev)
> > +{
> > +	struct ata_device *dev;
> > +	unsigned long flags;
> > +	int rc;
> > +
> > +	spin_lock_irqsave(ap->lock, flags);
> > +	dev = ata_scsi_find_dev(ap, sdev);
> > +	rc = dev ? !!(dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED) : -ENODEV;
> 
> same comment as above. Please expand.
> 
> > +	spin_unlock_irqrestore(ap->lock, flags);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(ata_ncq_prio_enabled);
> > +
> >  static ssize_t ata_ncq_prio_enable_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_enable;
> > -	int rc = 0;
> > -
> > -	spin_lock_irq(ap->lock);
> > -	dev = ata_scsi_find_dev(ap, sdev);
> > -	if (!dev)
> > -		rc = -ENODEV;
> > -	else
> > -		ncq_prio_enable = dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED;
> > -	spin_unlock_irq(ap->lock);
> > +	int rc = ata_ncq_prio_enabled(ap, sdev);
> >  
> > -	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_enable);
> > +	return (rc < 0) ? rc : sysfs_emit(buf, "%u\n", rc);
> 
> same comment as above.
> 
> >  }
> >  
> > -static ssize_t ata_ncq_prio_enable_store(struct device *device,
> > -					 struct device_attribute *attr,
> > -					 const char *buf, size_t len)
> > +/**
> > + *	ata_ncq_prio_enable - Enable/disable NCQ Priority
> > + *	@ap: ATA port of the target device
> > + *	@sdev: SCSI device
> > + *	@enable: true - enable NCQ Priority, false - disable NCQ Priority
> > + *
> > + *	Helper to enable/disable NCQ Priority feature, usable with both
> > + *	libsas and libata.
> > + */
> > +int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
> > +			bool enable)
> >  {
> > -	struct scsi_device *sdev = to_scsi_device(device);
> > -	struct ata_port *ap;
> >  	struct ata_device *dev;
> > -	long int input;
> > +	unsigned long flags;
> >  	int rc = 0;
> >  
> > -	rc = kstrtol(buf, 10, &input);
> > -	if (rc)
> > -		return rc;
> > -	if ((input < 0) || (input > 1))
> > -		return -EINVAL;
> > +	spin_lock_irqsave(ap->lock, flags);
> 
> Any reason to not use spin_lock_irq() ?

In the future someone might call these helper functions when interrupts
are disabled. spin_unlock_irq() might re-enable interrupts prematurely.
spin_unlock_irqrestore() will restore the same interrupts state that was
before the call to spin_lock_irqsave().

> 
> >  
> > -	ap = ata_shost_to_port(sdev->host);
> >  	dev = ata_scsi_find_dev(ap, sdev);
> > -	if (unlikely(!dev))
> > -		return  -ENODEV;
> > -
> > -	spin_lock_irq(ap->lock);
> > +	if (unlikely(!dev)) {
> > +		rc = -ENODEV;
> > +		goto unlock;
> > +	}
> >  
> >  	if (!(dev->flags & ATA_DFLAG_NCQ_PRIO)) {
> >  		rc = -EINVAL;
> >  		goto unlock;
> >  	}
> >  
> > -	if (input) {
> > +	if (enable) {
> >  		if (dev->flags & ATA_DFLAG_CDL_ENABLED) {
> >  			ata_dev_err(dev,
> >  				"CDL must be disabled to enable NCQ priority\n");
> > @@ -934,9 +958,27 @@ static ssize_t ata_ncq_prio_enable_store(struct device *device,
> >  	}
> >  
> >  unlock:
> > -	spin_unlock_irq(ap->lock);
> > +	spin_unlock_irqrestore(ap->lock, flags);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(ata_ncq_prio_enable);
> > +
> > +static ssize_t ata_ncq_prio_enable_store(struct device *device,
> > +					 struct device_attribute *attr,
> > +					 const char *buf, size_t len)
> > +{
> > +	struct scsi_device *sdev = to_scsi_device(device);
> > +	struct ata_port *ap = ata_shost_to_port(sdev->host);
> > +	long input;
> > +	int rc = 0;
> > +
> > +	rc = kstrtol(buf, 10, &input);
> 
> Please use kstrtobool().
> 
> > +	if (rc)
> > +		return rc;
> > +	if ((input < 0) || (input > 1))
> > +		return -EINVAL;
> >  
> > -	return rc ? rc : len;
> > +	return ata_ncq_prio_enable(ap, sdev, input) ? : len;
> >  }
> >  
> >  DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 26d68115afb8..f3ff2bf3ec6b 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1157,6 +1157,10 @@ extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,
> >  				       int queue_depth);
> >  extern int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev,
> >  				  int queue_depth);
> > +extern int ata_ncq_prio_supported(struct ata_port *ap, struct scsi_device *sdev);
> > +extern int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev);
> > +extern int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
> > +			       bool enable);
> >  extern struct ata_device *ata_dev_pair(struct ata_device *adev);
> >  extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
> >  extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
> 
> -- 
> Damien Le Moal
> Western Digital Research
>