[PATCH v8 0/7] NCQ Priority sysfs sttributes for libsas

[PATCH v8 0/7] NCQ Priority sysfs sttributes for libsas

[Igor Pylypiv]

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

Changes since v7:
- Dropped the WARN_ON_ONCE(!dev_is_sata(ddev)) checks from sas_ncq_prio_*
  sysfs functions. The is_visible() callback hides the corresponding sysfs
  attributes from userspace for non-SATA devices.
- Added missing "Return" descriptions to ata_ncq_prio_* kdocs.
- Updated the commit message of the "ata: libata-sata: Factor out
  NCQ Priority configuration helpers" patch with the explanation
  why spin_lock_irq() was changed to spin_lock_irqsave().
- Restored blank lines between spin_unlock and return in ata_ncq_prio_*
  helpers.
- Updated the commit message of the hisi_sas patch to indicate that
  hisi_sas_v1_hw.c was not modified because v1 HW does not support SATA.

Changes since v6:
- Replaced sas_ata_sdev_attr_group definition with a macro for
  the "CONFIG_SCSI_SAS_ATA is not set" case. The macro defines
  an empty rvalue struct eliminating the variable definition.

Changes since v5:
- Added __maybe_unused attribute to sas_ata_sdev_attr_group to prevent
  an unused-const-variable warning when CONFIG_SCSI_SAS_ATA is not set.

Changes since v4:
- Updated sas_ncq_prio_* sysfs functions to use WARN_ON_ONCE() instead
  of WARN_ON().

Changes since v3:
- Changed ata_ncq_prio_supported() and ata_ncq_prio_enabled() to store
  the result into a boolean variable passed by address.
- Removed the "usable with both libsas and libata" wording from
  ata_ncq_prio_* helper's function comments.
- Removed the unlikely() in ata_ncq_prio_enable() because the function
  is not in a fastpath.
- Dropped hisi_sas v1 HW driver changes because it doesn't support SATA.

Changes since v2:
- Added libsas SATA sysfs attributes to aic94xx and isci.

Changes since v1:
- Dropped the "sas_" prefix to align sysfs sttributes naming with AHCI.
- Dropped ternary operators to make the code more readable.
- Corrected the formatting %u -> %d in sysfs_emit().
- Changed kstrtol() to kstrtobool() in [ata|sas]_ncq_prio_enable_store().
- Changed comments to use the "/* */" style instead of "//".
- Added libsas SATA sysfs attributes to mvsas and hisi_sas.
- Dropped the 'Reviewed-by' tags because they were not sent in-reply
  to the patch emails.

Igor Pylypiv (7):
  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
  scsi: mvsas: Add libsas SATA sysfs attributes group
  scsi: hisi_sas: Add libsas SATA sysfs attributes group
  scsi: aic94xx: Add libsas SATA sysfs attributes group
  scsi: isci: Add libsas SATA sysfs attributes group

 drivers/ata/libata-sata.c              | 160 +++++++++++++++++++------
 drivers/scsi/aic94xx/aic94xx_init.c    |   8 ++
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |   6 +
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |   6 +
 drivers/scsi/isci/init.c               |   6 +
 drivers/scsi/libsas/sas_ata.c          |  82 +++++++++++++
 drivers/scsi/mvsas/mv_init.c           |   7 ++
 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                 |   6 +
 include/scsi/sas_ata.h                 |   6 +
 12 files changed, 256 insertions(+), 38 deletions(-)

-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v8 1/7] 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.

Switched locking from spin_lock_irq() to spin_lock_irqsave().
In the future someone might call these helper functions when interrupts
are disabled. spin_unlock_irq() could lead to a premature re-enabling
of interrupts, whereas spin_unlock_irqrestore() restores the interrupt
state to its condition prior to the spin_lock_irqsave() call.

Acked-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-sata.c | 160 +++++++++++++++++++++++++++++---------
 include/linux/libata.h    |   6 ++
 2 files changed, 128 insertions(+), 38 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 0fb1934875f2..a8d773003d74 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -848,80 +848,143 @@ 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)
+/**
+ *	ata_ncq_prio_supported - Check if device supports NCQ Priority
+ *	@ap: ATA port of the target device
+ *	@sdev: SCSI device
+ *	@supported: Address of a boolean to store the result
+ *
+ *	Helper to check if device supports NCQ Priority feature.
+ *
+ *	Context: Any context. Takes and releases @ap->lock.
+ *
+ *	Return:
+ *	* %0		- OK. Status is stored into @supported
+ *	* %-ENODEV	- Failed to find the ATA device
+ */
+int ata_ncq_prio_supported(struct ata_port *ap, struct scsi_device *sdev,
+			   bool *supported)
 {
-	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;
+	unsigned long flags;
 	int rc = 0;
 
-	spin_lock_irq(ap->lock);
+	spin_lock_irqsave(ap->lock, flags);
 	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);
+		*supported = 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);
+	bool supported;
+	int rc;
+
+	rc = ata_ncq_prio_supported(ap, sdev, &supported);
+	if (rc)
+		return rc;
 
-	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_supported);
+	return sysfs_emit(buf, "%d\n", 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)
+/**
+ *	ata_ncq_prio_enabled - Check if NCQ Priority is enabled
+ *	@ap: ATA port of the target device
+ *	@sdev: SCSI device
+ *	@enabled: Address of a boolean to store the result
+ *
+ *	Helper to check if NCQ Priority feature is enabled.
+ *
+ *	Context: Any context. Takes and releases @ap->lock.
+ *
+ *	Return:
+ *	* %0		- OK. Status is stored into @enabled
+ *	* %-ENODEV	- Failed to find the ATA device
+ */
+int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev,
+			 bool *enabled)
 {
-	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;
+	unsigned long flags;
 	int rc = 0;
 
-	spin_lock_irq(ap->lock);
+	spin_lock_irqsave(ap->lock, flags);
 	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);
+		*enabled = dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED;
+	spin_unlock_irqrestore(ap->lock, flags);
 
-	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_enable);
+	return rc;
 }
+EXPORT_SYMBOL_GPL(ata_ncq_prio_enabled);
 
-static ssize_t ata_ncq_prio_enable_store(struct device *device,
-					 struct device_attribute *attr,
-					 const char *buf, size_t len)
+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;
-	struct ata_device *dev;
-	long int input;
-	int rc = 0;
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
+	bool enabled;
+	int rc;
 
-	rc = kstrtol(buf, 10, &input);
+	rc = ata_ncq_prio_enabled(ap, sdev, &enabled);
 	if (rc)
 		return rc;
-	if ((input < 0) || (input > 1))
-		return -EINVAL;
 
-	ap = ata_shost_to_port(sdev->host);
-	dev = ata_scsi_find_dev(ap, sdev);
-	if (unlikely(!dev))
-		return  -ENODEV;
+	return sysfs_emit(buf, "%d\n", enabled);
+}
+
+/**
+ *	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.
+ *
+ *	Context: Any context. Takes and releases @ap->lock.
+ *
+ *	Return:
+ *	* %0		- OK. Status is stored into @enabled
+ *	* %-ENODEV	- Failed to find the ATA device
+ *	* %-EINVAL	- NCQ Priority is not supported or CDL is enabled
+ */
+int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
+			bool enable)
+{
+	struct ata_device *dev;
+	unsigned long flags;
+	int rc = 0;
+
+	spin_lock_irqsave(ap->lock, flags);
 
-	spin_lock_irq(ap->lock);
+	dev = ata_scsi_find_dev(ap, sdev);
+	if (!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 +997,30 @@ 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);
+	bool enable;
+	int rc;
+
+	rc = kstrtobool(buf, &enable);
+	if (rc)
+		return rc;
+
+	rc = ata_ncq_prio_enable(ap, sdev, enable);
+	if (rc)
+		return rc;
 
-	return rc ? rc : len;
+	return len;
 }
 
 DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 26d68115afb8..6dd9a4f9ca7c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1157,6 +1157,12 @@ 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,
+				  bool *supported);
+extern int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev,
+				bool *enabled);
+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

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

[Niklas Cassel]

On Thu, Mar 07, 2024 at 01:44:12PM -0800, Igor Pylypiv wrote:
> Export libata NCQ Priority configuration helpers to be reused
> for libsas managed SATA devices.
> 
> Switched locking from spin_lock_irq() to spin_lock_irqsave().
> In the future someone might call these helper functions when interrupts
> are disabled. spin_unlock_irq() could lead to a premature re-enabling
> of interrupts, whereas spin_unlock_irqrestore() restores the interrupt
> state to its condition prior to the spin_lock_irqsave() call.

Seems like a mistake in the existing code, why would
ata_ncq_prio_supported_show() and ata_ncq_prio_enable_show()
use spin_lock_irq() ?

Seems like spin_lock() would be better.

For ata_ncq_prio_enable_store(), you probably want to
spin_lock_irq() or spin_lock_irqsave().


Anyway, like you said, as you are now creating helper functions:
ata_ncq_prio_supported(), ata_ncq_prio_enabled(), ata_ncq_prio_enable()
these function might no longer only be called from sysfs code,
so it is probably a bad idea to let these functions use spin_lock_irq().

However, can't ata_ncq_prio_supported() and ata_ncq_prio_enabled()
still use a simple spin_lock(), why would they need to disable irqs?

Damien, you are the author of ata_ncq_prio_supported_show(), thoughts?


Kind regards,
Niklas

> 
> Acked-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-sata.c | 160 +++++++++++++++++++++++++++++---------
>  include/linux/libata.h    |   6 ++
>  2 files changed, 128 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 0fb1934875f2..a8d773003d74 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -848,80 +848,143 @@ 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)
> +/**
> + *	ata_ncq_prio_supported - Check if device supports NCQ Priority
> + *	@ap: ATA port of the target device
> + *	@sdev: SCSI device
> + *	@supported: Address of a boolean to store the result
> + *
> + *	Helper to check if device supports NCQ Priority feature.
> + *
> + *	Context: Any context. Takes and releases @ap->lock.
> + *
> + *	Return:
> + *	* %0		- OK. Status is stored into @supported
> + *	* %-ENODEV	- Failed to find the ATA device
> + */
> +int ata_ncq_prio_supported(struct ata_port *ap, struct scsi_device *sdev,
> +			   bool *supported)
>  {
> -	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;
> +	unsigned long flags;
>  	int rc = 0;
>  
> -	spin_lock_irq(ap->lock);
> +	spin_lock_irqsave(ap->lock, flags);
>  	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);
> +		*supported = 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);
> +	bool supported;
> +	int rc;
> +
> +	rc = ata_ncq_prio_supported(ap, sdev, &supported);
> +	if (rc)
> +		return rc;
>  
> -	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_supported);
> +	return sysfs_emit(buf, "%d\n", 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)
> +/**
> + *	ata_ncq_prio_enabled - Check if NCQ Priority is enabled
> + *	@ap: ATA port of the target device
> + *	@sdev: SCSI device
> + *	@enabled: Address of a boolean to store the result
> + *
> + *	Helper to check if NCQ Priority feature is enabled.
> + *
> + *	Context: Any context. Takes and releases @ap->lock.
> + *
> + *	Return:
> + *	* %0		- OK. Status is stored into @enabled
> + *	* %-ENODEV	- Failed to find the ATA device
> + */
> +int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev,
> +			 bool *enabled)
>  {
> -	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;
> +	unsigned long flags;
>  	int rc = 0;
>  
> -	spin_lock_irq(ap->lock);
> +	spin_lock_irqsave(ap->lock, flags);
>  	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);
> +		*enabled = dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED;
> +	spin_unlock_irqrestore(ap->lock, flags);
>  
> -	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_enable);
> +	return rc;
>  }
> +EXPORT_SYMBOL_GPL(ata_ncq_prio_enabled);
>  
> -static ssize_t ata_ncq_prio_enable_store(struct device *device,
> -					 struct device_attribute *attr,
> -					 const char *buf, size_t len)
> +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;
> -	struct ata_device *dev;
> -	long int input;
> -	int rc = 0;
> +	struct ata_port *ap = ata_shost_to_port(sdev->host);
> +	bool enabled;
> +	int rc;
>  
> -	rc = kstrtol(buf, 10, &input);
> +	rc = ata_ncq_prio_enabled(ap, sdev, &enabled);
>  	if (rc)
>  		return rc;
> -	if ((input < 0) || (input > 1))
> -		return -EINVAL;
>  
> -	ap = ata_shost_to_port(sdev->host);
> -	dev = ata_scsi_find_dev(ap, sdev);
> -	if (unlikely(!dev))
> -		return  -ENODEV;
> +	return sysfs_emit(buf, "%d\n", enabled);
> +}
> +
> +/**
> + *	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.
> + *
> + *	Context: Any context. Takes and releases @ap->lock.
> + *
> + *	Return:
> + *	* %0		- OK. Status is stored into @enabled
> + *	* %-ENODEV	- Failed to find the ATA device
> + *	* %-EINVAL	- NCQ Priority is not supported or CDL is enabled
> + */
> +int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
> +			bool enable)
> +{
> +	struct ata_device *dev;
> +	unsigned long flags;
> +	int rc = 0;
> +
> +	spin_lock_irqsave(ap->lock, flags);
>  
> -	spin_lock_irq(ap->lock);
> +	dev = ata_scsi_find_dev(ap, sdev);
> +	if (!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 +997,30 @@ 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);
> +	bool enable;
> +	int rc;
> +
> +	rc = kstrtobool(buf, &enable);
> +	if (rc)
> +		return rc;
> +
> +	rc = ata_ncq_prio_enable(ap, sdev, enable);
> +	if (rc)
> +		return rc;
>  
> -	return rc ? rc : len;
> +	return len;
>  }
>  
>  DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 26d68115afb8..6dd9a4f9ca7c 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1157,6 +1157,12 @@ 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,
> +				  bool *supported);
> +extern int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev,
> +				bool *enabled);
> +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
> 

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

[Damien Le Moal]

On 2024/03/08 19:03, Niklas Cassel wrote:
> On Thu, Mar 07, 2024 at 01:44:12PM -0800, Igor Pylypiv wrote:
>> Export libata NCQ Priority configuration helpers to be reused
>> for libsas managed SATA devices.
>>
>> Switched locking from spin_lock_irq() to spin_lock_irqsave().
>> In the future someone might call these helper functions when interrupts
>> are disabled. spin_unlock_irq() could lead to a premature re-enabling
>> of interrupts, whereas spin_unlock_irqrestore() restores the interrupt
>> state to its condition prior to the spin_lock_irqsave() call.
> 
> Seems like a mistake in the existing code, why would
> ata_ncq_prio_supported_show() and ata_ncq_prio_enable_show()
> use spin_lock_irq() ?
> 
> Seems like spin_lock() would be better.

Nope, you cannot do that. The port lock is taken from command completion
context/IRQ. So using spin_lock could lead to deadlocks.

> 
> For ata_ncq_prio_enable_store(), you probably want to
> spin_lock_irq() or spin_lock_irqsave().
> 
> 
> Anyway, like you said, as you are now creating helper functions:
> ata_ncq_prio_supported(), ata_ncq_prio_enabled(), ata_ncq_prio_enable()
> these function might no longer only be called from sysfs code,
> so it is probably a bad idea to let these functions use spin_lock_irq().
> 
> However, can't ata_ncq_prio_supported() and ata_ncq_prio_enabled()
> still use a simple spin_lock(), why would they need to disable irqs?
> 
> Damien, you are the author of ata_ncq_prio_supported_show(), thoughts?

See above. The spin lock irq-disabling variant is needed because the port lock
is taken from command completion context.

As for ata_ncq_prio_supported() and ata_ncq_prio_enabled() being called from
somewhere else than the sysfs context, I seriously doubt it, and if I see
someone doing it, I will most definitively say no. These functions are overkill
to use anywhere else but the sysfs store/show because in most other places we
likely already have the dev (no need for searching it) and in many instances
likely looking at the device flags with the port already locked. So these
functions in fact likely cannot be used...

Given that Igor rewrote/cleaned this up nicely, keeping the change to
spin_lock_irqsave() from the original spin_lock_irq() is fine to me. All good !

> 
> 
> Kind regards,
> Niklas
> 
>>
>> Acked-by: Damien Le Moal <dlemoal@kernel.org>
>> Reviewed-by: Jason Yan <yanaijie@huawei.com>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
>> ---
>>  drivers/ata/libata-sata.c | 160 +++++++++++++++++++++++++++++---------
>>  include/linux/libata.h    |   6 ++
>>  2 files changed, 128 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>> index 0fb1934875f2..a8d773003d74 100644
>> --- a/drivers/ata/libata-sata.c
>> +++ b/drivers/ata/libata-sata.c
>> @@ -848,80 +848,143 @@ 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)
>> +/**
>> + *	ata_ncq_prio_supported - Check if device supports NCQ Priority
>> + *	@ap: ATA port of the target device
>> + *	@sdev: SCSI device
>> + *	@supported: Address of a boolean to store the result
>> + *
>> + *	Helper to check if device supports NCQ Priority feature.
>> + *
>> + *	Context: Any context. Takes and releases @ap->lock.
>> + *
>> + *	Return:
>> + *	* %0		- OK. Status is stored into @supported
>> + *	* %-ENODEV	- Failed to find the ATA device
>> + */
>> +int ata_ncq_prio_supported(struct ata_port *ap, struct scsi_device *sdev,
>> +			   bool *supported)
>>  {
>> -	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;
>> +	unsigned long flags;
>>  	int rc = 0;
>>  
>> -	spin_lock_irq(ap->lock);
>> +	spin_lock_irqsave(ap->lock, flags);
>>  	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);
>> +		*supported = 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);
>> +	bool supported;
>> +	int rc;
>> +
>> +	rc = ata_ncq_prio_supported(ap, sdev, &supported);
>> +	if (rc)
>> +		return rc;
>>  
>> -	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_supported);
>> +	return sysfs_emit(buf, "%d\n", 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)
>> +/**
>> + *	ata_ncq_prio_enabled - Check if NCQ Priority is enabled
>> + *	@ap: ATA port of the target device
>> + *	@sdev: SCSI device
>> + *	@enabled: Address of a boolean to store the result
>> + *
>> + *	Helper to check if NCQ Priority feature is enabled.
>> + *
>> + *	Context: Any context. Takes and releases @ap->lock.
>> + *
>> + *	Return:
>> + *	* %0		- OK. Status is stored into @enabled
>> + *	* %-ENODEV	- Failed to find the ATA device
>> + */
>> +int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev,
>> +			 bool *enabled)
>>  {
>> -	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;
>> +	unsigned long flags;
>>  	int rc = 0;
>>  
>> -	spin_lock_irq(ap->lock);
>> +	spin_lock_irqsave(ap->lock, flags);
>>  	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);
>> +		*enabled = dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED;
>> +	spin_unlock_irqrestore(ap->lock, flags);
>>  
>> -	return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_enable);
>> +	return rc;
>>  }
>> +EXPORT_SYMBOL_GPL(ata_ncq_prio_enabled);
>>  
>> -static ssize_t ata_ncq_prio_enable_store(struct device *device,
>> -					 struct device_attribute *attr,
>> -					 const char *buf, size_t len)
>> +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;
>> -	struct ata_device *dev;
>> -	long int input;
>> -	int rc = 0;
>> +	struct ata_port *ap = ata_shost_to_port(sdev->host);
>> +	bool enabled;
>> +	int rc;
>>  
>> -	rc = kstrtol(buf, 10, &input);
>> +	rc = ata_ncq_prio_enabled(ap, sdev, &enabled);
>>  	if (rc)
>>  		return rc;
>> -	if ((input < 0) || (input > 1))
>> -		return -EINVAL;
>>  
>> -	ap = ata_shost_to_port(sdev->host);
>> -	dev = ata_scsi_find_dev(ap, sdev);
>> -	if (unlikely(!dev))
>> -		return  -ENODEV;
>> +	return sysfs_emit(buf, "%d\n", enabled);
>> +}
>> +
>> +/**
>> + *	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.
>> + *
>> + *	Context: Any context. Takes and releases @ap->lock.
>> + *
>> + *	Return:
>> + *	* %0		- OK. Status is stored into @enabled
>> + *	* %-ENODEV	- Failed to find the ATA device
>> + *	* %-EINVAL	- NCQ Priority is not supported or CDL is enabled
>> + */
>> +int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
>> +			bool enable)
>> +{
>> +	struct ata_device *dev;
>> +	unsigned long flags;
>> +	int rc = 0;
>> +
>> +	spin_lock_irqsave(ap->lock, flags);
>>  
>> -	spin_lock_irq(ap->lock);
>> +	dev = ata_scsi_find_dev(ap, sdev);
>> +	if (!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 +997,30 @@ 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);
>> +	bool enable;
>> +	int rc;
>> +
>> +	rc = kstrtobool(buf, &enable);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = ata_ncq_prio_enable(ap, sdev, enable);
>> +	if (rc)
>> +		return rc;
>>  
>> -	return rc ? rc : len;
>> +	return len;
>>  }
>>  
>>  DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 26d68115afb8..6dd9a4f9ca7c 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -1157,6 +1157,12 @@ 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,
>> +				  bool *supported);
>> +extern int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev,
>> +				bool *enabled);
>> +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
>>

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Tue, Mar 12, 2024 at 03:17:43PM +0900, Damien Le Moal wrote:
> On 2024/03/08 19:03, Niklas Cassel wrote:
> > 
> > Anyway, like you said, as you are now creating helper functions:
> > ata_ncq_prio_supported(), ata_ncq_prio_enabled(), ata_ncq_prio_enable()
> > these function might no longer only be called from sysfs code,
> > so it is probably a bad idea to let these functions use spin_lock_irq().
> > 
> > However, can't ata_ncq_prio_supported() and ata_ncq_prio_enabled()
> > still use a simple spin_lock(), why would they need to disable irqs?
> > 
> > Damien, you are the author of ata_ncq_prio_supported_show(), thoughts?
> 
> See above. The spin lock irq-disabling variant is needed because the port lock
> is taken from command completion context.

Yes of course, I don't know what I was thinking...

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

[Niklas Cassel]

On Thu, Mar 07, 2024 at 01:44:12PM -0800, Igor Pylypiv wrote:
> Export libata NCQ Priority configuration helpers to be reused
> for libsas managed SATA devices.
> 
> Switched locking from spin_lock_irq() to spin_lock_irqsave().
> In the future someone might call these helper functions when interrupts
> are disabled. spin_unlock_irq() could lead to a premature re-enabling
> of interrupts, whereas spin_unlock_irqrestore() restores the interrupt
> state to its condition prior to the spin_lock_irqsave() call.
> 
> Acked-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

[PATCH v8 2/7] 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/sda/device/ncq_prio_enable
- /sys/block/sda/device/ncq_prio_supported

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

Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Jason Yan <yanaijie@huawei.com>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/scsi/libsas/sas_ata.c | 82 +++++++++++++++++++++++++++++++++++
 include/scsi/sas_ata.h        |  6 +++
 2 files changed, 88 insertions(+)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 12e2653846e3..b57c041a5544 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -964,3 +964,85 @@ 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);
+	bool supported;
+	int rc;
+
+	rc = ata_ncq_prio_supported(ddev->sata_dev.ap, sdev, &supported);
+	if (rc)
+		return rc;
+
+	return sysfs_emit(buf, "%d\n", supported);
+}
+
+DEVICE_ATTR(ncq_prio_supported, S_IRUGO, sas_ncq_prio_supported_show, NULL);
+
+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);
+	bool enabled;
+	int rc;
+
+	rc = ata_ncq_prio_enabled(ddev->sata_dev.ap, sdev, &enabled);
+	if (rc)
+		return rc;
+
+	return sysfs_emit(buf, "%d\n", enabled);
+}
+
+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);
+	bool enable;
+	int rc;
+
+	rc = kstrtobool(buf, &enable);
+	if (rc)
+		return rc;
+
+	rc = ata_ncq_prio_enable(ddev->sata_dev.ap, sdev, enable);
+	if (rc)
+		return rc;
+
+	return len;
+}
+
+DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
+	    sas_ncq_prio_enable_show, sas_ncq_prio_enable_store);
+
+static struct attribute *sas_ata_sdev_attrs[] = {
+	&dev_attr_ncq_prio_supported.attr,
+	&dev_attr_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..92e27e7bf088 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -39,6 +39,9 @@ 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 +126,9 @@ static inline int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *p
 	sas_ata_disabled_notice();
 	return -ENODEV;
 }
+
+#define sas_ata_sdev_attr_group ((struct attribute_group) {})
+
 #endif
 
 #endif /* _SAS_ATA_H_ */
-- 
2.44.0.278.ge034bb2e1d-goog

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

[Niklas Cassel]

On Thu, Mar 07, 2024 at 01:44:13PM -0800, 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/sda/device/ncq_prio_enable
> - /sys/block/sda/device/ncq_prio_supported
> 
> The newly defined attributes will pass the correct ata_port to libata
> helper functions.
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Jason Yan <yanaijie@huawei.com>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

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

[Geert Uytterhoeven]

Hi Igor,

On Thu, Mar 7, 2024 at 10:55 PM Igor Pylypiv <ipylypiv@google.com> 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/sda/device/ncq_prio_enable
> - /sys/block/sda/device/ncq_prio_supported
>
> The newly defined attributes will pass the correct ata_port to libata
> helper functions.
>
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Jason Yan <yanaijie@huawei.com>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

Thanks for your patch, which is now commit b4d3ddd2df7531e3 ("scsi:
libsas: Define NCQ Priority sysfs attributes for SATA devices")
in scsi-mkp/for-next

> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c

> +
> +DEVICE_ATTR(ncq_prio_supported, S_IRUGO, sas_ncq_prio_supported_show, NULL);
> +

[...]

> +
> +DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
> +           sas_ncq_prio_enable_show, sas_ncq_prio_enable_store);
> +

When both CONFIG_SCSI_SAS_ATA and CONFIG_SATA_HOST are enabled:

aarch64-linux-gnu-ld: drivers/ata/libata-sata.o:(.data+0x110):
multiple definition of `dev_attr_ncq_prio_supported';
drivers/scsi/libsas/sas_ata.o:(.data+0x260): first defined here
aarch64-linux-gnu-ld: drivers/ata/libata-sata.o:(.data+0xd8): multiple
definition of `dev_attr_ncq_prio_enable';
drivers/scsi/libsas/sas_ata.o:(.data+0x228): first defined here

Making both new DEVICE_ATTR() declarations static doesn't work,
as <linux/libata.h> contains a forward declaration for the existing global
dev_attr_ncq_prio_supported in libata:

In file included from include/linux/async.h:14,
                 from drivers/scsi/libsas/sas_ata.c:12:
include/linux/device.h:156:33: error: static declaration of
‘dev_attr_ncq_prio_supported’ follows non-static declaration
  156 |         struct device_attribute dev_attr_##_name =
__ATTR(_name, _mode, _show, _store)
      |                                 ^~~~~~~~~
drivers/scsi/libsas/sas_ata.c:984:8: note: in expansion of macro ‘DEVICE_ATTR’
  984 | static DEVICE_ATTR(ncq_prio_supported, S_IRUGO,
sas_ncq_prio_supported_show,
      |        ^~~~~~~~~~~
In file included from include/scsi/sas_ata.h:13,
                 from drivers/scsi/libsas/sas_ata.c:15:
include/linux/libata.h:508:32: note: previous declaration of
‘dev_attr_ncq_prio_supported’ with type ‘struct device_attribute’
  508 | extern struct device_attribute dev_attr_ncq_prio_supported;
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/async.h:14,
                 from drivers/scsi/libsas/sas_ata.c:12:
include/linux/device.h:156:33: error: static declaration of
‘dev_attr_ncq_prio_enable’ follows non-static declaration
  156 |         struct device_attribute dev_attr_##_name =
__ATTR(_name, _mode, _show, _store)
      |                                 ^~~~~~~~~
drivers/scsi/libsas/sas_ata.c:1023:8: note: in expansion of macro ‘DEVICE_ATTR’
 1023 | static DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
      |        ^~~~~~~~~~~
In file included from include/scsi/sas_ata.h:13,
                 from drivers/scsi/libsas/sas_ata.c:15:
include/linux/libata.h:509:32: note: previous declaration of
‘dev_attr_ncq_prio_enable’ with type ‘struct device_attribute’
  509 | extern struct device_attribute dev_attr_ncq_prio_enable;
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~

Perhaps the new attributes can be renamed?
Alternatively, the DEVICE_ATTR() can be open-coded, so the actual
device_attribute structures are named differently.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Damien Le Moal]

On 3/26/24 18:53, Geert Uytterhoeven wrote:
> Hi Igor,
> 
> On Thu, Mar 7, 2024 at 10:55 PM Igor Pylypiv <ipylypiv@google.com> 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/sda/device/ncq_prio_enable
>> - /sys/block/sda/device/ncq_prio_supported
>>
>> The newly defined attributes will pass the correct ata_port to libata
>> helper functions.
>>
>> Reviewed-by: John Garry <john.g.garry@oracle.com>
>> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
>> Reviewed-by: Jason Yan <yanaijie@huawei.com>
>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> 
> Thanks for your patch, which is now commit b4d3ddd2df7531e3 ("scsi:
> libsas: Define NCQ Priority sysfs attributes for SATA devices")
> in scsi-mkp/for-next
> 
>> --- a/drivers/scsi/libsas/sas_ata.c
>> +++ b/drivers/scsi/libsas/sas_ata.c
> 
>> +
>> +DEVICE_ATTR(ncq_prio_supported, S_IRUGO, sas_ncq_prio_supported_show, NULL);
>> +
> 
> [...]
> 
>> +
>> +DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
>> +           sas_ncq_prio_enable_show, sas_ncq_prio_enable_store);
>> +
> 
> When both CONFIG_SCSI_SAS_ATA and CONFIG_SATA_HOST are enabled:
> 
> aarch64-linux-gnu-ld: drivers/ata/libata-sata.o:(.data+0x110):
> multiple definition of `dev_attr_ncq_prio_supported';
> drivers/scsi/libsas/sas_ata.o:(.data+0x260): first defined here
> aarch64-linux-gnu-ld: drivers/ata/libata-sata.o:(.data+0xd8): multiple
> definition of `dev_attr_ncq_prio_enable';
> drivers/scsi/libsas/sas_ata.o:(.data+0x228): first defined here
> 
> Making both new DEVICE_ATTR() declarations static doesn't work,
> as <linux/libata.h> contains a forward declaration for the existing global
> dev_attr_ncq_prio_supported in libata:
> 
> In file included from include/linux/async.h:14,
>                  from drivers/scsi/libsas/sas_ata.c:12:
> include/linux/device.h:156:33: error: static declaration of
> ‘dev_attr_ncq_prio_supported’ follows non-static declaration
>   156 |         struct device_attribute dev_attr_##_name =
> __ATTR(_name, _mode, _show, _store)
>       |                                 ^~~~~~~~~
> drivers/scsi/libsas/sas_ata.c:984:8: note: in expansion of macro ‘DEVICE_ATTR’
>   984 | static DEVICE_ATTR(ncq_prio_supported, S_IRUGO,
> sas_ncq_prio_supported_show,
>       |        ^~~~~~~~~~~
> In file included from include/scsi/sas_ata.h:13,
>                  from drivers/scsi/libsas/sas_ata.c:15:
> include/linux/libata.h:508:32: note: previous declaration of
> ‘dev_attr_ncq_prio_supported’ with type ‘struct device_attribute’
>   508 | extern struct device_attribute dev_attr_ncq_prio_supported;
>       |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from include/linux/async.h:14,
>                  from drivers/scsi/libsas/sas_ata.c:12:
> include/linux/device.h:156:33: error: static declaration of
> ‘dev_attr_ncq_prio_enable’ follows non-static declaration
>   156 |         struct device_attribute dev_attr_##_name =
> __ATTR(_name, _mode, _show, _store)
>       |                                 ^~~~~~~~~
> drivers/scsi/libsas/sas_ata.c:1023:8: note: in expansion of macro ‘DEVICE_ATTR’
>  1023 | static DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
>       |        ^~~~~~~~~~~
> In file included from include/scsi/sas_ata.h:13,
>                  from drivers/scsi/libsas/sas_ata.c:15:
> include/linux/libata.h:509:32: note: previous declaration of
> ‘dev_attr_ncq_prio_enable’ with type ‘struct device_attribute’
>   509 | extern struct device_attribute dev_attr_ncq_prio_enable;
>       |                                ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> Perhaps the new attributes can be renamed?
> Alternatively, the DEVICE_ATTR() can be open-coded, so the actual
> device_attribute structures are named differently.

I think we need to do that because I do not want the attribute name to change in
sysfs as that creates hell for the user to control a feature that is identical
beside the different transport (which the user should not care about).
I will send something asap.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 3/26/24 18:53, Geert Uytterhoeven wrote:
> Hi Igor,
> 
> On Thu, Mar 7, 2024 at 10:55 PM Igor Pylypiv <ipylypiv@google.com> 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/sda/device/ncq_prio_enable
>> - /sys/block/sda/device/ncq_prio_supported
>>
>> The newly defined attributes will pass the correct ata_port to libata
>> helper functions.
>>
>> Reviewed-by: John Garry <john.g.garry@oracle.com>
>> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
>> Reviewed-by: Jason Yan <yanaijie@huawei.com>
>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> 
> Thanks for your patch, which is now commit b4d3ddd2df7531e3 ("scsi:
> libsas: Define NCQ Priority sysfs attributes for SATA devices")
> in scsi-mkp/for-next
> 
>> --- a/drivers/scsi/libsas/sas_ata.c
>> +++ b/drivers/scsi/libsas/sas_ata.c
> 
>> +
>> +DEVICE_ATTR(ncq_prio_supported, S_IRUGO, sas_ncq_prio_supported_show, NULL);
>> +
> 
> [...]
> 
>> +
>> +DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
>> +           sas_ncq_prio_enable_show, sas_ncq_prio_enable_store);
>> +
> 
> When both CONFIG_SCSI_SAS_ATA and CONFIG_SATA_HOST are enabled:

I have both enabled in my config and I do not see any issue. What is special
with these on ARM ?

> 
> aarch64-linux-gnu-ld: drivers/ata/libata-sata.o:(.data+0x110):
> multiple definition of `dev_attr_ncq_prio_supported';
> drivers/scsi/libsas/sas_ata.o:(.data+0x260): first defined here
> aarch64-linux-gnu-ld: drivers/ata/libata-sata.o:(.data+0xd8): multiple
> definition of `dev_attr_ncq_prio_enable';
> drivers/scsi/libsas/sas_ata.o:(.data+0x228): first defined here
> 
> Making both new DEVICE_ATTR() declarations static doesn't work,
> as <linux/libata.h> contains a forward declaration for the existing global
> dev_attr_ncq_prio_supported in libata:
> 
> In file included from include/linux/async.h:14,
>                  from drivers/scsi/libsas/sas_ata.c:12:
> include/linux/device.h:156:33: error: static declaration of
> ‘dev_attr_ncq_prio_supported’ follows non-static declaration
>   156 |         struct device_attribute dev_attr_##_name =
> __ATTR(_name, _mode, _show, _store)
>       |                                 ^~~~~~~~~
> drivers/scsi/libsas/sas_ata.c:984:8: note: in expansion of macro ‘DEVICE_ATTR’
>   984 | static DEVICE_ATTR(ncq_prio_supported, S_IRUGO,
> sas_ncq_prio_supported_show,
>       |        ^~~~~~~~~~~
> In file included from include/scsi/sas_ata.h:13,
>                  from drivers/scsi/libsas/sas_ata.c:15:
> include/linux/libata.h:508:32: note: previous declaration of
> ‘dev_attr_ncq_prio_supported’ with type ‘struct device_attribute’
>   508 | extern struct device_attribute dev_attr_ncq_prio_supported;
>       |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from include/linux/async.h:14,
>                  from drivers/scsi/libsas/sas_ata.c:12:
> include/linux/device.h:156:33: error: static declaration of
> ‘dev_attr_ncq_prio_enable’ follows non-static declaration
>   156 |         struct device_attribute dev_attr_##_name =
> __ATTR(_name, _mode, _show, _store)
>       |                                 ^~~~~~~~~
> drivers/scsi/libsas/sas_ata.c:1023:8: note: in expansion of macro ‘DEVICE_ATTR’
>  1023 | static DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
>       |        ^~~~~~~~~~~
> In file included from include/scsi/sas_ata.h:13,
>                  from drivers/scsi/libsas/sas_ata.c:15:
> include/linux/libata.h:509:32: note: previous declaration of
> ‘dev_attr_ncq_prio_enable’ with type ‘struct device_attribute’
>   509 | extern struct device_attribute dev_attr_ncq_prio_enable;
>       |                                ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> Perhaps the new attributes can be renamed?
> Alternatively, the DEVICE_ATTR() can be open-coded, so the actual
> device_attribute structures are named differently.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

-- 
Damien Le Moal
Western Digital Research

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

[Geert Uytterhoeven]

Hi Damien,

On Tue, Mar 26, 2024 at 11:07 AM Damien Le Moal <dlemoal@kernel.org> wrote:
> On 3/26/24 18:53, Geert Uytterhoeven wrote:
> > On Thu, Mar 7, 2024 at 10:55 PM Igor Pylypiv <ipylypiv@google.com> 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/sda/device/ncq_prio_enable
> >> - /sys/block/sda/device/ncq_prio_supported
> >>
> >> The newly defined attributes will pass the correct ata_port to libata
> >> helper functions.
> >>
> >> Reviewed-by: John Garry <john.g.garry@oracle.com>
> >> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> >> Reviewed-by: Jason Yan <yanaijie@huawei.com>
> >> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> >
> > Thanks for your patch, which is now commit b4d3ddd2df7531e3 ("scsi:
> > libsas: Define NCQ Priority sysfs attributes for SATA devices")
> > in scsi-mkp/for-next
> >
> >> --- a/drivers/scsi/libsas/sas_ata.c
> >> +++ b/drivers/scsi/libsas/sas_ata.c
> >
> >> +
> >> +DEVICE_ATTR(ncq_prio_supported, S_IRUGO, sas_ncq_prio_supported_show, NULL);
> >> +
> >
> > [...]
> >
> >> +
> >> +DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
> >> +           sas_ncq_prio_enable_show, sas_ncq_prio_enable_store);
> >> +
> >
> > When both CONFIG_SCSI_SAS_ATA and CONFIG_SATA_HOST are enabled:
>
> I have both enabled in my config and I do not see any issue. What is special
> with these on ARM ?

Modular or built-in?
I have them built-in, and it fails on arm64 (with renesas_defconfig,
which is not upstream).
It also fails with shmobile_defconfig on arm32, after manually adding
CONFIG_SCSI_SAS_LIBSAS=y and CONFIG_SCSI_SAS_ATA=y.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Damien Le Moal]

On 3/26/24 19:16, Geert Uytterhoeven wrote:
> Hi Damien,
> 
> On Tue, Mar 26, 2024 at 11:07 AM Damien Le Moal <dlemoal@kernel.org> wrote:
>> On 3/26/24 18:53, Geert Uytterhoeven wrote:
>>> On Thu, Mar 7, 2024 at 10:55 PM Igor Pylypiv <ipylypiv@google.com> 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/sda/device/ncq_prio_enable
>>>> - /sys/block/sda/device/ncq_prio_supported
>>>>
>>>> The newly defined attributes will pass the correct ata_port to libata
>>>> helper functions.
>>>>
>>>> Reviewed-by: John Garry <john.g.garry@oracle.com>
>>>> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
>>>> Reviewed-by: Jason Yan <yanaijie@huawei.com>
>>>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
>>>
>>> Thanks for your patch, which is now commit b4d3ddd2df7531e3 ("scsi:
>>> libsas: Define NCQ Priority sysfs attributes for SATA devices")
>>> in scsi-mkp/for-next
>>>
>>>> --- a/drivers/scsi/libsas/sas_ata.c
>>>> +++ b/drivers/scsi/libsas/sas_ata.c
>>>
>>>> +
>>>> +DEVICE_ATTR(ncq_prio_supported, S_IRUGO, sas_ncq_prio_supported_show, NULL);
>>>> +
>>>
>>> [...]
>>>
>>>> +
>>>> +DEVICE_ATTR(ncq_prio_enable, S_IRUGO | S_IWUSR,
>>>> +           sas_ncq_prio_enable_show, sas_ncq_prio_enable_store);
>>>> +
>>>
>>> When both CONFIG_SCSI_SAS_ATA and CONFIG_SATA_HOST are enabled:
>>
>> I have both enabled in my config and I do not see any issue. What is special
>> with these on ARM ?
> 
> Modular or built-in?
> I have them built-in, and it fails on arm64 (with renesas_defconfig,
> which is not upstream).
> It also fails with shmobile_defconfig on arm32, after manually adding
> CONFIG_SCSI_SAS_LIBSAS=y and CONFIG_SCSI_SAS_ATA=y.

Hmm... That must be it. I did a modular build.
Will check that again and send a fix. Thanks.


-- 
Damien Le Moal
Western Digital Research

[PATCH v8 3/7] 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.

Acked-by: Jack Wang <jinpu.wang@ionos.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Igor Pylypiv <ipylypiv@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

[PATCH v8 4/7] scsi: mvsas: 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.

Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/scsi/mvsas/mv_init.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 43ebb331e216..f1090bb5f2c9 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -26,6 +26,7 @@ static const struct mvs_chip_info mvs_chips[] = {
 };
 
 static const struct attribute_group *mvst_host_groups[];
+static const struct attribute_group *mvst_sdev_groups[];
 
 #define SOC_SAS_NUM 2
 
@@ -53,6 +54,7 @@ static const struct scsi_host_template mvs_sht = {
 	.compat_ioctl		= sas_ioctl,
 #endif
 	.shost_groups		= mvst_host_groups,
+	.sdev_groups		= mvst_sdev_groups,
 	.track_queue_depth	= 1,
 };
 
@@ -779,6 +781,11 @@ static struct attribute *mvst_host_attrs[] = {
 
 ATTRIBUTE_GROUPS(mvst_host);
 
+static const struct attribute_group *mvst_sdev_groups[] = {
+	&sas_ata_sdev_attr_group,
+	NULL
+};
+
 module_init(mvs_init);
 module_exit(mvs_exit);
 
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH v8 4/7] scsi: mvsas: Add libsas SATA sysfs attributes group

[Niklas Cassel]

On Thu, Mar 07, 2024 at 01:44:15PM -0800, 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.
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

[PATCH v8 5/7] scsi: hisi_sas: 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.

Omitted hisi_sas_v1_hw.c because v1 HW doesn't support SATA.

Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 6 ++++++
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 73b378837da7..b5d379ebe05d 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -3544,6 +3544,11 @@ static struct attribute *host_v2_hw_attrs[] = {
 
 ATTRIBUTE_GROUPS(host_v2_hw);
 
+static const struct attribute_group *sdev_groups_v2_hw[] = {
+	&sas_ata_sdev_attr_group,
+	NULL
+};
+
 static void map_queues_v2_hw(struct Scsi_Host *shost)
 {
 	struct hisi_hba *hisi_hba = shost_priv(shost);
@@ -3585,6 +3590,7 @@ static const struct scsi_host_template sht_v2_hw = {
 	.compat_ioctl		= sas_ioctl,
 #endif
 	.shost_groups		= host_v2_hw_groups,
+	.sdev_groups		= sdev_groups_v2_hw,
 	.host_reset		= hisi_sas_host_reset,
 	.map_queues		= map_queues_v2_hw,
 	.host_tagset		= 1,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index b56fbc61a15a..9b69ea16a1e6 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -2929,6 +2929,11 @@ static struct attribute *host_v3_hw_attrs[] = {
 
 ATTRIBUTE_GROUPS(host_v3_hw);
 
+static const struct attribute_group *sdev_groups_v3_hw[] = {
+	&sas_ata_sdev_attr_group,
+	NULL
+};
+
 #define HISI_SAS_DEBUGFS_REG(x) {#x, x}
 
 struct hisi_sas_debugfs_reg_lu {
@@ -3340,6 +3345,7 @@ static const struct scsi_host_template sht_v3_hw = {
 	.compat_ioctl		= sas_ioctl,
 #endif
 	.shost_groups		= host_v3_hw_groups,
+	.sdev_groups		= sdev_groups_v3_hw,
 	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,
 	.host_reset             = hisi_sas_host_reset,
 	.host_tagset		= 1,
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH v8 5/7] scsi: hisi_sas: Add libsas SATA sysfs attributes group

[Niklas Cassel]

On Thu, Mar 07, 2024 at 01:44:16PM -0800, 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.
> 
> Omitted hisi_sas_v1_hw.c because v1 HW doesn't support SATA.
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

[PATCH v8 6/7] scsi: aic94xx: 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.

Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/scsi/aic94xx/aic94xx_init.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index 8a3340d8d7ad..ccccd0eb6275 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -14,6 +14,7 @@
 #include <linux/firmware.h>
 #include <linux/slab.h>
 
+#include <scsi/sas_ata.h>
 #include <scsi/scsi_host.h>
 
 #include "aic94xx.h"
@@ -34,6 +35,7 @@ MODULE_PARM_DESC(use_msi, "\n"
 static struct scsi_transport_template *aic94xx_transport_template;
 static int asd_scan_finished(struct Scsi_Host *, unsigned long);
 static void asd_scan_start(struct Scsi_Host *);
+static const struct attribute_group *asd_sdev_groups[];
 
 static const struct scsi_host_template aic94xx_sht = {
 	.module			= THIS_MODULE,
@@ -60,6 +62,7 @@ static const struct scsi_host_template aic94xx_sht = {
 	.compat_ioctl		= sas_ioctl,
 #endif
 	.track_queue_depth	= 1,
+	.sdev_groups		= asd_sdev_groups,
 };
 
 static int asd_map_memio(struct asd_ha_struct *asd_ha)
@@ -951,6 +954,11 @@ static void asd_remove_driver_attrs(struct device_driver *driver)
 	driver_remove_file(driver, &driver_attr_version);
 }
 
+static const struct attribute_group *asd_sdev_groups[] = {
+	&sas_ata_sdev_attr_group,
+	NULL
+};
+
 static struct sas_domain_function_template aic94xx_transport_functions = {
 	.lldd_dev_found		= asd_dev_found,
 	.lldd_dev_gone		= asd_dev_gone,
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH v8 6/7] scsi: aic94xx: Add libsas SATA sysfs attributes group

[Niklas Cassel]

On Thu, Mar 07, 2024 at 01:44:17PM -0800, 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.
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

[PATCH v8 7/7] scsi: isci: 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.

Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/scsi/isci/init.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index 6277162a028b..8658dcd61b87 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -149,6 +149,11 @@ static struct attribute *isci_host_attrs[] = {
 
 ATTRIBUTE_GROUPS(isci_host);
 
+static const struct attribute_group *isci_sdev_groups[] = {
+	&sas_ata_sdev_attr_group,
+	NULL
+};
+
 static const struct scsi_host_template isci_sht = {
 
 	.module				= THIS_MODULE,
@@ -176,6 +181,7 @@ static const struct scsi_host_template isci_sht = {
 	.compat_ioctl			= sas_ioctl,
 #endif
 	.shost_groups			= isci_host_groups,
+	.sdev_groups			= isci_sdev_groups,
 	.track_queue_depth		= 1,
 };
 
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH v8 0/7] NCQ Priority sysfs sttributes for libsas

[Martin K. Petersen]

Igor,

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

Applied to 6.10/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering