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

[Pete Wyckoff]

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.

> 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.

[..]
> -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?
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.

[..]
> +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