scsi: core: Add BLIST_NO_ASK_VPD_SIZE for some VDASD

Commit Message

Lee Duncan Sept. 28, 2022, 6:13 p.m. UTC

From: Lee Duncan <[e-mail]>

Some storage, such as AIX VDASD (virtual storage) and IBM 2076
(front end) do not like the recent commit:

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

Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
Reported-by: Martin Wilck <[e-mail]>
Suggested-by: Hannes Reinecke <[e-mail]>
Signed-off-by: Lee Duncan <[e-mail]>
---
 drivers/scsi/scsi.c         | 14 +++++++++++---
 drivers/scsi/scsi_devinfo.c |  3 ++-
 drivers/scsi/scsi_scan.c    |  3 +++
 include/scsi/scsi_device.h  |  2 ++
 include/scsi/scsi_devinfo.h |  6 +++---
 5 files changed, 21 insertions(+), 7 deletions(-)

Comments

Martin Wilck Sept. 29, 2022, 10:42 a.m. UTC | #1

On Wed, 2022-09-28 at 11:13 -0700, Lee Duncan wrote:
> From: Lee Duncan <[e-mail]>
> 
> Some storage, such as AIX VDASD (virtual storage) and IBM 2076
> (front end) do not like the recent commit:
> 
> 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 black 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.
> 
> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full
> page")
> Reported-by: Martin Wilck <[e-mail]>
> Suggested-by: Hannes Reinecke <[e-mail]>
> Signed-off-by: Lee Duncan <[e-mail]>

Reviewed-by: Martin Wilck <[e-mail]>

Bart Van Assche Oct. 2, 2022, 9:16 p.m. UTC | #2

On 9/28/22 11:13, Lee Duncan wrote:
> From: Lee Duncan <[e-mail]>
> 
> Some storage, such as AIX VDASD (virtual storage) and IBM 2076
> (front end) do not like the recent commit:
> 
> 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 black 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.
> 
> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")

Hi Lee,

If we introduce a blacklist flag to skip querying the VPD page size then 
we will have to find all SCSI devices that do not handle querying the 
VPD page size correctly. Has it been considered instead of introducing a 
blacklist flag to not use the reported VPD page size if the device 
reports that the VPD page size is zero? I am not aware of any VPD pages 
for which zero is a valid size.

Thanks,

Bart.

Lee Duncan Oct. 2, 2022, 10:21 p.m. UTC | #3

On 10/2/22 14:16, Bart Van Assche wrote:
> On 9/28/22 11:13, Lee Duncan wrote:
>> From: Lee Duncan <[e-mail]>
>>
>> Some storage, such as AIX VDASD (virtual storage) and IBM 2076
>> (front end) do not like the recent commit:
>>
>> 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 black 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.
>>
>> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full 
>> page")
> 
> Hi Lee,
> 
> If we introduce a blacklist flag to skip querying the VPD page size then 
> we will have to find all SCSI devices that do not handle querying the 
> VPD page size correctly. Has it been considered instead of introducing a 
> blacklist flag to not use the reported VPD page size if the device 
> reports that the VPD page size is zero? I am not aware of any VPD pages 
> for which zero is a valid size.
> 
> Thanks,
> 
> Bart.

Hi Bart:

The problem with the broken firmware in my case is that it reports a 
size of zero, but it actually has the data! So the "size" returned for 
this one VPD page is just wrong. And I haven't researched it yet, but I 
assume that this hardware returned the failing page in question as a 
page it supported. In other words, you can't count on this hardware to 
report correctly. [I will check and update this email thread if this is 
wrong.]

This broken firmware was never an issue before commit c92a6b5d6335, 
since we used to just try to read 255 bytes, expecting that we would get 
back 255 or less. This worked almost all the time -- except for buggy 
hardware!

I suspect there isn't many pieces of hardware that return zero length 
incorrectly, and that if such hardware shoes up then they'll be able to 
use this flag to work around it.

So, for my hardware use case, if I add my commit, the VPD page shows up 
in sysfs, and before my commit no VPD page showed up. [Also, reverting 
commit c92a6b5d6335 made the VPD page show up, as a side note.]

Lastly, as for pages that might validly return size zero, Hannes seems 
to think some of the older hardware (under the older standards) returned 
zero as a valid page size for some VPD pages. For this reason I decided 
to not use a simpler approach of just trying to read the VPD page with a 
size of 255 if the "read length" returned zero (as in this case), i.e. 
since Hannes thinks some hardware might legitimately do this.

Bart Van Assche Oct. 3, 2022, 4:32 p.m. UTC | #4

On 9/28/22 11:13, Lee Duncan wrote:
> Some storage, such as AIX VDASD (virtual storage) and IBM 2076
> (front end) do not like the recent commit:
> 
> 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 black 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.

Reviewed-by: Bart Van Assche <[e-mail]>

Hannes Reinecke Oct. 4, 2022, 6 a.m. UTC | #5

On 10/2/22 23:16, Bart Van Assche wrote:
> On 9/28/22 11:13, Lee Duncan wrote:
>> From: Lee Duncan <[e-mail]>
>>
>> Some storage, such as AIX VDASD (virtual storage) and IBM 2076
>> (front end) do not like the recent commit:
>>
>> 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 black 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.
>>
>> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full 
>> page")
> 
> Hi Lee,
> 
> If we introduce a blacklist flag to skip querying the VPD page size then 
> we will have to find all SCSI devices that do not handle querying the 
> VPD page size correctly. Has it been considered instead of introducing a 
> blacklist flag to not use the reported VPD page size if the device 
> reports that the VPD page size is zero? I am not aware of any VPD pages 
> for which zero is a valid size.
> 
True.
But pre-SPC drives will ignore the VPD bit in the inquiry size. And 
these devices do not set an additional length in the inquiry data we 
will interpret the VPD page response as a zero-length VPD page.
Not good.

And really, we've seen only _one_ instance of this particular behaviour.
And even that could arguably been fixed with a firmware update on the 
target side. But to not introduce regressions we've opted for this flag.

Cheers,

Hannes

Martin K. Petersen Nov. 8, 2022, 2:50 a.m. UTC | #6

Hannes,

I have been contemplating this for a bit.

>> Has it been considered instead of introducing a blacklist flag to not
>> use the reported VPD page size if the device reports that the VPD
>> page size is zero? I am not aware of any VPD pages for which zero is
>> a valid size.

That would also be my preferred approach, I think. I haven't received
any bug reports about devices returning short VPD pages since this
change was introduced. So I think I'd prefer falling back to a
(hopefully small) default if a device returns a 0 page length.

Now, my question is which VPD pages are actually supported by this
device and how large are they?

> But pre-SPC drives will ignore the VPD bit in the inquiry size. And
> these devices do not set an additional length in the inquiry data

Can you elaborate a bit on your experience with older devices? I checked
SCSI-2 (1991) and don't see any indication this would be valid behavior
even back then.

Hannes Reinecke Nov. 8, 2022, 6:57 a.m. UTC | #7

On 11/8/22 03:50, Martin K. Petersen wrote:
> 
> Hannes,
> 
> I have been contemplating this for a bit.
> 
>>> Has it been considered instead of introducing a blacklist flag to not
>>> use the reported VPD page size if the device reports that the VPD
>>> page size is zero? I am not aware of any VPD pages for which zero is
>>> a valid size.
> 
> That would also be my preferred approach, I think. I haven't received
> any bug reports about devices returning short VPD pages since this
> change was introduced. So I think I'd prefer falling back to a
> (hopefully small) default if a device returns a 0 page length.
> 
> Now, my question is which VPD pages are actually supported by this
> device and how large are they?
> 
>> But pre-SPC drives will ignore the VPD bit in the inquiry size. And
>> these devices do not set an additional length in the inquiry data
> 
> Can you elaborate a bit on your experience with older devices? I checked
> SCSI-2 (1991) and don't see any indication this would be valid behavior
> even back then.
> 
This is primarily crappy USB devices, which implement only the absolute 
minimum to get SCSI rolling.
In particular, if devices do _not_ check the VPD bit in the inquiry 
command they will continue to return the standard inquiry data.
And if the additional length is zero we have exactly the scenario above.

However, we _could_ turn things around, and use the BLIST_NO_VPD flag 
for these cases; so I'd be fine with having a default length for the VPD 
page and delegate any fallout from the to use the BLIST_NO_VPD flags.

Cheers,

Hannes

Martin Wilck Nov. 21, 2022, 2:53 p.m. UTC | #8

On Mon, 2022-11-07 at 21:50 -0500, Martin K. Petersen wrote:
> 
> Hannes,
> 
> I have been contemplating this for a bit.
> 
> > > Has it been considered instead of introducing a blacklist flag to
> > > not
> > > use the reported VPD page size if the device reports that the VPD
> > > page size is zero? I am not aware of any VPD pages for which zero
> > > is
> > > a valid size.
> 
> That would also be my preferred approach, I think. I haven't received
> any bug reports about devices returning short VPD pages since this
> change was introduced. So I think I'd prefer falling back to a
> (hopefully small) default if a device returns a 0 page length.
> 
> Now, my question is which VPD pages are actually supported by this
> device and how large are they?

I've tried to obtain an answer to this question from IBM, but they
couldn't come up with a concrete number for the page length. Especially
for VDASD devices, it's difficult to say, because these virtual devices
just pass the INQUIRY down to the backing device.

How do we proceed? Could we simply fall back to a page length of 255
bytes (like before c92a6b5d6335) if the reported page length is zero?

Regards,
Martin W.

> 
> > But pre-SPC drives will ignore the VPD bit in the inquiry size. And
> > these devices do not set an additional length in the inquiry data
> 
> Can you elaborate a bit on your experience with older devices? I
> checked
> SCSI-2 (1991) and don't see any indication this would be valid
> behavior
> even back then.
>

Srikar Dronamraju Feb. 20, 2023, 11:13 a.m. UTC | #9

* Lee Duncan <[e-mail]> [2022-09-28 11:13:50]:

> From: Lee Duncan <[e-mail]>
> 
> Some storage, such as AIX VDASD (virtual storage) and IBM 2076
> (front end) do not like the recent commit:
> 
> 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 black 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.
> 
> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
> Reported-by: Martin Wilck <[e-mail]>
> Suggested-by: Hannes Reinecke <[e-mail]>
> Signed-off-by: Lee Duncan <[e-mail]>

Facing similar problem on latest upstream kernel and this fixes it in my
testing.

Incase this helps:

$ lsslot
# Slot                     Description       Linux Name    Device(s)
U9080.HEX.134C1E8-V9-C0    Virtual I/O Slot  30000000      vty
U9080.HEX.134C1E8-V9-C2    Virtual I/O Slot  30000002      l-lan
U9080.HEX.134C1E8-V9-C109  Virtual I/O Slot  3000006d      v-scsi

$ ls-vscsi
host0 U9080.HEX.134C1E8-V9-C109-T0

$ lsscsi
[0:0:1:0]    disk    AIX      VDASD            0001  /dev/sda
[0:0:2:0]    cd/dvd  AIX      VOPTA                  /dev/sr0


Tested-by: Srikar Dronamraju <[e-mail]>

Hannes Reinecke Feb. 27, 2023, 7:02 a.m. UTC | #10

On 9/28/22 20:13, Lee Duncan wrote:
> From: Lee Duncan <[e-mail]>
> 
> Some storage, such as AIX VDASD (virtual storage) and IBM 2076
> (front end) do not like the recent commit:
> 
> 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 black 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.
> 
> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
> Reported-by: Martin Wilck <[e-mail]>
> Suggested-by: Hannes Reinecke <[e-mail]>
> Signed-off-by: Lee Duncan <[e-mail]>
> ---
>   drivers/scsi/scsi.c         | 14 +++++++++++---
>   drivers/scsi/scsi_devinfo.c |  3 ++-
>   drivers/scsi/scsi_scan.c    |  3 +++
>   include/scsi/scsi_device.h  |  2 ++
>   include/scsi/scsi_devinfo.h |  6 +++---
>   5 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index c59eac7a32f2..f2db4b846190 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -321,11 +321,19 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>   	return get_unaligned_be16(&buffer[2]) + 4;
>   }
>   
> -static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
> +static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page, int buf_len)
>   {
>   	unsigned char vpd_header[SCSI_VPD_HEADER_SIZE] __aligned(4);
>   	int result;
>   
> +	/*
> +	 * if this hardware is blacklisted then don't bother asking
> +	 * the page size, since it will repy with zero -- just assume it
> +	 * is the buffer size
> +	 */
> +	if (sdev->no_ask_vpd_sz_first)
> +		return buf_len;
> +
>   	/*
>   	 * Fetch the VPD page header to find out how big the page
>   	 * is. This is done to prevent problems on legacy devices
> @@ -367,7 +375,7 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
>   	if (!scsi_device_supports_vpd(sdev))
>   		return -EINVAL;
>   
> -	vpd_len = scsi_get_vpd_size(sdev, page);
> +	vpd_len = scsi_get_vpd_size(sdev, page, buf_len);
>   	if (vpd_len <= 0)
>   		return -EINVAL;
>   
> @@ -402,7 +410,7 @@ static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
>   	struct scsi_vpd *vpd_buf;
>   	int vpd_len, result;
>   
> -	vpd_len = scsi_get_vpd_size(sdev, page);
> +	vpd_len = scsi_get_vpd_size(sdev, page, SCSI_VPD_PG_LEN);
>   	if (vpd_len <= 0)
>   		return NULL;
>   
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index c7080454aea9..d2b2e841e570 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_ASK_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_ASK_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 5d27f5196de6..b67743e32089 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1056,6 +1056,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_ASK_VPD_SIZE)
> +		sdev->no_ask_vpd_sz_first = 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 2493bd65351a..5d15784ccefc 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_VPD_PG_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;
> @@ -214,6 +215,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_ask_vpd_sz_first:1;	/* Do not ask for VPD size first */
>   
>   	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..ec12dbaff0e8 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_ASK_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 | \

To prod people a bit:

Reviewed-by: Hannes Reinecke <[e-mail]>

Cheers,

Hannes

Thorsten Leemhuis March 3, 2023, 9:02 a.m. UTC | #11

Hi, this is your Linux kernel regression tracker.

On 28.09.22 20:13, Lee Duncan wrote:
> From: Lee Duncan <[e-mail]>
> 
> Some storage, such as AIX VDASD (virtual storage) and IBM 2076
> (front end) do not like the recent commit:
> 
> 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 black 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.

As this is a fix for a regression (one that Srikar Dronamraju recently
ran into as well and bisected again :-/ ), please allow me to ask:

James, Martin, what is needed to get this or some other solution for the
regression finally mainlined?

FWIW, the thread afaics accumulated three Reviewed-by an one Tested-by
in the meantime.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
> Reported-by: Martin Wilck <[e-mail]>
> Suggested-by: Hannes Reinecke <[e-mail]>
> Signed-off-by: Lee Duncan <[e-mail]>
> ---
>  drivers/scsi/scsi.c         | 14 +++++++++++---
>  drivers/scsi/scsi_devinfo.c |  3 ++-
>  drivers/scsi/scsi_scan.c    |  3 +++
>  include/scsi/scsi_device.h  |  2 ++
>  include/scsi/scsi_devinfo.h |  6 +++---
>  5 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index c59eac7a32f2..f2db4b846190 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -321,11 +321,19 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>  	return get_unaligned_be16(&buffer[2]) + 4;
>  }
>  
> -static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
> +static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page, int buf_len)
>  {
>  	unsigned char vpd_header[SCSI_VPD_HEADER_SIZE] __aligned(4);
>  	int result;
>  
> +	/*
> +	 * if this hardware is blacklisted then don't bother asking
> +	 * the page size, since it will repy with zero -- just assume it
> +	 * is the buffer size
> +	 */
> +	if (sdev->no_ask_vpd_sz_first)
> +		return buf_len;
> +
>  	/*
>  	 * Fetch the VPD page header to find out how big the page
>  	 * is. This is done to prevent problems on legacy devices
> @@ -367,7 +375,7 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
>  	if (!scsi_device_supports_vpd(sdev))
>  		return -EINVAL;
>  
> -	vpd_len = scsi_get_vpd_size(sdev, page);
> +	vpd_len = scsi_get_vpd_size(sdev, page, buf_len);
>  	if (vpd_len <= 0)
>  		return -EINVAL;
>  
> @@ -402,7 +410,7 @@ static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
>  	struct scsi_vpd *vpd_buf;
>  	int vpd_len, result;
>  
> -	vpd_len = scsi_get_vpd_size(sdev, page);
> +	vpd_len = scsi_get_vpd_size(sdev, page, SCSI_VPD_PG_LEN);
>  	if (vpd_len <= 0)
>  		return NULL;
>  
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index c7080454aea9..d2b2e841e570 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_ASK_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_ASK_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 5d27f5196de6..b67743e32089 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1056,6 +1056,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_ASK_VPD_SIZE)
> +		sdev->no_ask_vpd_sz_first = 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 2493bd65351a..5d15784ccefc 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_VPD_PG_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;
> @@ -214,6 +215,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_ask_vpd_sz_first:1;	/* Do not ask for VPD size first */
>  
>  	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..ec12dbaff0e8 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_ASK_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 | \

Lee Duncan March 3, 2023, 6:54 p.m. UTC | #12

On Mar 3, 2023, at 1:02 AM, Linux regression tracking (Thorsten Leemhuis) <[e-mail]> wrote:
> 
> Hi, this is your Linux kernel regression tracker.
> 
> On 28.09.22 20:13, Lee Duncan wrote:
>> From: Lee Duncan <[e-mail]>
>> 
>> Some storage, such as AIX VDASD (virtual storage) and IBM 2076
>> (front end) do not like the recent commit:
>> 
>> 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 black 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.
> 
> As this is a fix for a regression (one that Srikar Dronamraju recently
> ran into as well and bisected again :-/ ), please allow me to ask:
> 
> James, Martin, what is needed to get this or some other solution for the
> regression finally mainlined?
> 
> FWIW, the thread afaics accumulated three Reviewed-by an one Tested-by
> in the meantime.
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
> #regzbot poke

Martin:

I know you had reservations about this approach, but the fact that another
case has shown up where this patch helps means this isn’t just a one-off
problem.

I know the alternative was to have the code that reads mode pages just
automatically handle all cases where the size was returned to zero, but
I really prefer specifically listing “offending” hardware, rather than
automatically covering for it.

Please let me know if you still have reservations. If not, I could rebase
and resubmit this, if you like.

Thanks.

> 
>> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
>> Reported-by: Martin Wilck <[e-mail]>
>> Suggested-by: Hannes Reinecke <[e-mail]>
>> Signed-off-by: Lee Duncan <[e-mail]>
>> ---
>> drivers/scsi/scsi.c         | 14 +++++++++++---
>> drivers/scsi/scsi_devinfo.c |  3 ++-
>> drivers/scsi/scsi_scan.c    |  3 +++
>> include/scsi/scsi_device.h  |  2 ++
>> include/scsi/scsi_devinfo.h |  6 +++---
>> 5 files changed, 21 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index c59eac7a32f2..f2db4b846190 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -321,11 +321,19 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>> return get_unaligned_be16(&buffer[2]) + 4;
>> }
>> 
>> -static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
>> +static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page, int buf_len)
>> {
>> unsigned char vpd_header[SCSI_VPD_HEADER_SIZE] __aligned(4);
>> int result;
>> 
>> + /*
>> + * if this hardware is blacklisted then don't bother asking
>> + * the page size, since it will repy with zero -- just assume it
>> + * is the buffer size
>> + */
>> + if (sdev->no_ask_vpd_sz_first)
>> + return buf_len;
>> +
>> /*
>> * Fetch the VPD page header to find out how big the page
>> * is. This is done to prevent problems on legacy devices
>> @@ -367,7 +375,7 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
>> if (!scsi_device_supports_vpd(sdev))
>> return -EINVAL;
>> 
>> - vpd_len = scsi_get_vpd_size(sdev, page);
>> + vpd_len = scsi_get_vpd_size(sdev, page, buf_len);
>> if (vpd_len <= 0)
>> return -EINVAL;
>> 
>> @@ -402,7 +410,7 @@ static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
>> struct scsi_vpd *vpd_buf;
>> int vpd_len, result;
>> 
>> - vpd_len = scsi_get_vpd_size(sdev, page);
>> + vpd_len = scsi_get_vpd_size(sdev, page, SCSI_VPD_PG_LEN);
>> if (vpd_len <= 0)
>> return NULL;
>> 
>> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
>> index c7080454aea9..d2b2e841e570 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_ASK_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_ASK_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 5d27f5196de6..b67743e32089 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -1056,6 +1056,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_ASK_VPD_SIZE)
>> + sdev->no_ask_vpd_sz_first = 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 2493bd65351a..5d15784ccefc 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_VPD_PG_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;
>> @@ -214,6 +215,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_ask_vpd_sz_first:1; /* Do not ask for VPD size first */
>> 
>> 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..ec12dbaff0e8 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_ASK_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 | \

Martin K. Petersen March 6, 2023, 10:14 p.m. UTC | #13

Lee,

> I know you had reservations about this approach, but the fact that
> another case has shown up where this patch helps means this isn’t just
> a one-off problem.
>
> I know the alternative was to have the code that reads mode pages just
> automatically handle all cases where the size was returned to zero,
> but I really prefer specifically listing “offending” hardware, rather
> than automatically covering for it.

I'm not particularly keen on either approach. But I'll take another look
today...

Martin K. Petersen March 7, 2023, 2:54 a.m. UTC | #14

Lee,

> I really prefer specifically listing “offending” hardware, rather than
> automatically covering for it.

Would the following patch work?

Martin

---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/[e-mail]
Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
Reported-by: Martin Wilck <[e-mail]>
Suggested-by: Hannes Reinecke <[e-mail]>
Signed-off-by: Lee Duncan <[e-mail]>
Signed-off-by: Martin K. Petersen <[e-mail]>

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

Srikar Dronamraju March 7, 2023, 10:32 a.m. UTC | #15

* Martin K. Petersen <[e-mail]> [2023-03-06 21:54:42]:

Hi Martin,
> 
> Lee,
> 
> > I really prefer specifically listing ???offending??? hardware, rather than
> > automatically covering for it.
> 
> Would the following patch work?
> 

Yes, this patch also works atleast for me.

> Martin
> 
> ---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/[e-mail]
> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
> Reported-by: Martin Wilck <[e-mail]>
> Suggested-by: Hannes Reinecke <[e-mail]>
> Signed-off-by: Lee Duncan <[e-mail]>
> Signed-off-by: Martin K. Petersen <[e-mail]>
> 
> 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 | \

Lee Duncan March 7, 2023, 4:33 p.m. UTC | #16

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/[e-mail]
> Fixes: c92a6b5d6335 ("scsi: core: Query VPD size before getting full page")
> Reported-by: Martin Wilck <[e-mail]>
> Suggested-by: Hannes Reinecke <[e-mail]>
> Signed-off-by: Lee Duncan <[e-mail]>
> Signed-off-by: Martin K. Petersen <[e-mail]>
> 
> 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 | \

Martin K. Petersen March 7, 2023, 11:40 p.m. UTC | #17

Lee,

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

In the scsi_get_vpd_buf() case we will allocate a 255 byte buffer since
that's what scsi_get_vpd_size() returns for a VDASD.

And in the scsi_get_vpd_page() case, where a buffer already exists, we
clamp the INQUIRY size to the minimum of scsi_get_vpd_size() and the
buffer length provided by the caller.

Lee Duncan March 8, 2023, 6:41 p.m. UTC | #18

On 3/7/23 15:40, Martin K. Petersen wrote:
> 
> Lee,
> 
>> 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.
> 
> In the scsi_get_vpd_buf() case we will allocate a 255 byte buffer since
> that's what scsi_get_vpd_size() returns for a VDASD.
> 
> And in the scsi_get_vpd_page() case, where a buffer already exists, we
> clamp the INQUIRY size to the minimum of scsi_get_vpd_size() and the
> buffer length provided by the caller.
> 

Please add my Reviewed-by tag then.

Martin K. Petersen March 10, 2023, 3:17 a.m. UTC | #19

Lee,

>> In the scsi_get_vpd_buf() case we will allocate a 255 byte buffer
>> since that's what scsi_get_vpd_size() returns for a VDASD.  And in
>> the scsi_get_vpd_page() case, where a buffer already exists, we clamp
>> the INQUIRY size to the minimum of scsi_get_vpd_size() and the buffer
>> length provided by the caller.
>> 
>
> Please add my Reviewed-by tag then.

Applied to 6.3/scsi-fixes, thanks!

Patch

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index c59eac7a32f2..f2db4b846190 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -321,11 +321,19 @@  static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 	return get_unaligned_be16(&buffer[2]) + 4;
 }
 
-static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
+static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page, int buf_len)
 {
 	unsigned char vpd_header[SCSI_VPD_HEADER_SIZE] __aligned(4);
 	int result;
 
+	/*
+	 * if this hardware is blacklisted then don't bother asking
+	 * the page size, since it will repy with zero -- just assume it
+	 * is the buffer size
+	 */
+	if (sdev->no_ask_vpd_sz_first)
+		return buf_len;
+
 	/*
 	 * Fetch the VPD page header to find out how big the page
 	 * is. This is done to prevent problems on legacy devices
@@ -367,7 +375,7 @@  int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 	if (!scsi_device_supports_vpd(sdev))
 		return -EINVAL;
 
-	vpd_len = scsi_get_vpd_size(sdev, page);
+	vpd_len = scsi_get_vpd_size(sdev, page, buf_len);
 	if (vpd_len <= 0)
 		return -EINVAL;
 
@@ -402,7 +410,7 @@  static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
 	struct scsi_vpd *vpd_buf;
 	int vpd_len, result;
 
-	vpd_len = scsi_get_vpd_size(sdev, page);
+	vpd_len = scsi_get_vpd_size(sdev, page, SCSI_VPD_PG_LEN);
 	if (vpd_len <= 0)
 		return NULL;
 
diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index c7080454aea9..d2b2e841e570 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_ASK_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_ASK_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 5d27f5196de6..b67743e32089 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1056,6 +1056,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_ASK_VPD_SIZE)
+		sdev->no_ask_vpd_sz_first = 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 2493bd65351a..5d15784ccefc 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_VPD_PG_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;
@@ -214,6 +215,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_ask_vpd_sz_first:1;	/* Do not ask for VPD size first */
 
 	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..ec12dbaff0e8 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_ASK_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 | \