| Message ID | 20200430131904.5847-3-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On 2020-04-30 9:18 a.m., Hannes Reinecke wrote: > Add helper functions to retrieve SCSI commands from the reserved > tag pool. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/scsi/scsi_lib.c | 35 +++++++++++++++++++++++++++++++++++ > include/scsi/scsi_device.h | 3 +++ > 2 files changed, 38 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 5358f553f526..d0972744ea7f 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1901,6 +1901,41 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) > blk_mq_free_tag_set(&shost->tag_set); > } > > +/** > + * scsi_get_reserved_cmd - allocate a SCSI command from reserved tags > + * @sdev: SCSI device from which to allocate the command > + * @data_direction: Data direction for the allocated command > + */ > +struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev, > + int data_direction) > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + > + rq = blk_mq_alloc_request(sdev->request_queue, > + data_direction == DMA_TO_DEVICE ? > + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN | REQ_NOWAIT, after consulting: https://en.cppreference.com/w/c/language/operator_precedence I think that the above expression can be written as: (data_direction == DMA_TO_DEVICE) ? REQ_OP_SCSI_OUT : (REQ_OP_SCSI_IN | REQ_NOWAIT), So is REQ_NOWAIT not needed with REQ_OP_SCSI_OUT ? Please note that scripts/checkpatch.pl does _not_ complain when parentheses are used around bitwise operators in complex expressions. And I reckon this expression qualifies as complex. Doug Gilbert Note: in that reference the terniary operator gets this note: "The expression in the middle of the conditional operator (between ? and :) is parsed as if parenthesized: its precedence relative to ?: is ignored". Who knew?
On 2020-04-30 06:18, Hannes Reinecke wrote: > +/** > + * scsi_get_reserved_cmd - allocate a SCSI command from reserved tags > + * @sdev: SCSI device from which to allocate the command > + * @data_direction: Data direction for the allocated command > + */ > +struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev, > + int data_direction) > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + > + rq = blk_mq_alloc_request(sdev->request_queue, > + data_direction == DMA_TO_DEVICE ? > + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN | REQ_NOWAIT, > + BLK_MQ_REQ_RESERVED); > + if (IS_ERR(rq)) > + return NULL; > + scmd = blk_mq_rq_to_pdu(rq); > + scmd->request = rq; > + return scmd; > +} Isn't REQ_NOWAIT something the caller should decide about instead of always setting that flag? Additionally, I think some parentheses are missing. I think the compiler will interpret the blk_mq_alloc_request() call as follows, which is probably not what was intended: rq = blk_mq_alloc_request(sdev->request_queue, data_direction == DMA_TO_DEVICE ? REQ_OP_SCSI_OUT : (REQ_OP_SCSI_IN | REQ_NOWAIT), BLK_MQ_REQ_RESERVED); Thanks, Bart.
On 30/04/2020 14:18, Hannes Reinecke wrote: > + rq = blk_mq_alloc_request(sdev->request_queue, > + data_direction == DMA_TO_DEVICE ? > + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN | REQ_NOWAIT, > + BLK_MQ_REQ_RESERVED); > + if (IS_ERR(rq)) > + return NULL; > + scmd = blk_mq_rq_to_pdu(rq); > + scmd->request = rq; Should we just set scmd->device = sdev also for completeness? Thanks, John > + return scmd; > +} > +EXPORT_SYMBOL_GPL(scsi_get_reserved_cmd);
On 4/30/20 7:11 PM, Douglas Gilbert wrote: > On 2020-04-30 9:18 a.m., Hannes Reinecke wrote: >> Add helper functions to retrieve SCSI commands from the reserved >> tag pool. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> drivers/scsi/scsi_lib.c | 35 +++++++++++++++++++++++++++++++++++ >> include/scsi/scsi_device.h | 3 +++ >> 2 files changed, 38 insertions(+) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 5358f553f526..d0972744ea7f 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -1901,6 +1901,41 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) >> blk_mq_free_tag_set(&shost->tag_set); >> } >> +/** >> + * scsi_get_reserved_cmd - allocate a SCSI command from reserved tags >> + * @sdev: SCSI device from which to allocate the command >> + * @data_direction: Data direction for the allocated command >> + */ >> +struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev, >> + int data_direction) >> +{ >> + struct request *rq; >> + struct scsi_cmnd *scmd; >> + >> + rq = blk_mq_alloc_request(sdev->request_queue, >> + data_direction == DMA_TO_DEVICE ? >> + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN | REQ_NOWAIT, > > after consulting: > https://en.cppreference.com/w/c/language/operator_precedence > I think that the above expression can be written as: > > (data_direction == DMA_TO_DEVICE) ? REQ_OP_SCSI_OUT : (REQ_OP_SCSI_IN | > REQ_NOWAIT), > > So is REQ_NOWAIT not needed with REQ_OP_SCSI_OUT ? > Nope, that's an error on my side. REQ_NOWAIT should be set for both directions. I'll fix it up. Cheers, Hannes
On Thu, Apr 30, 2020 at 03:18:25PM +0200, Hannes Reinecke wrote: > Add helper functions to retrieve SCSI commands from the reserved > tag pool. I'm still quite worried about the fact that we have a pretty much half-initialized command that now goes down the whole stack.
On 5/1/20 7:39 PM, Christoph Hellwig wrote: > On Thu, Apr 30, 2020 at 03:18:25PM +0200, Hannes Reinecke wrote: >> Add helper functions to retrieve SCSI commands from the reserved >> tag pool. > > I'm still quite worried about the fact that we have a pretty much > half-initialized command that now goes down the whole stack. > Reserved commands just serve as a placeholder to get a valid tag from the block layer; the SCSI commands themselves are never ever passed through the whole stack, but rather allocated internally within the driver, and passed to the hardware by driver-specific means. So really the SCSI specific parts of the commands are never used. We can add a check in queuecommand to abort reserved commands if that's what you worried about, though. Cheers, Hannes
On Sat, May 02, 2020 at 10:45:24AM +0200, Hannes Reinecke wrote: > Reserved commands just serve as a placeholder to get a valid tag from the > block layer; the SCSI commands themselves are never ever passed through the > whole stack, but rather allocated internally within the driver, and passed > to the hardware by driver-specific means. > So really the SCSI specific parts of the commands are never used. > We can add a check in queuecommand to abort reserved commands if that's > what you worried about, though. How about an interface that just returns a tag then, so that it can't be misused?
On 5/1/20 2:01 PM, John Garry wrote: > On 30/04/2020 14:18, Hannes Reinecke wrote: >> + rq = blk_mq_alloc_request(sdev->request_queue, >> + data_direction == DMA_TO_DEVICE ? >> + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN | REQ_NOWAIT, >> + BLK_MQ_REQ_RESERVED); >> + if (IS_ERR(rq)) >> + return NULL; >> + scmd = blk_mq_rq_to_pdu(rq); >> + scmd->request = rq; > > Should we just set scmd->device = sdev also for completeness? > Inded, we should. Cheers, Hannes
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 5358f553f526..d0972744ea7f 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1901,6 +1901,41 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) blk_mq_free_tag_set(&shost->tag_set); } +/** + * scsi_get_reserved_cmd - allocate a SCSI command from reserved tags + * @sdev: SCSI device from which to allocate the command + * @data_direction: Data direction for the allocated command + */ +struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev, + int data_direction) +{ + struct request *rq; + struct scsi_cmnd *scmd; + + rq = blk_mq_alloc_request(sdev->request_queue, + data_direction == DMA_TO_DEVICE ? + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN | REQ_NOWAIT, + BLK_MQ_REQ_RESERVED); + if (IS_ERR(rq)) + return NULL; + scmd = blk_mq_rq_to_pdu(rq); + scmd->request = rq; + return scmd; +} +EXPORT_SYMBOL_GPL(scsi_get_reserved_cmd); + +/** + * scsi_put_reserved_cmd - free a SCSI command allocated from reserved tags + * @scmd: SCSI command to be freed + */ +void scsi_put_reserved_cmd(struct scsi_cmnd *scmd) +{ + struct request *rq = blk_mq_rq_from_pdu(scmd); + + blk_mq_free_request(rq); +} +EXPORT_SYMBOL_GPL(scsi_put_reserved_cmd); + /** * scsi_device_from_queue - return sdev associated with a request_queue * @q: The request queue to return the sdev from diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index c3cba2aaf934..e74c7e671aa0 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -457,6 +457,9 @@ static inline int scsi_execute_req(struct scsi_device *sdev, return scsi_execute(sdev, cmd, data_direction, buffer, bufflen, NULL, sshdr, timeout, retries, 0, 0, resid); } +struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev, + int data_direction); +void scsi_put_reserved_cmd(struct scsi_cmnd *scmd); extern void sdev_disable_disk_events(struct scsi_device *sdev); extern void sdev_enable_disk_events(struct scsi_device *sdev); extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);
Add helper functions to retrieve SCSI commands from the reserved tag pool. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/scsi/scsi_lib.c | 35 +++++++++++++++++++++++++++++++++++ include/scsi/scsi_device.h | 3 +++ 2 files changed, 38 insertions(+)