| Message ID | 20240305005103.1849325-3-ipylypiv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | NCQ Priority sysfs sttributes for libsas | expand |
On 2024/3/5 8:50, 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> > Signed-off-by: Igor Pylypiv<ipylypiv@google.com> > --- > drivers/scsi/libsas/sas_ata.c | 94 +++++++++++++++++++++++++++++++++++ > include/scsi/sas_ata.h | 6 +++ > 2 files changed, 100 insertions(+) Reviewed-by: Jason Yan <yanaijie@huawei.com>
On 05/03/2024 00:50, Igor Pylypiv wrote: > > 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, > +}; I just noticed a build issue. With CONFIG_SCSI_SAS_ATA not set, I get this for W=1 build: In file included from drivers/scsi/hisi_sas/hisi_sas.h:29, from drivers/scsi/hisi_sas/hisi_sas_v1_hw.c:7: ./include/scsi/sas_ata.h:129:37: error: ‘sas_ata_sdev_attr_group’ defined but not used [-Werror=unused-const-variable=] 129 | static const struct attribute_group sas_ata_sdev_attr_group = { I suppose that marking sas_ata_sdev_attr_group as __maybe_unused is ok, but less than ideal. The linker should strip it out of files when unused. I think that this is also ok: #define sas_ata_sdev_attr_group (struct attribute_group) {} The compiler here will create a empty structure and have &sas_ata_sdev_attr_group point at it. Thanks, John
On Tue, Mar 05, 2024 at 11:29:11AM +0000, John Garry wrote: > On 05/03/2024 00:50, Igor Pylypiv wrote: > > 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, > > +}; > > I just noticed a build issue. > > With CONFIG_SCSI_SAS_ATA not set, I get this for W=1 build: > > In file included from drivers/scsi/hisi_sas/hisi_sas.h:29, > from drivers/scsi/hisi_sas/hisi_sas_v1_hw.c:7: > ./include/scsi/sas_ata.h:129:37: error: ‘sas_ata_sdev_attr_group’ > defined but not used [-Werror=unused-const-variable=] > 129 | static const struct attribute_group sas_ata_sdev_attr_group = { Thanks for catching this, John! For some reason I only get this warning with gcc but not with clang. > > I suppose that marking sas_ata_sdev_attr_group as __maybe_unused is ok, but > less than ideal. The linker should strip it out of files when unused. Looks like adding the __maybe_unused attribute is a prefferred way since it is mentioned in the Linux kernel coding style: https://www.kernel.org/doc/html/v6.7/process/coding-style.html#conditional-compilation Added the __maybe_unused attribute in v6. Thank you! > > I think that this is also ok: > > #define sas_ata_sdev_attr_group (struct attribute_group) {} > > The compiler here will create a empty structure and have > &sas_ata_sdev_attr_group point at it. > > Thanks, > John
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index 12e2653846e3..04b0bd9a4e01 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -964,3 +964,97 @@ 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; + + /* This attribute shall be visible for SATA devices only */ + if (WARN_ON_ONCE(!dev_is_sata(ddev))) + return -EINVAL; + + 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; + + /* This attribute shall be visible for SATA devices only */ + if (WARN_ON_ONCE(!dev_is_sata(ddev))) + return -EINVAL; + + 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; + + /* This attribute shall be visible for SATA devices only */ + if (WARN_ON_ONCE(!dev_is_sata(ddev))) + return -EINVAL; + + 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..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_ */