| Message ID | 20200629072021.9864-4-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On 29/06/2020 08:20, Hannes Reinecke wrote: > 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 | 44 ++++++++++++++++++++++++++++++++++++++++++++ > include/scsi/scsi_device.h | 4 ++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 0ba7a65e7c8d..c2277eff4e06 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1903,6 +1903,50 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) > blk_mq_free_tag_set(&shost->tag_set); > } > > +/** > + * scsi_get_internal_cmd - allocate an internal 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, > + enum dma_data_direction 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; As an aside, REQ_OP_SCSI_OUT seems to have same effect as REQ_OP_SCSI_IN, as we only ever check if either is set, here: bool blk_op_is_scsi(unsigned int op) { return op == REQ_OP_SCSI_OUT || op == REQ_OP_SCSI_IN; } but maybe I missed something. > + 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 > + * > + * Check if @scmd is an internal command, and call > + * blk_mq_free_request() if true. > + */ > +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); I haven't gone through all users in the series, but is it worth warning when we're passed a scmd which isn't an internal command? Doing so seems a programming error which we should yell about. Thanks > +} > +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..2759a538adae 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -8,6 +8,7 @@ > #include <linux/blkdev.h> > #include <scsi/scsi.h> > #include <linux/atomic.h> > +#include <linux/dma-direction.h> > > struct device; > struct request_queue; > @@ -460,6 +461,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, > + enum dma_data_direction 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); >
On 6/29/20 2:48 PM, John Garry wrote: > On 29/06/2020 08:20, Hannes Reinecke wrote: >> 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 | 44 >> ++++++++++++++++++++++++++++++++++++++++++++ >> include/scsi/scsi_device.h | 4 ++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 0ba7a65e7c8d..c2277eff4e06 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -1903,6 +1903,50 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) >> blk_mq_free_tag_set(&shost->tag_set); >> } >> +/** >> + * scsi_get_internal_cmd - allocate an internal 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, >> + enum dma_data_direction 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; > > As an aside, REQ_OP_SCSI_OUT seems to have same effect as > REQ_OP_SCSI_IN, as we only ever check if either is set, here: > > bool blk_op_is_scsi(unsigned int op) > { > return op == REQ_OP_SCSI_OUT || op == REQ_OP_SCSI_IN; > } > > but maybe I missed something. > Indeed you do :-) That's one of the best-kept secrets in the block layer: op_is_write() The assumption is that every request with a REQ_OP which has the lowest bit set is a write. And quite a lot of accounting the the block layer revolves around that. So we'll need to keep it. ... and we probably should document it somewhere. >> + 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 >> + * >> + * Check if @scmd is an internal command, and call >> + * blk_mq_free_request() if true. >> + */ >> +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); > > I haven't gone through all users in the series, but is it worth warning > when we're passed a scmd which isn't an internal command? Doing so seems > a programming error which we should yell about. > Will be part of the next round; others have complained, too. Cheers, Hannes
On 02/07/2020 09:59, Hannes Reinecke wrote: > That's one of the best-kept secrets in the block layer: > > op_is_write() > > The assumption is that every request with a REQ_OP which has the lowest > bit set is a write. > And quite a lot of accounting the the block layer revolves around that. > So we'll need to keep it. > > ... and we probably should document it somewhere. I personally find this very obvious: static inline bool op_is_write(unsigned int op) { return (op & 1); }
On 02/07/2020 09:21, Johannes Thumshirn wrote: > On 02/07/2020 09:59, Hannes Reinecke wrote: >> That's one of the best-kept secrets in the block layer: >> >> op_is_write() >> >> The assumption is that every request with a REQ_OP which has the lowest >> bit set is a write. >> And quite a lot of accounting the the block layer revolves around that. >> So we'll need to keep it. >> >> ... and we probably should document it somewhere. > Now that I check further, it is documented in blk_types.h: * The least significant bit of the operation number indicates the data * transfer direction: * * - if the least significant bit is set transfers are TO the device * - if the least significant bit is not set transfers are FROM the device * > I personally find this very obvious: > > static inline bool op_is_write(unsigned int op) > { > return (op & 1); > } > > . >
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 0ba7a65e7c8d..c2277eff4e06 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1903,6 +1903,50 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) blk_mq_free_tag_set(&shost->tag_set); } +/** + * scsi_get_internal_cmd - allocate an internal 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, + enum dma_data_direction 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 + * + * Check if @scmd is an internal command, and call + * blk_mq_free_request() if true. + */ +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..2759a538adae 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -8,6 +8,7 @@ #include <linux/blkdev.h> #include <scsi/scsi.h> #include <linux/atomic.h> +#include <linux/dma-direction.h> struct device; struct request_queue; @@ -460,6 +461,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, + enum dma_data_direction 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 | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/scsi/scsi_device.h | 4 ++++ 2 files changed, 48 insertions(+)