On 02/28/2014 06:01 PM, Christoph Hellwig wrote:
> On Thu, Feb 13, 2014 at 11:07:11AM +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.
>
> This just shows binary data from the protocol, so shouldn't it be a
> binary sysfs attribute?
>
> In general I have to I prefer the actual text attributes, but this is
> still better than having to do all the SG_IO inquirys.
>
Binary sysfs attributes have a rather special handling, and IIRC
they should be used for direct hardware interaction only.
Also the hexdump is easier to parse for the unsuspecting user.
>> - * Returns 0 on success or a negative error number.
>> + * Returns size of the vpg page on success or a negative error number.
>> */
>> static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>> u8 page, unsigned len)
>> @@ -962,6 +962,9 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev,
>> unsigned char *buffer,
>> int result;
>> unsigned char cmd[16];
>>
>> + if (len < 4)
>> + return -EINVAL;
>
> Seems the change in calling conventions for these existing functions
> should be split into a separate patch?
>
Ok.
>> /**
>> + * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
>> + * @sdev: The device to ask
>> + *
>> + * Attach the 'Device Identification' VPD page (0x83) to a SCSI device
>> + * structure. This information can be used to identify the device
>> + * uniquely.
>> + */
>> +void scsi_attach_vpd(struct scsi_device *sdev)
>> +{
>> + int result, i;
>> + int vpd_len = 255;
>> + int pg83_supported = 0;
>> + unsigned char *vpd_buf;
>> +
>> + if (sdev->skip_vpd_pages)
>> + return;
>> +retry_pg0:
>> + vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
>> + if (!vpd_buf)
>> + return;
>> +
>> + /* Ask for all the pages supported by this device */
>> + result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
>> + if (result < 0) {
>> + kfree(vpd_buf);
>> + return;
>> + }
>> + if (result > vpd_len) {
>> + vpd_len = result;
>> + kfree(vpd_buf);
>> + goto retry_pg0;
>> + }
>> +
>> + for (i = 4; i < result; i++) {
>> + if (vpd_buf[i] == 0x83) {
>> + pg83_supported = 1;
>> + }
>> + }
>> + kfree(vpd_buf);
>
> Given how many checks all over the place we have which EVPD pages are
> suppored shouldn't we have query for evpd 0, and then set flags in the
> scsi device which are present?
>
> Either way I think the call to query evpd 0 should be a separate
> function, so even if we don't store the information it's abstracted out.
>
Hmm. That would work if we were just asking for a single page; but
when we're checking several pages (like 0x83 and 0x80) we'd need
either to pass in a page array or querying page 0 several times.
Neither of which is very appealing.
However, specifying additional flags for the individual pages might
work. I'll see what I can come up with.
>
> Also the ses code has another query for 0x83, which now could use the
> one attached to the scsi_device.
>
Ah. Missed that one. Will be converting it.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
