[03/18] scsi: add scsi_{get,put}_internal_cmd() helper

Message ID 20210503150333.130310-4-hare@suse.de
State New
Headers show
Series
  • scsi: enabled reserved commands for LLDDs
Related show

Commit Message

Hannes Reinecke May 3, 2021, 3:03 p.m.
Add helper functions to allow LLDDs to allocate and free
internal commands.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/scsi_lib.c    | 38 ++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  3 +++
 2 files changed, 41 insertions(+)

Comments

Bart Van Assche May 4, 2021, 2:21 a.m. | #1
On 5/3/21 8:03 AM, Hannes Reinecke wrote:
> +/**

> + * scsi_get_internal_cmd - allocate an internal SCSI command

> + * @sdev: SCSI device from which to allocate the command

> + * @op: operation (REQ_OP_SCSI_IN or REQ_OP_SCSI_OUT)

> + * @flags: BLK_MQ_REQ_* flags, e.g. BLK_MQ_REQ_NOWAIT.

> + *

> + * Allocates a SCSI command for internal LLDD use.

> + */

> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,

> +	unsigned int op, blk_mq_req_flags_t flags)

> +{

> +	struct request *rq;

> +	struct scsi_cmnd *scmd;

> +

> +	WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) &&

> +		     ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT));

> +	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);


Multiple fields that are initialized by the regular command submission
path are not initialized by the above function, e.g. scmd->tag. Has it
been considered to call scsi_init_command() instead of adding yet
another code path that initializes scmd->request?

Thanks,

Bart.
Hannes Reinecke May 4, 2021, 6:12 a.m. | #2
On 5/4/21 4:21 AM, Bart Van Assche wrote:
> On 5/3/21 8:03 AM, Hannes Reinecke wrote:

>> +/**

>> + * scsi_get_internal_cmd - allocate an internal SCSI command

>> + * @sdev: SCSI device from which to allocate the command

>> + * @op: operation (REQ_OP_SCSI_IN or REQ_OP_SCSI_OUT)

>> + * @flags: BLK_MQ_REQ_* flags, e.g. BLK_MQ_REQ_NOWAIT.

>> + *

>> + * Allocates a SCSI command for internal LLDD use.

>> + */

>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,

>> +	unsigned int op, blk_mq_req_flags_t flags)

>> +{

>> +	struct request *rq;

>> +	struct scsi_cmnd *scmd;

>> +

>> +	WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) &&

>> +		     ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT));

>> +	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);

> 

> Multiple fields that are initialized by the regular command submission

> path are not initialized by the above function, e.g. scmd->tag. Has it

> been considered to call scsi_init_command() instead of adding yet

> another code path that initializes scmd->request?

> 

Hmm. No, I don't think it's a good idea.
Basic idea is that the SCSI request serves as a container for (non-SCSI) 
management commands, _and_ that they are submitted directly from within 
the driver, ie never ever enter ->queue_rq().
As such we don't need an initialisation vie scsi_init_request(), and it 
would actually be counter-productive as we would be setting up fields 
which we'll never need anyway, and might need to tear down afterwards.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
Christoph Hellwig May 4, 2021, 9:53 a.m. | #3
On Mon, May 03, 2021 at 05:03:18PM +0200, Hannes Reinecke wrote:
> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,

> +	unsigned int op, blk_mq_req_flags_t flags)


Weird indentation - prototype continuations either use two tabs or
are aligned after the opening brace (I generally prefer the former).

> +{

> +	struct request *rq;

> +	struct scsi_cmnd *scmd;

> +

> +	WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) &&

> +		     ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT));


Woudn't a simple bool write command make more sense than passing the
actual op here?

> +	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;


Maybe a comment that explains what part of the scmd are initialized
and which not would be useful.
Hannes Reinecke May 4, 2021, 12:54 p.m. | #4
On 5/4/21 11:53 AM, Christoph Hellwig wrote:
> On Mon, May 03, 2021 at 05:03:18PM +0200, Hannes Reinecke wrote:

>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,

>> +	unsigned int op, blk_mq_req_flags_t flags)

> 

> Weird indentation - prototype continuations either use two tabs or

> are aligned after the opening brace (I generally prefer the former).

> 

Yeah, I'm never sure how one should be indenting the second line for a 
function declaration. But I'll fix it.

>> +{

>> +	struct request *rq;

>> +	struct scsi_cmnd *scmd;

>> +

>> +	WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) &&

>> +		     ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT));

> 

> Woudn't a simple bool write command make more sense than passing the

> actual op here?

> 

Yep, can do.

>> +	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;

> 

> Maybe a comment that explains what part of the scmd are initialized

> and which not would be useful.

> 

Okay.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d7c0d5a5f263..f83b04e49bae 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1989,6 +1989,44 @@  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
+ * @op: operation (REQ_OP_SCSI_IN or REQ_OP_SCSI_OUT)
+ * @flags: BLK_MQ_REQ_* flags, e.g. BLK_MQ_REQ_NOWAIT.
+ *
+ * Allocates a SCSI command for internal LLDD use.
+ */
+struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
+	unsigned int op, blk_mq_req_flags_t flags)
+{
+	struct request *rq;
+	struct scsi_cmnd *scmd;
+
+	WARN_ON_ONCE(((op & REQ_OP_MASK) != REQ_OP_SCSI_IN) &&
+		     ((op & REQ_OP_MASK) != REQ_OP_SCSI_OUT));
+	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);
+
+	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 ac6ab16abee7..eaa5414ff220 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -462,6 +462,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,
+	unsigned int op, blk_mq_req_flags_t 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);