Re: [PATCH 2/2] Add EVPD page 0x83 entries to sysfs

[Hannes Reinecke <hare@suse.de>]

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@lst.de>
> 
>> +#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@suse.de			      +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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html