> On May 8, 2023, at 3:09 AM, Benjamin Block <bblock@xxxxxxxxxxxxx> wrote:
>
> On Fri, May 05, 2023 at 01:49:50PM -0700, Brian Bunker wrote:
>> When SCSI devices are discovered the function sd_read_cpr gets called.
>> This call results in an INQUIRY to page 0xb9. This VPD page is called
>> regardless of whether the target has advertised this page as supported.
>>
>> Instead of just sending this INQUIRY page, first check to see if that
>> page is in the supported pages. This will avoid sending requests to
>> targets which do not support the page. The error is unexpected on the
>> target and leads to questions. I am not sure what percentage of SCSI
>> devices support this page, but this will eliminate at least one
>> request to the target in the discovery phase for all that do not. The
>> function added could also have potential users besides this specific
>> one.
>>
>> Signed-off-by: Brian Bunker <brian@xxxxxxxxxxxxxxx>
>> Reviewed-by: Seamus Connor <sconnor@xxxxxxxxxxxxxxx>
>> Reviewed-by: Krishna Kant <krishna.kant@xxxxxxxxxxxxxxx>
>> ---
>> drivers/scsi/scsi.c | 40 ++++++++++++++++++++++++++++++++++++++
>> drivers/scsi/sd.c | 4 +++-
>> include/scsi/scsi_device.h | 1 +
>> 3 files changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index 09ef0b31dfc0..9265b3d6a18f 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -356,6 +356,46 @@ static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
>> return result;
>> }
>>
>> +/**
>> + * scsi_vpd_page_supported - Check if a VPD page is supported on a SCSI device
>> + * @sdev: The device to ask
>> + * @page: Check existence of this Vital Product Data page
>> + *
>> + * Functions which explicitly request a given VPD page
>> + * should first check whether that page is among the
>> + * supported VPD pages. This will avoid targets returning
>> + * unnecessary errors which can cause confusion. -EINVAL is
>> + * returned if the page is not supported and 0 if it is.
>> + */
>> +int scsi_vpd_page_supported(struct scsi_device *sdev, u8 page)
>> +{
>> + const struct scsi_vpd *vpd;
>> + uint16_t page_len;
>
> Probably rather `u16` as per kernel-style.
Sure. This can easily be done.
>
>> + int ret = -EINVAL;
>
> Been wondering, whether it would make sense to have two different error
> levels here. One for the case where the page is not found in the loop
> that searches within page 0, and one for when page 0 is not present when
> we try to dereference the RCU protected pointer.
>
> That way we could have a safe fallback. If the page is there, we use its
> data, if it is not, we blindly send the INQUIRY like we do today.
>
> Not sure whether this is a bit too paranoid.. VPD page 0 is mandatory
> after all.
That could be done, but the problem would still exist for the PURE target.
We don’t support the page 0xb9, and we don’t advertise we do in the response
to VPD 0. This approach would still lead to the INQUIRY being sent to devices
who don’t support it, don’t expect it, and report an unexpected error. What I am
trying to avoid is the INQUIRY being sent to devices who don’t invite it.
Thanks,
Brian
>
>> + int pos = 0;
>> +
>> + rcu_read_lock();
>> + vpd = rcu_dereference(sdev->vpd_pg0);
>> + if (!vpd)
>> + goto out;
>> +
>> + page_len = get_unaligned_be16(&vpd->data[2]);
>> +
>> + /*
>> + * The first supported page starts at byte 4 in the buffer.
>> + * Read from that byte to the last dictated by page_len above.
>> + */
>> + for (pos = 4; pos < page_len + 4; ++pos) {
>> + if (vpd->data[pos] == page)
>> + ret = 0;
>> + }
>> +
>> +out:
>> + rcu_read_unlock();
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(scsi_vpd_page_supported);
>> +
>> /**
>> * scsi_get_vpd_page - Get Vital Product Data from a SCSI device
>> * @sdev: The device to ask
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 1624d528aa1f..0304b7d60747 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -3127,7 +3127,9 @@ static void sd_read_cpr(struct scsi_disk *sdkp)
>> */
>> buf_len = 64 + 256*32;
>> buffer = kmalloc(buf_len, GFP_KERNEL);
>> - if (!buffer || scsi_get_vpd_page(sdkp->device, 0xb9, buffer, buf_len))
>> + if (!buffer ||
>> + scsi_vpd_page_supported(sdkp->device, 0xb9) ||
>
> Wouldn't it make sense to do this before the allocation? If it really
> turns out to be unsupported, that seems like a waste.
>
>> + scsi_get_vpd_page(sdkp->device, 0xb9, buffer, buf_len))
>> goto out;
>>
>> /* We must have at least a 64B header and one 32B range descriptor */
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index f10a008e5bfa..359cd8b94312 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -431,6 +431,7 @@ extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp,
>> struct scsi_sense_hdr *);
>> extern int scsi_test_unit_ready(struct scsi_device *sdev, int timeout,
>> int retries, struct scsi_sense_hdr *sshdr);
>> +extern int scsi_vpd_page_supported(struct scsi_device *sdev, u8 page);
>> extern int scsi_get_vpd_page(struct scsi_device *, u8 page, unsigned char *buf,
>> int buf_len);
>> extern int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer,
>
> --
> Best Regards, Benjamin Block / Linux on IBM Z Kernel Development
> IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy
> Vors. Aufs.-R.: Gregor Pillen / Geschäftsführung: David Faller
> Sitz der Ges.: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
