| Message ID | 20200625140124.17201-4-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On 2020-06-25 07:01, Hannes Reinecke wrote: > +/** > + * scsi_get_internal_cmd - allocate an intenral SCSI command ^^^^^^^^ internal? > + * @sdev: SCSI device from which to allocate the command > + * @data_direction: Data direction for the allocated command > + * @op_flags: request allocation flags > + * > + * Allocates a SCSI command for internal LLDD use. > + */ > +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, > + int data_direction, int op_flags) How about using enum dma_data_direction for data_direction and unsigned int, or even better, a new __bitwise type for op_flags? > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + blk_mq_req_flags_t flags = 0; > + unsigned int op = REQ_INTERNAL | op_flags; > + > + op |= (data_direction == DMA_TO_DEVICE) ? > + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN; > + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); > + if (IS_ERR(rq)) > + return NULL; > + scmd = blk_mq_rq_to_pdu(rq); > + scmd->request = rq; > + scmd->device = sdev; > + return scmd; > +} > +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); Since the 'flags' variable always has the value 0, can it be left out? > +/** > + * scsi_put_internal_cmd - free an internal SCSI command > + * @scmd: SCSI command to be freed > + */ > +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) > +{ > + struct request *rq = blk_mq_rq_from_pdu(scmd); > + > + if (blk_rq_is_internal(rq)) > + blk_mq_free_request(rq); > +} > +EXPORT_SYMBOL_GPL(scsi_put_internal_cmd); How about triggering a warning for the !blk_rq_is_internal(rq) case instead of silently ignoring regular SCSI commands? Thanks, Bart.
On 6/28/20 5:48 AM, Bart Van Assche wrote: > On 2020-06-25 07:01, Hannes Reinecke wrote: >> +/** >> + * scsi_get_internal_cmd - allocate an intenral SCSI command > ^^^^^^^^ > internal? >> + * @sdev: SCSI device from which to allocate the command >> + * @data_direction: Data direction for the allocated command >> + * @op_flags: request allocation flags >> + * >> + * Allocates a SCSI command for internal LLDD use. >> + */ >> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, >> + int data_direction, int op_flags) > > How about using enum dma_data_direction for data_direction and unsigned > int, or even better, a new __bitwise type for op_flags? > Okay for data direction, but converting op_flags into __bitwise (or even a new type) should be relegated to a different patchset. >> +{ >> + struct request *rq; >> + struct scsi_cmnd *scmd; >> + blk_mq_req_flags_t flags = 0; >> + unsigned int op = REQ_INTERNAL | op_flags; >> + >> + op |= (data_direction == DMA_TO_DEVICE) ? >> + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN; >> + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); >> + if (IS_ERR(rq)) >> + return NULL; >> + scmd = blk_mq_rq_to_pdu(rq); >> + scmd->request = rq; >> + scmd->device = sdev; >> + return scmd; >> +} >> +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); > > Since the 'flags' variable always has the value 0, can it be left out? > Yep, true. >> +/** >> + * scsi_put_internal_cmd - free an internal SCSI command >> + * @scmd: SCSI command to be freed >> + */ >> +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) >> +{ >> + struct request *rq = blk_mq_rq_from_pdu(scmd); >> + >> + if (blk_rq_is_internal(rq)) >> + blk_mq_free_request(rq); >> +} >> +EXPORT_SYMBOL_GPL(scsi_put_internal_cmd); > > How about triggering a warning for the !blk_rq_is_internal(rq) case > instead of silently ignoring regular SCSI commands? > That's by design. Some drivers have a common routine for freeing up commands, so it'd be quite tricky to separate these two cases out at the driver level. So it's far easier to call the common routine for all commands, and let this function do the right thing for all commands. Cheers, Hannes
On 2020-06-28 02:02, Hannes Reinecke wrote: > On 6/28/20 5:48 AM, Bart Van Assche wrote: >> On 2020-06-25 07:01, Hannes Reinecke wrote: >>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, >>> + int data_direction, int op_flags) >> >> How about using enum dma_data_direction for data_direction and unsigned >> int, or even better, a new __bitwise type for op_flags? >> > Okay for data direction, but converting op_flags into __bitwise (or even > a new type) should be relegated to a different patchset. OK. >>> +/** >>> + * scsi_put_internal_cmd - free an internal SCSI command >>> + * @scmd: SCSI command to be freed >>> + */ >>> +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) >>> +{ >>> + struct request *rq = blk_mq_rq_from_pdu(scmd); >>> + >>> + if (blk_rq_is_internal(rq)) >>> + blk_mq_free_request(rq); >>> +} >>> +EXPORT_SYMBOL_GPL(scsi_put_internal_cmd); >> >> How about triggering a warning for the !blk_rq_is_internal(rq) case >> instead of silently ignoring regular SCSI commands? >> > That's by design. > Some drivers have a common routine for freeing up commands, so it'd be > quite tricky to separate these two cases out at the driver level. > So it's far easier to call the common routine for all commands, and > let this function do the right thing for all commands. That sounds fair to me, but is an example available in this patch series of a call to scsi_put_internal_cmd() from such a common routine? It seems to me that all calls to scsi_put_internal_cmd() introduced in this patch series happen from code paths that handle internal commands only? Thanks, Bart.
On 6/28/20 5:08 PM, Bart Van Assche wrote: > On 2020-06-28 02:02, Hannes Reinecke wrote: >> On 6/28/20 5:48 AM, Bart Van Assche wrote: >>> On 2020-06-25 07:01, Hannes Reinecke wrote: >>>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, >>>> + int data_direction, int op_flags) >>> >>> How about using enum dma_data_direction for data_direction and unsigned >>> int, or even better, a new __bitwise type for op_flags? >>> >> Okay for data direction, but converting op_flags into __bitwise (or even >> a new type) should be relegated to a different patchset. > > OK. > >>>> +/** >>>> + * scsi_put_internal_cmd - free an internal SCSI command >>>> + * @scmd: SCSI command to be freed >>>> + */ >>>> +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) >>>> +{ >>>> + struct request *rq = blk_mq_rq_from_pdu(scmd); >>>> + >>>> + if (blk_rq_is_internal(rq)) >>>> + blk_mq_free_request(rq); >>>> +} >>>> +EXPORT_SYMBOL_GPL(scsi_put_internal_cmd); >>> >>> How about triggering a warning for the !blk_rq_is_internal(rq) case >>> instead of silently ignoring regular SCSI commands? >>> >> That's by design. >> Some drivers have a common routine for freeing up commands, so it'd be >> quite tricky to separate these two cases out at the driver level. >> So it's far easier to call the common routine for all commands, and >> let this function do the right thing for all commands. > > That sounds fair to me, but is an example available in this patch series > of a call to scsi_put_internal_cmd() from such a common routine? It > seems to me that all calls to scsi_put_internal_cmd() introduced in this > patch series happen from code paths that handle internal commands only? > aacraid. The function aac_fib_free() is called unconditionally for every fib, and doesn't have the means to differentiate between 'normal' and 'internal' commands. Cheers, Hannes
On 29/06/2020 07:32, Hannes Reinecke wrote: >> >> That sounds fair to me, but is an example available in this patch series >> of a call to scsi_put_internal_cmd() from such a common routine? It >> seems to me that all calls to scsi_put_internal_cmd() introduced in this >> patch series happen from code paths that handle internal commands only? >> Ah, I commented on the same thing in your latest series. > aacraid. > The function aac_fib_free() is called unconditionally for every fib, and > doesn't have the means to differentiate between 'normal' and 'internal' > commands. > Surely some fib structure flag could be set in aac_fib_alloc() for this. Thanks
On 2020-06-28 23:32, Hannes Reinecke wrote: > On 6/28/20 5:08 PM, Bart Van Assche wrote: >> On 2020-06-28 02:02, Hannes Reinecke wrote: >>> On 6/28/20 5:48 AM, Bart Van Assche wrote: >>>> On 2020-06-25 07:01, Hannes Reinecke wrote: >>>>> +/** >>>>> + * scsi_put_internal_cmd - free an internal SCSI command >>>>> + * @scmd: SCSI command to be freed >>>>> + */ >>>>> +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) >>>>> +{ >>>>> + struct request *rq = blk_mq_rq_from_pdu(scmd); >>>>> + >>>>> + if (blk_rq_is_internal(rq)) >>>>> + blk_mq_free_request(rq); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(scsi_put_internal_cmd); >>>> >>>> How about triggering a warning for the !blk_rq_is_internal(rq) case >>>> instead of silently ignoring regular SCSI commands? >>>> >>> That's by design. >>> Some drivers have a common routine for freeing up commands, so it'd be >>> quite tricky to separate these two cases out at the driver level. >>> So it's far easier to call the common routine for all commands, and >>> let this function do the right thing for all commands. >> >> That sounds fair to me, but is an example available in this patch series >> of a call to scsi_put_internal_cmd() from such a common routine? It >> seems to me that all calls to scsi_put_internal_cmd() introduced in this >> patch series happen from code paths that handle internal commands only? >> > aacraid. > The function aac_fib_free() is called unconditionally for every fib, and > doesn't have the means to differentiate between 'normal' and 'internal' > commands. Please make scsi_put_internal_cmd() complain if it is invoked for anything else than an internal command and surround the scsi_put_internal_cmd() call in aacraid with if (blk_rq_is_internal(rq)) { ... }. Thanks, Bart.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 0ba7a65e7c8d..bd378e1bd3fc 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1903,6 +1903,47 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) blk_mq_free_tag_set(&shost->tag_set); } +/** + * scsi_get_internal_cmd - allocate an intenral SCSI command + * @sdev: SCSI device from which to allocate the command + * @data_direction: Data direction for the allocated command + * @op_flags: request allocation flags + * + * Allocates a SCSI command for internal LLDD use. + */ +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, + int data_direction, int op_flags) +{ + struct request *rq; + struct scsi_cmnd *scmd; + blk_mq_req_flags_t flags = 0; + unsigned int op = REQ_INTERNAL | op_flags; + + op |= (data_direction == DMA_TO_DEVICE) ? + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN; + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); + if (IS_ERR(rq)) + return NULL; + scmd = blk_mq_rq_to_pdu(rq); + scmd->request = rq; + scmd->device = sdev; + return scmd; +} +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); + +/** + * scsi_put_internal_cmd - free an internal SCSI command + * @scmd: SCSI command to be freed + */ +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) +{ + struct request *rq = blk_mq_rq_from_pdu(scmd); + + if (blk_rq_is_internal(rq)) + blk_mq_free_request(rq); +} +EXPORT_SYMBOL_GPL(scsi_put_internal_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 bc5909033d13..bd1e52213d65 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -460,6 +460,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_internal_cmd(struct scsi_device *sdev, + int data_direction, int op_flags); +void scsi_put_internal_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 allow LLDDs to allocate and free internal commands. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/scsi_lib.c | 41 +++++++++++++++++++++++++++++++++++++++++ include/scsi/scsi_device.h | 3 +++ 2 files changed, 44 insertions(+)