Message ID | 20240819090934.2130592-1-liyihang9@huawei.com |
---|---|
State | New |
Headers | show |
Series | [v5] scsi: sd: Ignore command SYNC CACHE error if format in progress | expand |
On 8/19/24 2:09 AM, Yihang Li wrote:
> + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) ||
Shouldn't symbolic names be introduced for these numeric constants?
Although there is more code in the SCSI core that compares ASC / ASCQ
values with numeric constants, I think we need symbolic names for these
constants to make code like the above easier to read. There is already
a header file for definitions that come directly from the SCSI standard
and that is used by both SCSI initiator and SCSI target code:
<scsi/scsi_proto.h>.
Thanks,
Bart.
On 8/20/24 01:59, Bart Van Assche wrote: > On 8/19/24 2:09 AM, Yihang Li wrote: >> + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) || > > Shouldn't symbolic names be introduced for these numeric constants? > Although there is more code in the SCSI core that compares ASC / ASCQ > values with numeric constants, I think we need symbolic names for these > constants to make code like the above easier to read. There is already > a header file for definitions that come directly from the SCSI standard > and that is used by both SCSI initiator and SCSI target code: > <scsi/scsi_proto.h>. That would be *a lot* to define... So in keeping with the current practice of using numerical values + comment documenting what the values are, it is why I suggested the comment change above this also as the asc/ascq names for this are added. It would be odd to have macros for just this asc/ascq here with so many other places using numerical values... Note: I personally find https://www.t10.org/lists/1spc-lst.htm more than enough to work with the scsi code. If we have to define macros for all this, that would be a lot of lines... > > Thanks, > > Bart.
On 2024/8/20 0:59, Bart Van Assche wrote: > On 8/19/24 2:09 AM, Yihang Li wrote: >> + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) || > > Shouldn't symbolic names be introduced for these numeric constants? > Although there is more code in the SCSI core that compares ASC / ASCQ > values with numeric constants, I think we need symbolic names for these > constants to make code like the above easier to read. There is already > a header file for definitions that come directly from the SCSI standard > and that is used by both SCSI initiator and SCSI target code: > <scsi/scsi_proto.h>. > My idea is to be consistent with the style of the code context. That's why I use numerical values. If we want to use symbolic names to replace all numeric constants, I think that would be another series of patches, and the changes would be more. Thanks, Yihang.
On 2024/8/20 0:54, Bart Van Assche wrote: > On 8/19/24 3:57 AM, Damien Le Moal wrote: >> The patch changed significantly, so I do not think you can retain Bart's review >> tag... > > Agreed, my Reviewed-by definitely should have been removed. > Sorry about that forgot remove your Reviewed-by tag.
On 8/19/24 4:19 PM, Damien Le Moal wrote: > On 8/20/24 01:59, Bart Van Assche wrote: >> On 8/19/24 2:09 AM, Yihang Li wrote: >>> + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) || >> >> Shouldn't symbolic names be introduced for these numeric constants? >> Although there is more code in the SCSI core that compares ASC / ASCQ >> values with numeric constants, I think we need symbolic names for these >> constants to make code like the above easier to read. There is already >> a header file for definitions that come directly from the SCSI standard >> and that is used by both SCSI initiator and SCSI target code: >> <scsi/scsi_proto.h>. > > That would be *a lot* to define... I meant introducing symbolic names only for the numerical constants that occur in this patch. Anyway, I'm fine with a descriptive comment too. Bart.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index adeaa8ab9951..2d7240a24b52 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1823,13 +1823,15 @@ static int sd_sync_cache(struct scsi_disk *sdkp) (sshdr.asc == 0x74 && sshdr.ascq == 0x71)) /* drive is password locked */ /* this is no error here */ return 0; + /* - * This drive doesn't support sync and there's not much - * we can do because this is called during shutdown - * or suspend so just return success so those operations - * can proceed. + * If a format is in progress or if the drive does not + * support sync, there is not much we can do because + * this is called during shutdown or suspend so just + * return success so those operations can proceed. */ - if (sshdr.sense_key == ILLEGAL_REQUEST) + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) || + sshdr.sense_key == ILLEGAL_REQUEST) return 0; }