[Stgt-devel] [Patch 1/1] Add support for extra VPD pages within INQUIRY op code

[Mark Harvey]

Pete Wyckoff wrote:
> markh794 at gmail.com wrote on Fri, 24 Aug 2007 18:15 +1000:
>   
>> Unfortunately I had to change the lu_init() interface so I could more 
>> easily include the TID in the SCSI VPD page 80h and 83h
>>
>> Hence the number of files touched with this patch.
>>
>> As indicated in the patch, I am unsure from the osd2r01.pdf 
>> documentation of what data should be included within VPD pages B0h and 
>> B1h for the OSD module.
>>     
>
> This looks great.  Thanks for looking at the OSD oddball parameters.
> I'll patch them up once this goes in.
>
>   

I need them for the SSC devices as well :)

>> From: Mark Harvey <markh794 at gmail.com>
>> Date: Fri, 24 Aug 2007 18:04:24 +1000
>> Subject: Add support for VPD pages 0x80 - 0xff
>>
>> Ability to add VPD pages between 80h and FFh per SCSI device type.
>>
>> An array of 128 vpd structures added to ly_phy_attr struct.
>>
>> Use alloc_vpd(data size) to pre-alloc data for custom VPD page.
>> - This data is appended to the 4 byte VPD header at runtime
>>   i.e. When an INQUIRY for the VPD page is received.
>>
>> - A custom vpd_update(struct scsi_lu *lu, void *) is used to
>>   set/change data pre-allocated by alloc_vpd()
>>
>> - All modules except use default page 80h & 83h defined in spc.c
>>   The osd module has two extra for VPD pages B0h and B1h
>>   - Note: garbage values are set and should be reviewed/updated
>>           by somebody who knows what should be set here.
>>
>> Signed-off-by: Mark Harvey <markh794 at gmail.com>
>> ---
>>     
> [..]
>   
>> diff --git a/scripts/tgt-core-test b/scripts/tgt-core-test
>>     
> [..]
>   
>> +tgtadm --lld iscsi --mode logicalunit --op update --tid $TID --lun $LUN \
>> +	--params vendor_id=OSD,product_id=OSD00,product_rev=0010,removable=1,sense_format=1
>>     
>
> OSD sense format is always descriptor.  Cannot be old style, says the
> spec.  But there's lots more ways root can break things.
>
> [..]
>   
Yep - Give the user a loaded gun....
Its up the them if they ignore the standards and 'pull the trigger'. 
Maybe I should log an error via the OSD device if set contrary to the spec.
- Hopefully make it a little easier for somebody setting up an OSD 
device and discover what they did wrong.

>> -int spc_lu_init(struct scsi_lu *lu)
>> +int spc_lu_init(struct scsi_lu *lu, int tid)
>>  {
>> +	struct vpd **lu_vpd = lu->attrs.vpd;
>> +
>> +	lu->attrs.device_type = lu->dev_type_template.type;
>> +	lu->attrs.qualifier = 0x0;
>> +
>>  	snprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
>>  		 "%-16s", VENDOR_ID);
>>  	snprintf(lu->attrs.product_rev, sizeof(lu->attrs.product_rev),
>>  		 "%s", "0001");
>> +	snprintf(lu->attrs.scsi_id, sizeof(lu->attrs.scsi_id),
>> +		 "deadbeaf%d:%" PRIu64, tid, lu->lun);
>> +	snprintf(lu->attrs.scsi_sn, sizeof(lu->attrs.scsi_sn),
>> +		 "beaf%d%" PRIu64, tid, lu->lun);
>>     
>
> The only reason you pass in the tid is to create these fake values?
>   
Yes

> Would it be better to add a struct target * in struct scsi_lu
> instead?  Seems a bit hacky to pass around tid just for this.
>   

Adding TID into the scsi_lu struct did cross my mind when I was doing this.
- More space consumed in the structure for a 'one off' use vs passing 
the TID.. I went with the passing of TID.

I don't mind either way.


> [..]
>   
>> +struct vpd {
>> +	uint16_t size;
>> +	void (*vpd_update)(struct scsi_lu *lu, void *data);
>> +	uint8_t data[0];
>> +};
>> +
>>  struct lu_phy_attr {
>>  	char scsi_id[SCSI_ID_LEN + 1];
>>  	char scsi_sn[SCSI_SN_LEN + 1];
>> @@ -52,10 +61,9 @@ struct lu_phy_attr {
>>  	char online;		/* Logical Unit online */
>>  	char reset;		/* Power-on or reset has occured */
>>  	char sense_format;	/* Descrptor format sense data supported */
>> -};
>>  
>> -struct scsi_lu;
>> -struct scsi_cmd;
>> +	struct vpd *vpd[0x80];	/* VPD pages 0x80 -> 0xff masked with 0x80*/
>> +};
>>     
>
> 1 kB of pointer space.  Guess that's not a big deal.  A linked list
> would have been the other option you could consider.  I was going to
> say the need to add and subtract 0x80 here and there was cumbersome,
> but fixing that would cost another 1 kB.
>
> 		-- Pete
>
>   

Original patches for MODE SENSE/SELECT used a linked list, which was 
then simplified to an array. Hence I went with the array this time..

As you say allocating 256 entries for less then 10 active entries 
(typically 2) seemed a little extreme. The mask of 0x7f seemed to be a 
simple enough trade off.

I have a similar issue when I implement LOG SENSE/SELECT set of commands 
as well.

Cheers
Mark