diff mbox series

[v1] scsi: Check the SPC version in sd_read_cpr

Message ID 20240720150039.843-1-yohan.joung@sk.com (mailing list archive)
State Rejected
Headers show
Series [v1] scsi: Check the SPC version in sd_read_cpr | expand

Commit Message

Yohan Joung July 20, 2024, 3 p.m. UTC
Add SPC version verification to avoid unnecessary inquiry command

Signed-off-by: Yohan Joung <yohan.joung@sk.com>
---
 drivers/scsi/sd.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Martin K. Petersen July 23, 2024, 12:49 a.m. UTC | #1
Yohan,

> Add SPC version verification to avoid unnecessary inquiry command

Are you experiencing an actual error condition as a result of this
INQUIRY operation?

Devices often adopt new protocol features prior to the spec being
finalized. Therefore we generally use INQUIRY to discover capabilities
rather than relying on the reported spec compliance.

> +	/* Support for CPR was defined in SPC-5. */
> +	if (sdev->scsi_level < SCSI_SPC_5)
> +		return;
> +

I'm OK with the change but I'll defer to Damien as to whether this is an
acceptable heuristic.
Damien Le Moal July 23, 2024, 12:57 a.m. UTC | #2
On 7/23/24 09:49, Martin K. Petersen wrote:
> 
> Yohan,
> 
>> Add SPC version verification to avoid unnecessary inquiry command
> 
> Are you experiencing an actual error condition as a result of this
> INQUIRY operation?
> 
> Devices often adopt new protocol features prior to the spec being
> finalized. Therefore we generally use INQUIRY to discover capabilities
> rather than relying on the reported spec compliance.
> 
>> +	/* Support for CPR was defined in SPC-5. */
>> +	if (sdev->scsi_level < SCSI_SPC_5)
>> +		return;
>> +
> 
> I'm OK with the change but I'll defer to Damien as to whether this is an
> acceptable heuristic.

If this is solving an issue with an ATA device managed by libata, we can handle
that in libata. Otherwise, if this is an ATA device connected to a SAS HBA or a
SAS drive, then the issue could be with the HBA.

I missed this patch so I am not aware of the actual issue this is trying to solve...
Damien Le Moal July 23, 2024, 2:08 a.m. UTC | #3
On 7/23/24 09:49, Martin K. Petersen wrote:
> 
> Yohan,
> 
>> Add SPC version verification to avoid unnecessary inquiry command
> 
> Are you experiencing an actual error condition as a result of this
> INQUIRY operation?
> 
> Devices often adopt new protocol features prior to the spec being
> finalized. Therefore we generally use INQUIRY to discover capabilities
> rather than relying on the reported spec compliance.
> 
>> +	/* Support for CPR was defined in SPC-5. */
>> +	if (sdev->scsi_level < SCSI_SPC_5)
>> +		return;
>> +
> 
> I'm OK with the change but I'll defer to Damien as to whether this is an
> acceptable heuristic.

By the way, that may not be adequate as ATA devices that implement CPR may
advertise an ACS version that does not translate to SPC-5, which is exactly the
point you said.
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6203915945a4..9d71ad24d8e3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3413,11 +3413,16 @@  static inline sector_t sd64_to_sectors(struct scsi_disk *sdkp, u8 *buf)
 static void sd_read_cpr(struct scsi_disk *sdkp)
 {
 	struct blk_independent_access_ranges *iars = NULL;
+	struct scsi_device *sdev = sdkp->device;
 	unsigned char *buffer = NULL;
 	unsigned int nr_cpr = 0;
 	int i, vpd_len, buf_len = SD_BUF_SIZE;
 	u8 *desc;
 
+	/* Support for CPR was defined in SPC-5. */
+	if (sdev->scsi_level < SCSI_SPC_5)
+		return;
+
 	/*
 	 * We need to have the capacity set first for the block layer to be
 	 * able to check the ranges.