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

[John Garry <john.g.garry@oracle.com>]

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