| Message ID | 20230209185328.2762796-2-bvanassche@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Simplify ufshcd_execute_start_stop() | expand |
On 09/02/2023 18:53, Bart Van Assche wrote: > Allow SCSI LLDs to specify SCMD_* flags. > > Cc: Mike Christie <michael.christie@oracle.com> > Cc: John Garry <john.g.garry@oracle.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Just a couple of nits, below: Reviewed-by: John Garry <john.g.garry@oracle.com> > --- > drivers/scsi/scsi_lib.c | 1 + > include/scsi/scsi_device.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index abe93ec8b7d0..b7c569a42aa4 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -229,6 +229,7 @@ int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd, > scmd->cmd_len = COMMAND_SIZE(cmd[0]); > memcpy(scmd->cmnd, cmd, scmd->cmd_len); > scmd->allowed = retries; > + scmd->flags |= args->scmd_flags; nit: we zero the scsi_cmnd payload in scsi_alloc_request() -> scsi_initialize_rq(), so don't really need the bitwise OR. However it may be better to keep for change of scsi_initialize_rq() changing in terms of init. > req->timeout = timeout; > req->rq_flags |= RQF_QUIET; > > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h > index 7e95ec45138f..c7bfb1f5a8e7 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -462,6 +462,7 @@ struct scsi_exec_args { > unsigned int sense_len; /* sense buffer len */ > struct scsi_sense_hdr *sshdr; /* decoded sense header */ > blk_mq_req_flags_t req_flags; /* BLK_MQ_REQ flags */ > + unsigned int scmd_flags; /* SCMD flags */ nit: scsi_cmnd.flags is an int, so prob should keep the same type > int *resid; /* residual length */ > }; >
On 2/10/23 01:40, John Garry wrote: > On 09/02/2023 18:53, Bart Van Assche wrote: >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index abe93ec8b7d0..b7c569a42aa4 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -229,6 +229,7 @@ int scsi_execute_cmd(struct scsi_device *sdev, >> const unsigned char *cmd, >> scmd->cmd_len = COMMAND_SIZE(cmd[0]); >> memcpy(scmd->cmnd, cmd, scmd->cmd_len); >> scmd->allowed = retries; >> + scmd->flags |= args->scmd_flags; > > nit: we zero the scsi_cmnd payload in scsi_alloc_request() -> > scsi_initialize_rq(), so don't really need the bitwise OR. However it > may be better to keep for change of scsi_initialize_rq() changing in > terms of init. Hi John, Thanks for the review. If scmd->flags would be initialized to a non-zero value in the future then it would be non-trivial to remember that the above assignment would have to be changed into a logical or. So I'd like to keep the logical or. >> req->timeout = timeout; >> req->rq_flags |= RQF_QUIET; >> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h >> index 7e95ec45138f..c7bfb1f5a8e7 100644 >> --- a/include/scsi/scsi_device.h >> +++ b/include/scsi/scsi_device.h >> @@ -462,6 +462,7 @@ struct scsi_exec_args { >> unsigned int sense_len; /* sense buffer len */ >> struct scsi_sense_hdr *sshdr; /* decoded sense header */ >> blk_mq_req_flags_t req_flags; /* BLK_MQ_REQ flags */ >> + unsigned int scmd_flags; /* SCMD flags */ > > nit: scsi_cmnd.flags is an int, so prob should keep the same type How about changing the type of scsi_cmnd.flags from 'int' into 'unsigned int'? I can't think of any reason to make a flags variable signed instead of unsigned. Thanks, Bart.
On 10/02/2023 16:14, Bart Van Assche wrote: > If scmd->flags would be initialized to a non-zero value in the future > then it would be non-trivial to remember that the above assignment would > have to be changed into a logical or. So I'd like to keep the logical or. ok > >>> req->timeout = timeout; >>> req->rq_flags |= RQF_QUIET; >>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h >>> index 7e95ec45138f..c7bfb1f5a8e7 100644 >>> --- a/include/scsi/scsi_device.h >>> +++ b/include/scsi/scsi_device.h >>> @@ -462,6 +462,7 @@ struct scsi_exec_args { >>> unsigned int sense_len; /* sense buffer len */ >>> struct scsi_sense_hdr *sshdr; /* decoded sense header */ >>> blk_mq_req_flags_t req_flags; /* BLK_MQ_REQ flags */ >>> + unsigned int scmd_flags; /* SCMD flags */ >> >> nit: scsi_cmnd.flags is an int, so prob should keep the same type > > How about changing the type of scsi_cmnd.flags from 'int' into 'unsigned > int'? I can't think of any reason to make a flags variable signed > instead of unsigned. Yeah, it is odd to have a signed flags, but it's not the only one in scsi_cmnd. You suggest a reasonable change, however scsi core structures have many odd member types already.. so I'll leave it to you to decide on the proposed change. Thanks, John
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index abe93ec8b7d0..b7c569a42aa4 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -229,6 +229,7 @@ int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd, scmd->cmd_len = COMMAND_SIZE(cmd[0]); memcpy(scmd->cmnd, cmd, scmd->cmd_len); scmd->allowed = retries; + scmd->flags |= args->scmd_flags; req->timeout = timeout; req->rq_flags |= RQF_QUIET; diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 7e95ec45138f..c7bfb1f5a8e7 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -462,6 +462,7 @@ struct scsi_exec_args { unsigned int sense_len; /* sense buffer len */ struct scsi_sense_hdr *sshdr; /* decoded sense header */ blk_mq_req_flags_t req_flags; /* BLK_MQ_REQ flags */ + unsigned int scmd_flags; /* SCMD flags */ int *resid; /* residual length */ };
Allow SCSI LLDs to specify SCMD_* flags. Cc: Mike Christie <michael.christie@oracle.com> Cc: John Garry <john.g.garry@oracle.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/scsi_lib.c | 1 + include/scsi/scsi_device.h | 1 + 2 files changed, 2 insertions(+)