On Tue, Feb 13, 2024 at 9:33 AM Martin K. Petersen
<martin.petersen@xxxxxxxxxx> wrote:
>
> It has been observed that some USB/UAS devices return generic
> properties hardcoded in firmware for mode pages and vital product data
> for a period of time after a device has been discovered. The reported
> properties are either garbage or they do not accurately reflect the
> properties of the physical storage device attached in the case of a
> bridge.
>
> Prior to commit 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to
> avoid calling revalidate twice") we would call revalidate several
> times during device discovery. As a result, incorrect values would
> eventually get replaced with ones accurately describing the attached
> storage. When we did away with the redundant revalidate pass, several
> cases were reported where devices reported nonsensical values or would
> end up in write-protected state.
>
> An initial attempt at addressing this issue involved introducing a
> delayed second revalidate invocation. However, this approach still
> left some devices reporting incorrect characteristics.
>
> Tasos Sahanidis debugged the problem further and identified that
> introducing a READ operation prior to MODE SENSE fixed the problem and
> that it wasn't a timing issue. Issuing a READ appears to cause the
> devices to update their SCSI pages to reflect the actual properties of
> the storage media. Device properties like vendor, model, and storage
> capacity appear to be correctly reported from the get-go. It is
> unclear why these device defer populating the remaining
> characteristics.
>
> Match the behavior of a well known commercial operating system and
> trigger a READ operation prior to querying device characteristics to
> force the device to populate mode pages and VPDs.
>
> The additional READ is triggered by a flag set in the USB storage and
> UAS drivers. We avoid issuing the READ for other transport classes
> since some storage devices identify Linux through our particular
> discovery command sequence.
>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Fixes: 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to avoid calling revalidate twice")
> Reported-by: Tasos Sahanidis <tasos@xxxxxxxxxxxx>
> Signed-off-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
> ---
> drivers/scsi/sd.c | 27 ++++++++++++++++++++++++++-
> drivers/usb/storage/scsiglue.c | 7 +++++++
> drivers/usb/storage/uas.c | 7 +++++++
> include/scsi/scsi_device.h | 1 +
> 4 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 530918cbfce2..c284628f702c 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3405,6 +3405,24 @@ static bool sd_validate_opt_xfer_size(struct scsi_disk *sdkp,
> return true;
> }
>
> +static void sd_read_block_zero(struct scsi_disk *sdkp)
> +{
> + unsigned int buf_len = sdkp->device->sector_size;
> + char *buffer, cmd[10] = { };
> +
> + buffer = kmalloc(buf_len, GFP_KERNEL);
> + if (!buffer)
> + return;
> +
> + cmd[0] = READ_10;
> + put_unaligned_be32(0, &cmd[2]); /* Logical block address 0 */
> + put_unaligned_be16(1, &cmd[7]); /* Transfer 1 logical block */
> +
> + scsi_execute_cmd(sdkp->device, cmd, REQ_OP_DRV_IN, buffer, buf_len,
> + SD_TIMEOUT, sdkp->max_retries, NULL);
> + kfree(buffer);
> +}
> +
> /**
> * sd_revalidate_disk - called the first time a new disk is seen,
> * performs disk spin up, read_capacity, etc.
> @@ -3444,7 +3462,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
> */
> if (sdkp->media_present) {
> sd_read_capacity(sdkp, buffer);
> -
> + /*
> + * Some USB/UAS devices return generic values for mode pages
> + * and VPDs until the media has been accessed. Trigger a READ
> + * operation to force the device to populate mode pages and
> + * VPDs.
> + */
> + if (sdp->read_before_ms)
> + sd_read_block_zero(sdkp);
> /*
> * set the default to rotational. All non-rotational devices
> * support the block characteristics VPD page, which will
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index c54e9805da53..12cf9940e5b6 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -179,6 +179,13 @@ static int slave_configure(struct scsi_device *sdev)
> */
> sdev->use_192_bytes_for_3f = 1;
>
> + /*
> + * Some devices report generic values until the media has been
> + * accessed. Force a READ(10) prior to querying device
> + * characteristics.
> + */
> + sdev->read_before_ms = 1;
> +
> /*
> * Some devices don't like MODE SENSE with page=0x3f,
> * which is the command used for checking if a device
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 696bb0b23599..299a6767b7b3 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -878,6 +878,13 @@ static int uas_slave_configure(struct scsi_device *sdev)
> if (devinfo->flags & US_FL_CAPACITY_HEURISTICS)
> sdev->guess_capacity = 1;
>
> + /*
> + * Some devices report generic values until the media has been
> + * accessed. Force a READ(10) prior to querying device
> + * characteristics.
> + */
> + sdev->read_before_ms = 1;
> +
> /*
> * Some devices don't like MODE SENSE with page=0x3f,
> * which is the command used for checking if a device
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 10480eb582b2..cb019c80763b 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -202,6 +202,7 @@ struct scsi_device {
> unsigned use_10_for_rw:1; /* first try 10-byte read / write */
> unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
> unsigned set_dbd_for_ms:1; /* Set "DBD" field in mode sense */
> + unsigned read_before_ms:1; /* perform a READ before MODE SENSE */
> unsigned no_report_opcodes:1; /* no REPORT SUPPORTED OPERATION CODES */
> unsigned no_write_same:1; /* no WRITE SAME command */
> unsigned use_16_for_rw:1; /* Use read/write(16) over read/write(10) */
> --
> 2.42.1
>
>
So the READ CAPACITY is correct but the VPD pages etc are not? OK.
Reviewed-by: Ewan D. Milne <emilne@xxxxxxxxxx>
