* [PATCH 0/2] Compat IOCTL handler for Media controller
@ 2013-01-22 16:23 Sakari Ailus
2013-01-22 16:27 ` [PATCH 1/2] media: Add 64--32 bit compat ioctl handler Sakari Ailus
0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2013-01-22 16:23 UTC (permalink / raw)
To: LMML
Hi,
I noticed recently that we're missing the compat IOCTL handling in Media
controller API to support 32-bit binaries on 64-bit systems. I guess the
issue hasn't bitten anyone too badly since this hasn't been noticed earlier.
I can think of the reasons, too: Media controller is primarily used on
embedded systems that are still mainly 32-bit and the binaries are
targeted to those very systems specifically.
I noticed this since I'm currently running my 32-bit Debian system on a
64-bit kernel (I like to run 64-bit virtual machines but I'm lazy to
update; waiting for multiarch) so my native media-ctl binary just failed
to report anything about a UVC device Laurent kindly lended me.
The patchset implements separate compat ioctl handling and splits link
enumeration handling into 32-bit and 64-bit portions.
Many thanks to Laurent for testing these patches!
--
Kind regards,
Sakari Ailus
sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] media: Add 64--32 bit compat ioctl handler
2013-01-22 16:23 [PATCH 0/2] Compat IOCTL handler for Media controller Sakari Ailus
@ 2013-01-22 16:27 ` Sakari Ailus
2013-01-22 16:27 ` [PATCH 2/2] media: implement 32-on-64 bit compat IOCTL handling Sakari Ailus
0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2013-01-22 16:27 UTC (permalink / raw)
To: linux-media
Provide an ioctl handler for 32-bit binaries on 64-bit systems.
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/media-devnode.c | 31 ++++++++++++++++++++++++++++---
include/media/media-devnode.h | 1 +
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 023b2a1..fb0f046 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -116,19 +116,41 @@ static unsigned int media_poll(struct file *filp,
return mdev->fops->poll(filp, poll);
}
-static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+static long
+__media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
+ long (*ioctl_func)(struct file *filp, unsigned int cmd,
+ unsigned long arg))
{
struct media_devnode *mdev = media_devnode_data(filp);
- if (!mdev->fops->ioctl)
+ if (!ioctl_func)
return -ENOTTY;
if (!media_devnode_is_registered(mdev))
return -EIO;
- return mdev->fops->ioctl(filp, cmd, arg);
+ return ioctl_func(filp, cmd, arg);
+}
+
+static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+ struct media_devnode *mdev = media_devnode_data(filp);
+
+ return __media_ioctl(filp, cmd, arg, mdev->fops->ioctl);
}
+#ifdef CONFIG_COMPAT
+
+static long media_compat_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct media_devnode *mdev = media_devnode_data(filp);
+
+ return __media_ioctl(filp, cmd, arg, mdev->fops->compat_ioctl);
+}
+
+#endif /* CONFIG_COMPAT */
+
/* Override for the open function */
static int media_open(struct inode *inode, struct file *filp)
{
@@ -188,6 +210,9 @@ static const struct file_operations media_devnode_fops = {
.write = media_write,
.open = media_open,
.unlocked_ioctl = media_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = media_compat_ioctl,
+#endif /* CONFIG_COMPAT */
.release = media_release,
.poll = media_poll,
.llseek = no_llseek,
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index f6caafc..3446af2 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -46,6 +46,7 @@ struct media_file_operations {
ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
unsigned int (*poll) (struct file *, struct poll_table_struct *);
long (*ioctl) (struct file *, unsigned int, unsigned long);
+ long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
int (*open) (struct file *);
int (*release) (struct file *);
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] media: implement 32-on-64 bit compat IOCTL handling
2013-01-22 16:27 ` [PATCH 1/2] media: Add 64--32 bit compat ioctl handler Sakari Ailus
@ 2013-01-22 16:27 ` Sakari Ailus
2013-01-22 19:03 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2013-01-22 16:27 UTC (permalink / raw)
To: linux-media
Use the same handlers where the structs are the same. Implement a new
handler for link enumeration since struct media_links_enum is different on
32-bit and 64-bit systems.
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/media-device.c | 102 ++++++++++++++++++++++++++++++++++++------
1 file changed, 89 insertions(+), 13 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index d01fcb7..99b80b6 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -20,10 +20,11 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
-#include <linux/types.h>
+#include <linux/compat.h>
+#include <linux/export.h>
#include <linux/ioctl.h>
#include <linux/media.h>
-#include <linux/export.h>
+#include <linux/types.h>
#include <media/media-device.h>
#include <media/media-devnode.h>
@@ -124,35 +125,31 @@ static void media_device_kpad_to_upad(const struct media_pad *kpad,
upad->flags = kpad->flags;
}
-static long media_device_enum_links(struct media_device *mdev,
- struct media_links_enum __user *ulinks)
+static long __media_device_enum_links(struct media_device *mdev,
+ struct media_links_enum *links)
{
struct media_entity *entity;
- struct media_links_enum links;
- if (copy_from_user(&links, ulinks, sizeof(links)))
- return -EFAULT;
-
- entity = find_entity(mdev, links.entity);
+ entity = find_entity(mdev, links->entity);
if (entity == NULL)
return -EINVAL;
- if (links.pads) {
+ if (links->pads) {
unsigned int p;
for (p = 0; p < entity->num_pads; p++) {
struct media_pad_desc pad;
media_device_kpad_to_upad(&entity->pads[p], &pad);
- if (copy_to_user(&links.pads[p], &pad, sizeof(pad)))
+ if (copy_to_user(&links->pads[p], &pad, sizeof(pad)))
return -EFAULT;
}
}
- if (links.links) {
+ if (links->links) {
struct media_link_desc __user *ulink;
unsigned int l;
- for (l = 0, ulink = links.links; l < entity->num_links; l++) {
+ for (l = 0, ulink = links->links; l < entity->num_links; l++) {
struct media_link_desc link;
/* Ignore backlinks. */
@@ -169,8 +166,26 @@ static long media_device_enum_links(struct media_device *mdev,
ulink++;
}
}
+
+ return 0;
+}
+
+static long media_device_enum_links(struct media_device *mdev,
+ struct media_links_enum __user *ulinks)
+{
+ struct media_links_enum links;
+ int rval;
+
+ if (copy_from_user(&links, ulinks, sizeof(links)))
+ return -EFAULT;
+
+ rval = __media_device_enum_links(mdev, &links);
+ if (rval < 0)
+ return rval;
+
if (copy_to_user(ulinks, &links, sizeof(*ulinks)))
return -EFAULT;
+
return 0;
}
@@ -251,10 +266,71 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
return ret;
}
+#ifdef CONFIG_COMPAT
+
+struct media_links_enum32 {
+ __u32 entity;
+ compat_uptr_t pads; /* struct media_pad_desc * */
+ compat_uptr_t links; /* struct media_link_desc * */
+ __u32 reserved[4];
+};
+
+static long media_device_enum_links32(struct media_device *mdev,
+ struct media_links_enum32 __user *ulinks)
+{
+ struct media_links_enum links;
+ compat_uptr_t pads_ptr, links_ptr;
+
+ memset(&links, 0, sizeof(links));
+
+ if (get_user(links.entity, &ulinks->entity)
+ || get_user(pads_ptr, &ulinks->pads)
+ || get_user(links_ptr, &ulinks->links))
+ return -EFAULT;
+
+ links.pads = compat_ptr(pads_ptr);
+ links.links = compat_ptr(links_ptr);
+
+ return __media_device_enum_links(mdev, &links);
+}
+
+#define MEDIA_IOC_ENUM_LINKS32 _IOWR('|', 0x02, struct media_links_enum32)
+
+static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct media_devnode *devnode = media_devnode_data(filp);
+ struct media_device *dev = to_media_device(devnode);
+ long ret;
+
+ switch (cmd) {
+ case MEDIA_IOC_DEVICE_INFO:
+ case MEDIA_IOC_ENUM_ENTITIES:
+ case MEDIA_IOC_SETUP_LINK:
+ return media_device_ioctl(filp, cmd, arg);
+
+ case MEDIA_IOC_ENUM_LINKS32:
+ mutex_lock(&dev->graph_mutex);
+ ret = media_device_enum_links32(dev,
+ (struct media_links_enum32 __user *)arg);
+ mutex_unlock(&dev->graph_mutex);
+ break;
+
+ default:
+ ret = -ENOIOCTLCMD;
+ }
+
+ return ret;
+}
+#endif /* CONFIG_COMPAT */
+
static const struct media_file_operations media_device_fops = {
.owner = THIS_MODULE,
.open = media_device_open,
.ioctl = media_device_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = media_device_compat_ioctl,
+#endif /* CONFIG_COMPAT */
.release = media_device_close,
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] media: implement 32-on-64 bit compat IOCTL handling
2013-01-22 16:27 ` [PATCH 2/2] media: implement 32-on-64 bit compat IOCTL handling Sakari Ailus
@ 2013-01-22 19:03 ` Hans Verkuil
2013-01-22 19:43 ` Sakari Ailus
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2013-01-22 19:03 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
On Tue January 22 2013 17:27:56 Sakari Ailus wrote:
> Use the same handlers where the structs are the same. Implement a new
> handler for link enumeration since struct media_links_enum is different on
> 32-bit and 64-bit systems.
I think I would prefer to have the compat handling split off into a
seperate source. I know it is just a small amount of code at the moment,
but that's the way it is done as well for the V4L2 ioctls and I rather
like that approach.
What do others think about that?
Regards,
Hans
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/media-device.c | 102 ++++++++++++++++++++++++++++++++++++------
> 1 file changed, 89 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index d01fcb7..99b80b6 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -20,10 +20,11 @@
> * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> */
>
> -#include <linux/types.h>
> +#include <linux/compat.h>
> +#include <linux/export.h>
> #include <linux/ioctl.h>
> #include <linux/media.h>
> -#include <linux/export.h>
> +#include <linux/types.h>
>
> #include <media/media-device.h>
> #include <media/media-devnode.h>
> @@ -124,35 +125,31 @@ static void media_device_kpad_to_upad(const struct media_pad *kpad,
> upad->flags = kpad->flags;
> }
>
> -static long media_device_enum_links(struct media_device *mdev,
> - struct media_links_enum __user *ulinks)
> +static long __media_device_enum_links(struct media_device *mdev,
> + struct media_links_enum *links)
> {
> struct media_entity *entity;
> - struct media_links_enum links;
>
> - if (copy_from_user(&links, ulinks, sizeof(links)))
> - return -EFAULT;
> -
> - entity = find_entity(mdev, links.entity);
> + entity = find_entity(mdev, links->entity);
> if (entity == NULL)
> return -EINVAL;
>
> - if (links.pads) {
> + if (links->pads) {
> unsigned int p;
>
> for (p = 0; p < entity->num_pads; p++) {
> struct media_pad_desc pad;
> media_device_kpad_to_upad(&entity->pads[p], &pad);
> - if (copy_to_user(&links.pads[p], &pad, sizeof(pad)))
> + if (copy_to_user(&links->pads[p], &pad, sizeof(pad)))
> return -EFAULT;
> }
> }
>
> - if (links.links) {
> + if (links->links) {
> struct media_link_desc __user *ulink;
> unsigned int l;
>
> - for (l = 0, ulink = links.links; l < entity->num_links; l++) {
> + for (l = 0, ulink = links->links; l < entity->num_links; l++) {
> struct media_link_desc link;
>
> /* Ignore backlinks. */
> @@ -169,8 +166,26 @@ static long media_device_enum_links(struct media_device *mdev,
> ulink++;
> }
> }
> +
> + return 0;
> +}
> +
> +static long media_device_enum_links(struct media_device *mdev,
> + struct media_links_enum __user *ulinks)
> +{
> + struct media_links_enum links;
> + int rval;
> +
> + if (copy_from_user(&links, ulinks, sizeof(links)))
> + return -EFAULT;
> +
> + rval = __media_device_enum_links(mdev, &links);
> + if (rval < 0)
> + return rval;
> +
> if (copy_to_user(ulinks, &links, sizeof(*ulinks)))
> return -EFAULT;
> +
> return 0;
> }
>
> @@ -251,10 +266,71 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
> return ret;
> }
>
> +#ifdef CONFIG_COMPAT
> +
> +struct media_links_enum32 {
> + __u32 entity;
> + compat_uptr_t pads; /* struct media_pad_desc * */
> + compat_uptr_t links; /* struct media_link_desc * */
> + __u32 reserved[4];
> +};
> +
> +static long media_device_enum_links32(struct media_device *mdev,
> + struct media_links_enum32 __user *ulinks)
> +{
> + struct media_links_enum links;
> + compat_uptr_t pads_ptr, links_ptr;
> +
> + memset(&links, 0, sizeof(links));
> +
> + if (get_user(links.entity, &ulinks->entity)
> + || get_user(pads_ptr, &ulinks->pads)
> + || get_user(links_ptr, &ulinks->links))
> + return -EFAULT;
> +
> + links.pads = compat_ptr(pads_ptr);
> + links.links = compat_ptr(links_ptr);
> +
> + return __media_device_enum_links(mdev, &links);
> +}
> +
> +#define MEDIA_IOC_ENUM_LINKS32 _IOWR('|', 0x02, struct media_links_enum32)
> +
> +static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct media_devnode *devnode = media_devnode_data(filp);
> + struct media_device *dev = to_media_device(devnode);
> + long ret;
> +
> + switch (cmd) {
> + case MEDIA_IOC_DEVICE_INFO:
> + case MEDIA_IOC_ENUM_ENTITIES:
> + case MEDIA_IOC_SETUP_LINK:
> + return media_device_ioctl(filp, cmd, arg);
> +
> + case MEDIA_IOC_ENUM_LINKS32:
> + mutex_lock(&dev->graph_mutex);
> + ret = media_device_enum_links32(dev,
> + (struct media_links_enum32 __user *)arg);
> + mutex_unlock(&dev->graph_mutex);
> + break;
> +
> + default:
> + ret = -ENOIOCTLCMD;
> + }
> +
> + return ret;
> +}
> +#endif /* CONFIG_COMPAT */
> +
> static const struct media_file_operations media_device_fops = {
> .owner = THIS_MODULE,
> .open = media_device_open,
> .ioctl = media_device_ioctl,
> +#ifdef CONFIG_COMPAT
> + .compat_ioctl = media_device_compat_ioctl,
> +#endif /* CONFIG_COMPAT */
> .release = media_device_close,
> };
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] media: implement 32-on-64 bit compat IOCTL handling
2013-01-22 19:03 ` Hans Verkuil
@ 2013-01-22 19:43 ` Sakari Ailus
0 siblings, 0 replies; 5+ messages in thread
From: Sakari Ailus @ 2013-01-22 19:43 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media
Hi Hans,
Thanks for the comments!
On Tue, Jan 22, 2013 at 08:03:01PM +0100, Hans Verkuil wrote:
> On Tue January 22 2013 17:27:56 Sakari Ailus wrote:
> > Use the same handlers where the structs are the same. Implement a new
> > handler for link enumeration since struct media_links_enum is different on
> > 32-bit and 64-bit systems.
>
> I think I would prefer to have the compat handling split off into a
> seperate source. I know it is just a small amount of code at the moment,
> but that's the way it is done as well for the V4L2 ioctls and I rather
> like that approach.
>
> What do others think about that?
I pondered the possibility but thought that as it's not much code so it'd be
fine in the same file. I agree that hiding the compat code out of sight in
V4L2 works really well --- it's mostly uninteresting and only needs to be
touched when the API changes. But there's also much, much more code and
handling to do in V4L2. I doubt the MC will ever be nearly as large unless
the functionality and the problem area of the API changes significantly.
I have nothing against putting this into a separate file, though. :-)
--
Cheers,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-01-22 19:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-22 16:23 [PATCH 0/2] Compat IOCTL handler for Media controller Sakari Ailus
2013-01-22 16:27 ` [PATCH 1/2] media: Add 64--32 bit compat ioctl handler Sakari Ailus
2013-01-22 16:27 ` [PATCH 2/2] media: implement 32-on-64 bit compat IOCTL handling Sakari Ailus
2013-01-22 19:03 ` Hans Verkuil
2013-01-22 19:43 ` Sakari Ailus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.