Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

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

On 11/22/21 12:58 AM, John Garry wrote:
> On 19/11/2021 19:57, Bart Van Assche wrote:
>> +/**
>> + * scsi_get_internal_cmd - Allocate an internal SCSI command
>> + * @q: request queue from which to allocate the command. This request 
>> queue may
>> + *    but does not have to be associated with a SCSI device. This 
>> request
>> + *    queue must be associated with a SCSI tag set. See also
>> + *    scsi_mq_setup_tags().
>> + * @data_direction: Data direction for the allocated command.
>> + * @flags: Zero or more BLK_MQ_REQ_* flags.
>> + *
>> + * Allocates a request for driver-internal use. The tag of the 
>> returned SCSI
>> + * command is guaranteed to be unique.
>> + */
>> +struct scsi_cmnd *scsi_get_internal_cmd(struct request_queue *q,
>> +                    enum dma_data_direction data_direction,
>> +                    blk_mq_req_flags_t flags)
> 
> I'd pass the Scsi_Host or scsi_device rather than a request q, so maybe:
> 
> struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, ..)
> struct scsi_cmnd *scsi_host_get_internal_cmd(struct Scsi_Host *shost, ..)

Passing a request queue pointer as first argument instead of a struct 
scsi_device is a deliberate choice. In the UFS driver (and probably also 
in other SCSI LLDs) we want to allocate internal requests without these 
requests being visible in any existing SCSI device statistics. Creating 
a new SCSI device for the allocation of internal requests is not a good 
choice because that new SCSI device would have to be assigned a LUN 
number and would be visible in sysfs. Hence the choice to allocate 
internal requests from a request queue that is not associated with any 
SCSI device.

>> +{
>> +    unsigned int opf = REQ_INTERNAL;
>> +    struct request *rq;
>> +
>> +    opf |= data_direction == DMA_TO_DEVICE ? REQ_OP_DRV_OUT : 
>> REQ_OP_DRV_IN;
>> +    rq = blk_mq_alloc_request(q, opf, flags);
>> +    if (IS_ERR(rq))
>> +        return ERR_CAST(rq);
> 
> I think that Christoph suggested elsewhere that we should poison all the 
> scsi_cmnd

I had overlooked that comment. I will look into this.

Thanks,

Bart.