| Message ID | 20240720150039.843-1-yohan.joung@sk.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] scsi: Check the SPC version in sd_read_cpr | expand |
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.
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...
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 --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.
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(+)