Em Fri, 10 Aug 2018 09:21:59 +0200
Hans Verkuil <hverk...@xs4all.nl> escreveu:
> On 08/09/2018 07:53 PM, Mauro Carvalho Chehab wrote:
> > Em Sat, 4 Aug 2018 14:44:54 +0200
> > Hans Verkuil <hverk...@xs4all.nl> escreveu:
> >
> >> From: Hans Verkuil <hans.verk...@cisco.com>
> >>
> >> 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.verk...@cisco.com>
> >> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> >> ---
> >> 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.
> >
> > 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.
Thanks,
Mauro
