On 14/08/18 10:46, Mauro Carvalho Chehab wrote:
> Em Fri, 10 Aug 2018 09:21:59 +0200
> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>
>> On 08/09/2018 07:53 PM, Mauro Carvalho Chehab wrote:
>>> Em Sat, 4 Aug 2018 14:44:54 +0200
>>> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>>>
>>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>>
>>>> Define the public request API.
>>>>
>>>> This adds the new MEDIA_IOC_REQUEST_ALLOC ioctl to allocate a request
>>>> and two ioctls that operate on a request in order to queue the
>>>> contents of the request to the driver and to re-initialize the
>>>> request.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>> Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>>> ---
>>>> include/uapi/linux/media.h | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>>> index 36f76e777ef9..cf77f00a0f2d 100644
>>>> --- a/include/uapi/linux/media.h
>>>> +++ b/include/uapi/linux/media.h
>>>> @@ -364,11 +364,23 @@ struct media_v2_topology {
>>>>
>>>> /* ioctls */
>>>>
>>>> +struct __attribute__ ((packed)) media_request_alloc {
>>>> + __s32 fd;
>>>> +};
>>>> +
>>>> #define MEDIA_IOC_DEVICE_INFO _IOWR('|', 0x00, struct media_device_info)
>>>> #define MEDIA_IOC_ENUM_ENTITIES _IOWR('|', 0x01, struct media_entity_desc)
>>>> #define MEDIA_IOC_ENUM_LINKS _IOWR('|', 0x02, struct media_links_enum)
>>>> #define MEDIA_IOC_SETUP_LINK _IOWR('|', 0x03, struct media_link_desc)
>>>> #define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
>>>> +#define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, struct media_request_alloc)
>
> The definition here is wrong... the fd field is not R/W, it is just R, as no
> fields inside this struct should be filled by userspace.
> The right declaration for it would be:
>
> #define MEDIA_IOC_REQUEST_ALLOC _IOR('|', 0x05, struct media_request_alloc)
>
> I do have a strong opinion here: ioctls that just return stuff should use _IOR.
You are right, this should be _IOR.
>
>>>
>>> Same comment as in patch 1: keep it simpler: just pass a s32 * as the
>>> argument for this ioctl.
>>
>> Same comment as in patch 1: I have no strong opinion, but I want the input from others
>> as well.
>
> I'm transcribing a comment you wrote on patch 01/34 here, for the sake of
> keeping everything on a single thread:
>
>> The first version just had a s32 argument, not a struct. The main reason for
>> going back to a struct was indeed to make it easier to add new fields in the
>> future. I don't foresee any, but then, you never do.
>
> First of all, if we declare it as it should be, e. g.:
>
> #define MEDIA_IOC_REQUEST_ALLOC _IOR('|', 0x05, int)
>
> If later find the need for some struct:
>
> struct media_request_alloc {
> __s32 fd;
> __s32 foo;
> } __packed;
>
> Assuming that "foo" is a write argument, we'll then have:
>
> #define MEDIA_IOC_REQUEST_ALLOC _IOR('|', 0x05, int)
> #define MEDIA_IOC_REQUEST_ALLOC_V2 _IOWR('|', 0x05, struct media_request_alloc)
>
> The size of the ioctl will also be different, and also the direction.
> So, the magic number will be different.
>
> The Kernel can easily handle it on both ways, and, as
> MEDIA_IOC_REQUEST_ALLOC has only an integer output parameter,
> there's no need for compat32 or to keep any old struct.
> The MEDIA_IOC_REQUEST_ALLOC code handler will still be very simple,
> and backward compatible comes for free.
>
> If, on the other hand, we declare it as:
> #define MEDIA_IOC_REQUEST_ALLOC _IOR('|', 0x05, struct media_request_alloc_old)
>
> And then we change it to:
> #define MEDIA_IOC_REQUEST_ALLOC _IORW('|', 0x05, struct media_request_alloc_new)
>
> Keeping backward compatible will be painful, and will require efforts for
> no gain.
In the kernel it doesn't matter much, I agree. But applications that have to
be able to handle both old and new ioctls will have to switch between either
an fd or a struct. Whereas if we use a struct from the beginning, then the
only difference between the old and new ioctls are whether or not additional
fields beyond the first fd field are set.
As mentioned before, I don't have very strong feelings about this, but I
do want input from others on this before making this change.
Frankly, I think it is unlikely that we will need more fields beyond just
the fd, but I've been wrong about such things before...
Regards,
Hans
