Hello Igor, On Tue, Mar 05, 2024 at 05:22:20PM -0800, Igor Pylypiv wrote: > Export libata NCQ Priority configuration helpers to be reused > for libsas managed SATA devices. > > Acked-by: Damien Le Moal <dlemoal@xxxxxxxxxx> > Reviewed-by: Jason Yan <yanaijie@xxxxxxxxxx> > Signed-off-by: Igor Pylypiv <ipylypiv@xxxxxxxxxx> > --- > drivers/ata/libata-sata.c | 140 +++++++++++++++++++++++++++----------- > include/linux/libata.h | 6 ++ > 2 files changed, 107 insertions(+), 39 deletions(-) > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index 0fb1934875f2..f00dd02dc6f8 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -848,80 +848,122 @@ 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. This kdoc is missing a "Return:", see: https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html (Same comment for other functions.) > + */ > +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); You should mention why you are changing this from a spin_lock_irq() to a spin_lock_irqsave() in the commit message. > 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; You removed the blank link between spin_unlock and return, please keep this empty line. (Same comment for other functions.) > +} > +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; > > - return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_supported); > + rc = ata_ncq_prio_supported(ap, sdev, &supported); > + if (rc) > + return rc; > + > + 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. > + */ > +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); > - > - return rc ? rc : sysfs_emit(buf, "%u\n", ncq_prio_enable); > + *enabled = dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED; > + spin_unlock_irqrestore(ap->lock, flags); > + 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. > + */ > +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 +976,29 @@ 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 >
