On 02/12/2014 09:02 AM, Christoph Hellwig wrote:
> 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?
>
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 ...
>> + 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.
>> +#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!)
>> + if (attr == &dev_attr_vpd_pg83.attr) {
>> + if (sdev->vpd_ident)
>> + return S_IRUGO;
>> + else
>> + return 0;
>> + }
>
> Same here.
>
:-)
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxx +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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
