Re: [PATCH v2 01/15] scsi: Add struct for args to execution functions

John Garry
2022-12-12T16:17:43Z

On 09/12/2022 18:47, Bart Van Assche wrote:
>>> around it then we have to do a WARN/BUG. We do the macro approach now
>>> so we can do the BUILD_BUG_ON.
>>
>> Maybe we have to switch to a WARN/BUG.
>>
>> It looks like some compilers don't like:
>>
>> const struct scsi_exec_args exec_args = {
>>     .sshdr = &sshdr,
>> };
>>
>> scsi_execute_args(.... exec_args);
>>
>> and will hit the:
>>
>> #define scsi_execute_args(sdev, cmd, opf, buffer, bufflen, timeout,     \
>>                            retries,
>> args)                                \
>> ({                                                                      \
>>          BUILD_BUG_ON(args.sense
>> &&                                      \
>>                       args.sense_len !=
>> SCSI_SENSE_BUFFERSIZE);          \
>>
>> because the args's sense and sense_len are not cleared yet.
>
> My understanding is that the __scsi_execute() macro was introduced to
> prevent that every single scsi_execute() caller would have to be
> modified. I'm fine with removing the BUILD_BUG_ON(sense_len !=
> SCSI_SENSE_BUFFER_SIZE) check and replacing it with a WARN_ON_ONCE()
> statement, e.g. inside __scsi_execute().

Another option is to have __scsi_execute() allocate the sense buf by
kmemdup, and hold the sense pointer as unsigned char ** in struct
scsi_exec_args; but then the caller needs to kfree the allocated sense
buf, which I suppose is less than ideal. However there is only single
driver which uses this AFAICS.

Thanks,
John