mbox series

[00/15] Add struct to pass in optional args to scsi_execute

Message ID 20221122033934.33797-1-michael.christie@oracle.com
Headers show
Series Add struct to pass in optional args to scsi_execute | expand

Message

Mike Christie Nov. 22, 2022, 3:39 a.m. UTC 
The following patches made over Martin's 6.1 scsi queue branch adds a
struct that contains optinal arguments to the scsi_execute* functions.
Right now, it's just a nice cleanup, but will be needed for the patches
that allow the scsi passthrough users to control retries. I separated
the 2 sets to make it easier to review and post.

Comments

Christoph Hellwig Nov. 22, 2022, 6:36 a.m. UTC | #1 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Mike Christie Nov. 22, 2022, 4:13 p.m. UTC | #2 
On 11/22/22 12:38 AM, Christoph Hellwig wrote:
> On Mon, Nov 21, 2022 at 09:39:25PM -0600, Mike Christie wrote:
>> +	result = scsi_execute_cmd(sdev, cmd, REQ_OP_DRV_IN, buffer,
>> +				  request_len, 30 * HZ, 3,
>> +				  ((struct scsi_exec_args) { .sshdr = &sshdr }));
> 
> Maybe it's me, but I hate the syntax to declare structs inside argument

I'll change it to setup the scsi_exec_args struct like normal. I've got this
comment several times now.

With the current design we know all the args when we declare the variables so
I can do it then like normal.

> list.  But even when we go with it, splitting it into multiple lines
> would be a lot more readable:
> 				  ((struct scsi_exec_args) {
> 				  	.sshdr = &sshdr,
> 				  }));
>
Mike Christie Nov. 22, 2022, 8:06 p.m. UTC | #3 
On 11/22/22 12:46 PM, Bart Van Assche wrote:
> On 11/22/22 08:13, Mike Christie wrote:
>> On 11/22/22 12:38 AM, Christoph Hellwig wrote:
>>> On Mon, Nov 21, 2022 at 09:39:25PM -0600, Mike Christie wrote:
>>>> +    result = scsi_execute_cmd(sdev, cmd, REQ_OP_DRV_IN, buffer,
>>>> +                  request_len, 30 * HZ, 3,
>>>> +                  ((struct scsi_exec_args) { .sshdr = &sshdr }));
>>>
>>> Maybe it's me, but I hate the syntax to declare structs inside argument
>>
>> I'll change it to setup the scsi_exec_args struct like normal. I've got this
>> comment several times now.
>>
>> With the current design we know all the args when we declare the variables so
>> I can do it then like normal.
> 
> Will struct scsi_exec_args be retained? I'm asking this because I'd like to add another argument to the (already too large) argument list of scsi_execute().

Yes. I just meant for the above chunk I would move the struct setup like this:

@@ -509,6 +509,9 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer,
 {
 	unsigned char cmd[16];
 	struct scsi_sense_hdr sshdr;
+	struct scsi_exec_args exec_args = {
+		.sshdr = &sshdr,
+	};
 	int result, request_len;
 
 	if (sdev->no_report_opcodes || sdev->scsi_level < SCSI_SPC_3)
@@ -532,8 +535,7 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer,
 	memset(buffer, 0, len);
 
 	result = scsi_execute_cmd(sdev, cmd, REQ_OP_DRV_IN, buffer,
-				  request_len, 30 * HZ, 3,
-				  ((struct scsi_exec_args) { .sshdr = &sshdr }));
+				  request_len, 30 * HZ, 3, exec_args);
 
 	if (result < 0)
 		return result;