Add EVPD page 0x83 entries to sysfs
RFC PATCH 1/1 Add EVPD page 0x83 entries to sysfs
EVPD 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.
Cc Jeremy Linton <[email protected]>
Cc Doug Gilbert <[email protected]>
Cc Kai Makisara <[email protected]>
Signed-off-by Hannes Reinecke <[email protected]>
--- drivers/scsi/scsi_scan.c | 3 + drivers/scsi/scsi_sysfs.c | 166 ++++++++++++++++++++++++++++++++++++++++++++- include/scsi/scsi_device.h | 3 + 3 files changed, 171 insertions(+), 1 deletion(-)
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 +165 -1
179 lines not on this page · raw message
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;unsigned char current_tag;/* current tag */ struct scsi_target *sdev_target;/* used only for single_lun */
@@ -309,6 +311,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
Quoted 48 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.Cc Jeremy Linton <[email protected]>Cc Doug Gilbert <[email protected]>Cc Kai Makisara <[email protected]>Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/scsi_scan.c | 3 + drivers/scsi/scsi_sysfs.c | 166 ++++++++++++++++++++++++++++++++++++++++++++- include/scsi/scsi_device.h | 3 + 3 files changed, 171 insertions(+), 1 deletion(-)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 +11
-0@@ -725,8 +725,170 @@ show_queue_type_field(struct device *dev, struct device_attribute *attr, static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field, NULL); +void scsi_attach_vpd_ident (struct scsi_device *sdev) +{ +int ret; +int vpd_len =255 ; +unsigned char *buffer; +retry: + buffer =kmalloc (vpd_len,GFP_KERNEL ); +if (buffer) { + ret =scsi_get_vpd_page (sdev,0x83 , buffer, vpd_len); +if (ret ==0 ) { + vpd_len = (buffer[2 ] <<8 ) + buffer[3 ];
drivers/scsi/scsi_sysfs.c show_queue_type_field(struct device *dev, struct device_attribute *attr, · line 748
Quoted 10 lines
drivers/scsi/scsi_sysfs.c +10
-0+if (vpd_len >255 ) { +kfree (buffer); + goto retry; + } + sdev->vpd_ident_len = vpd_len; + sdev->vpd_ident = buffer; + } + } +} +
Bummer. A missing kfree() if scsi_get_vpd_page() returns an error. But this is just an RFC for now, so I'll be fixing it up once it looks as if it would be moving forward...
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
Quoted 7 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.Cc Jeremy Linton <[email protected]>Cc Doug Gilbert <[email protected]>
See below.
Quoted 67 lines
Cc Kai Makisara <[email protected]>Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/scsi_scan.c | 3 + drivers/scsi/scsi_sysfs.c | 166 ++++++++++++++++++++++++++++++++++++++++++++- include/scsi/scsi_device.h | 3 + 3 files changed, 171 insertions(+), 1 deletion(-)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 +37
-0@@ -725,8 +725,170 @@ show_queue_type_field(struct device *dev, struct device_attribute *attr, static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field, NULL); +void scsi_attach_vpd_ident (struct scsi_device *sdev) +{ +int ret; +int vpd_len =255 ; +unsigned char *buffer; +retry: + buffer =kmalloc (vpd_len,GFP_KERNEL ); +if (buffer) { + ret =scsi_get_vpd_page (sdev,0x83 , buffer, vpd_len); +if (ret ==0 ) { + vpd_len = (buffer[2 ] <<8 ) + buffer[3 ]; +if (vpd_len >255 ) { +kfree (buffer); + goto retry; + } + sdev->vpd_ident_len = vpd_len; + sdev->vpd_ident = buffer; + } + } +} + +#define SCSI_VPD_ASSOC_lun 0x0 +#define SCSI_VPD_ASSOC_port 0x10 +#define SCSI_VPD_ASSOC_target 0x20 + +#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 + +int scsi_parse_vpd_ident (struct scsi_device *sdev, +int assoc,int desig,char *buf)
drivers/scsi/scsi_sysfs.c show_queue_type_field(struct device *dev, struct device_attribute *attr, · line 774
Quoted 10 lines
drivers/scsi/scsi_sysfs.c +10
-0+{ +unsigned char *d; +int len =0 ; + +if (!sdev->vpd_ident) +return -EINVAL ; + + d = sdev->vpd_ident +4 ; +while (d < sdev->vpd_ident + sdev->vpd_ident_len) { + if ((d[1] & 0x30) == assoc &&
Wouldn't it be clearer if:
if (((d[1] & 0x30) >> 4) == assoc &&
and the lun, port and target defines were changed to agree with T10?
Quoted 46 lines
drivers/scsi/scsi_sysfs.c +46
-0+ (d[1] & 0xf) == desig) { +switch (d[1 ] &0xf ) { +case SCSI_VPD_DESIG_eui: +case SCSI_VPD_DESIG_naa: +case SCSI_VPD_DESIG_md5: +switch (d[3 ]) { +case 8 : + len +=sprintf (buf + len, +"%8phN\n" , d +4 ); +break ; +case 12 : + len +=sprintf (buf + len, +"%12phN\n" , d +4 ); +break ; +case 16 : + len +=sprintf (buf + len, +"%16phN\n" , d +4 ); +break ; + } +break ; +case SCSI_VPD_DESIG_relport: +case SCSI_VPD_DESIG_tpgrp: +case SCSI_VPD_DESIG_lugrp: + len +=sprintf (buf + len,"%d\n" , + (int )(d[6 ] <<8 ) + d[7 ]); +break ; +case SCSI_VPD_DESIG_vendor: +case SCSI_VPD_DESIG_t10: +case SCSI_VPD_DESIG_scsi_name: + len +=snprintf (buf + len, d[3 ] +2 ,"%s\n" , + d +4 ); +break ; + } + } + d += d[3 ] +4 ; + } + +return len ? len : -EINVAL ; +} + +#define sdev_evpd_ident (assoc,desig) \ +static ssize_t \ +sdev_show_evpd_ident_##assoc##_##desig (struct device *dev, \ + struct device_attribute *attr, \ + char * buf) \ +{ \
drivers/scsi/scsi_sysfs.c show_queue_type_field(struct device *dev, struct device_attribute *attr, · line 830
Quoted 10 lines
drivers/scsi/scsi_sysfs.c +10
-0+struct scsi_device *sdev =to_scsi_device (dev); \ + return scsi_parse_vpd_ident(sdev, SCSI_VPD_ASSOC_##assoc, \ + SCSI_VPD_DESIG_##desig, buf); \ +} + +#define sdev_evpd_attr (assoc, desig) \ + sdev_evpd_ident(assoc, desig) \ +static DEVICE_ATTR(ident_##assoc##_##desig, S_IRUGO, \ + sdev_show_evpd_ident_##assoc##_##desig, NULL); +
You macros will have trouble picking up UAS and SOP port info stuff hiding behind the designator code 9h (Protocol specific port identifier). That may not matter.
Doug Gilbert
Quoted 96 lines
drivers/scsi/scsi_sysfs.c +62
-1+sdev_evpd_attr (lun, vendor); +sdev_evpd_attr (lun, t10); +sdev_evpd_attr (lun, eui); +sdev_evpd_attr (lun, naa); +sdev_evpd_attr (lun, lugrp); +sdev_evpd_attr (lun, md5); +sdev_evpd_attr (lun, scsi_name); +sdev_evpd_attr (port, relport); +sdev_evpd_attr (port, tpgrp); +sdev_evpd_attr (port, eui); +sdev_evpd_attr (port, naa); +sdev_evpd_attr (port, scsi_name); +sdev_evpd_attr (target, eui); +sdev_evpd_attr (target, naa); +sdev_evpd_attr (target, scsi_name); + +#define sdev_evpd_test_and_add_attr (sdev, assoc, desig, valid) \ + if (test_bit(SCSI_VPD_DESIG_##desig, (valid))) \ + device_create_file(&(sdev)->sdev_gendev, \ + &dev_attr_ident_##assoc##_##desig) + +static void scsi_sysfs_filter_vpd_ident (struct scsi_device *sdev) +{ +unsigned char *d; +unsigned long desig =0 , assoc =0 ; + +if (!sdev->vpd_ident) +return ; + + d = sdev->vpd_ident +4 ; +while (d < sdev->vpd_ident + sdev->vpd_ident_len) { +set_bit ((d[1 ] >>4 ) &3 , &assoc); +set_bit (d[1 ] &0xf , &desig); + d += d[3 ] +4 ; + } +if (test_bit (SCSI_VPD_ASSOC_lun, &assoc)) { +sdev_evpd_test_and_add_attr (sdev, lun, vendor, &desig); +sdev_evpd_test_and_add_attr (sdev, lun, t10, &desig); +sdev_evpd_test_and_add_attr (sdev, lun, eui, &desig); +sdev_evpd_test_and_add_attr (sdev, lun, naa, &desig); +sdev_evpd_test_and_add_attr (sdev, lun, lugrp, &desig); +sdev_evpd_test_and_add_attr (sdev, lun, md5, &desig); +sdev_evpd_test_and_add_attr (sdev, lun, scsi_name, &desig); + } +if (test_bit (SCSI_VPD_ASSOC_port, &assoc)) { +sdev_evpd_test_and_add_attr (sdev, port, eui, &desig); +sdev_evpd_test_and_add_attr (sdev, port, naa, &desig); +sdev_evpd_test_and_add_attr (sdev, port, relport, &desig); +sdev_evpd_test_and_add_attr (sdev, port, tpgrp, &desig); +sdev_evpd_test_and_add_attr (sdev, port, scsi_name, &desig); + } +if (test_bit (SCSI_VPD_ASSOC_target, &assoc)) { +sdev_evpd_test_and_add_attr (sdev, target, eui, &desig); +sdev_evpd_test_and_add_attr (sdev, target, naa, &desig); +sdev_evpd_test_and_add_attr (sdev, target, scsi_name, &desig); + } +} +static ssize_t -show_iostat_counterbits (struct device *dev,struct device_attribute *attr,char *buf) +show_iostat_counterbits (struct device *dev,struct device_attribute *attr, +char *buf) {return snprintf (buf,20 ,"%d\n" , (int )sizeof (atomic_t) *8 ); }@@ -1003,6 +1165,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)transport_add_device (&sdev->sdev_gendev); sdev->is_visible =1 ; +scsi_sysfs_filter_vpd_ident (sdev); +/* 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,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;unsigned char current_tag;/* current tag */ struct scsi_target *sdev_target;/* used only for single_lun */ @@ -309,6 +311,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 *);
Quoted 94 lines · Douglas Gilbert
Quoted 7 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.Cc Jeremy Linton <[email protected]>Cc Doug Gilbert <[email protected]>See below.
Quoted 71 lines
Cc Kai Makisara <[email protected]>Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/scsi_scan.c | 3 + drivers/scsi/scsi_sysfs.c | 166 ++++++++++++++++++++++++++++++++++++++++++++- include/scsi/scsi_device.h | 3 + 3 files changed, 171 insertions(+), 1 deletion(-)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 +37
-0@@ -725,8 +725,170 @@ show_queue_type_field(struct device *dev,struct device_attribute *attr, static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field,NULL ); +void scsi_attach_vpd_ident (struct scsi_device *sdev) +{ +int ret; +int vpd_len =255 ; +unsigned char *buffer; +retry: + buffer =kmalloc (vpd_len,GFP_KERNEL ); +if (buffer) { + ret =scsi_get_vpd_page (sdev,0x83 , buffer, vpd_len); +if (ret ==0 ) { + vpd_len = (buffer[2 ] <<8 ) + buffer[3 ]; +if (vpd_len >255 ) { +kfree (buffer); + goto retry; + } + sdev->vpd_ident_len = vpd_len; + sdev->vpd_ident = buffer; + } + } +} + +#define SCSI_VPD_ASSOC_lun 0x0 +#define SCSI_VPD_ASSOC_port 0x10 +#define SCSI_VPD_ASSOC_target 0x20 + +#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 + +int scsi_parse_vpd_ident (struct scsi_device *sdev, +int assoc,int desig,char *buf)drivers/scsi/scsi_sysfs.c show_queue_type_field(struct device *dev, · line 776
Quoted 10 lines
drivers/scsi/scsi_sysfs.c +10
-0+{ +unsigned char *d; +int len =0 ; + +if (!sdev->vpd_ident) +return -EINVAL ; + + d = sdev->vpd_ident +4 ; +while (d < sdev->vpd_ident + sdev->vpd_ident_len) { + if ((d[1] & 0x30) == assoc &&Wouldn't it be clearer if:
Quoted 5 lines
if (((d[1] & 0x30) >> 4) == assoc &&and the lun, port and target defines were changed to agree with T10?
Yes, I could. Just thought it a bit pointless given that we're (currently) the only ones using it.
Will be fixing it up with the next version.
Quoted 52 lines
Quoted 52 lines
drivers/scsi/scsi_sysfs.c +52
-0+ (d[1] & 0xf) == desig) { +switch (d[1 ] &0xf ) { +case SCSI_VPD_DESIG_eui: +case SCSI_VPD_DESIG_naa: +case SCSI_VPD_DESIG_md5: +switch (d[3 ]) { +case 8 : + len +=sprintf (buf + len, +"%8phN\n" , d +4 ); +break ; +case 12 : + len +=sprintf (buf + len, +"%12phN\n" , d +4 ); +break ; +case 16 : + len +=sprintf (buf + len, +"%16phN\n" , d +4 ); +break ; + } +break ; +case SCSI_VPD_DESIG_relport: +case SCSI_VPD_DESIG_tpgrp: +case SCSI_VPD_DESIG_lugrp: + len +=sprintf (buf + len,"%d\n" , + (int )(d[6 ] <<8 ) + d[7 ]); +break ; +case SCSI_VPD_DESIG_vendor: +case SCSI_VPD_DESIG_t10: +case SCSI_VPD_DESIG_scsi_name: + len +=snprintf (buf + len, d[3 ] +2 ,"%s\n" , + d +4 ); +break ; + } + } + d += d[3 ] +4 ; + } + +return len ? len : -EINVAL ; +} + +#define sdev_evpd_ident (assoc,desig) \ +static ssize_t \ +sdev_show_evpd_ident_##assoc##_##desig (struct device *dev, \ + struct device_attribute *attr, \ + char * buf) \ +{ \ + struct scsi_device *sdev = to_scsi_device(dev); \ + return scsi_parse_vpd_ident(sdev, SCSI_VPD_ASSOC_##assoc, \ + SCSI_VPD_DESIG_##desig, buf); \ +} + +#define sdev_evpd_attr (assoc, desig) \
Quoted 9 lines
drivers/scsi/scsi_sysfs.c show_queue_type_field(struct device *dev, struct device_attribute *attr, · line 830
Quoted 4 lines
drivers/scsi/scsi_sysfs.c +4
-0+ sdev_evpd_ident(assoc, desig) \ +static DEVICE_ATTR(ident_##assoc##_##desig, S_IRUGO, \ + sdev_show_evpd_ident_##assoc##_##desig, NULL); +You macros will have trouble picking up UAS and SOP port info stuff hiding behind the designator code 9h (Protocol specific port identifier). That may not matter.
As I don't have UAS and no SOP devices are on the market it's not _that_ urgent.
But yeah, I'll be fixing it up with the next version.
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
Quoted 4 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.
Can someone please tell me what actual benefits we get from retrieving an parsing this in the kernel instead of in userspace. Just adding more heuristics made your code grow by 150% since Thursday. I'm really worried about incomplete heuristics in the kernel because the process for updating them is much more involved than simply doing an update to a user package.
Please define what you're trying to achieve first, because if it's just the best persistent name for the device it might be better to reverse all of this and have a user writeable field in SCSI that udev populates after it does all the heuristic stuff. Is it a range (like name + transport ID + other stuff) in which case we might get additional plug ins to do that.
James