Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

Hannes Reinecke
2021-11-28T12:46:56Z

On 11/27/21 12:12 AM, Bart Van Assche wrote:
> On 11/25/21 07:10, Hannes Reinecke wrote:
>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
>> +    int data_direction, bool nowait)
>
> Please use enum dma_data_direction instead of 'int' for the data direction.

I have oriented myself at __scsi_execute(), which also has
'data_direction' as an integer.
Presumably to avoid header clutter.
Martin?

>> +{
>> +    struct request *rq;
>> +    struct scsi_cmnd *scmd;
>> +    blk_mq_req_flags_t flags = 0;
>> +    int op;
>
> The name 'op' is confusing since that variable is a bitfield that
> includes the operation and operation flags. Consider one of the names
> 'opf', 'op_and_flags' or 'cmd_and_flags'. Please also change the data
> type from 'int' into 'unsigned int' or 'u32'.
>
'op' is the name the variable has in blk_mq_alloc_request(), so I prefer
to stay with that name. Agree with the 'unsigned int', though.

>> +    op = (data_direction == DMA_TO_DEVICE) ?
>> +        REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
>
> Please leave out the parentheses from the above assignment.
>
Okay.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer