Display EVPD pages in sysfs
v5 cover Display EVPD pages in sysfs
Hi all,
here's now the (hopefully) final version of my patch to display EVPD pages in sysfs. After some discussion with Kay Sievers I've found that the whole thing can be made far simpler by using the 'is_visible' callback to the sysfs attribute group. And we can even simplify existing code here.
The patch itself has been modified to add boundary checks to handle invalid pages gracefully.
Hannes Reinecke (2):
scsi_sysfs: Implement 'is_visible' callback
Add EVPD page 0x83 entries to sysfs
drivers/scsi/scsi_scan.c | 3 + drivers/scsi/scsi_sysfs.c | 518 +++++++++++++++++++++++++++++++++++++-------- include/scsi/scsi.h | 4 +- include/scsi/scsi_device.h | 3 + 4 files changed, 433 insertions(+), 95 deletions(-)
Signature · 1 line
1.7.12.4
PATCH v5 1/2 scsi_sysfs: Implement 'is_visible' callback
Instead of modifying attributes after the device has been created we should be using the 'is_visible' callback to avoid races.
--- drivers/scsi/scsi_sysfs.c | 195 ++++++++++++++++++++++++---------------------- 1 file changed, 102 insertions(+), 93 deletions(-)
drivers/scsi/scsi_sysfs.c +102 -93
@@ -579,7 +579,6 @@ static int scsi_sdev_check_buf_bit(const char *buf)* Create the actual show/store functions and data structures. */ sdev_rd_attr (device_blocked,"%d\n" ); -sdev_rd_attr (queue_depth,"%d\n" );sdev_rd_attr (device_busy,"%d\n" );sdev_rd_attr (type,"%d\n" );sdev_rd_attr (scsi_level,"%d\n" );
@@ -723,7 +722,37 @@ show_queue_type_field(struct device *dev, struct device_attribute *attr,return snprintf (buf,20 ,"%s\n" , name); } -static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field, NULL); +static ssize_t +store_queue_type_field (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +struct scsi_device *sdev =to_scsi_device (dev); +struct scsi_host_template *sht = sdev->host->hostt; +int tag_type =0 , retval; +int prev_tag_type =scsi_get_tag_type (sdev); + +if (!sdev->tagged_supported || !sht->change_queue_type) +return -EINVAL ; + +if (strncmp (buf,"ordered" ,7 ) ==0 ) + tag_type =MSG_ORDERED_TAG ; +else if (strncmp (buf,"simple" ,6 ) ==0 ) + tag_type =MSG_SIMPLE_TAG ; +else if (strncmp (buf,"none" ,4 ) !=0 ) +return -EINVAL ; + +if (tag_type == prev_tag_type) +return count; + + retval = sht->change_queue_type (sdev, tag_type); +if (retval <0 ) +return retval; + +return count; +} + +static DEVICE_ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field, + store_queue_type_field);static ssize_t show_iostat_counterbits (struct device *dev,struct device_attribute *attr,char *buf)
@@ -797,46 +826,9 @@ DECLARE_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED_REPORTED) DECLARE_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED) DECLARE_EVT(lun_change_reported, LUN_CHANGE_REPORTED) -/* Default template for device attributes. May NOT be modified */ -static struct attribute *scsi_sdev_attrs[] = { - &dev_attr_device_blocked.attr, - &dev_attr_type.attr, - &dev_attr_scsi_level.attr, - &dev_attr_device_busy.attr, - &dev_attr_vendor.attr, - &dev_attr_model.attr, - &dev_attr_rev.attr, - &dev_attr_rescan.attr, - &dev_attr_delete.attr, - &dev_attr_state.attr, - &dev_attr_timeout.attr, - &dev_attr_eh_timeout.attr, - &dev_attr_iocounterbits.attr, - &dev_attr_iorequest_cnt.attr, - &dev_attr_iodone_cnt.attr, - &dev_attr_ioerr_cnt.attr, - &dev_attr_modalias.attr, -REF_EVT (media_change), -REF_EVT (inquiry_change_reported), -REF_EVT (capacity_change_reported), -REF_EVT (soft_threshold_reached), -REF_EVT (mode_parameter_change_reported), -REF_EVT (lun_change_reported), -NULL -}; - -static struct attribute_group scsi_sdev_attr_group = { - .attrs = scsi_sdev_attrs, -}; - -static const struct attribute_group *scsi_sdev_attr_groups[] = { - &scsi_sdev_attr_group, -NULL -}; -static ssize_t -sdev_store_queue_depth_rw (struct device *dev,struct device_attribute *attr, -const char *buf,size_t count) +sdev_store_queue_depth (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) {int depth, retval;struct scsi_device *sdev =to_scsi_device (dev);
@@ -859,10 +851,10 @@ sdev_store_queue_depth_rw(struct device *dev, struct device_attribute *attr,return count; } +sdev_show_function (queue_depth,"%d\n" ); -static struct device_attribute sdev_attr_queue_depth_rw = -__ATTR (queue_depth,S_IRUGO |S_IWUSR , sdev_show_queue_depth, - sdev_store_queue_depth_rw); +static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth, + sdev_store_queue_depth); static ssize_t sdev_show_queue_ramp_up_period(struct device *dev,
@@ -890,39 +882,79 @@ sdev_store_queue_ramp_up_period(struct device *dev,return period; } -static struct device_attribute sdev_attr_queue_ramp_up_period = -__ATTR (queue_ramp_up_period,S_IRUGO |S_IWUSR , - sdev_show_queue_ramp_up_period, - sdev_store_queue_ramp_up_period); +static DEVICE_ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR, + sdev_show_queue_ramp_up_period, + sdev_store_queue_ramp_up_period); -static ssize_t -sdev_store_queue_type_rw (struct device *dev,struct device_attribute *attr, -const char *buf,size_t count) +static umode_t scsi_sdev_attr_is_visible (struct kobject *kobj, +struct attribute *attr,int i) { -struct scsi_device *sdev =to_scsi_device (dev); -struct scsi_host_template *sht = sdev->host->hostt; -int tag_type =0 , retval; -int prev_tag_type =scsi_get_tag_type (sdev); + struct device *dev = container_of(kobj, struct device, kobj); +struct scsi_device *sdev =to_scsi_device (dev); \ -if (!sdev->tagged_supported || !sht->change_queue_type) -return -EINVAL ; -if (strncmp (buf,"ordered" ,7 ) ==0 ) - tag_type =MSG_ORDERED_TAG ; -else if (strncmp (buf,"simple" ,6 ) ==0 ) - tag_type =MSG_SIMPLE_TAG ; -else if (strncmp (buf,"none" ,4 ) !=0 ) -return -EINVAL ; +if (attr == &dev_attr_queue_depth.attr) { +if (sdev->host->hostt->change_queue_depth) +return S_IRUGO |S_IWUSR ; +else +return S_IRUGO ; + } +if (attr == &dev_attr_queue_ramp_up_period.attr) { +if (sdev->host->hostt->change_queue_depth) +return S_IRUGO |S_IWUSR ; +else +return 0 ; + } +if (attr == &dev_attr_queue_type.attr) { +if (sdev->host->hostt->change_queue_type) +return S_IRUGO |S_IWUSR ; +else +return S_IRUGO ; + } -if (tag_type == prev_tag_type) -return count; +return attr->mode; +} - retval = sht->change_queue_type (sdev, tag_type); -if (retval <0 ) -return retval; +/* Default template for device attributes. May NOT be modified */ +static struct attribute *scsi_sdev_attrs[] = { + &dev_attr_device_blocked.attr, + &dev_attr_type.attr, + &dev_attr_scsi_level.attr, + &dev_attr_device_busy.attr, + &dev_attr_vendor.attr, + &dev_attr_model.attr, + &dev_attr_rev.attr, + &dev_attr_rescan.attr, + &dev_attr_delete.attr, + &dev_attr_state.attr, + &dev_attr_timeout.attr, + &dev_attr_eh_timeout.attr, + &dev_attr_iocounterbits.attr, + &dev_attr_iorequest_cnt.attr, + &dev_attr_iodone_cnt.attr, + &dev_attr_ioerr_cnt.attr, + &dev_attr_modalias.attr, + &dev_attr_queue_depth.attr, + &dev_attr_queue_type.attr, + &dev_attr_queue_ramp_up_period.attr, +REF_EVT (media_change), +REF_EVT (inquiry_change_reported), +REF_EVT (capacity_change_reported), +REF_EVT (soft_threshold_reached), +REF_EVT (mode_parameter_change_reported), +REF_EVT (lun_change_reported), +NULL +}; -return count; -} +static struct attribute_group scsi_sdev_attr_group = { + .attrs = scsi_sdev_attrs, + .is_visible = scsi_sdev_attr_is_visible, +}; + +static const struct attribute_group *scsi_sdev_attr_groups[] = { + &scsi_sdev_attr_group, +NULL +};static int scsi_target_add (struct scsi_target *starget) {
@@ -946,10 +978,6 @@ static int scsi_target_add(struct scsi_target *starget)return 0 ; } -static struct device_attribute sdev_attr_queue_type_rw = -__ATTR (queue_type,S_IRUGO |S_IWUSR , show_queue_type_field, - sdev_store_queue_type_rw); -/** * scsi_sysfs_add_sdev - add scsi device to sysfs * @sdev: scsi_device to add
@@ -1003,25 +1031,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)transport_add_device (&sdev->sdev_gendev); sdev->is_visible =1 ; -/* create queue files, which may be writable, depending on the host */ -if (sdev->host->hostt->change_queue_depth) { - error =device_create_file (&sdev->sdev_gendev, - &sdev_attr_queue_depth_rw); - error =device_create_file (&sdev->sdev_gendev, - &sdev_attr_queue_ramp_up_period); - } -else - error =device_create_file (&sdev->sdev_gendev, &dev_attr_queue_depth); -if (error) -return error; - -if (sdev->host->hostt->change_queue_type) - error =device_create_file (&sdev->sdev_gendev, &sdev_attr_queue_type_rw); -else - error =device_create_file (&sdev->sdev_gendev, &dev_attr_queue_type); -if (error) -return error; - error =bsg_register_queue (rq, &sdev->sdev_gendev,NULL ,NULL );if (error)
Signature · 1 line
1.7.12.4
Re: [PATCH 1/2] scsi_sysfs: Implement 'is_visible' callback
Quoted 2 lines · Hannes Reinecke
Instead of modifying attributes after the device has been created we should be using the '
is_visible' callback to avoid races.
Can't say I like the fact that the is_visible function needs is per-group and not per attribute and thus needs to explicitly check which attribute we call it for, but that seems to be a design issue in the driver core and not the patch.
Otherwise looks good with the minor nitpick that it would have looked cleaner in the patch if you had left the queue_type store function in the location it previously was.
PATCH v5 2/2 Add EVPD page 0x83 entries to sysfs
In reply to Hannes ReineckeEVPD page 0x83 is used to uniquely identify the device. So instead of having each and every program issue a separate SG_IO call to retrieve this information it does make far more sense to display it in sysfs.
The page is displayed in its entirety with the attribute 'vpd_pg83', and the individual designators are stored in ident_<association>_<designator> attributes. Duplicate designators are added as individual lines to the corresponding attribute.
--- drivers/scsi/scsi_scan.c | 3 + drivers/scsi/scsi_sysfs.c | 323 ++++++++++++++++++++++++++++++++++++++++++++- include/scsi/scsi.h | 4 +- include/scsi/scsi_device.h | 3 + 4 files changed, 331 insertions(+), 2 deletions(-)
drivers/scsi/scsi_scan.c +3 -0
@@ -929,6 +929,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,if (*bflags &BLIST_SKIP_VPD_PAGES ) sdev->skip_vpd_pages =1 ; +if (sdev->scsi_level >=SCSI_3 ) +scsi_attach_vpd_ident (sdev); +transport_configure_device (&sdev->sdev_gendev);if (sdev->host->hostt->slave_configure) {
drivers/scsi/scsi_sysfs.c +322 -1
348 lines not on this page · raw message
include/scsi/scsi.h +3 -1
@@ -334,7 +334,7 @@ static inline int scsi_status_is_good(int status)#define TYPE_OSD 0x11#define TYPE_NO_LUN 0x7f -/* SCSI protocols; these are taken from SPC-3 section 7.5 */ +/* SCSI protocols; these are taken from SPC-4 section 7.6 */ enum scsi_protocol {SCSI_PROTOCOL_FCP =0 ,/* Fibre Channel */ SCSI_PROTOCOL_SPI =1 ,/* parallel SCSI */
@@ -345,6 +345,8 @@ enum scsi_protocol {SCSI_PROTOCOL_SAS =6 ,SCSI_PROTOCOL_ADT =7 ,/* Media Changers */ SCSI_PROTOCOL_ATA =8 , +SCSI_PROTOCOL_UAS =9 , +SCSI_PROTOCOL_SOP =0xa , SCSI_PROTOCOL_UNSPEC = 0xf, /* No specific protocol */ };
include/scsi/scsi_device.h +3 -0
@@ -113,6 +113,8 @@ struct scsi_device {const char * vendor;/* [back_compat] point into 'inquiry' ... */ const char * model;/* ... after scan; point to static string */ const char * rev;/* ... "nullnullnullnull" before scan */ +unsigned char vpd_ident_len; +unsigned char *vpd_ident;struct scsi_target *sdev_target;/* used only for single_lun */ unsigned int sdev_bflags;/* black/white flags as also found in
@@ -308,6 +310,7 @@ extern int scsi_add_device(struct Scsi_Host *host, uint channel,extern int scsi_register_device_handler (struct scsi_device_handler *scsi_dh);extern void scsi_remove_device (struct scsi_device *);extern int scsi_unregister_device_handler (struct scsi_device_handler *scsi_dh); +void scsi_attach_vpd_ident (struct scsi_device *sdev);extern int scsi_device_get (struct scsi_device *);extern void scsi_device_put (struct scsi_device *);
Signature · 1 line
1.7.12.4
Re: [PATCH 2/2] Add EVPD page 0x83 entries to sysfs
Quoted 9 lines · Hannes Reinecke
EVPD page
0x83is used to uniquely identify the device. So instead of having each and every program issue a separateSG_IOcall to retrieve this information it does make far more sense to display it in sysfs.
The page is displayed in its entirety with the attribute 'vpd_pg83', and the individual designators are stored in ident_<association>_<designator> attributes. Duplicate designators are added as individual lines to the corresponding attribute.
Looks correct to me, suggestions for a few possible cleanups below.
Quoted 4 lines
drivers/scsi/scsi_sysfs.c +4
-0+#define SCSI_VPD_ASSOC_lun 0x0 +#define SCSI_VPD_ASSOC_port 0x1 +#define SCSI_VPD_ASSOC_target 0x2 +
drivers/scsi/scsi_sysfs.c store_queue_type_field(struct device *dev, struct device_attribute *attr, · line 817
Quoted 10 lines
drivers/scsi/scsi_sysfs.c +10
-0+#define SCSI_VPD_DESIG_vendor 0x0 +#define SCSI_VPD_DESIG_t10 0x1 +#define SCSI_VPD_DESIG_eui 0x2 +#define SCSI_VPD_DESIG_naa 0x3 +#define SCSI_VPD_DESIG_relport 0x4 +#define SCSI_VPD_DESIG_tpgrp 0x5 +#define SCSI_VPD_DESIG_lugrp 0x6 +#define SCSI_VPD_DESIG_md5 0x7 +#define SCSI_VPD_DESIG_scsi_name 0x8 +#define SCSI_VPD_DESIG_proto 0x9
Shouldn't these constants be in a header?
drivers/scsi/scsi_sysfs.c store_queue_type_field(struct device *dev, struct device_attribute *attr, · line 866
Quoted 7 lines
drivers/scsi/scsi_sysfs.c +7
-0+switch (d[0 ] >>4 ) { +case SCSI_PROTOCOL_FCP : + proto ="fcp" ; +break ; +case SCSI_PROTOCOL_SPI : + proto ="spi" ; +break ;
Splitting the protocol mapping into a separate helper woulkd be good. In fact this could easily be an array lookup as well.
drivers/scsi/scsi_sysfs.c store_queue_type_field(struct device *dev, struct device_attribute *attr, · line 1023
Quoted 7 lines
drivers/scsi/scsi_sysfs.c +7
-0+#define sdev_evpd_test_and_show_attr (sdev, attr, assoc, desig) \ + if (attr == &dev_attr_ident_##assoc##_##desig.attr) { \ + if (scsi_test_vpd_ident(sdev, SCSI_VPD_ASSOC_##assoc, \ + SCSI_VPD_DESIG_##desig)) \ + return S_IRUGO; \ + else \ + return 0; \
No need for the else here.
drivers/scsi/scsi_sysfs.c static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj, · line 1190
Quoted 6 lines
drivers/scsi/scsi_sysfs.c +6
-0+if (attr == &dev_attr_vpd_pg83.attr) { +if (sdev->vpd_ident) +return S_IRUGO ; +else +return 0 ; + }
Same here.
Re: [PATCH 2/2] Add EVPD page 0x83 entries to sysfs
Quoted 15 lines · Christoph Hellwig
Quoted 9 lines · Hannes Reinecke
EVPD page
0x83is used to uniquely identify the device. So instead of having each and every program issue a separateSG_IOcall to retrieve this information it does make far more sense to display it in sysfs.
The page is displayed in its entirety with the attribute 'vpd_pg83', and the individual designators are stored in ident_<association>_<designator> attributes. Duplicate designators are added as individual lines to the corresponding attribute.Looks correct to me, suggestions for a few possible cleanups below.
Signed-off-by Christoph Hellwig <[email protected]>
Quoted 17 lines
Quoted 4 lines
drivers/scsi/scsi_sysfs.c +4
-0+#define SCSI_VPD_ASSOC_lun 0x0 +#define SCSI_VPD_ASSOC_port 0x1 +#define SCSI_VPD_ASSOC_target 0x2 +drivers/scsi/scsi_sysfs.c store_queue_type_field(struct device *dev, struct device_attribute *attr, · line 817
Quoted 10 lines
drivers/scsi/scsi_sysfs.c +10
-0+#define SCSI_VPD_DESIG_vendor 0x0 +#define SCSI_VPD_DESIG_t10 0x1 +#define SCSI_VPD_DESIG_eui 0x2 +#define SCSI_VPD_DESIG_naa 0x3 +#define SCSI_VPD_DESIG_relport 0x4 +#define SCSI_VPD_DESIG_tpgrp 0x5 +#define SCSI_VPD_DESIG_lugrp 0x6 +#define SCSI_VPD_DESIG_md5 0x7 +#define SCSI_VPD_DESIG_scsi_name 0x8 +#define SCSI_VPD_DESIG_proto 0x9Shouldn't these constants be in a header?
Radio Yerewan answers: In principle, yes.
But then the definitions a closely tied to the following macro (hence the lowercase in the last string), so it's not _that_ useful for a general definition.
If we were to move the constants into a header I'd have to re-do all the macros (up to and including DEVICE_ATTR) building up the sysfs attributes. Which currently doesn't give any benefits whatsoever.
But if someone insists ...
Quoted 11 lines
drivers/scsi/scsi_sysfs.c store_queue_type_field(struct device *dev, struct device_attribute *attr, · line 866
Quoted 7 lines
drivers/scsi/scsi_sysfs.c +7
-0+switch (d[0 ] >>4 ) { +case SCSI_PROTOCOL_FCP : + proto ="fcp" ; +break ; +case SCSI_PROTOCOL_SPI : + proto ="spi" ; +break ;Splitting the protocol mapping into a separate helper woulkd be good. In fact this could easily be an array lookup as well.
Ok.
Quoted 10 lines
drivers/scsi/scsi_sysfs.c store_queue_type_field(struct device *dev, struct device_attribute *attr, · line 1023
Quoted 7 lines
drivers/scsi/scsi_sysfs.c +7
-0+#define sdev_evpd_test_and_show_attr (sdev, attr, assoc, desig) \ + if (attr == &dev_attr_ident_##assoc##_##desig.attr) { \ + if (scsi_test_vpd_ident(sdev, SCSI_VPD_ASSOC_##assoc, \ + SCSI_VPD_DESIG_##desig)) \ + return S_IRUGO; \ + else \ + return 0; \No need for the else here.
Bzzt. The "else" branch is required, the "if" branch can be skipped.
(hch was wrong! Quick, mark the day!)
Quoted 9 lines
drivers/scsi/scsi_sysfs.c static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj, · line 1190
Quoted 6 lines
drivers/scsi/scsi_sysfs.c +6
-0+if (attr == &dev_attr_vpd_pg83.attr) { +if (sdev->vpd_ident) +return S_IRUGO ; +else +return 0 ; + }Same here.
:-)
Cheers,
Hannes
Signature · 8 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html