| Message ID | 20221209061325.705999-2-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: Add struct for args to execution functions | expand |
On 09/12/2022 06:13, Mike Christie wrote: > Subject: > [PATCH v2 01/15] scsi: Add struct for args to execution functions > From: > Mike Christie <michael.christie@oracle.com> > Date: > 09/12/2022, 06:13 > > To: > john.g.garry@oracle.com, bvanassche@acm.org, mwilck@suse.com, > hch@lst.de, martin.petersen@oracle.com, linux-scsi@vger.kernel.org, > james.bottomley@hansenpartnership.com > CC: > Mike Christie <michael.christie@oracle.com> Generally I think that this looks ok, but I have a bit of a niggle about the sense_len argument, below. > > > This begins to move the SCSI execution functions to use a struct for > passing in optional args. This patch adds the new struct, temporarily > converts scsi_execute and scsi_execute_req and add two helpers: > 1. scsi_execute_args which takes the scsi_exec_args struct. > 2. scsi_execute_cmd does not support the scsi_exec_args struct. > > The next patches will convert scsi_execute and scsi_execute_req users to > the new helpers then remove scsi_execute and scsi_execute_req. > > Signed-off-by: Mike Christie<michael.christie@oracle.com> > --- ... > > /* > * head injection*required* here otherwise quiesce won't work > @@ -249,13 +238,14 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > if (unlikely(scmd->resid_len > 0 && scmd->resid_len <= bufflen)) > memset(buffer + bufflen - scmd->resid_len, 0, scmd->resid_len); > > - if (resid) > - *resid = scmd->resid_len; > - if (sense && scmd->sense_len) > - memcpy(sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); > - if (sshdr) > - scsi_normalize_sense(scmd->sense_buffer, scmd->sense_len, > - sshdr); > + if (args.resid) > + *args.resid = scmd->resid_len; > + if (args.sense && scmd->sense_len) I am not sure that you require the sense_len check as you effectively have that same check in scsi_execute_args(), which is the only caller which would have args.sense set. But I suppose __scsi_execute() is still a public API (so should still check); but, by that same token, we have no sanity check for args.sense_len value here then. Is it possible to make __scsi_execute() non-public or move/add the check for proper sense_len here? I'm being extra cautious about this, I suppose. > + memcpy(args.sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); > + if (args.sshdr) > + scsi_normalize_sense(scmd->sense_buffer, > + scmd->sense_len, args.sshdr); > + > ret = scmd->result; > out: > blk_mq_free_request(req); > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h > index 3642b8e3928b..eb960aa73b3b 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h ... > + > +#define scsi_execute_cmd(sdev, cmd, opf, buffer, bufflen, timeout, \ > + retries) \ > +({ \ > + struct scsi_exec_args exec_args = {}; \ nit: I think that this can be static const, but no biggie > + \ > + __scsi_execute(sdev, cmd, opf, buffer, bufflen, timeout, \ > + retries, exec_args); \ > +}) > +
On 12/9/22 4:40 AM, John Garry wrote: >> * head injection*required* here otherwise quiesce won't work >> @@ -249,13 +238,14 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, >> if (unlikely(scmd->resid_len > 0 && scmd->resid_len <= bufflen)) >> memset(buffer + bufflen - scmd->resid_len, 0, scmd->resid_len); >> - if (resid) >> - *resid = scmd->resid_len; >> - if (sense && scmd->sense_len) >> - memcpy(sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); >> - if (sshdr) >> - scsi_normalize_sense(scmd->sense_buffer, scmd->sense_len, >> - sshdr); >> + if (args.resid) >> + *args.resid = scmd->resid_len; >> + if (args.sense && scmd->sense_len) > > I am not sure that you require the sense_len check as you effectively have that same check in scsi_execute_args(), which is the only caller which would have args.sense set. But I suppose __scsi_execute() is still a public API (so should still check); but, by that same token, we have no sanity check for args.sense_len value here then. Is it possible to make __scsi_execute() non-public or move/add the check for proper sense_len here? I'm being extra cautious about this, I suppose. Do people want the BUILD_BUG_ON we have now or a WARN/BUG? If we move to a single check in __scsi_execute or some non-macro wrapper around it then we have to do a WARN/BUG. We do the macro approach now so we can do the BUILD_BUG_ON.
On 12/9/22 11:15 AM, Mike Christie wrote: > On 12/9/22 4:40 AM, John Garry wrote: >>> * head injection*required* here otherwise quiesce won't work >>> @@ -249,13 +238,14 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, >>> if (unlikely(scmd->resid_len > 0 && scmd->resid_len <= bufflen)) >>> memset(buffer + bufflen - scmd->resid_len, 0, scmd->resid_len); >>> - if (resid) >>> - *resid = scmd->resid_len; >>> - if (sense && scmd->sense_len) >>> - memcpy(sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); >>> - if (sshdr) >>> - scsi_normalize_sense(scmd->sense_buffer, scmd->sense_len, >>> - sshdr); >>> + if (args.resid) >>> + *args.resid = scmd->resid_len; >>> + if (args.sense && scmd->sense_len) >> >> I am not sure that you require the sense_len check as you effectively have that same check in scsi_execute_args(), which is the only caller which would have args.sense set. But I suppose __scsi_execute() is still a public API (so should still check); but, by that same token, we have no sanity check for args.sense_len value here then. Is it possible to make __scsi_execute() non-public or move/add the check for proper sense_len here? I'm being extra cautious about this, I suppose. > Do people want the BUILD_BUG_ON we have now or a WARN/BUG? > > If we move to a single check in __scsi_execute or some non-macro wrapper > around it then we have to do a WARN/BUG. We do the macro approach now > so we can do the BUILD_BUG_ON. Maybe we have to switch to a WARN/BUG. It looks like some compilers don't like: const struct scsi_exec_args exec_args = { .sshdr = &sshdr, }; scsi_execute_args(.... exec_args); and will hit the: #define scsi_execute_args(sdev, cmd, opf, buffer, bufflen, timeout, \ retries, args) \ ({ \ BUILD_BUG_ON(args.sense && \ args.sense_len != SCSI_SENSE_BUFFERSIZE); \ because the args's sense and sense_len are not cleared yet.
On 12/9/22 10:37, Mike Christie wrote: > On 12/9/22 11:15 AM, Mike Christie wrote: >> On 12/9/22 4:40 AM, John Garry wrote: >>>> * head injection*required* here otherwise quiesce won't work >>>> @@ -249,13 +238,14 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, >>>> if (unlikely(scmd->resid_len > 0 && scmd->resid_len <= bufflen)) >>>> memset(buffer + bufflen - scmd->resid_len, 0, scmd->resid_len); >>>> - if (resid) >>>> - *resid = scmd->resid_len; >>>> - if (sense && scmd->sense_len) >>>> - memcpy(sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); >>>> - if (sshdr) >>>> - scsi_normalize_sense(scmd->sense_buffer, scmd->sense_len, >>>> - sshdr); >>>> + if (args.resid) >>>> + *args.resid = scmd->resid_len; >>>> + if (args.sense && scmd->sense_len) >>> >>> I am not sure that you require the sense_len check as you effectively have that same check in scsi_execute_args(), which is the only caller which would have args.sense set. But I suppose __scsi_execute() is still a public API (so should still check); but, by that same token, we have no sanity check for args.sense_len value here then. Is it possible to make __scsi_execute() non-public or move/add the check for proper sense_len here? I'm being extra cautious about this, I suppose. >> Do people want the BUILD_BUG_ON we have now or a WARN/BUG? >> >> If we move to a single check in __scsi_execute or some non-macro wrapper >> around it then we have to do a WARN/BUG. We do the macro approach now >> so we can do the BUILD_BUG_ON. > > Maybe we have to switch to a WARN/BUG. > > It looks like some compilers don't like: > > const struct scsi_exec_args exec_args = { > .sshdr = &sshdr, > }; > > scsi_execute_args(.... exec_args); > > and will hit the: > > #define scsi_execute_args(sdev, cmd, opf, buffer, bufflen, timeout, \ > retries, args) \ > ({ \ > BUILD_BUG_ON(args.sense && \ > args.sense_len != SCSI_SENSE_BUFFERSIZE); \ > > because the args's sense and sense_len are not cleared yet. My understanding is that the __scsi_execute() macro was introduced to prevent that every single scsi_execute() caller would have to be modified. I'm fine with removing the BUILD_BUG_ON(sense_len != SCSI_SENSE_BUFFER_SIZE) check and replacing it with a WARN_ON_ONCE() statement, e.g. inside __scsi_execute(). Thanks, Bart.
On 09/12/2022 18:47, Bart Van Assche wrote: >>> around it then we have to do a WARN/BUG. We do the macro approach now >>> so we can do the BUILD_BUG_ON. >> >> Maybe we have to switch to a WARN/BUG. >> >> It looks like some compilers don't like: >> >> const struct scsi_exec_args exec_args = { >> .sshdr = &sshdr, >> }; >> >> scsi_execute_args(.... exec_args); >> >> and will hit the: >> >> #define scsi_execute_args(sdev, cmd, opf, buffer, bufflen, timeout, \ >> retries, >> args) \ >> ({ \ >> BUILD_BUG_ON(args.sense >> && \ >> args.sense_len != >> SCSI_SENSE_BUFFERSIZE); \ >> >> because the args's sense and sense_len are not cleared yet. > > My understanding is that the __scsi_execute() macro was introduced to > prevent that every single scsi_execute() caller would have to be > modified. I'm fine with removing the BUILD_BUG_ON(sense_len != > SCSI_SENSE_BUFFER_SIZE) check and replacing it with a WARN_ON_ONCE() > statement, e.g. inside __scsi_execute(). Another option is to have __scsi_execute() allocate the sense buf by kmemdup, and hold the sense pointer as unsigned char ** in struct scsi_exec_args; but then the caller needs to kfree the allocated sense buf, which I suppose is less than ideal. However there is only single driver which uses this AFAICS. Thanks, John
On 12/12/22 10:17 AM, John Garry wrote: > On 09/12/2022 18:47, Bart Van Assche wrote: >>>> around it then we have to do a WARN/BUG. We do the macro approach now >>>> so we can do the BUILD_BUG_ON. >>> >>> Maybe we have to switch to a WARN/BUG. >>> >>> It looks like some compilers don't like: >>> >>> const struct scsi_exec_args exec_args = { >>> .sshdr = &sshdr, >>> }; >>> >>> scsi_execute_args(.... exec_args); >>> >>> and will hit the: >>> >>> #define scsi_execute_args(sdev, cmd, opf, buffer, bufflen, timeout, \ >>> retries, args) \ >>> ({ \ >>> BUILD_BUG_ON(args.sense && \ >>> args.sense_len != SCSI_SENSE_BUFFERSIZE); \ >>> >>> because the args's sense and sense_len are not cleared yet. >> >> My understanding is that the __scsi_execute() macro was introduced to prevent that every single scsi_execute() caller would have to be modified. I'm fine with removing the BUILD_BUG_ON(sense_len != SCSI_SENSE_BUFFER_SIZE) check and replacing it with a WARN_ON_ONCE() statement, e.g. inside __scsi_execute(). > > Another option is to have __scsi_execute() allocate the sense buf by kmemdup, and hold the sense pointer as unsigned char ** in struct scsi_exec_args; but then the caller needs to kfree the allocated sense buf, which I suppose is less than ideal. However there is only single driver which uses this AFAICS. I did the WARN_ON_ONCE.
On 12/8/22 22:13, Mike Christie wrote: > This begins to move the SCSI execution functions to use a struct for > passing in optional args. This patch adds the new struct, temporarily > converts scsi_execute and scsi_execute_req and add two helpers: > 1. scsi_execute_args which takes the scsi_exec_args struct. > 2. scsi_execute_cmd does not support the scsi_exec_args struct. ^^^ which? > @@ -232,8 +222,7 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > memcpy(scmd->cmnd, cmd, scmd->cmd_len); > scmd->allowed = retries; > req->timeout = timeout; > - req->cmd_flags |= flags; > - req->rq_flags |= rq_flags | RQF_QUIET; > + req->rq_flags |= RQF_QUIET; My understanding is that neither scsi_alloc_request() nor any of the functions it calls copies its 'flags' argument into req->rq_flags. I think this is a behavior change that has not been described in the patch description. I'm not saying that this change is wrong but that careful review is required if this change is retained. Thanks, Bart.
On 12/12/22 1:45 PM, Bart Van Assche wrote: > On 12/8/22 22:13, Mike Christie wrote: >> This begins to move the SCSI execution functions to use a struct for >> passing in optional args. This patch adds the new struct, temporarily >> converts scsi_execute and scsi_execute_req and add two helpers: >> 1. scsi_execute_args which takes the scsi_exec_args struct. >> 2. scsi_execute_cmd does not support the scsi_exec_args struct. > ^^^ > which? > >> @@ -232,8 +222,7 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, >> memcpy(scmd->cmnd, cmd, scmd->cmd_len); >> scmd->allowed = retries; >> req->timeout = timeout; >> - req->cmd_flags |= flags; >> - req->rq_flags |= rq_flags | RQF_QUIET; >> + req->rq_flags |= RQF_QUIET; > > My understanding is that neither scsi_alloc_request() nor any of the > functions it calls copies its 'flags' argument into req->rq_flags. I > think this is a behavior change that has not been described in the > patch description. I'm not saying that this change is wrong but that > careful review is required if this change is retained. It's not directly copied but we only used the one flag RQF_PM. The new code has us pass in the BLK_MQ flag which is used by blk_mq_alloc_request. Those flags we pass blk_mq_alloc_request eventually get set to blk_mq_alloc_data->flags so when blk_mq_rq_ctx_init is called it checks for its BLK_MQ flags and does BLK_MQ_REQ_PM: if (data->flags & BLK_MQ_REQ_PM) data->rq_flags |= RQF_PM; ... rq->rq_flags = data->rq_flags; Note that if you are scanning the code and see the scsi_dh module's req_flags, the variable name was misleading as they were really the blk_opf_t flags.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index a29d87e57430..f76acb468abb 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -185,39 +185,29 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason) __scsi_queue_insert(cmd, reason, true); } - /** * __scsi_execute - insert request and wait for the result - * @sdev: scsi device + * @sdev: scsi_device * @cmd: scsi command - * @data_direction: data direction + * @opf: block layer request cmd_flags * @buffer: data buffer * @bufflen: len of buffer - * @sense: optional sense buffer - * @sshdr: optional decoded sense header * @timeout: request timeout in HZ * @retries: number of times to retry request - * @flags: flags for ->cmd_flags - * @rq_flags: flags for ->rq_flags - * @resid: optional residual length + * @args: Optional args. See struct definition for field descriptions * * Returns the scsi_cmnd result field if a command was executed, or a negative * Linux error code if we didn't get that far. */ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, - int data_direction, void *buffer, unsigned bufflen, - unsigned char *sense, struct scsi_sense_hdr *sshdr, - int timeout, int retries, blk_opf_t flags, - req_flags_t rq_flags, int *resid) + blk_opf_t opf, void *buffer, unsigned int bufflen, + int timeout, int retries, const struct scsi_exec_args args) { struct request *req; struct scsi_cmnd *scmd; int ret; - req = scsi_alloc_request(sdev->request_queue, - data_direction == DMA_TO_DEVICE ? - REQ_OP_DRV_OUT : REQ_OP_DRV_IN, - rq_flags & RQF_PM ? BLK_MQ_REQ_PM : 0); + req = scsi_alloc_request(sdev->request_queue, opf, args.req_flags); if (IS_ERR(req)) return PTR_ERR(req); @@ -232,8 +222,7 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, memcpy(scmd->cmnd, cmd, scmd->cmd_len); scmd->allowed = retries; req->timeout = timeout; - req->cmd_flags |= flags; - req->rq_flags |= rq_flags | RQF_QUIET; + req->rq_flags |= RQF_QUIET; /* * head injection *required* here otherwise quiesce won't work @@ -249,13 +238,14 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, if (unlikely(scmd->resid_len > 0 && scmd->resid_len <= bufflen)) memset(buffer + bufflen - scmd->resid_len, 0, scmd->resid_len); - if (resid) - *resid = scmd->resid_len; - if (sense && scmd->sense_len) - memcpy(sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); - if (sshdr) - scsi_normalize_sense(scmd->sense_buffer, scmd->sense_len, - sshdr); + if (args.resid) + *args.resid = scmd->resid_len; + if (args.sense && scmd->sense_len) + memcpy(args.sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); + if (args.sshdr) + scsi_normalize_sense(scmd->sense_buffer, + scmd->sense_len, args.sshdr); + ret = scmd->result; out: blk_mq_free_request(req); diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 3642b8e3928b..eb960aa73b3b 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -455,28 +455,70 @@ extern const char *scsi_device_state_name(enum scsi_device_state); extern int scsi_is_sdev_device(const struct device *); extern int scsi_is_target_device(const struct device *); extern void scsi_sanitize_inquiry_string(unsigned char *s, int len); -extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, - int data_direction, void *buffer, unsigned bufflen, - unsigned char *sense, struct scsi_sense_hdr *sshdr, - int timeout, int retries, blk_opf_t flags, - req_flags_t rq_flags, int *resid); + +/* Optional arguments to __scsi_execute */ +struct scsi_exec_args { + unsigned char *sense; /* sense buffer */ + 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 */ + int *resid; /* residual length */ +}; + +int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, + blk_opf_t opf, void *buffer, unsigned int bufflen, + int timeout, int retries, const struct scsi_exec_args args); + +#define scsi_execute_args(sdev, cmd, opf, buffer, bufflen, timeout, \ + retries, args) \ +({ \ + BUILD_BUG_ON(args.sense && \ + args.sense_len != SCSI_SENSE_BUFFERSIZE); \ + __scsi_execute(sdev, cmd, opf, buffer, bufflen, timeout, \ + retries, args); \ +}) + +#define scsi_execute_cmd(sdev, cmd, opf, buffer, bufflen, timeout, \ + retries) \ +({ \ + struct scsi_exec_args exec_args = {}; \ + \ + __scsi_execute(sdev, cmd, opf, buffer, bufflen, timeout, \ + retries, exec_args); \ +}) + /* Make sure any sense buffer is the correct size. */ -#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense, \ - sshdr, timeout, retries, flags, rq_flags, resid) \ +#define scsi_execute(_sdev, _cmd, _data_dir, _buffer, _bufflen, _sense, \ + _sshdr, _timeout, _retries, _flags, _rq_flags, \ + _resid) \ ({ \ - BUILD_BUG_ON((sense) != NULL && \ - sizeof(sense) != SCSI_SENSE_BUFFERSIZE); \ - __scsi_execute(sdev, cmd, data_direction, buffer, bufflen, \ - sense, sshdr, timeout, retries, flags, rq_flags, \ - resid); \ + BUILD_BUG_ON((_sense) != NULL && \ + sizeof(_sense) != SCSI_SENSE_BUFFERSIZE); \ + __scsi_execute(_sdev, _cmd, (_data_dir == DMA_TO_DEVICE ? \ + REQ_OP_DRV_OUT : REQ_OP_DRV_IN) | _flags, \ + _buffer, _bufflen, _timeout, _retries, \ + (struct scsi_exec_args) { \ + .sense = _sense, \ + .sshdr = _sshdr, \ + .req_flags = _rq_flags & RQF_PM ? \ + BLK_MQ_REQ_PM : 0, \ + .resid = _resid, \ + }); \ }) + static inline int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd, int data_direction, void *buffer, unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout, int retries, int *resid) { - return scsi_execute(sdev, cmd, data_direction, buffer, - bufflen, NULL, sshdr, timeout, retries, 0, 0, resid); + return __scsi_execute(sdev, cmd, + data_direction == DMA_TO_DEVICE ? + REQ_OP_DRV_OUT : REQ_OP_DRV_IN, buffer, + bufflen, timeout, retries, + (struct scsi_exec_args) { + .sshdr = sshdr, + .resid = resid, + }); } extern void sdev_disable_disk_events(struct scsi_device *sdev); extern void sdev_enable_disk_events(struct scsi_device *sdev);
This begins to move the SCSI execution functions to use a struct for passing in optional args. This patch adds the new struct, temporarily converts scsi_execute and scsi_execute_req and add two helpers: 1. scsi_execute_args which takes the scsi_exec_args struct. 2. scsi_execute_cmd does not support the scsi_exec_args struct. The next patches will convert scsi_execute and scsi_execute_req users to the new helpers then remove scsi_execute and scsi_execute_req. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/scsi/scsi_lib.c | 40 ++++++++-------------- include/scsi/scsi_device.h | 70 ++++++++++++++++++++++++++++++-------- 2 files changed, 71 insertions(+), 39 deletions(-)