From: Daniel Vetter <daniel@ffwll.ch>
To: Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org,
Daniel Vetter <daniel.vetter@ffwll.ch>,
David Airlie <airlied@linux.ie>,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: use simple attribute in debugfs routines
Date: Tue, 12 Mar 2013 00:03:30 +0100 [thread overview]
Message-ID: <20130311230330.GC3872@bremse> (raw)
In-Reply-To: <20130310211006.GA26200@www.outflux.net>
On Sun, Mar 10, 2013 at 02:10:06PM -0700, Kees Cook wrote:
> This replaces the manual read/write routines in debugfs with the common
> simple attribute helpers. Doing this gets rid of repeated copy/pasting
> of copy_from_user and value formatting code.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Queued for -next, thanks for the patch. When sending drm/i915 patches,
please also cc intel-gfx@lists.freedesktop.org (it's open for
non-subscribers non without nagging "you're mail is in the moderation
queue" replies).
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 394 +++++++++--------------------------
> 1 file changed, 98 insertions(+), 296 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index aae3148..d86c304 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -849,76 +849,42 @@ static const struct file_operations i915_error_state_fops = {
> .release = i915_error_state_release,
> };
>
> -static ssize_t
> -i915_next_seqno_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_next_seqno_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> - int len;
> int ret;
>
> ret = mutex_lock_interruptible(&dev->struct_mutex);
> if (ret)
> return ret;
>
> - len = snprintf(buf, sizeof(buf),
> - "next_seqno : 0x%x\n",
> - dev_priv->next_seqno);
> -
> + *val = dev_priv->next_seqno;
> mutex_unlock(&dev->struct_mutex);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> -
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_next_seqno_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> -{
> - struct drm_device *dev = filp->private_data;
> - char buf[20];
> - u32 val = 1;
> +static int
> +i915_next_seqno_set(void *data, u64 val)
> +{
> + struct drm_device *dev = data;
> int ret;
>
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - ret = kstrtouint(buf, 0, &val);
> - if (ret < 0)
> - return ret;
> - }
> -
> ret = mutex_lock_interruptible(&dev->struct_mutex);
> if (ret)
> return ret;
>
> ret = i915_gem_set_seqno(dev, val);
> -
> mutex_unlock(&dev->struct_mutex);
>
> - return ret ?: cnt;
> + return ret;
> }
>
> -static const struct file_operations i915_next_seqno_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_next_seqno_read,
> - .write = i915_next_seqno_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_next_seqno_fops,
> + i915_next_seqno_get, i915_next_seqno_set,
> + "next_seqno : 0x%llx\n");
>
> static int i915_rstdby_delays(struct seq_file *m, void *unused)
> {
> @@ -1680,105 +1646,51 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> return 0;
> }
>
> -static ssize_t
> -i915_wedged_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_wedged_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> - int len;
>
> - len = snprintf(buf, sizeof(buf),
> - "wedged : %d\n",
> - atomic_read(&dev_priv->gpu_error.reset_counter));
> + *val = atomic_read(&dev_priv->gpu_error.reset_counter);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> -
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_wedged_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_wedged_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> - char buf[20];
> - int val = 1;
> -
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> + struct drm_device *dev = data;
>
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> -
> - DRM_INFO("Manually setting wedged to %d\n", val);
> + DRM_INFO("Manually setting wedged to %llu\n", val);
> i915_handle_error(dev, val);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_wedged_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_wedged_read,
> - .write = i915_wedged_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_wedged_fops,
> + i915_wedged_get, i915_wedged_set,
> + "wedged : %llu\n");
>
> -static ssize_t
> -i915_ring_stop_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_ring_stop_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[20];
> - int len;
> -
> - len = snprintf(buf, sizeof(buf),
> - "0x%08x\n", dev_priv->gpu_error.stop_rings);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> + *val = dev_priv->gpu_error.stop_rings;
>
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_ring_stop_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_ring_stop_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - char buf[20];
> - int val = 0, ret;
> -
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> + int ret;
>
> - DRM_DEBUG_DRIVER("Stopping rings 0x%08x\n", val);
> + DRM_DEBUG_DRIVER("Stopping rings 0x%08llx\n", val);
>
> ret = mutex_lock_interruptible(&dev->struct_mutex);
> if (ret)
> @@ -1787,16 +1699,12 @@ i915_ring_stop_write(struct file *filp,
> dev_priv->gpu_error.stop_rings = val;
> mutex_unlock(&dev->struct_mutex);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_ring_stop_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_ring_stop_read,
> - .write = i915_ring_stop_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_ring_stop_fops,
> + i915_ring_stop_get, i915_ring_stop_set,
> + "0x%08llx\n");
>
> #define DROP_UNBOUND 0x1
> #define DROP_BOUND 0x2
> @@ -1806,46 +1714,23 @@ static const struct file_operations i915_ring_stop_fops = {
> DROP_BOUND | \
> DROP_RETIRE | \
> DROP_ACTIVE)
> -static ssize_t
> -i915_drop_caches_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_drop_caches_get(void *data, u64 *val)
> {
> - char buf[20];
> - int len;
> -
> - len = snprintf(buf, sizeof(buf), "0x%08x\n", DROP_ALL);
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> + *val = DROP_ALL;
>
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_drop_caches_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_drop_caches_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_i915_gem_object *obj, *next;
> - char buf[20];
> - int val = 0, ret;
> -
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> + int ret;
>
> - DRM_DEBUG_DRIVER("Dropping caches: 0x%08x\n", val);
> + DRM_DEBUG_DRIVER("Dropping caches: 0x%08llx\n", val);
>
> /* No need to check and wait for gpu resets, only libdrm auto-restarts
> * on ioctls on -EAGAIN. */
> @@ -1883,27 +1768,19 @@ i915_drop_caches_write(struct file *filp,
> unlock:
> mutex_unlock(&dev->struct_mutex);
>
> - return ret ?: cnt;
> + return ret;
> }
>
> -static const struct file_operations i915_drop_caches_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_drop_caches_read,
> - .write = i915_drop_caches_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops,
> + i915_drop_caches_get, i915_drop_caches_set,
> + "0x%08llx\n");
>
> -static ssize_t
> -i915_max_freq_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_max_freq_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> - int len, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
> @@ -1912,42 +1789,23 @@ i915_max_freq_read(struct file *filp,
> if (ret)
> return ret;
>
> - len = snprintf(buf, sizeof(buf),
> - "max freq: %d\n", dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER);
> + *val = dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER;
> mutex_unlock(&dev_priv->rps.hw_lock);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> -
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_max_freq_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_max_freq_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - char buf[20];
> - int val = 1, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
>
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> -
> - DRM_DEBUG_DRIVER("Manually setting max freq to %d\n", val);
> + DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val);
>
> ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> if (ret)
> @@ -1961,25 +1819,19 @@ i915_max_freq_write(struct file *filp,
> gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER);
> mutex_unlock(&dev_priv->rps.hw_lock);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_max_freq_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_max_freq_read,
> - .write = i915_max_freq_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_max_freq_fops,
> + i915_max_freq_get, i915_max_freq_set,
> + "max freq: %llu\n");
>
> -static ssize_t
> -i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
> - loff_t *ppos)
> +static int
> +i915_min_freq_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> - int len, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
> @@ -1988,40 +1840,23 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
> if (ret)
> return ret;
>
> - len = snprintf(buf, sizeof(buf),
> - "min freq: %d\n", dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER);
> + *val = dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER;
> mutex_unlock(&dev_priv->rps.hw_lock);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> -
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_min_freq_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - char buf[20];
> - int val = 1, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
>
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> -
> - DRM_DEBUG_DRIVER("Manually setting min freq to %d\n", val);
> + DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val);
>
> ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> if (ret)
> @@ -2035,28 +1870,20 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
> gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER);
> mutex_unlock(&dev_priv->rps.hw_lock);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_min_freq_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_min_freq_read,
> - .write = i915_min_freq_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_min_freq_fops,
> + i915_min_freq_get, i915_min_freq_set,
> + "min freq: %llu\n");
>
> -static ssize_t
> -i915_cache_sharing_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_cache_sharing_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> u32 snpcr;
> - int len, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
> @@ -2068,46 +1895,25 @@ i915_cache_sharing_read(struct file *filp,
> snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
> mutex_unlock(&dev_priv->dev->struct_mutex);
>
> - len = snprintf(buf, sizeof(buf),
> - "%d\n", (snpcr & GEN6_MBC_SNPCR_MASK) >>
> - GEN6_MBC_SNPCR_SHIFT);
> -
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> + *val = (snpcr & GEN6_MBC_SNPCR_MASK) >> GEN6_MBC_SNPCR_SHIFT;
>
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_cache_sharing_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_cache_sharing_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - char buf[20];
> u32 snpcr;
> - int val = 1;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
>
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> -
> - if (val < 0 || val > 3)
> + if (val > 3)
> return -EINVAL;
>
> - DRM_DEBUG_DRIVER("Manually setting uncore sharing to %d\n", val);
> + DRM_DEBUG_DRIVER("Manually setting uncore sharing to %llu\n", val);
>
> /* Update the cache sharing policy here as well */
> snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
> @@ -2115,16 +1921,12 @@ i915_cache_sharing_write(struct file *filp,
> snpcr |= (val << GEN6_MBC_SNPCR_SHIFT);
> I915_WRITE(GEN6_MBCUNIT_SNPCR, snpcr);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_cache_sharing_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_cache_sharing_read,
> - .write = i915_cache_sharing_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_cache_sharing_fops,
> + i915_cache_sharing_get, i915_cache_sharing_set,
> + "%llu\n");
>
> /* As the drm_debugfs_init() routines are called before dev->dev_private is
> * allocated we need to hook into the minor for release. */
> --
> 1.7.9.5
>
>
> --
> Kees Cook
> Chrome OS Security
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
next prev parent reply other threads:[~2013-03-11 23:03 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-10 21:10 [PATCH] drm/i915: use simple attribute in debugfs routines Kees Cook
2013-03-11 23:03 ` Daniel Vetter [this message]
2013-03-12 0:20 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130311230330.GC3872@bremse \
--to=daniel@ffwll.ch \
--cc=airlied@linux.ie \
--cc=daniel.vetter@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.