Message ID | 1433849204-4125-3-git-send-email-patrik.jakobsson@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, 9 Jun 2015 13:26:42 +0200 Patrik Jakobsson <patrik.jakobsson@linux.intel.com> wrote: > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > --- > Makefile.am | 1 + > defs.h | 6 ++++- > drm.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > io.c | 2 +- > ioctl.c | 13 ++++++++- > 5 files changed, 107 insertions(+), 3 deletions(-) > create mode 100644 drm.c > > diff --git a/Makefile.am b/Makefile.am > index 549aebc..50d5140 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -121,6 +121,7 @@ strace_SOURCES = \ > utime.c \ > utimes.c \ > v4l2.c \ > + drm.c \ > vsprintf.c \ > wait.c \ > xattr.c If I remember correctly source files were sorted in alphabetic order. Same remark for drm_i915.c in the next patch. (This is nitpicking) > diff --git a/defs.h b/defs.h > index 77c819c..f77330b 100644 > --- a/defs.h > +++ b/defs.h > @@ -559,7 +559,7 @@ extern const struct_ioctlent *ioctl_lookup(const unsigned int); > extern const struct_ioctlent *ioctl_next_match(const struct_ioctlent *); > extern void ioctl_print_code(const unsigned int); > extern int ioctl_decode(struct tcb *, const unsigned int, long); > -extern int ioctl_decode_command_number(const unsigned int); > +extern int ioctl_decode_command_number(struct tcb *, const unsigned int); > extern int block_ioctl(struct tcb *, const unsigned int, long); > extern int evdev_ioctl(struct tcb *, const unsigned int, long); > extern int loop_ioctl(struct tcb *, const unsigned int, long); > @@ -572,6 +572,10 @@ extern int term_ioctl(struct tcb *, const unsigned int, long); > extern int ubi_ioctl(struct tcb *, const unsigned int, long); > extern int v4l2_ioctl(struct tcb *, const unsigned int, long); > > +extern int drm_is_priv(const unsigned int); > +extern int drm_is_driver(struct tcb *tcp, const char *name); > +extern int drm_ioctl(struct tcb *, const unsigned int, long); > + > extern int tv_nz(const struct timeval *); > extern int tv_cmp(const struct timeval *, const struct timeval *); > extern double tv_float(const struct timeval *); > diff --git a/drm.c b/drm.c > new file mode 100644 > index 0000000..56ef98b > --- /dev/null > +++ b/drm.c > @@ -0,0 +1,88 @@ > +/* > + * Copyright (c) 2015 Intel Corporation > + * All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * 3. The name of the author may not be used to endorse or promote products > + * derived from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR > + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES > + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. > + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, > + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT > + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + * > + * Authors: > + * Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > + */ > + > +#include "defs.h" > + > +#include <sys/types.h> > +#include <unistd.h> > +#include <string.h> > +#include <linux/limits.h> > +#include <stdint.h> > +#include <sys/ioctl.h> > +#include <linux/types.h> > +#include <drm.h> If I remember correctly, headers are mostly sorted in strace files also > + > +#define DRM_MAX_NAME_LEN 128 > + > +inline int drm_is_priv(const unsigned int num) > +{ > + return (_IOC_NR(num) >= DRM_COMMAND_BASE && > + _IOC_NR(num) < DRM_COMMAND_END); > +} > + > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) > +{ > + char path[PATH_MAX]; > + char link[PATH_MAX]; > + int ret; > + > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); > + if (!ret) > + return ret; > + > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", > + basename(path)); > + > + ret = readlink(link, path, PATH_MAX - 1); > + if (ret < 0) > + return ret; > + > + path[ret] = '\0'; > + strncpy(name, basename(path), bufsize); > + > + return 0; > +} > + > +int drm_is_driver(struct tcb *tcp, const char *name) > +{ > + char drv[DRM_MAX_NAME_LEN]; > + int ret; > + > + ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN); > + if (ret) > + return 0; > + > + return strcmp(name, drv) == 0; > +} > + > +int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg) > +{ > + return 0; > +} > diff --git a/io.c b/io.c > index 30ed578..6810a45 100644 > --- a/io.c > +++ b/io.c > @@ -391,7 +391,7 @@ SYS_FUNC(ioctl) > if (entering(tcp)) { > printfd(tcp, tcp->u_arg[0]); > tprints(", "); > - if (!ioctl_decode_command_number(tcp->u_arg[1])) { > + if (!ioctl_decode_command_number(tcp, tcp->u_arg[1])) { > iop = ioctl_lookup(tcp->u_arg[1]); > if (iop) { > tprints(iop->symbol); > diff --git a/ioctl.c b/ioctl.c > index c67d048..690e7aa 100644 > --- a/ioctl.c > +++ b/ioctl.c > @@ -181,8 +181,14 @@ hiddev_decode_number(unsigned int arg) > return 0; > } > > +static int > +drm_decode_number(struct tcb *tcp, unsigned int arg) > +{ > + return 0; > +} > + If you push drm_decode_number() inside drm.c you will have less function to export, and avoir polluting to much defs.h. For the other *_decode_number(), there was no files with the specific argument decoding attached to it, so it maked sense at that time to not have separated files for each of them (this makes me think that for evdev ioctls, we could move that part inside evdev.c.) > int > -ioctl_decode_command_number(unsigned int arg) > +ioctl_decode_command_number(struct tcb *tcp, unsigned int arg) > { > switch (_IOC_TYPE(arg)) { > case 'E': > @@ -216,6 +222,8 @@ ioctl_decode_command_number(unsigned int arg) > return 1; > } > return 0; > + case 'd': > + return drm_decode_number(tcp, arg); > default: > return 0; > } > @@ -252,6 +260,8 @@ ioctl_decode(struct tcb *tcp, unsigned int code, long arg) > return ubi_ioctl(tcp, code, arg); > case 'V': > return v4l2_ioctl(tcp, code, arg); > + case 'd': > + return drm_ioctl(tcp, code, arg); > case '=': > return ptp_ioctl(tcp, code, arg); > #ifdef HAVE_LINUX_INPUT_H > @@ -284,6 +294,7 @@ ioctl_decode(struct tcb *tcp, unsigned int code, long arg) > * d sys/des.h (possible overlap) > * d vax/dkio.h (possible overlap) > * d vaxuba/rxreg.h (possible overlap) > + * d drm/drm.h > * f sys/filio.h > * g sunwindow/win_ioctl.h -no overlap- > * g sunwindowdev/winioctl.c !no manifest constant! -no overlap-
On Tue, Jun 09, 2015 at 03:51:10PM +0200, Gabriel Laskar wrote: > On Tue, 9 Jun 2015 13:26:42 +0200 > Patrik Jakobsson <patrik.jakobsson@linux.intel.com> wrote: > > > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > --- > > Makefile.am | 1 + > > defs.h | 6 ++++- > > drm.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > io.c | 2 +- > > ioctl.c | 13 ++++++++- > > 5 files changed, 107 insertions(+), 3 deletions(-) > > create mode 100644 drm.c > > > > diff --git a/Makefile.am b/Makefile.am > > index 549aebc..50d5140 100644 > > --- a/Makefile.am > > +++ b/Makefile.am > > @@ -121,6 +121,7 @@ strace_SOURCES = \ > > utime.c \ > > utimes.c \ > > v4l2.c \ > > + drm.c \ > > vsprintf.c \ > > wait.c \ > > xattr.c > > If I remember correctly source files were sorted in alphabetic order. > Same remark for drm_i915.c in the next patch. > > (This is nitpicking) Ah yes, totally unacceptable ;). Will fix. > > > diff --git a/defs.h b/defs.h > > index 77c819c..f77330b 100644 > > --- a/defs.h > > +++ b/defs.h > > @@ -559,7 +559,7 @@ extern const struct_ioctlent *ioctl_lookup(const unsigned int); > > extern const struct_ioctlent *ioctl_next_match(const struct_ioctlent *); > > extern void ioctl_print_code(const unsigned int); > > extern int ioctl_decode(struct tcb *, const unsigned int, long); > > -extern int ioctl_decode_command_number(const unsigned int); > > +extern int ioctl_decode_command_number(struct tcb *, const unsigned int); > > extern int block_ioctl(struct tcb *, const unsigned int, long); > > extern int evdev_ioctl(struct tcb *, const unsigned int, long); > > extern int loop_ioctl(struct tcb *, const unsigned int, long); > > @@ -572,6 +572,10 @@ extern int term_ioctl(struct tcb *, const unsigned int, long); > > extern int ubi_ioctl(struct tcb *, const unsigned int, long); > > extern int v4l2_ioctl(struct tcb *, const unsigned int, long); > > > > +extern int drm_is_priv(const unsigned int); > > +extern int drm_is_driver(struct tcb *tcp, const char *name); > > +extern int drm_ioctl(struct tcb *, const unsigned int, long); > > + > > extern int tv_nz(const struct timeval *); > > extern int tv_cmp(const struct timeval *, const struct timeval *); > > extern double tv_float(const struct timeval *); > > diff --git a/drm.c b/drm.c > > new file mode 100644 > > index 0000000..56ef98b > > --- /dev/null > > +++ b/drm.c > > @@ -0,0 +1,88 @@ > > +/* > > + * Copyright (c) 2015 Intel Corporation > > + * All rights reserved. > > + * > > + * Redistribution and use in source and binary forms, with or without > > + * modification, are permitted provided that the following conditions > > + * are met: > > + * 1. Redistributions of source code must retain the above copyright > > + * notice, this list of conditions and the following disclaimer. > > + * 2. Redistributions in binary form must reproduce the above copyright > > + * notice, this list of conditions and the following disclaimer in the > > + * documentation and/or other materials provided with the distribution. > > + * 3. The name of the author may not be used to endorse or promote products > > + * derived from this software without specific prior written permission. > > + * > > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR > > + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES > > + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. > > + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, > > + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT > > + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF > > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > + * > > + * Authors: > > + * Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > + */ > > + > > +#include "defs.h" > > + > > +#include <sys/types.h> > > +#include <unistd.h> > > +#include <string.h> > > +#include <linux/limits.h> > > +#include <stdint.h> > > +#include <sys/ioctl.h> > > +#include <linux/types.h> > > +#include <drm.h> > > If I remember correctly, headers are mostly sorted in strace files also I'll take a look to see if I can find what pattern to stick with here. > > > + > > +#define DRM_MAX_NAME_LEN 128 > > + > > +inline int drm_is_priv(const unsigned int num) > > +{ > > + return (_IOC_NR(num) >= DRM_COMMAND_BASE && > > + _IOC_NR(num) < DRM_COMMAND_END); > > +} > > + > > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) > > +{ > > + char path[PATH_MAX]; > > + char link[PATH_MAX]; > > + int ret; > > + > > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); > > + if (!ret) > > + return ret; > > + > > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", > > + basename(path)); > > + > > + ret = readlink(link, path, PATH_MAX - 1); > > + if (ret < 0) > > + return ret; > > + > > + path[ret] = '\0'; > > + strncpy(name, basename(path), bufsize); > > + > > + return 0; > > +} > > + > > +int drm_is_driver(struct tcb *tcp, const char *name) > > +{ > > + char drv[DRM_MAX_NAME_LEN]; > > + int ret; > > + > > + ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN); > > + if (ret) > > + return 0; > > + > > + return strcmp(name, drv) == 0; > > +} > > + > > +int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg) > > +{ > > + return 0; > > +} > > diff --git a/io.c b/io.c > > index 30ed578..6810a45 100644 > > --- a/io.c > > +++ b/io.c > > @@ -391,7 +391,7 @@ SYS_FUNC(ioctl) > > if (entering(tcp)) { > > printfd(tcp, tcp->u_arg[0]); > > tprints(", "); > > - if (!ioctl_decode_command_number(tcp->u_arg[1])) { > > + if (!ioctl_decode_command_number(tcp, tcp->u_arg[1])) { > > iop = ioctl_lookup(tcp->u_arg[1]); > > if (iop) { > > tprints(iop->symbol); > > diff --git a/ioctl.c b/ioctl.c > > index c67d048..690e7aa 100644 > > --- a/ioctl.c > > +++ b/ioctl.c > > @@ -181,8 +181,14 @@ hiddev_decode_number(unsigned int arg) > > return 0; > > } > > > > +static int > > +drm_decode_number(struct tcb *tcp, unsigned int arg) > > +{ > > + return 0; > > +} > > + > > If you push drm_decode_number() inside drm.c you will have less > function to export, and avoir polluting to much defs.h. > > For the other *_decode_number(), there was no files with the specific > argument decoding attached to it, so it maked sense at that time to not > have separated files for each of them (this makes me think that for > evdev ioctls, we could move that part inside evdev.c.) > I agree, will do. > > int > > -ioctl_decode_command_number(unsigned int arg) > > +ioctl_decode_command_number(struct tcb *tcp, unsigned int arg) > > { > > switch (_IOC_TYPE(arg)) { > > case 'E': > > @@ -216,6 +222,8 @@ ioctl_decode_command_number(unsigned int arg) > > return 1; > > } > > return 0; > > + case 'd': > > + return drm_decode_number(tcp, arg); > > default: > > return 0; > > } > > @@ -252,6 +260,8 @@ ioctl_decode(struct tcb *tcp, unsigned int code, long arg) > > return ubi_ioctl(tcp, code, arg); > > case 'V': > > return v4l2_ioctl(tcp, code, arg); > > + case 'd': > > + return drm_ioctl(tcp, code, arg); > > case '=': > > return ptp_ioctl(tcp, code, arg); > > #ifdef HAVE_LINUX_INPUT_H > > @@ -284,6 +294,7 @@ ioctl_decode(struct tcb *tcp, unsigned int code, long arg) > > * d sys/des.h (possible overlap) > > * d vax/dkio.h (possible overlap) > > * d vaxuba/rxreg.h (possible overlap) > > + * d drm/drm.h > > * f sys/filio.h > > * g sunwindow/win_ioctl.h -no overlap- > > * g sunwindowdev/winioctl.c !no manifest constant! -no overlap- > > > > -- > Gabriel Laskar
On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote: [...] > --- a/Makefile.am > +++ b/Makefile.am > @@ -121,6 +121,7 @@ strace_SOURCES = \ > utime.c \ > utimes.c \ > v4l2.c \ > + drm.c \ > vsprintf.c \ > wait.c \ > xattr.c Starting with v4.7-166-g7ae73a9, we keep strace_SOURCES list sorted. > --- /dev/null > +++ b/drm.c [...] > +#include "defs.h" > + > +#include <sys/types.h> > +#include <unistd.h> > +#include <string.h> > +#include <linux/limits.h> > +#include <stdint.h> > +#include <sys/ioctl.h> > +#include <linux/types.h> > +#include <drm.h> No need to include header files already included by "defs.h". Most likely no need to include <linux/limits.h> or <linux/types.h>. > +#define DRM_MAX_NAME_LEN 128 > + > +inline int drm_is_priv(const unsigned int num) > +{ > + return (_IOC_NR(num) >= DRM_COMMAND_BASE && > + _IOC_NR(num) < DRM_COMMAND_END); > +} > + > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) > +{ > + char path[PATH_MAX]; > + char link[PATH_MAX]; > + int ret; > + > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); > + if (!ret) > + return ret; > + > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", > + basename(path)); > + > + ret = readlink(link, path, PATH_MAX - 1); > + if (ret < 0) > + return ret; > + > + path[ret] = '\0'; > + strncpy(name, basename(path), bufsize); > + > + return 0; > +} > + > +int drm_is_driver(struct tcb *tcp, const char *name) > +{ > + char drv[DRM_MAX_NAME_LEN]; > + int ret; > + > + ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN); > + if (ret) > + return 0; > + > + return strcmp(name, drv) == 0; > +} This interface will result to several getfdpath() calls per ioctl_decode(). If the only purpose of drm_is_driver() is to help finding the most appropriate function, let's create a table of pairs {driver name, function} and pass this table to a function that will do a single getfdpath() call, calculate the driver name, and choose the right function from the table. [...] > @@ -284,6 +294,7 @@ ioctl_decode(struct tcb *tcp, unsigned int code, long arg) > * d sys/des.h (possible overlap) > * d vax/dkio.h (possible overlap) > * d vaxuba/rxreg.h (possible overlap) > + * d drm/drm.h > * f sys/filio.h > * g sunwindow/win_ioctl.h -no overlap- > * g sunwindowdev/winioctl.c !no manifest constant! -no overlap- This is a history, no need to patch it.
On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote: [...] > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) > +{ > + char path[PATH_MAX]; > + char link[PATH_MAX]; > + int ret; > + > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); > + if (!ret) > + return ret; The return code of getfdpath() is essentially the return code of readlink(), so the check should be the same as after readlink() below. > + > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", > + basename(path)); > + > + ret = readlink(link, path, PATH_MAX - 1); > + if (ret < 0) > + return ret; > + > + path[ret] = '\0'; > + strncpy(name, basename(path), bufsize); > + > + return 0; > +}
On Wed, Jun 10, 2015 at 01:14:20AM +0300, Dmitry V. Levin wrote: > On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote: > [...] > > --- a/Makefile.am > > +++ b/Makefile.am > > @@ -121,6 +121,7 @@ strace_SOURCES = \ > > utime.c \ > > utimes.c \ > > v4l2.c \ > > + drm.c \ > > vsprintf.c \ > > wait.c \ > > xattr.c > > Starting with v4.7-166-g7ae73a9, we keep strace_SOURCES list sorted. Fixed in v2 > > > --- /dev/null > > +++ b/drm.c > [...] > > +#include "defs.h" > > + > > +#include <sys/types.h> > > +#include <unistd.h> > > +#include <string.h> > > +#include <linux/limits.h> > > +#include <stdint.h> > > +#include <sys/ioctl.h> > > +#include <linux/types.h> > > +#include <drm.h> > > No need to include header files already included by "defs.h". > Most likely no need to include <linux/limits.h> or <linux/types.h>. Fixed in v2 > > > +#define DRM_MAX_NAME_LEN 128 > > + > > +inline int drm_is_priv(const unsigned int num) > > +{ > > + return (_IOC_NR(num) >= DRM_COMMAND_BASE && > > + _IOC_NR(num) < DRM_COMMAND_END); > > +} > > + > > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) > > +{ > > + char path[PATH_MAX]; > > + char link[PATH_MAX]; > > + int ret; > > + > > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); > > + if (!ret) > > + return ret; > > + > > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", > > + basename(path)); > > + > > + ret = readlink(link, path, PATH_MAX - 1); > > + if (ret < 0) > > + return ret; > > + > > + path[ret] = '\0'; > > + strncpy(name, basename(path), bufsize); > > + > > + return 0; > > +} > > + > > +int drm_is_driver(struct tcb *tcp, const char *name) > > +{ > > + char drv[DRM_MAX_NAME_LEN]; > > + int ret; > > + > > + ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN); > > + if (ret) > > + return 0; > > + > > + return strcmp(name, drv) == 0; > > +} > > This interface will result to several getfdpath() calls per > ioctl_decode(). If the only purpose of drm_is_driver() is to help finding > the most appropriate function, let's create a table of pairs > {driver name, function} and pass this table to a function that will do a > single getfdpath() call, calculate the driver name, and choose the right > function from the table. Yes I was thinking the same thing but it's a bit tricky. What I need is: fd -> path -> driver name. And fd -> path could change between ioctls. It is not a likely scenario but it's possible. I could get rid of the extra call in drm_decode_number() if I put it back in drm_ioctl as in my RFC. I could also optimize path -> driver name with a table but I don't know how expensive those calls actually are. Not sure what would be the best solution here. > > [...] > > @@ -284,6 +294,7 @@ ioctl_decode(struct tcb *tcp, unsigned int code, long arg) > > * d sys/des.h (possible overlap) > > * d vax/dkio.h (possible overlap) > > * d vaxuba/rxreg.h (possible overlap) > > + * d drm/drm.h > > * f sys/filio.h > > * g sunwindow/win_ioctl.h -no overlap- > > * g sunwindowdev/winioctl.c !no manifest constant! -no overlap- > > This is a history, no need to patch it. Fixed in v2 > > > -- > ldv
On Wed, Jun 10, 2015 at 01:52:33PM +0200, Patrik Jakobsson wrote: > On Wed, Jun 10, 2015 at 01:14:20AM +0300, Dmitry V. Levin wrote: > > On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote: [...] > > > +#define DRM_MAX_NAME_LEN 128 > > > + > > > +inline int drm_is_priv(const unsigned int num) > > > +{ > > > + return (_IOC_NR(num) >= DRM_COMMAND_BASE && > > > + _IOC_NR(num) < DRM_COMMAND_END); > > > +} > > > + > > > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) > > > +{ > > > + char path[PATH_MAX]; > > > + char link[PATH_MAX]; > > > + int ret; > > > + > > > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); > > > + if (!ret) > > > + return ret; > > > + > > > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", > > > + basename(path)); > > > + > > > + ret = readlink(link, path, PATH_MAX - 1); > > > + if (ret < 0) > > > + return ret; > > > + > > > + path[ret] = '\0'; > > > + strncpy(name, basename(path), bufsize); > > > + > > > + return 0; > > > +} > > > + > > > +int drm_is_driver(struct tcb *tcp, const char *name) > > > +{ > > > + char drv[DRM_MAX_NAME_LEN]; > > > + int ret; > > > + > > > + ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN); > > > + if (ret) > > > + return 0; > > > + > > > + return strcmp(name, drv) == 0; > > > +} > > > > This interface will result to several getfdpath() calls per > > ioctl_decode(). If the only purpose of drm_is_driver() is to help finding > > the most appropriate function, let's create a table of pairs > > {driver name, function} and pass this table to a function that will do a > > single getfdpath() call, calculate the driver name, and choose the right > > function from the table. > > Yes I was thinking the same thing but it's a bit tricky. What I need is: > fd -> path -> driver name. And fd -> path could change between ioctls. It is not > a likely scenario but it's possible. I could get rid of the extra call in > drm_decode_number() if I put it back in drm_ioctl as in my RFC. I could also > optimize path -> driver name with a table but I don't know how expensive those > calls actually are. Not sure what would be the best solution here. drm_get_driver_name() is quite expensive as it does two readlink syscalls, so it should be called at most once per ioctl_decode(). Another method to achieve this is to change drm_get_driver_name() to return basename(path) instead of return code, so that drm_ioctl() would call it once and pass the result to strcmp calls: int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg) { if (verbose(tcp) && drm_is_priv(code)) { const char *driver = drm_get_driver_name(tcp); if (!driver) return 0; if (!strcmp(driver, "i915")) return drm_i915_ioctl(tcp, code, arg); } return 0; }
On Thu, Jun 11, 2015 at 02:26:59AM +0300, Dmitry V. Levin wrote: > On Wed, Jun 10, 2015 at 01:52:33PM +0200, Patrik Jakobsson wrote: > > On Wed, Jun 10, 2015 at 01:14:20AM +0300, Dmitry V. Levin wrote: > > > On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote: > [...] > > > > +#define DRM_MAX_NAME_LEN 128 > > > > + > > > > +inline int drm_is_priv(const unsigned int num) > > > > +{ > > > > + return (_IOC_NR(num) >= DRM_COMMAND_BASE && > > > > + _IOC_NR(num) < DRM_COMMAND_END); > > > > +} > > > > + > > > > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) > > > > +{ > > > > + char path[PATH_MAX]; > > > > + char link[PATH_MAX]; > > > > + int ret; > > > > + > > > > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); > > > > + if (!ret) > > > > + return ret; > > > > + > > > > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", > > > > + basename(path)); > > > > + > > > > + ret = readlink(link, path, PATH_MAX - 1); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + path[ret] = '\0'; > > > > + strncpy(name, basename(path), bufsize); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int drm_is_driver(struct tcb *tcp, const char *name) > > > > +{ > > > > + char drv[DRM_MAX_NAME_LEN]; > > > > + int ret; > > > > + > > > > + ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN); > > > > + if (ret) > > > > + return 0; > > > > + > > > > + return strcmp(name, drv) == 0; > > > > +} > > > > > > This interface will result to several getfdpath() calls per > > > ioctl_decode(). If the only purpose of drm_is_driver() is to help finding > > > the most appropriate function, let's create a table of pairs > > > {driver name, function} and pass this table to a function that will do a > > > single getfdpath() call, calculate the driver name, and choose the right > > > function from the table. > > > > Yes I was thinking the same thing but it's a bit tricky. What I need is: > > fd -> path -> driver name. And fd -> path could change between ioctls. It is not > > a likely scenario but it's possible. I could get rid of the extra call in > > drm_decode_number() if I put it back in drm_ioctl as in my RFC. I could also > > optimize path -> driver name with a table but I don't know how expensive those > > calls actually are. Not sure what would be the best solution here. > > drm_get_driver_name() is quite expensive as it does two readlink syscalls, > so it should be called at most once per ioctl_decode(). > > Another method to achieve this is to change drm_get_driver_name() to return > basename(path) instead of return code, so that drm_ioctl() would call it > once and pass the result to strcmp calls: > > int > drm_ioctl(struct tcb *tcp, const unsigned int code, long arg) > { > if (verbose(tcp) && drm_is_priv(code)) { > const char *driver = drm_get_driver_name(tcp); > if (!driver) > return 0; > if (!strcmp(driver, "i915")) > return drm_i915_ioctl(tcp, code, arg); > } > return 0; > } I misunderstood you. As you say, we shouldn't do a drm_get_driver_name() more than once (no matter how many drm drivers we are looking for) so your suggestion above would fix that. I was thinking about how to get rid of the extra call in drm_decode_number() (if we could somehow squash them together). But that would make things rather ugly. If ok with you we could just have the same approach in drm_decode_number() as above and live with the fact that we get two calls to drm_get_driver_name() per DRM device specific ioctl. One for drm_decode_number() and one for drm_ioctl(). > > > -- > ldv > ------------------------------------------------------------------------------ > _______________________________________________ > Strace-devel mailing list > Strace-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/strace-devel
On Thu, Jun 11, 2015 at 04:11:49PM +0200, Patrik Jakobsson wrote: > On Thu, Jun 11, 2015 at 02:26:59AM +0300, Dmitry V. Levin wrote: > > On Wed, Jun 10, 2015 at 01:52:33PM +0200, Patrik Jakobsson wrote: > > > On Wed, Jun 10, 2015 at 01:14:20AM +0300, Dmitry V. Levin wrote: > > > > On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote: > > [...] > > > > > +#define DRM_MAX_NAME_LEN 128 > > > > > + > > > > > +inline int drm_is_priv(const unsigned int num) > > > > > +{ > > > > > + return (_IOC_NR(num) >= DRM_COMMAND_BASE && > > > > > + _IOC_NR(num) < DRM_COMMAND_END); > > > > > +} > > > > > + > > > > > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) > > > > > +{ > > > > > + char path[PATH_MAX]; > > > > > + char link[PATH_MAX]; > > > > > + int ret; > > > > > + > > > > > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); > > > > > + if (!ret) > > > > > + return ret; > > > > > + > > > > > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", > > > > > + basename(path)); > > > > > + > > > > > + ret = readlink(link, path, PATH_MAX - 1); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > + path[ret] = '\0'; > > > > > + strncpy(name, basename(path), bufsize); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +int drm_is_driver(struct tcb *tcp, const char *name) > > > > > +{ > > > > > + char drv[DRM_MAX_NAME_LEN]; > > > > > + int ret; > > > > > + > > > > > + ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN); > > > > > + if (ret) > > > > > + return 0; > > > > > + > > > > > + return strcmp(name, drv) == 0; > > > > > +} > > > > > > > > This interface will result to several getfdpath() calls per > > > > ioctl_decode(). If the only purpose of drm_is_driver() is to help finding > > > > the most appropriate function, let's create a table of pairs > > > > {driver name, function} and pass this table to a function that will do a > > > > single getfdpath() call, calculate the driver name, and choose the right > > > > function from the table. > > > > > > Yes I was thinking the same thing but it's a bit tricky. What I need is: > > > fd -> path -> driver name. And fd -> path could change between ioctls. It is not > > > a likely scenario but it's possible. I could get rid of the extra call in > > > drm_decode_number() if I put it back in drm_ioctl as in my RFC. I could also > > > optimize path -> driver name with a table but I don't know how expensive those > > > calls actually are. Not sure what would be the best solution here. > > > > drm_get_driver_name() is quite expensive as it does two readlink syscalls, > > so it should be called at most once per ioctl_decode(). > > > > Another method to achieve this is to change drm_get_driver_name() to return > > basename(path) instead of return code, so that drm_ioctl() would call it > > once and pass the result to strcmp calls: > > > > int > > drm_ioctl(struct tcb *tcp, const unsigned int code, long arg) > > { > > if (verbose(tcp) && drm_is_priv(code)) { > > const char *driver = drm_get_driver_name(tcp); > > if (!driver) > > return 0; > > if (!strcmp(driver, "i915")) > > return drm_i915_ioctl(tcp, code, arg); > > } > > return 0; > > } > > I misunderstood you. As you say, we shouldn't do a drm_get_driver_name() more > than once (no matter how many drm drivers we are looking for) so your suggestion > above would fix that. I was thinking about how to get rid of the extra call in > drm_decode_number() (if we could somehow squash them together). But that would > make things rather ugly. If ok with you we could just have the same approach in > drm_decode_number() as above and live with the fact that we get two calls to > drm_get_driver_name() per DRM device specific ioctl. One for drm_decode_number() > and one for drm_ioctl(). This way we would end up with three drm_get_driver_name() calls per ioctl: twice on entering syscall and once on exiting. Maybe we could cache result of the first of these three calls somewhere?
On Sat, Jun 13, 2015 at 1:41 AM, Dmitry V. Levin <ldv@altlinux.org> wrote: > On Thu, Jun 11, 2015 at 04:11:49PM +0200, Patrik Jakobsson wrote: >> On Thu, Jun 11, 2015 at 02:26:59AM +0300, Dmitry V. Levin wrote: >> > On Wed, Jun 10, 2015 at 01:52:33PM +0200, Patrik Jakobsson wrote: >> > > On Wed, Jun 10, 2015 at 01:14:20AM +0300, Dmitry V. Levin wrote: >> > > > On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote: >> > [...] >> > > > > +#define DRM_MAX_NAME_LEN 128 >> > > > > + >> > > > > +inline int drm_is_priv(const unsigned int num) >> > > > > +{ >> > > > > + return (_IOC_NR(num) >= DRM_COMMAND_BASE && >> > > > > + _IOC_NR(num) < DRM_COMMAND_END); >> > > > > +} >> > > > > + >> > > > > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) >> > > > > +{ >> > > > > + char path[PATH_MAX]; >> > > > > + char link[PATH_MAX]; >> > > > > + int ret; >> > > > > + >> > > > > + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); >> > > > > + if (!ret) >> > > > > + return ret; >> > > > > + >> > > > > + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", >> > > > > + basename(path)); >> > > > > + >> > > > > + ret = readlink(link, path, PATH_MAX - 1); >> > > > > + if (ret < 0) >> > > > > + return ret; >> > > > > + >> > > > > + path[ret] = '\0'; >> > > > > + strncpy(name, basename(path), bufsize); >> > > > > + >> > > > > + return 0; >> > > > > +} >> > > > > + >> > > > > +int drm_is_driver(struct tcb *tcp, const char *name) >> > > > > +{ >> > > > > + char drv[DRM_MAX_NAME_LEN]; >> > > > > + int ret; >> > > > > + >> > > > > + ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN); >> > > > > + if (ret) >> > > > > + return 0; >> > > > > + >> > > > > + return strcmp(name, drv) == 0; >> > > > > +} >> > > > >> > > > This interface will result to several getfdpath() calls per >> > > > ioctl_decode(). If the only purpose of drm_is_driver() is to help finding >> > > > the most appropriate function, let's create a table of pairs >> > > > {driver name, function} and pass this table to a function that will do a >> > > > single getfdpath() call, calculate the driver name, and choose the right >> > > > function from the table. >> > > >> > > Yes I was thinking the same thing but it's a bit tricky. What I need is: >> > > fd -> path -> driver name. And fd -> path could change between ioctls. It is not >> > > a likely scenario but it's possible. I could get rid of the extra call in >> > > drm_decode_number() if I put it back in drm_ioctl as in my RFC. I could also >> > > optimize path -> driver name with a table but I don't know how expensive those >> > > calls actually are. Not sure what would be the best solution here. >> > >> > drm_get_driver_name() is quite expensive as it does two readlink syscalls, >> > so it should be called at most once per ioctl_decode(). >> > >> > Another method to achieve this is to change drm_get_driver_name() to return >> > basename(path) instead of return code, so that drm_ioctl() would call it >> > once and pass the result to strcmp calls: >> > >> > int >> > drm_ioctl(struct tcb *tcp, const unsigned int code, long arg) >> > { >> > if (verbose(tcp) && drm_is_priv(code)) { >> > const char *driver = drm_get_driver_name(tcp); >> > if (!driver) >> > return 0; >> > if (!strcmp(driver, "i915")) >> > return drm_i915_ioctl(tcp, code, arg); >> > } >> > return 0; >> > } >> >> I misunderstood you. As you say, we shouldn't do a drm_get_driver_name() more >> than once (no matter how many drm drivers we are looking for) so your suggestion >> above would fix that. I was thinking about how to get rid of the extra call in >> drm_decode_number() (if we could somehow squash them together). But that would >> make things rather ugly. If ok with you we could just have the same approach in >> drm_decode_number() as above and live with the fact that we get two calls to >> drm_get_driver_name() per DRM device specific ioctl. One for drm_decode_number() >> and one for drm_ioctl(). > > This way we would end up with three drm_get_driver_name() calls per ioctl: > twice on entering syscall and once on exiting. Maybe we could cache > result of the first of these three calls somewhere? > How about adding a "void *private" field to struct tcb. That way any syscall can store additional data across the life of the tcb. > > -- > ldv > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Sun, Jun 14, 2015 at 01:12:45PM +0200, Patrik Jakobsson wrote: [...] > How about adding a "void *private" field to struct tcb. That way any > syscall can store additional data across the life of the tcb. We can add a field to struct tcb, but its semantics wrt memory management should be strictly defined. For example, who is responsible for memory deallocation, what droptcb() should do about this field, etc. In this case, we probably need to store a pointer to a string dynamically allocated in drm_get_driver_name() (on entering syscall) which is automatically deallocated later in trace_syscall_exiting() and droptcb().
diff --git a/Makefile.am b/Makefile.am index 549aebc..50d5140 100644 --- a/Makefile.am +++ b/Makefile.am @@ -121,6 +121,7 @@ strace_SOURCES = \ utime.c \ utimes.c \ v4l2.c \ + drm.c \ vsprintf.c \ wait.c \ xattr.c diff --git a/defs.h b/defs.h index 77c819c..f77330b 100644 --- a/defs.h +++ b/defs.h @@ -559,7 +559,7 @@ extern const struct_ioctlent *ioctl_lookup(const unsigned int); extern const struct_ioctlent *ioctl_next_match(const struct_ioctlent *); extern void ioctl_print_code(const unsigned int); extern int ioctl_decode(struct tcb *, const unsigned int, long); -extern int ioctl_decode_command_number(const unsigned int); +extern int ioctl_decode_command_number(struct tcb *, const unsigned int); extern int block_ioctl(struct tcb *, const unsigned int, long); extern int evdev_ioctl(struct tcb *, const unsigned int, long); extern int loop_ioctl(struct tcb *, const unsigned int, long); @@ -572,6 +572,10 @@ extern int term_ioctl(struct tcb *, const unsigned int, long); extern int ubi_ioctl(struct tcb *, const unsigned int, long); extern int v4l2_ioctl(struct tcb *, const unsigned int, long); +extern int drm_is_priv(const unsigned int); +extern int drm_is_driver(struct tcb *tcp, const char *name); +extern int drm_ioctl(struct tcb *, const unsigned int, long); + extern int tv_nz(const struct timeval *); extern int tv_cmp(const struct timeval *, const struct timeval *); extern double tv_float(const struct timeval *); diff --git a/drm.c b/drm.c new file mode 100644 index 0000000..56ef98b --- /dev/null +++ b/drm.c @@ -0,0 +1,88 @@ +/* + * Copyright (c) 2015 Intel Corporation + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. The name of the author may not be used to endorse or promote products + * derived from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * Authors: + * Patrik Jakobsson <patrik.jakobsson@linux.intel.com> + */ + +#include "defs.h" + +#include <sys/types.h> +#include <unistd.h> +#include <string.h> +#include <linux/limits.h> +#include <stdint.h> +#include <sys/ioctl.h> +#include <linux/types.h> +#include <drm.h> + +#define DRM_MAX_NAME_LEN 128 + +inline int drm_is_priv(const unsigned int num) +{ + return (_IOC_NR(num) >= DRM_COMMAND_BASE && + _IOC_NR(num) < DRM_COMMAND_END); +} + +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize) +{ + char path[PATH_MAX]; + char link[PATH_MAX]; + int ret; + + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1); + if (!ret) + return ret; + + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver", + basename(path)); + + ret = readlink(link, path, PATH_MAX - 1); + if (ret < 0) + return ret; + + path[ret] = '\0'; + strncpy(name, basename(path), bufsize); + + return 0; +} + +int drm_is_driver(struct tcb *tcp, const char *name) +{ + char drv[DRM_MAX_NAME_LEN]; + int ret; + + ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN); + if (ret) + return 0; + + return strcmp(name, drv) == 0; +} + +int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg) +{ + return 0; +} diff --git a/io.c b/io.c index 30ed578..6810a45 100644 --- a/io.c +++ b/io.c @@ -391,7 +391,7 @@ SYS_FUNC(ioctl) if (entering(tcp)) { printfd(tcp, tcp->u_arg[0]); tprints(", "); - if (!ioctl_decode_command_number(tcp->u_arg[1])) { + if (!ioctl_decode_command_number(tcp, tcp->u_arg[1])) { iop = ioctl_lookup(tcp->u_arg[1]); if (iop) { tprints(iop->symbol); diff --git a/ioctl.c b/ioctl.c index c67d048..690e7aa 100644 --- a/ioctl.c +++ b/ioctl.c @@ -181,8 +181,14 @@ hiddev_decode_number(unsigned int arg) return 0; } +static int +drm_decode_number(struct tcb *tcp, unsigned int arg) +{ + return 0; +} + int -ioctl_decode_command_number(unsigned int arg) +ioctl_decode_command_number(struct tcb *tcp, unsigned int arg) { switch (_IOC_TYPE(arg)) { case 'E': @@ -216,6 +222,8 @@ ioctl_decode_command_number(unsigned int arg) return 1; } return 0; + case 'd': + return drm_decode_number(tcp, arg); default: return 0; } @@ -252,6 +260,8 @@ ioctl_decode(struct tcb *tcp, unsigned int code, long arg) return ubi_ioctl(tcp, code, arg); case 'V': return v4l2_ioctl(tcp, code, arg); + case 'd': + return drm_ioctl(tcp, code, arg); case '=': return ptp_ioctl(tcp, code, arg); #ifdef HAVE_LINUX_INPUT_H @@ -284,6 +294,7 @@ ioctl_decode(struct tcb *tcp, unsigned int code, long arg) * d sys/des.h (possible overlap) * d vax/dkio.h (possible overlap) * d vaxuba/rxreg.h (possible overlap) + * d drm/drm.h * f sys/filio.h * g sunwindow/win_ioctl.h -no overlap- * g sunwindowdev/winioctl.c !no manifest constant! -no overlap-
Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> --- Makefile.am | 1 + defs.h | 6 ++++- drm.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ io.c | 2 +- ioctl.c | 13 ++++++++- 5 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 drm.c