Re: [PATCH] scsi: core: Add BLIST_NO_ASK_VPD_SIZE for some VDASD

[Lee Duncan <lduncan@suse.com>]

On 3/6/23 18:54, Martin K. Petersen wrote:
> 
> Lee,
> 
>> I really prefer specifically listing “offending” hardware, rather than
>> automatically covering for it.
> 
> Would the following patch work?
> 
> Martin

Hi Martin:

It seems the main difference here is that you don't modify the arguments 
to scsi_get_vpd_size(), assuming 255 for the buffer length.

My worry is that this won't always work. Looking at the code, the buffer 
sizes used for VPD pages include 8, 32, 64, and 252 bytes. I'm not sure 
how reading 255 bytes into an 8-byte buffer would work.

As far as testing this, my customer is using my proposed patch in 
production and is unlikely to be willing to test this for me. But, 
looking at the code, I suspect strongly that it would in fact work for them.

So, bottom line, if you strongly prefer your approach, I'm ok with it, 
but with some reservations.

> 
> ---8<---
> 
> Subject: [PATCH] scsi: core: Add BLIST_NO_VPD_SIZE for some VDASD
> 
> Some storage, such as AIX VDASD (virtual storage) and IBM 2076 (front
> end) do not like commit c92a6b5d6335 ("scsi: core: Query VPD size
> before getting full page").
> 
> That commit changed getting SCSI VPD pages so that we now read just
> enough of the page to get the actual page size, then read the whole
> page in a second read. The problem is that the above mentioned
> hardware returns zero for the page size, because of a firmware
> error. In such cases, until the firmware is fixed, this new blacklist
> flag says to revert to the original method of reading the VPD pages,
> i.e. try to read as a whole buffer's worth on the first try.
> 
> [mkp: reworked somewhat]
> 
> Link: https://lore.kernel.org/r/20220928181350.9948-1-leeman.duncan@gmail.com
> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
> Reported-by: Martin Wilck <mwilck@suse.com>
> Suggested-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Lee Duncan <lduncan@suse.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 9feb0323bc44..dff1d692e756 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -326,6 +326,9 @@ static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
>   	unsigned char vpd_header[SCSI_VPD_HEADER_SIZE] __aligned(4);
>   	int result;
>   
> +	if (sdev->no_vpd_size)
> +		return SCSI_DEFAULT_VPD_LEN;
> +
>   	/*
>   	 * Fetch the VPD page header to find out how big the page
>   	 * is. This is done to prevent problems on legacy devices
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index c7080454aea9..bc9d280417f6 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -134,7 +134,7 @@ static struct {
>   	{"3PARdata", "VV", NULL, BLIST_REPORTLUN2},
>   	{"ADAPTEC", "AACRAID", NULL, BLIST_FORCELUN},
>   	{"ADAPTEC", "Adaptec 5400S", NULL, BLIST_FORCELUN},
> -	{"AIX", "VDASD", NULL, BLIST_TRY_VPD_PAGES},
> +	{"AIX", "VDASD", NULL, BLIST_TRY_VPD_PAGES | BLIST_NO_VPD_SIZE},
>   	{"AFT PRO", "-IX CF", "0.0>", BLIST_FORCELUN},
>   	{"BELKIN", "USB 2 HS-CF", "1.95",  BLIST_FORCELUN | BLIST_INQUIRY_36},
>   	{"BROWNIE", "1200U3P", NULL, BLIST_NOREPORTLUN},
> @@ -188,6 +188,7 @@ static struct {
>   	{"HPE", "OPEN-", "*", BLIST_REPORTLUN2 | BLIST_TRY_VPD_PAGES},
>   	{"IBM", "AuSaV1S2", NULL, BLIST_FORCELUN},
>   	{"IBM", "ProFibre 4000R", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
> +	{"IBM", "2076", NULL, BLIST_NO_VPD_SIZE},
>   	{"IBM", "2105", NULL, BLIST_RETRY_HWERROR},
>   	{"iomega", "jaz 1GB", "J.86", BLIST_NOTQ | BLIST_NOLUN},
>   	{"IOMEGA", "ZIP", NULL, BLIST_NOTQ | BLIST_NOLUN},
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index f9b18fdc7b3c..6042a5587bc3 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1055,6 +1055,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>   	else if (*bflags & BLIST_SKIP_VPD_PAGES)
>   		sdev->skip_vpd_pages = 1;
>   
> +	if (*bflags & BLIST_NO_VPD_SIZE)
> +		sdev->no_vpd_size = 1;
> +
>   	transport_configure_device(&sdev->sdev_gendev);
>   
>   	if (sdev->host->hostt->slave_configure) {
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 3642b8e3928b..15169d75c251 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -145,6 +145,7 @@ struct scsi_device {
>   	const char * model;		/* ... after scan; point to static string */
>   	const char * rev;		/* ... "nullnullnullnull" before scan */
>   
> +#define SCSI_DEFAULT_VPD_LEN	255	/* default SCSI VPD page size (max) */
>   	struct scsi_vpd __rcu *vpd_pg0;
>   	struct scsi_vpd __rcu *vpd_pg83;
>   	struct scsi_vpd __rcu *vpd_pg80;
> @@ -215,6 +216,7 @@ struct scsi_device {
>   					 * creation time */
>   	unsigned ignore_media_change:1; /* Ignore MEDIA CHANGE on resume */
>   	unsigned silence_suspend:1;	/* Do not print runtime PM related messages */
> +	unsigned no_vpd_size:1;		/* No VPD size reported in header */
>   
>   	unsigned int queue_stopped;	/* request queue is quiesced */
>   	bool offline_already;		/* Device offline message logged */
> diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
> index 5d14adae21c7..6b548dc2c496 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -32,7 +32,8 @@
>   #define BLIST_IGN_MEDIA_CHANGE	((__force blist_flags_t)(1ULL << 11))
>   /* do not do automatic start on add */
>   #define BLIST_NOSTARTONADD	((__force blist_flags_t)(1ULL << 12))
> -#define __BLIST_UNUSED_13	((__force blist_flags_t)(1ULL << 13))
> +/* do not ask for VPD page size first on some broken targets */
> +#define BLIST_NO_VPD_SIZE	((__force blist_flags_t)(1ULL << 13))
>   #define __BLIST_UNUSED_14	((__force blist_flags_t)(1ULL << 14))
>   #define __BLIST_UNUSED_15	((__force blist_flags_t)(1ULL << 15))
>   #define __BLIST_UNUSED_16	((__force blist_flags_t)(1ULL << 16))
> @@ -74,8 +75,7 @@
>   #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
>   			       (__force blist_flags_t) \
>   			       ((__force __u64)__BLIST_LAST_USED - 1ULL)))
> -#define __BLIST_UNUSED_MASK (__BLIST_UNUSED_13 | \
> -			     __BLIST_UNUSED_14 | \
> +#define __BLIST_UNUSED_MASK (__BLIST_UNUSED_14 | \
>   			     __BLIST_UNUSED_15 | \
>   			     __BLIST_UNUSED_16 | \
>   			     __BLIST_UNUSED_24 | \