[PATCH v3 0/3] Bring the BusLogic host bus adapter driver up to Y2021

[PATCH v3 0/3] Bring the BusLogic host bus adapter driver up to Y2021

[Maciej W. Rozycki]

Hi,

 Or was it Y2022?

 Anyway, here's v3 of the series, with original patches 1/5 and 2/5 
removed as they have since gone in (thanks!).  No code or description 
change with the remaining patches, just a mechanical regeneration, except 
for Nick's Tested-by annotation for 3/3 (thanks!).  Parts of the original 
cover letter follow that are still relevant, for reference.

 So we are here owing to Christoph's recent ISA bounce buffering sweep: 
<https://lore.kernel.org/linux-scsi/20210331073001.46776-1-hch@lst.de/T/#m981284e74e93216626a0728ce1601ca18fca92e8> 
which has prompted me to verify the current version of Linux with my old 
server, which has been long equipped with venerable Linux 2.6.18 and which 
I now have available for general experimenting, and the BusLogic BT-958 
PCI SCSI host bus adapter the server has used for 20-something years now. 
This revealed an issue with the BusLogic driver.

 It has become obvious the BusLogic driver would have been non-functional, 
should I have upgraded the kernel, at least with this configuration for 
some 8 years now, and the underlying cause has been a long-known issue 
with the MultiMaster firmware I have dealt with already, back in 2003.  
To put it short the firmware cannot cope with commands that request an 
allocation length exceeding the length of actual data returned.

 I have originally observed it with a LOG SENSE command in the course of 
investigating why smartmontools bring the system to a death, and worked it 
around: <https://sourceforge.net/p/smartmontools/mailman/message/4993087/> 
by issuing the command twice, first just to obtain the allocation length 
required.  As it turns out we need a similar workaround in the kernel now.

 But in the course of investigating this issue I have discovered there is 
a second bottom to it and hence I have prepared follow-up changes to 
address problems with our handling of Vital Product Data INQUIRY pages.

 See individual change descriptions for further details.

 Any questions, comments, concerns still?  Otherwise please apply.

  Maciej

[PATCH v3 1/3] scsi: Provide for avoiding trailing allocation length with VPD inquiries

[Maciej W. Rozycki]

Allow SCSI hosts to request avoiding trailing allocation length with VPD 
inquiries, and use the mechanism to work around an issue with at least 
some BusLogic MultiMaster host bus adapters and observed with the BT-958 
model specifically where issuing commands that return less data than 
provided for causes fatal failures:

scsi host0: BusLogic BT-958
scsi 0:0:0:0: Direct-Access     IBM      DDYS-T18350M     SA5A PQ: 0 ANSI: 3
scsi 0:0:1:0: Direct-Access     SEAGATE  ST336607LW       0006 PQ: 0 ANSI: 3
scsi 0:0:5:0: Direct-Access     IOMEGA   ZIP 100          E.08 PQ: 0 ANSI: 2
sd 0:0:1:0: [sdb] 71687372 512-byte logical blocks: (36.7 GB/34.2 GiB)
sd 0:0:0:0: [sda] 35843670 512-byte logical blocks: (18.4 GB/17.1 GiB)
sd 0:0:1:0: [sdb] Write Protect is off
sd 0:0:5:0: [sdc] Attached SCSI removable disk
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:1:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
scsi0: *** BusLogic BT-958 Initialized Successfully ***
scsi0: *** BusLogic BT-958 Initialized Successfully ***
scsi0: *** BusLogic BT-958 Initialized Successfully ***
scsi0: *** BusLogic BT-958 Initialized Successfully ***
sd 0:0:0:0: Device offlined - not ready after error recovery
sd 0:0:1:0: Device offlined - not ready after error recovery
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[64] => -5
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[64] => -5
sd 0:0:0:0: [sda] Attached SCSI disk
sd 0:0:1:0: [sdb] Attached SCSI disk
VFS: Cannot open root device "802" or unknown-block(8,2): error -6

(here and elsewhere reported with some instrumentation added so as to 
show the causing requests and with irrelevant messages filtered out).

As already observed back in 2003 and worked around in smartmontools at 
least some versions of BusLogic firmware such as 5.07B are unable to 
handle such commands, but it is possible to request enough data first 
for the length of the data response to be determined and then reissue 
the same command with the allocation length matching the response 
expected.

It is what this change does on a host-by-host basis, by providing a flag 
for individual HBA drivers to enable this workaround, currently set by 
the BusLogic driver, and then issuing these double calls as requested, 
which then produce results as expected:

sd 0:0:1:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[64] => 13
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[64] => 7
 sdb: sdb1 sdb2
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[64] => 13
sd 0:0:1:0: [sdb] Attached SCSI disk
 sda: sda1 sda2 sda3 sda4 < sda5 sda6 sda7 sda8 sda9 sda10 >
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[64] => 7
sd 0:0:0:0: [sda] Attached SCSI disk
EXT4-fs (sda2): mounting ext2 file system using the ext4 subsystem
EXT4-fs (sda2): mounted filesystem without journal. Opts: (null). Quota mode: disabled.
VFS: Mounted root (ext2 filesystem) readonly on device 8:2.

The minimum request size of 4 for the repeated call has been chosen to 
match one required for a successful return from `scsi_vpd_inquiry'.

Interestingly enough it has only started triggering with not so recent 
commit af73623f5f10 ("[SCSI] sd: Reduce buffer size for vpd request") 
that decreased the allocation length for the originating request from 
512 down to 64.  Previously the request was rejected outright by the 
respective targets as invalid and therefore did not trigger the issue 
with MultiMaster firmware as that would only happen for a command that 
succeeded but produced less data than provided for:

scsi0: CCB #36 Target 0: Result 2 Host Adapter Status 00 Target Status 02
scsi0: CDB    12 01 00 02 00 00
scsi0: Sense  70 00 05 00 00 00 00 18 00 00 00 00 24 00 00 C0 00 03 00 [...]
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[512] => -5
scsi0: CCB #37 Target 1: Result 2 Host Adapter Status 00 Target Status 02
scsi0: CDB    12 01 00 02 00 00
scsi0: Sense  70 00 05 00 00 00 00 0A 00 00 00 00 24 00 01 C9 00 03 00 [...]
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[512] => -5

(here with the buffer size set back to 512, the `BusLogic=TraceErrors' 
parameter and trailing sense data zeros trimmed for brevity).  Note the 
sense key of 0x5 returned denoting an illegal request even for page 0.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 881a256d84e6 ("[SCSI] Add VPD helper")
Cc: stable@vger.kernel.org # v2.6.30+
---
No changes from v2.

No changes from v1.
---
 drivers/scsi/BusLogic.c  |    1 +
 drivers/scsi/scsi.c      |   24 +++++++++++++++++++++---
 include/scsi/scsi_host.h |    3 +++
 3 files changed, 25 insertions(+), 3 deletions(-)

linux-buslogic-get-vpd-page-buffer.diff
Index: linux-macro/drivers/scsi/BusLogic.c
===================================================================
--- linux-macro.orig/drivers/scsi/BusLogic.c
+++ linux-macro/drivers/scsi/BusLogic.c
@@ -2149,6 +2149,7 @@ static void __init blogic_inithoststruct
 	host->can_queue = adapter->drvr_qdepth;
 	host->sg_tablesize = adapter->drvr_sglimit;
 	host->cmd_per_lun = adapter->untag_qdepth;
+	host->no_trailing_allocation_length = true;
 }
 
 /*
Index: linux-macro/drivers/scsi/scsi.c
===================================================================
--- linux-macro.orig/drivers/scsi/scsi.c
+++ linux-macro/drivers/scsi/scsi.c
@@ -344,8 +344,19 @@ int scsi_get_vpd_page(struct scsi_device
 	if (sdev->skip_vpd_pages)
 		goto fail;
 
-	/* Ask for all the pages supported by this device */
-	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
+	/*
+	 * Ask for all the pages supported by this device.  Determine the
+	 * actual data length first if so required by the host, e.g.
+	 * BusLogic BT-958.
+	 */
+	if (sdev->host->no_trailing_allocation_length) {
+		result = scsi_vpd_inquiry(sdev, buf, 0, min(4, buf_len));
+		if (result < 4)
+			goto fail;
+	} else {
+		result = buf_len;
+	}
+	result = scsi_vpd_inquiry(sdev, buf, 0, min(result, buf_len));
 	if (result < 4)
 		goto fail;
 
@@ -364,7 +375,14 @@ int scsi_get_vpd_page(struct scsi_device
 	goto fail;
 
  found:
-	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
+	if (sdev->host->no_trailing_allocation_length) {
+		result = scsi_vpd_inquiry(sdev, buf, page, min(4, buf_len));
+		if (result < 4)
+			goto fail;
+	} else {
+		result = buf_len;
+	}
+	result = scsi_vpd_inquiry(sdev, buf, page, min(result, buf_len));
 	if (result < 0)
 		goto fail;
 
Index: linux-macro/include/scsi/scsi_host.h
===================================================================
--- linux-macro.orig/include/scsi/scsi_host.h
+++ linux-macro/include/scsi/scsi_host.h
@@ -659,6 +659,9 @@ struct Scsi_Host {
 	/* The transport requires the LUN bits NOT to be stored in CDB[1] */
 	unsigned no_scsi2_lun_in_cdb:1;
 
+	/* Allocation length must not exceed actual data length. */
+	unsigned no_trailing_allocation_length:1;
+
 	/*
 	 * Optional work queue to be utilized by the transport
 	 */

Re: [PATCH v3 1/3] scsi: Provide for avoiding trailing allocation length with VPD inquiries

[Christoph Hellwig]

On Sun, Jan 02, 2022 at 11:23:45PM +0000, Maciej W. Rozycki wrote:
> Allow SCSI hosts to request avoiding trailing allocation length with VPD 
> inquiries, and use the mechanism to work around an issue with at least 
> some BusLogic MultiMaster host bus adapters and observed with the BT-958 
> model specifically where issuing commands that return less data than 
> provided for causes fatal failures:

Wouldn't it make more sesnse to hide this quirk insde of
scsi_vpd_inquiry to also handle the scsi_get_vpd_buf case?  Something
like:


diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 211aace69c22c..194a51f772aaa 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -289,8 +289,8 @@ EXPORT_SYMBOL(scsi_track_queue_full);
  *
  * Returns size of the vpd page on success or a negative error number.
  */
-static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
-							u8 page, unsigned len)
+static int __scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
+		u8 page, unsigned len)
 {
 	int result;
 	unsigned char cmd[16];
@@ -321,6 +321,20 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
 	return get_unaligned_be16(&buffer[2]) + 4;
 }
 
+static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
+		u8 page, unsigned len)
+{
+	if (sdev->host->no_trailing_allocation_length) {
+		int ret = __scsi_vpd_inquiry(sdev, buffer, page, min(4U, len));
+
+		if (ret < 4)
+			return ret;
+		len = min_t(unsigned int, ret, len);
+	}
+
+	return __scsi_vpd_inquiry(sdev, buffer, page, len);
+}
+
 /**
  * scsi_get_vpd_page - Get Vital Product Data from a SCSI device
  * @sdev: The device to ask

Re: [PATCH v3 1/3] scsi: Provide for avoiding trailing allocation length with VPD inquiries

[Khalid Aziz and Shuah Khan]

On 1/3/22 01:23, Christoph Hellwig wrote:
> On Sun, Jan 02, 2022 at 11:23:45PM +0000, Maciej W. Rozycki wrote:
>> Allow SCSI hosts to request avoiding trailing allocation length with VPD
>> inquiries, and use the mechanism to work around an issue with at least
>> some BusLogic MultiMaster host bus adapters and observed with the BT-958
>> model specifically where issuing commands that return less data than
>> provided for causes fatal failures:
> Wouldn't it make more sesnse to hide this quirk insde of
> scsi_vpd_inquiry to also handle the scsi_get_vpd_buf case?  Something
> like:
>
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 211aace69c22c..194a51f772aaa 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -289,8 +289,8 @@ EXPORT_SYMBOL(scsi_track_queue_full);
>    *
>    * Returns size of the vpd page on success or a negative error number.
>    */
> -static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
> -							u8 page, unsigned len)
> +static int __scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
> +		u8 page, unsigned len)
>   {
>   	int result;
>   	unsigned char cmd[16];
> @@ -321,6 +321,20 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>   	return get_unaligned_be16(&buffer[2]) + 4;
>   }
>   
> +static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
> +		u8 page, unsigned len)
> +{
> +	if (sdev->host->no_trailing_allocation_length) {
> +		int ret = __scsi_vpd_inquiry(sdev, buffer, page, min(4U, len));
> +
> +		if (ret < 4)
> +			return ret;
> +		len = min_t(unsigned int, ret, len);
> +	}
> +
> +	return __scsi_vpd_inquiry(sdev, buffer, page, len);
> +}
> +
>   /**
>    * scsi_get_vpd_page - Get Vital Product Data from a SCSI device
>    * @sdev: The device to ask

This is certainly better. It consolidates all the special cases for 
getting VPD pages in one location and ensures no cases are missed.

--
Khalid

Re: [PATCH v3 1/3] scsi: Provide for avoiding trailing allocation length with VPD inquiries

[Maciej W. Rozycki]

On Mon, 3 Jan 2022, Christoph Hellwig wrote:

> > Allow SCSI hosts to request avoiding trailing allocation length with VPD 
> > inquiries, and use the mechanism to work around an issue with at least 
> > some BusLogic MultiMaster host bus adapters and observed with the BT-958 
> > model specifically where issuing commands that return less data than 
> > provided for causes fatal failures:
> 
> Wouldn't it make more sesnse to hide this quirk insde of
> scsi_vpd_inquiry to also handle the scsi_get_vpd_buf case?  Something
> like:

 I guess so, except that...

> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 211aace69c22c..194a51f772aaa 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -289,8 +289,8 @@ EXPORT_SYMBOL(scsi_track_queue_full);
>   *
>   * Returns size of the vpd page on success or a negative error number.
>   */
> -static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
> -							u8 page, unsigned len)
> +static int __scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
> +		u8 page, unsigned len)
>  {
>  	int result;
>  	unsigned char cmd[16];
> @@ -321,6 +321,20 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>  	return get_unaligned_be16(&buffer[2]) + 4;
>  }
>  
> +static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
> +		u8 page, unsigned len)
> +{
> +	if (sdev->host->no_trailing_allocation_length) {
> +		int ret = __scsi_vpd_inquiry(sdev, buffer, page, min(4U, len));
> +
> +		if (ret < 4)
> +			return ret;

... I think this needs to be:

		if (ret <= 4)
			return ret;

because we don't need to repeat the call where (len == 4).

 Somehow I missed your reply in the flood of messages yesterday, sorry 
about that, and it's only Khalid's response that made me notice it, so 
I'll update and retest the change now, and then repost it as a new series 
along with Martin's proposal updated according to my earlier observations.

 Thank you and Khalid for your input!  I'm glad we're making progress now.

  Maciej

[PATCH v3 2/3] scsi: Avoid using reserved length byte with VPD inquiries

[Maciej W. Rozycki]

As discussed in a previous workaround for a BusLogic BT-958 problem with 
VPD inquiries with an allocation length of 512 bytes as requested before 
commit af73623f5f10 ("[SCSI] sd: Reduce buffer size for vpd request") 
are rejected outright as invalid at least by some SCSI target devices as 
are any requests with a non-zero value in byte #3:

scsi host0: BusLogic BT-958
scsi 0:0:0:0: Direct-Access     IBM      DDYS-T18350M     SA5A PQ: 0 ANSI: 3
scsi 0:0:1:0: Direct-Access     SEAGATE  ST336607LW       0006 PQ: 0 ANSI: 3
scsi 0:0:5:0: Direct-Access     IOMEGA   ZIP 100          E.08 PQ: 0 ANSI: 2
[...]
scsi0: CCB #36 Target 0: Result 2 Host Adapter Status 00 Target Status 02
scsi0: CDB    12 01 00 01 06 00
scsi0: Sense  70 00 05 00 00 00 00 18 00 00 00 00 24 00 00 C0 00 03 00 [...]
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[262] => -5
scsi0: CCB #37 Target 1: Result 2 Host Adapter Status 00 Target Status 02
scsi0: CDB    12 01 00 01 06 00
scsi0: Sense  70 00 05 00 00 00 00 0A 00 00 00 00 24 00 01 C8 00 03 00 [...]
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[262] => -5

(here with the buffer size tweaked to 262 so as to verify if a bit in 
byte #3 of the INQUIRY command is ignored and the length of 6 assumed or 
tripped over, the `BusLogic=TraceErrors' parameter and trailing sense 
data zeros trimmed for brevity).  Note the sense key of 0x5 denoting an 
illegal request.

For the record with the buffer size of 6 requests for page 0 complete 
successfully and due to page truncation `scsi_get_vpd_page' proceeds 
with an attempt to get inexistent page 0x89:

sd 0:0:0:0: scsi_vpd_inquiry(0): buf[6] => 7
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[6] => 13
sd 0:0:0:0: scsi_vpd_inquiry(137): buf[6] => -5
sd 0:0:1:0: scsi_vpd_inquiry(137): buf[6] => -5

Upon a further investigation it has turned out at least SCSI-2 considers 
byte #3 of the INQUIRY command[1] as well as byte #2 of vital product 
data pages[2] reserved and expects a value of zero there.  The response 
from SCSI-3 devices shown above indicates the same expectation.

Therefore it is unsafe to issue INQUIRY requests unconditionally with 
the allocation length beyond 255, as they may fail with an otherwise 
supported request or cause undefined behaviour with some hardware.

Now we actually never do that as all our callers of `scsi_get_vpd_page' 
either hardcode the buffer size to a value between 8 and 255 or 
calculate it from a structure size, of which the largest is:

struct c2_inquiry {
	u8                         peripheral_info;      /*     0     1 */
	u8                         page_code;            /*     1     1 */
	u8                         reserved1;            /*     2     1 */
	u8                         page_len;             /*     3     1 */
	u8                         page_id[4];           /*     4     4 */
	u8                         sw_version[3];        /*     8     3 */
	u8                         sw_date[3];           /*    11     3 */
	u8                         features_enabled;     /*    14     1 */
	u8                         max_lun_supported;    /*    15     1 */
	u8                         partitions[239];      /*    16   239 */

	/* size: 255, cachelines: 2, members: 10 */
	/* last cacheline: 127 bytes */
};

As from commit b3ae8780b429 ("[SCSI] Add EVPD page 0x83 and 0x80 to sysfs")
we now also have the SCSI_VPD_PG_LEN macro that reflects the limitation.

However for the sake of a possible future requirement to support VPD 
pages that do have a length exceeding 255 bytes and now that the danger 
of using the formerly reserved byte #3 of the INQUIRY command has been 
identified execute calls to `scsi_get_vpd_page' with a request size 
exceeding 255 bytes in two stages, by determining the actual length of 
data to be returned first and only then issuing the intended request for 
full data.

References:

[1] "Information technology - Small Computer System Interface - 2", 
    WORKING DRAFT, X3T9.2, Project 375D, Revision 10L, 7-SEP-93, Section 
    8.2.5 "INQUIRY command", pp.104-108

[2] same, Section 8.3.4 "Vital product data parameters", pp.154-159

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 881a256d84e6 ("[SCSI] Add VPD helper")
---
No changes from v2.

No changes from v1.
---
 drivers/scsi/scsi.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

linux-scsi-vpd-inquiry-buffer.diff
Index: linux-macro/drivers/scsi/scsi.c
===================================================================
--- linux-macro.orig/drivers/scsi/scsi.c
+++ linux-macro/drivers/scsi/scsi.c
@@ -346,10 +346,15 @@ int scsi_get_vpd_page(struct scsi_device
 
 	/*
 	 * Ask for all the pages supported by this device.  Determine the
-	 * actual data length first if so required by the host, e.g.
-	 * BusLogic BT-958.
+	 * actual data length first if the length requested is beyond 255
+	 * bytes as the high order length byte used to be reserved with
+	 * older SCSI standard revisions and a non-zero value there may
+	 * cause either such an INQUIRY command to be rejected by a target
+	 * or undefined behaviour to occur.  Also do so if so required by
+	 * the host, e.g. BusLogic BT-958.
 	 */
-	if (sdev->host->no_trailing_allocation_length) {
+	if (buf_len > SCSI_VPD_PG_LEN ||
+	    sdev->host->no_trailing_allocation_length) {
 		result = scsi_vpd_inquiry(sdev, buf, 0, min(4, buf_len));
 		if (result < 4)
 			goto fail;
@@ -375,7 +380,8 @@ int scsi_get_vpd_page(struct scsi_device
 	goto fail;
 
  found:
-	if (sdev->host->no_trailing_allocation_length) {
+	if (buf_len > SCSI_VPD_PG_LEN ||
+	    sdev->host->no_trailing_allocation_length) {
 		result = scsi_vpd_inquiry(sdev, buf, page, min(4, buf_len));
 		if (result < 4)
 			goto fail;

[PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Maciej W. Rozycki]

Set the allocation length to 255 for the ATA Information VPD page 
requested in the WRITE SAME handler, so as not to limit information 
examined by `scsi_get_vpd_page' in the supported vital product data 
pages unnecessarily.

Originally it was thought that Areca hardware may have issues with a 
valid allocation length supplied for a VPD inquiry, however older SCSI 
standard revisions[1] consider 255 the maximum length allowed and what 
has later become the high order byte is considered reserved and must be 
zero with the INQUIRY command.  Therefore it was unnecessary to reduce 
the amount of data requested from 512 as far down as to 64, arbitrarily 
chosen, and 255 would as well do.

With commit b3ae8780b429 ("[SCSI] Add EVPD page 0x83 and 0x80 to sysfs") 
we have since got the SCSI_VPD_PG_LEN macro, so use that instead.

References:

[1] "Information technology - Small Computer System Interface - 2",
    WORKING DRAFT, X3T9.2, Project 375D, Revision 10L, 7-SEP-93, Section
    8.2.5 "INQUIRY command", pp.104-108

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: af73623f5f10 ("[SCSI] sd: Reduce buffer size for vpd request")
Tested-by: Nick Alcock <nick.alcock@oracle.com>
---
Changes from v2:

- Add Nick's Tested-by annotation. 

No changes from v1.
---
 drivers/scsi/sd.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

linux-scsi-write-same-vpd-buffer.diff
Index: linux-macro/drivers/scsi/sd.c
===================================================================
--- linux-macro.orig/drivers/scsi/sd.c
+++ linux-macro/drivers/scsi/sd.c
@@ -3101,16 +3101,13 @@ static void sd_read_write_same(struct sc
 	}
 
 	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, INQUIRY) < 0) {
-		/* too large values might cause issues with arcmsr */
-		int vpd_buf_len = 64;
-
 		sdev->no_report_opcodes = 1;
 
 		/* Disable WRITE SAME if REPORT SUPPORTED OPERATION
 		 * CODES is unsupported and the device has an ATA
 		 * Information VPD page (SAT).
 		 */
-		if (!scsi_get_vpd_page(sdev, 0x89, buffer, vpd_buf_len))
+		if (!scsi_get_vpd_page(sdev, 0x89, buffer, SCSI_VPD_PG_LEN))
 			sdev->no_write_same = 1;
 	}
 

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Douglas Gilbert]

On 2022-01-02 6:23 p.m., Maciej W. Rozycki wrote:
> Set the allocation length to 255 for the ATA Information VPD page
> requested in the WRITE SAME handler, so as not to limit information
> examined by `scsi_get_vpd_page' in the supported vital product data
> pages unnecessarily.
> 
> Originally it was thought that Areca hardware may have issues with a
> valid allocation length supplied for a VPD inquiry, however older SCSI
> standard revisions[1] consider 255 the maximum length allowed and what
> has later become the high order byte is considered reserved and must be
> zero with the INQUIRY command.  Therefore it was unnecessary to reduce
> the amount of data requested from 512 as far down as to 64, arbitrarily
> chosen, and 255 would as well do.

Not arbitrary, 64 bytes would get all the fields less the 512 byte ATA
DEVICE IDENTIFY data field.

> With commit b3ae8780b429 ("[SCSI] Add EVPD page 0x83 and 0x80 to sysfs")
> we have since got the SCSI_VPD_PG_LEN macro, so use that instead.
> 
> References:
> 
> [1] "Information technology - Small Computer System Interface - 2",
>      WORKING DRAFT, X3T9.2, Project 375D, Revision 10L, 7-SEP-93, Section
>      8.2.5 "INQUIRY command", pp.104-108

Yes, 1992, long withdrawn and only used by several billion USB keys.

But the ATA Information VPD page first appeared in SAT around 2006 and the
length of that page was (and still is in SAT-5 drafts) "238h" (572 bytes
long (when the 4 byte header is considered)). So it needs 16 bits to
represent that length. SPC-3 (2005) bumped the allocation length field in
the INQUIRY command from 8 to 16 bits.

Finally SAT-1 in its approved references [2.2] says:
ISO/IEC 14776-453, SCSI Primary Commands - 3 (SPC-3) [ANSI INCITS 408-2005]

So any SAT layer that supplies the ATA Information VPD page should also
support (translating) an INQUIRY with a 16 bit allocation field.

How does your problem arise? Could USB mass storage be involved?

And this patch solves your problem by returning part of the ATA DEVICE
IDENTIFY data (which is 512 bytes long)? If so, why not say so.

And what about using 0x2ff as the INQUIRY allocation length? If the
broken device ignores the top byte, you get 255 bytes back. If a
correct device takes both bytes it should return 0x23c bytes after
resid is taken into account.



Not related to this patch:
sd_read_write_same() seems a strange name for a function given that
it is checking on WRITE SAME support. How about s/read/report/ ?
And calling scsi_report_opcode() on INQUIRY seems a weird time waster
(it actually checks if the SCSI version is < SPC-3 or does the check
on a _mandatory_ command). And for modern disks scsi_report_opcode()
is called 5 times. Why not call the REPORT SUPPORTED OPERATION CODES
command once and cache its result? It would save 4 commands in every
disk setup (or revalidation).

Doug Gilbert


> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> Fixes: af73623f5f10 ("[SCSI] sd: Reduce buffer size for vpd request")
> Tested-by: Nick Alcock <nick.alcock@oracle.com>
> ---
> Changes from v2:
> 
> - Add Nick's Tested-by annotation.
> 
> No changes from v1.
> ---
>   drivers/scsi/sd.c |    5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> linux-scsi-write-same-vpd-buffer.diff
> Index: linux-macro/drivers/scsi/sd.c
> ===================================================================
> --- linux-macro.orig/drivers/scsi/sd.c
> +++ linux-macro/drivers/scsi/sd.c
> @@ -3101,16 +3101,13 @@ static void sd_read_write_same(struct sc
>   	}
>   
>   	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, INQUIRY) < 0) {
> -		/* too large values might cause issues with arcmsr */
> -		int vpd_buf_len = 64;
> -
>   		sdev->no_report_opcodes = 1;
>   
>   		/* Disable WRITE SAME if REPORT SUPPORTED OPERATION
>   		 * CODES is unsupported and the device has an ATA
>   		 * Information VPD page (SAT).
>   		 */
> -		if (!scsi_get_vpd_page(sdev, 0x89, buffer, vpd_buf_len))
> +		if (!scsi_get_vpd_page(sdev, 0x89, buffer, SCSI_VPD_PG_LEN))
>   			sdev->no_write_same = 1;
>   	}
>   
> 

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Maciej W. Rozycki]

On Sun, 2 Jan 2022, Douglas Gilbert wrote:

> > Originally it was thought that Areca hardware may have issues with a
> > valid allocation length supplied for a VPD inquiry, however older SCSI
> > standard revisions[1] consider 255 the maximum length allowed and what
> > has later become the high order byte is considered reserved and must be
> > zero with the INQUIRY command.  Therefore it was unnecessary to reduce
> > the amount of data requested from 512 as far down as to 64, arbitrarily
> > chosen, and 255 would as well do.
> 
> Not arbitrary, 64 bytes would get all the fields less the 512 byte ATA
> DEVICE IDENTIFY data field.

 That may well be the case, however there is no justification given for 
the particular size of 64 bytes chosen either in the comment nearby or the 
change description associated with the commit referred this arrangement 
has originated from.  At the time of my original submission I examined the 
relevant thread of discussion[1] including the patch submission itself[2], 
and just to be sure I have double-checked it now and there is no mention 
as to why this value was chosen.  There is no associated macro that could 
give some explanation and which good coding style would expect rather than 
a magic number inline.

 So I do have all the reasons to conclude this value has indeed been 
arbitrarily chosen, don't I?

> > With commit b3ae8780b429 ("[SCSI] Add EVPD page 0x83 and 0x80 to sysfs")
> > we have since got the SCSI_VPD_PG_LEN macro, so use that instead.
> > 
> > References:
> > 
> > [1] "Information technology - Small Computer System Interface - 2",
> >      WORKING DRAFT, X3T9.2, Project 375D, Revision 10L, 7-SEP-93, Section
> >      8.2.5 "INQUIRY command", pp.104-108
> 
> Yes, 1992, long withdrawn and only used by several billion USB keys.

 Well, this has surfaced in a setup where devices dated 199x are used, so 
I guess they have all the rights to use whatever standard was most recent, 
or say second most recent at the time as we need to factor in design lead 
times.

> How does your problem arise? Could USB mass storage be involved?

 This command does crash the HBA involved where 1/3 and 2/3 have not been 
applied.  No USB involved, just these proper SCSI (SPI) targets:

scsi 0:0:0:0: Direct-Access     IBM      DDYS-T18350M     SA5A PQ: 0 ANSI: 3
scsi 0:0:1:0: Direct-Access     SEAGATE  ST336607LW       0006 PQ: 0 ANSI: 3
scsi 0:0:5:0: Direct-Access     IOMEGA   ZIP 100          E.08 PQ: 0 ANSI: 2

as noted with 1/3 and 2/3.

 Not noted here as not directly relevant though, and this is because this 
change is a clean-up only, to have the buffer size consistent across the 
various `scsi_get_vpd_page' calls, by using the SCSI_VPD_PG_LEN macro 
defined meanwhile, that sets the maximum supported by older SCSI standard 
revisions (which can therefore be safely used without asking the device 
how much data it can/wants to actually return) and consequently devices 
implementing them.

 I noted in the original submission[3]:

> Nix,
> 
> I can see you're still around.  Would you therefore please be so kind
> as to verify this change with your Areca hardware if you still have it?
> 
> It looks to me like you were thinking in the right direction with:
> <https://lore.kernel.org/linux-scsi/87vc3nuipg.fsf@spindle.srvr.nix/>.
> Sadly nobody seemed to have paid attention to your observation and neither
> were different buffer sizes considered (or at least it wasn't mentioned in
> the discussion).
> 
>  Maciej

-- and Nix was kind enough to verify my proposal works just fine with the 
piece of hardware the commit referred addressed a problem with, so the 
replacement buffer size is as good as the original one while making code 
consistent.  As you can see I did observe right away that the buffer size 
was not discussed.

 If you insist that the value of 64 stay, then please come up with a 
suitable macro name to define along with SCSI_VPD_PG_LEN that reflects the 
meaning of 64 in this context and I'll be happy to update 3/3 accordingly, 
but please explain why the value of 64 is any better than 255 here and the 
inconsistency with the buffer size justified.

> And this patch solves your problem by returning part of the ATA DEVICE
> IDENTIFY data (which is 512 bytes long)? If so, why not say so.

 As I noted above, this is for consistency with other `scsi_get_vpd_page' 
calls and to avoid an inline magic number.  If you think that it is not 
stated clearly enough in my change description and the change is otherwise 
acceptable, then I can update the explanation accordingly.

> And what about using 0x2ff as the INQUIRY allocation length? If the
> broken device ignores the top byte, you get 255 bytes back. If a
> correct device takes both bytes it should return 0x23c bytes after
> resid is taken into account.

 I have verified (some of) the devices listed above to correctly reject 
`scsi_get_vpd_page' requests with allocation length exceeding 255, as 
required by the SCSI standard revision at their time.  I can't speak of 
the INQUIRY command, as I haven't checked it in this context.

 Does my explanation clear your concerns?  If so, then please advise how 
to proceed with this change.  Thank you for your review.

References:

[1] "3.10.2 or 3.10.3: arcmsr failure at bootup / early userspace 
    transition", 
    <https://lore.kernel.org/linux-scsi/87r4ehfzhf.fsf@spindle.srvr.nix/>

[2] "scsi disk: Use its own buffer for the vpd request", 
    <https://lore.kernel.org/linux-scsi/51FA71E2.6010501@fastmail.fm/>

[3] "scsi: Set allocation length to 255 for ATA Information VPD page", 
    <https://lore.kernel.org/linux-scsi/alpine.DEB.2.21.2104141306130.44318@angie.orcam.me.uk/>

  Maciej

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Martin K. Petersen]

Maciej,

>  So I do have all the reasons to conclude this value has indeed been 
> arbitrarily chosen, don't I?

The SAT spec defines the contents of the first part of the page. The
trailing 512 bytes are defined in the ATA spec.

> If you insist that the value of 64 stay, then please come up with a
> suitable macro name to define along with SCSI_VPD_PG_LEN that reflects
> the meaning of 64 in this context and I'll be happy to update 3/3
> accordingly, but please explain why the value of 64 is any better than
> 255 here and the inconsistency with the buffer size justified.

Can please you try the following patch?

-- 
Martin K. Petersen	Oracle Linux Engineering

Subject: [PATCH] scsi: core: Request VPD page header before fetching full page

We currently default to 255 bytes when fetching for VPD pages during
discovery. However, we have had a few devices that are known to wedge if
the requested buffer exceeds a certain size. See commit af73623f5f10
("[SCSI] sd: Reduce buffer size for vpd request") which works around one
example of this problem in the SCSI disk driver.

With commit d188b0675b21 ("scsi: core: Add sysfs attributes for VPD pages
0h and 89h") we now risk triggering the same issue in the generic midlayer
code.

The problem with the ATA VPD page in particular is that the SCSI portion of
the page is trailed by 512 bytes of verbatim ATA Identify Device
information.  However, not all controllers actually provide the additional
512 bytes and will lock up if one asks for more than the 64 bytes
containing the SCSI protocol fields.

Instead of picking a new, somewhat arbitrary, number of bytes for the
default VPD buffer size, first fetch the 4-byte header for each page. The
header contains the size of the page as far as the device is concerned. Use
this reported size to allocate the permanent VPD buffer and then proceed to
fetch the full page up to a 1K limit.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Fixes: d188b0675b21 ("scsi: core: Add sysfs attributes for VPD pages 0h and 89h")

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 211aace69c22..d45c4d7526d5 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -384,7 +384,13 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
 {
 	struct scsi_vpd *vpd_buf;
-	int vpd_len = SCSI_VPD_PG_LEN, result;
+	int vpd_len, result;
+	unsigned char vpd_header[SCSI_VPD_HEADER_SIZE];
+
+	result = scsi_vpd_inquiry(sdev, vpd_header, page, SCSI_VPD_HEADER_SIZE);
+	if (result < SCSI_VPD_HEADER_SIZE || result > SCSI_VPD_MAX_SIZE)
+		return NULL;
+	vpd_len = result;
 
 retry_pg:
 	vpd_buf = kmalloc(sizeof(*vpd_buf) + vpd_len, GFP_KERNEL);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d1c6fc83b1e3..6d6c44e8da08 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -100,6 +100,11 @@ struct scsi_vpd {
 	unsigned char	data[];
 };
 
+enum scsi_vpd_parameters {
+	SCSI_VPD_HEADER_SIZE = 4,
+	SCSI_VPD_MAX_SIZE = 1024,
+};
+
 struct scsi_device {
 	struct Scsi_Host *host;
 	struct request_queue *request_queue;
@@ -141,7 +146,6 @@ struct scsi_device {
 	const char * model;		/* ... after scan; point to static string */
 	const char * rev;		/* ... "nullnullnullnull" before scan */
 
-#define SCSI_VPD_PG_LEN                255
 	struct scsi_vpd __rcu *vpd_pg0;
 	struct scsi_vpd __rcu *vpd_pg83;
 	struct scsi_vpd __rcu *vpd_pg80;

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Maciej W. Rozycki]

Martin,

> >  So I do have all the reasons to conclude this value has indeed been 
> > arbitrarily chosen, don't I?
> 
> The SAT spec defines the contents of the first part of the page. The
> trailing 512 bytes are defined in the ATA spec.

 I think that would best be reflected in code somehow as lone `64' doesn't 
say anything really.

> > If you insist that the value of 64 stay, then please come up with a
> > suitable macro name to define along with SCSI_VPD_PG_LEN that reflects
> > the meaning of 64 in this context and I'll be happy to update 3/3
> > accordingly, but please explain why the value of 64 is any better than
> > 255 here and the inconsistency with the buffer size justified.
> 
> Can please you try the following patch?

 I have tried it and it's neutral, that is with 1/3 applied the HBA still 
works and with 1/3 removed it still breaks (2/3 and 3/3 obviously don't 
build anymore).  Unsurprisingly, as it's the call to `scsi_get_vpd_page' 
rather than `scsi_get_vpd_buf' that causes an issue here.

 I think the latter function isn't called in my setup, and it's not clear 
to me anymore after so long why I didn't poke at it.  It looks to me like 
the `retry_pg' code there can be gone now with your update in place as it 
duplicates buffer sizing, and with that included it'll be a nice clean-up.

 NB you'll need to adjust drivers/scsi/mpt3sas/mpt3sas_scsih.c accordingly 
if we are to move forward with this change, as it's another user of the 
SCSI_VPD_PG_LEN macro.

 Given what has been said in the discussion so far would you consider 2/3
and 3/3 unnecessary?  In the course of verifying your change I have looked 
through our code again and found that inline magic numbers are there in 
several though not all places where `scsi_get_vpd_page' gets called, so I 
think it would make sense to get rid of them all at once with a single 
self-contained change.

 Thank you for your input and the extra fix.

  Maciej

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Martin K. Petersen]

Maciej,

>  I have tried it and it's neutral, that is with 1/3 applied the HBA still 
> works and with 1/3 removed it still breaks (2/3 and 3/3 obviously don't 
> build anymore).  Unsurprisingly, as it's the call to `scsi_get_vpd_page' 
> rather than `scsi_get_vpd_buf' that causes an issue here.

Oh, you'll also need a follow-on patch that uses the cached ATA
Information VPD page. I'll try to get my full series out today.

>  NB you'll need to adjust drivers/scsi/mpt3sas/mpt3sas_scsih.c accordingly 
> if we are to move forward with this change, as it's another user of the 
> SCSI_VPD_PG_LEN macro.

That'll also use cached information in my series.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Martin K. Petersen]

Maciej,

> Oh, you'll also need a follow-on patch that uses the cached ATA
> Information VPD page. I'll try to get my full series out today.

I would really appreciate it if you would be willing give this a whirl:

	https://git.kernel.org/mkp/h/5.18/discovery

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Damien Le Moal]

On 1/6/22 13:13, Martin K. Petersen wrote:
> 
> Maciej,
> 
>> Oh, you'll also need a follow-on patch that uses the cached ATA
>> Information VPD page. I'll try to get my full series out today.
> 
> I would really appreciate it if you would be willing give this a whirl:
> 
> 	https://git.kernel.org/mkp/h/5.18/discovery

Martin,

Indeed, my bad.

That said, it is weird that scsi_get_vpd_page() does not call
scsi_device_supports_vpd(). We could simplify everything like this:

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f6af1562cba4..c27eabedf9e3 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -341,7 +341,7 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8
page, unsigned char *buf,
 {
        int i, result;

-       if (sdev->skip_vpd_pages)
+       if (!scsi_device_supports_vpd(sdev))
                goto fail;

        /* Ask for all the pages supported by this device */
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 65875a598d62..2ef7953512ed 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3316,12 +3316,10 @@ static int sd_revalidate_disk(struct gendisk *disk)
                blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
                blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);

-               if (scsi_device_supports_vpd(sdp)) {
-                       sd_read_block_provisioning(sdkp);
-                       sd_read_block_limits(sdkp);
-                       sd_read_block_characteristics(sdkp);
-                       sd_zbc_read_zones(sdkp, buffer);
-               }
+               sd_read_block_provisioning(sdkp);
+               sd_read_block_limits(sdkp);
+               sd_read_block_characteristics(sdkp);
+               sd_zbc_read_zones(sdkp, buffer);

                sd_print_capacity(sdkp, old_capacity);

Since all the sd_read_xxx() functions do nothing if the vpd page needed
is not supported.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Martin K. Petersen]

Damien,

> That said, it is weird that scsi_get_vpd_page() does not call
> scsi_device_supports_vpd(). 

The first patch in the series already makes that change.

I noticed because the allocation for sd_read_cpr() is fairly big so it
stuck out in my test runs while reworking scsi_get_vpd_page().

I didn't remove the conditional in sd_revalidate_disk(). While it is
superfluous, I do like that the "fancy" protocol features are
grouped. Guess we could switch it to a comment instead. I'll think about
it...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Maciej W. Rozycki]

Martin,

> > Oh, you'll also need a follow-on patch that uses the cached ATA
> > Information VPD page. I'll try to get my full series out today.
> 
> I would really appreciate it if you would be willing give this a whirl:
> 
> 	https://git.kernel.org/mkp/h/5.18/discovery

 I have tried your tree and it does not clobber the HBA anymore, however 
partitions (of the MS-DOS type) are not recognised with any of the disks 
including one holding the root device, so the system fails to mount the 
root filesystem and therefore does not complete booting:

VFS: Cannot open root device "802" or unknown-block(8,2): error -6
Please append a correct "root=" boot option; here are the available partitions:
0800        17921835 sda
 driver: sd
0810        35843686 sdb
 driver: sd
0830          239816 sdd
 driver: sd
0b00         1048575 sr0
 driver: sr
Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(8,2)

-- is that expected?

 Here's the relevant part of the boot log:

scsi host0: BusLogic BT-958
scsi 0:0:0:0: Direct-Access     IBM      DDYS-T18350M     SA5A PQ: 0 ANSI: 3
scsi 0:0:1:0: Direct-Access     SEAGATE  ST336607LW       0006 PQ: 0 ANSI: 3
scsi 0:0:4:0: Sequential-Access HP       C5683A           C908 PQ: 0 ANSI: 2
scsi 0:0:5:0: Direct-Access     IOMEGA   ZIP 100          E.08 PQ: 0 ANSI: 2
st: Version 20160209, fixed bufsize 32768, s/g segs 256
st 0:0:4:0: Attached scsi tape st0
st 0:0:4:0: st0: try direct i/o: yes (alignment 4 B)
sd 0:0:0:0: [sda] 35843670 512-byte logical blocks: (18.4 GB/17.1 GiB)
sd 0:0:5:0: [sdc] Media removed, stopped polling
sd 0:0:1:0: [sdb] 71687372 512-byte logical blocks: (36.7 GB/34.2 GiB)
sd 0:0:5:0: [sdc] Attached SCSI removable disk
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:1:0: [sdb] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: cb 00 00 08
sd 0:0:1:0: [sdb] Mode Sense: ab 00 10 08
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sda: detected capacity change from 0 to 35843670
sd 0:0:0:0: [sda] Attached SCSI disk
sd 0:0:1:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
sdb: detected capacity change from 0 to 71687372
sd 0:0:1:0: [sdb] Attached SCSI disk
sd 0:0:0:0: Attached scsi generic sg0 type 0
sd 0:0:1:0: Attached scsi generic sg1 type 0
st 0:0:4:0: Attached scsi generic sg2 type 1
sd 0:0:5:0: Attached scsi generic sg3 type 0

while upon a succesful boot with the upstream kernel (and my patch(es) 
applied) it looks like:

scsi host0: BusLogic BT-958
scsi 0:0:0:0: Direct-Access     IBM      DDYS-T18350M     SA5A PQ: 0 ANSI: 3
scsi 0:0:1:0: Direct-Access     SEAGATE  ST336607LW       0006 PQ: 0 ANSI: 3
scsi 0:0:4:0: Sequential-Access HP       C5683A           C908 PQ: 0 ANSI: 2
scsi 0:0:5:0: Direct-Access     IOMEGA   ZIP 100          E.08 PQ: 0 ANSI: 2
st: Version 20160209, fixed bufsize 32768, s/g segs 256
st 0:0:4:0: Attached scsi tape st0
st 0:0:4:0: st0: try direct i/o: yes (alignment 4 B)
sd 0:0:0:0: [sda] 35843670 512-byte logical blocks: (18.4 GB/17.1 GiB)
sd 0:0:5:0: [sdc] Media removed, stopped polling
sd 0:0:1:0: [sdb] 71687372 512-byte logical blocks: (36.7 GB/34.2 GiB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:1:0: [sdb] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: cb 00 00 08
sd 0:0:1:0: [sdb] Mode Sense: ab 00 10 08
sd 0:0:5:0: [sdc] Attached SCSI removable disk
sd 0:0:1:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sdb: sdb1 sdb2
sd 0:0:1:0: [sdb] Attached SCSI disk
 sda: sda1 sda2 sda3 sda4 < sda5 sda6 sda7 sda8 sda9 sda10 >
sd 0:0:0:0: [sda] Attached SCSI disk
sd 0:0:0:0: Attached scsi generic sg0 type 0
sd 0:0:1:0: Attached scsi generic sg1 type 0
st 0:0:4:0: Attached scsi generic sg2 type 1
sd 0:0:5:0: Attached scsi generic sg3 type 0

The failure is not specific to this HBA as `hdd' is a PATA device and it 
doesn't get its partitions scanned either.

 There's no significant difference between the two .config files:

--- ../linux-macro/.config
+++ .config
@@ -1,6 +1,6 @@
 #
 # Automatically generated file; DO NOT EDIT.
-# Linux/i386 5.16.0-rc7 Kernel Configuration
+# Linux/i386 5.16.0-rc1 Kernel Configuration
 #
 CONFIG_CC_VERSION_TEXT="i386-linux-gnu-gcc (GCC) 11.0.0 20200919 (experimental)"
 CONFIG_CC_IS_GCC=y
@@ -518,7 +518,6 @@
 CONFIG_HAVE_ARCH_MMAP_RND_BITS=y
 CONFIG_HAVE_EXIT_THREAD=y
 CONFIG_ARCH_MMAP_RND_BITS=8
-CONFIG_PAGE_SIZE_LESS_THAN_64KB=y
 CONFIG_ISA_BUS_API=y
 CONFIG_CLONE_BACKWARDS=y
 CONFIG_OLD_SIGSUSPEND3=y
@@ -1061,7 +1060,6 @@
 # CONFIG_SCSI_MPI3MR is not set
 # CONFIG_SCSI_SMARTPQI is not set
 # CONFIG_SCSI_UFSHCD is not set
-# CONFIG_SCSI_UFS_HWMON is not set
 # CONFIG_SCSI_HPTIOP is not set
 CONFIG_SCSI_BUSLOGIC=y
 # CONFIG_SCSI_FLASHPOINT is not set

 Shall I try anything else?

  Maciej

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Martin K. Petersen]

Maciej,

> I have tried your tree and it does not clobber the HBA anymore,

Excellent!

> however partitions (of the MS-DOS type) are not recognised with any of
> the disks including one holding the root device, so the system fails
> to mount the root filesystem and therefore does not complete booting:

My mistake. An unrelated change to the revalidate logic in the last
patch. Fixed and pushed.

For your Mylex issue I believe the first patch in the series is all
that's needed:

06a471da0937 ("scsi: core: Query VPD size before getting full page")

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Maciej W. Rozycki]

Martin,

> > however partitions (of the MS-DOS type) are not recognised with any of
> > the disks including one holding the root device, so the system fails
> > to mount the root filesystem and therefore does not complete booting:
> 
> My mistake. An unrelated change to the revalidate logic in the last
> patch. Fixed and pushed.

 No worries, I'm glad I helped catch it early.  This version boots 
multi-user.

> For your Mylex issue I believe the first patch in the series is all
> that's needed:
> 
> 06a471da0937 ("scsi: core: Query VPD size before getting full page")

 It is.  Thanks for sorting out this issue!

  Maciej

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Martin K. Petersen]

Maciej,

>> For your Mylex issue I believe the first patch in the series is all
>> that's needed:
>> 
>> 06a471da0937 ("scsi: core: Query VPD size before getting full page")
>
>  It is.  Thanks for sorting out this issue!

Excellent, thanks for all the testing and for your work identifying this
issue!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

[Martin K. Petersen]

Doug,

> sd_read_write_same() seems a strange name for a function given that
> it is checking on WRITE SAME support. How about s/read/report/ ?

It was chosen to be consistent with all the other sd_read_$VPD()
functions. sd_read_cache_type(), sd_read_block_limits(), etc.

> And calling scsi_report_opcode() on INQUIRY seems a weird time waster
> (it actually checks if the SCSI version is < SPC-3 or does the check
> on a _mandatory_ command).

The call to validate INQUIRY is really to check whether REPORT SUPPORTED
OPERATION CODES command is supported.

> And for modern disks scsi_report_opcode() is called 5 times. Why not
> call the REPORT SUPPORTED OPERATION CODES command once and cache its
> result? It would save 4 commands in every disk setup (or
> revalidation).

I have some patches that clean up discovery and start using cached
VPDs. I hadn't thought of caching the RSOC output. Will look into that.

I held this series back since I was concerned about them clashing with
Christoph's recent revalidate changes. I'll get them sent out shortly.

-- 
Martin K. Petersen	Oracle Linux Engineering