Re: [PATCH 03/14] scsi: core: Do not truncate INQUIRY data on modern devices

From: Bart Van Assche <bvanassche@acm.org>
To: "Martin K. Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org
Subject: Re: [PATCH 03/14] scsi: core: Do not truncate INQUIRY data on modern devices
Date: Wed, 2 Mar 2022 16:14:44 -0800	[thread overview]
Message-ID: <82c21426-4d23-1e5f-ef9b-212d623eb8f7@acm.org> (raw)
In-Reply-To: <20220302053559.32147-4-martin.petersen@oracle.com>

On 3/1/22 21:35, Martin K. Petersen wrote:
> Low-level device drivers have had the ability to limit the size of an
> INQUIRY for many years. This made sense for a wide variety of legacy
> devices. However, we are unnecessarily truncating the INQUIRY response
> for many modern devices. This prevents us from consulting fields
> beyond the first 36 bytes.
> 
> If a device reports that it supports a larger INQUIRY response, and
> the device also reports that it implements SPC-4 or newer, allow the
> larger INQUIRY to proceed.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>   drivers/scsi/scsi_scan.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index f4e6c68ac99e..95bf9a1f35ce 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -728,7 +728,17 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
>   		if (pass == 1) {
>   			if (BLIST_INQUIRY_36 & *bflags)
>   				next_inquiry_len = 36;
> -			else if (sdev->inquiry_len)
> +			/*
> +			 * LLD specified a maximum sdev->inquiry_len
> +			 * but device claims it has more data. Capping
> +			 * the length only makes sense for legacy
> +			 * devices. If a device supports SPC-4 (2014)
> +			 * or newer, assume that it is safe to ask for
> +			 * as much as the device says it supports.
> +			 */
> +			else if (sdev->inquiry_len &&
> +				 response_len > sdev->inquiry_len &&
> +				 (inq_result[2] & 0x7) < 6) /* SPC-4 */
>   				next_inquiry_len = sdev->inquiry_len;
>   			else
>   				next_inquiry_len = response_len;

Hi Martin,

Do the benefits of this change outweigh the additional complexity 
introduced by this code and the risk of breaking support for certain 
devices? I'm asking this because the number of LLDs that sets 
inquiry_len is small:

$ git grep -nH '>inquiry_len[[:blank:]]*=[[:blank:]]'|grep -v scsi_scan
drivers/firewire/sbp2.c:1508:		sdev->inquiry_len = 36;
drivers/staging/rts5208/rtsx.c:67:	sdev->inquiry_len = 36;
drivers/usb/image/microtek.c:323:	s->inquiry_len = 0x24;
drivers/usb/storage/scsiglue.c:77:	sdev->inquiry_len = 36;

Does any of these LLDs support SPC-4 devices? Can this change e.g. break 
support for certain USB sticks?

Thanks,

Bart.