Re: [RFCv4 01/21] media: add request API core and UAPI

[Hans Verkuil]

On 02/22/18 10:30, Alexandre Courbot wrote:
> On Wed, Feb 21, 2018 at 4:29 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>> On 02/21/2018 07:01 AM, Alexandre Courbot wrote:
>>> Hi Hans,
>>>
>>> On Tue, Feb 20, 2018 at 7:36 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>>> On 02/20/18 05:44, Alexandre Courbot wrote:
>>
>> <snip>
>>
>>>>> +#define MEDIA_REQUEST_IOC(__cmd, func)                                       \
>>>>> +     [_IOC_NR(MEDIA_REQUEST_IOC_##__cmd) - 0x80] = {                 \
>>>>> +             .cmd = MEDIA_REQUEST_IOC_##__cmd,                       \
>>>>> +             .fn = func,                                             \
>>>>> +     }
>>>>> +
>>>>> +struct media_request_ioctl_info {
>>>>> +     unsigned int cmd;
>>>>> +     long (*fn)(struct media_request *req);
>>>>> +};
>>>>> +
>>>>> +static const struct media_request_ioctl_info ioctl_info[] = {
>>>>> +     MEDIA_REQUEST_IOC(SUBMIT, media_request_ioctl_submit),
>>>>> +     MEDIA_REQUEST_IOC(REINIT, media_request_ioctl_reinit),
>>>>
>>>> There are only two ioctls, so there is really no need for the
>>>> MEDIA_REQUEST_IOC define. Just keep it simple.
>>>
>>> The number of times it is used doesn't change the fact that it helps
>>> with readability IMHO.
>>
>> But this macro just boils down to:
>>
>> static const struct media_request_ioctl_info ioctl_info[] = {
>>         { MEDIA_REQUEST_IOC_SUBMIT, media_request_ioctl_submit },
>>         { MEDIA_REQUEST_IOC_REINIT, media_request_ioctl_reinit },
>> };
>>
>> It's absolutely identical! So it seems senseless to me.
> 
> This expands to more than that - the index needs to be offset by 0x80,
> something we probably don't want to repeat every line.
> 
>>
>>>
>>>>
>>>>> +};
>>>>> +
>>>>> +static long media_request_ioctl(struct file *filp, unsigned int cmd,
>>>>> +                             unsigned long __arg)
>>>>> +{
>>>>> +     struct media_request *req = filp->private_data;
>>>>> +     const struct media_request_ioctl_info *info;
>>>>> +
>>>>> +     if ((_IOC_NR(cmd) < 0x80) ||
>>>>
>>>> Why start the ioctl number at 0x80? Why not just 0?
>>>> It avoids all this hassle with the 0x80 offset.
>>
>> There is no clash with the MC ioctls, so I really don't believe the 0x80
>> offset is needed.
> 
> I suppose your comment in patch 16 supersedes this one. :)

Yes, it does. I realized later why this was done like this.

That said, I don't like the magic value. Something like this might be
cleaner:

	const unsigned int first_ioc_nr = _IOC_NR(MEDIA_REQUEST_IOC_SUBMIT);

Then use first_ioc_nr (or nr_offset or whatever) instead of 0x80.

Regards,

	Hans