On Tue, Feb 11, 2014 at 03:34:54PM +0100, Hannes Reinecke wrote: > 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. > 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]> > +#define SCSI_VPD_ASSOC_lun 0x0 > +#define SCSI_VPD_ASSOC_port 0x1 > +#define SCSI_VPD_ASSOC_target 0x2 > + > +#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? > + 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. > +#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. > + if (attr == &dev_attr_vpd_pg83.attr) { > + if (sdev->vpd_ident) > + return S_IRUGO; > + else > + return 0; > + } Same here. -- 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
