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

[Bart Van Assche <bvanassche@acm.org>]

On 9/16/25 2:09 AM, John Garry wrote:
> On 12/09/2025 19:21, Bart Van Assche wrote:
>> Make the @cmd argument optional. Add .init_cmd() and .copy_result()
>> callbacks in struct scsi_exec_args. Support allocating from a specific
>> hardware queue. This patch prepares for submitting reserved commands
>> with scsi_execute_cmd().
> 
> A comment: I did suggest trying to reuse scsi_execute_cmd() for this. 
> However considering the code changes and the comments, below. Maybe it 
> is not the best thing. Let's further investigate...

I followed your suggestion because I think it results in cleaner code.
This approach allowed me to drop the patch that introduces the
scsi_get_internal_cmd() and scsi_put_internal_cmd() functions. This
approach also eliminates some code duplication in the scsi_execute_cmd()
callers introduced by this patch series.

>>   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. 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.

>> +        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()?

I'm open to this but I'm not sure this would be an improvement because
there is only one scsi_alloc_request_hctx() caller and there are nine
scsi_alloc_request() calls. An argument like ANY_HCTX would have to be
added to all nine calls. On the other hand, the code duplication that is
the result of the current approach is minimal.

>> @@ -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.

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

>>       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()? 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.

Thanks,

Bart.