Re: [PATCH v4 06/29] scsi: core: Extend the scsi_execute_cmd() functionality

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

>>>   retry:
>>> -    req = scsi_alloc_request(sdev->request_queue, opf, args- 
>>> >req_flags);
>>> +    req = args->specify_hctx ?
>>
>> Can you check args->hctx_idx is a specific queue or something like 
>> NVME_QID_ANY?
> 
> That would require explicit initialization of the .hctx_idx member by
> all callers that don't care about which hardware queue a command is
> allocated from.

Yeah, doing that is not reliable. You could also add hctx_idx as 
a(nother) new argument to scsi_execute_cmd()

> I think the current approach (only code that cares about
> the hardware queue a command is allocated from has to specify
> information related to the hardware queue) is more user friendly
> because scsi_execute_cmd() callers won't forget by accident to set
> .hctx_idx to e.g. ANY_HCTX.

understood

> 
>>> +        scsi_alloc_request_hctx(sdev->request_queue, opf,
>>> +                    args->req_flags, args->hctx_idx) :
>>
>> did you consider passing this hctx info to scsi_alloc_request() and 
>> allow scsi_alloc_request() contain the logic as to call 
>> blk_mq_alloc_request_hctx() or blk_mq_alloc_request()?
> 
...

> 
>>> @@ -318,8 +321,12 @@ int scsi_execute_cmd(struct scsi_device *sdev, 
>>> const unsigned char *cmd,
>>>               goto out;
>>>       }
>>>       scmd = blk_mq_rq_to_pdu(req);
>>> -    scmd->cmd_len = COMMAND_SIZE(cmd[0]);
>>> -    memcpy(scmd->cmnd, cmd, scmd->cmd_len);
>>
>>
>>> +    if (cmd) {
>>> +        scmd->cmd_len = COMMAND_SIZE(cmd[0]);
>>> +        memcpy(scmd->cmnd, cmd, scmd->cmd_len);
>>> +    }
>>
>> you could just pass a dummy cmd instead of doing this
> 
> Sure, that's possible, but it would make some scsi_execute_cmd() calls 
> really confusing. Making the callers pass a CDB that is never used would
> make the reader of the callers wonder why e.g. a TEST UNIT READY CDB is
> passed to a scsi_execute_cmd() call that submits something that is not a
> SCSI command.

eh, don't we have space for vendor commands which could be used (instead 
of something like TEST UNIT READY)?

> 
>>> +    if (args->init_cmd)
>>> +        args->init_cmd(scmd, args);
>>
>> is it possible to do this in ufshcd_init_cmd_priv? Or too late?
>>
>> We could have a "is reserved command" check there (in 
>> ufshcd_init_cmd_priv), and do whatever processing is needed which is 
>> done in ufshcd_init_dev_cmd
> 
> This is not possible because the 'args' pointer is not passed to
> ufshcd_init_cmd_priv(). See also the conversions of the 'arg' pointer
> into a pointer to the surrounding data structure in the .init_cmd
> functions in patch 29/29. Maybe I should rename .init_cmd into
> .setup_cmd because all functions in the SCSI disk driver that have a
> similar role have "_setup_" in their function name.

I thought that you could pass the data like how I done it in the 
scsi_debug change which I proposed.

> 
>>>       scmd->allowed = ml_retries;
>>>       scmd->flags |= args->scmd_flags;
>>>       req->timeout = timeout;
>>> @@ -353,6 +360,9 @@ int scsi_execute_cmd(struct scsi_device *sdev, 
>>> const unsigned char *cmd,
>>>                        args->sshdr);
>>>       ret = scmd->result;
>>> +    if (ret == 0 && args->copy_result)
>>> +        args->copy_result(scmd, args);
>>
>> can this sort of thing be done in the LLD completion handler?
> Only if the 'args' pointer would be stored in the SCSI command private
> data. Do you perhaps prefer that the 'args' pointer would be stored in
> the SCSI command private data instead of adding a .copy_result function
> call in scsi_execute_cmd()? 

I don't think that passing args around is a good idea. We only have args 
as it minimizes the args to scsi_execute_cmd()

> This approach is more risky because it may
> result in scsi_execute_cmd() callers forgetting to clear the 'args'
> pointer in the SCSI command private data after scsi_execute_cmd() has
> finished.

Is there anywhere else where the result can be stored and passed back?

I know that it is not ideal, but could we use scsi_execute_cmd() @buffer 
arg for both in and out data?

Thanks,
John