Re: [PATCH v2 01/15] scsi: Add struct for args to execution functions

John Garry
2022-12-09T10:40:57Z

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