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 <hch@xxxxxx>
> +#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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
