lkml.org 
[lkml]   [2009]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/4] dma-debug: add debugfs file for driver filter
    On Mon, Jun 01, 2009 at 03:04:18PM -0700, Andrew Morton wrote:
    > On Thu, 28 May 2009 17:19:29 +0200
    > Joerg Roedel <joerg.roedel@amd.com> wrote:
    >
    > > This patch adds the dma-api/driver_filter file to debugfs. The root user
    > > can write a driver name into this file to see only dma-api errors for
    > > that particular driver in the kernel log. Writing an empty string to
    > > that file disables the driver filter.
    > >
    > >
    > > ...
    > >
    > > +static ssize_t filter_read(struct file *file, char __user *user_buf,
    > > + size_t count, loff_t *ppos)
    > > +{
    > > + unsigned long flags;
    > > + char buf[NAME_MAX_LEN + 1];
    > > + int len;
    > > +
    > > + if (!current_driver_name[0])
    > > + return 0;
    > > +
    > > + /*
    > > + * We can't copy to userspace directly because current_driver_name can
    > > + * only be read under the driver_name_lock with irqs disabled. So
    > > + * create a temporary copy first.
    > > + */
    > > + read_lock_irqsave(&driver_name_lock, flags);
    > > + strncpy(buf, current_driver_name, NAME_MAX_LEN - 1);
    >
    > If you use strlcpy() here,
    >
    > > + read_unlock_irqrestore(&driver_name_lock, flags);
    > > +
    > > + /* make it look nice :-) */
    > > + len = strnlen(buf, NAME_MAX_LEN - 1);
    > > + buf[len] = '\n';
    > > + buf[len + 1] = 0;
    >
    > the above can all be replaced with a simple strcat(). I think.

    Right, thanks.

    > Perhaps the whole lot can be replaced with a single scnprintf(), if
    > it's known that the input string has appropriate length.

    I will check that. Thanks.

    > > + return simple_read_from_buffer(user_buf, count, ppos, buf, len + 1);
    > > +}
    > > +
    > > +static ssize_t filter_write(struct file *file, const char __user *userbuf,
    > > + size_t count, loff_t *ppos)
    > > +{
    > > + unsigned long flags;
    > > + char buf[NAME_MAX_LEN];
    > > + size_t len = NAME_MAX_LEN - 1;
    > > + int i;
    > > +
    > > + /*
    > > + * We can't copy from userspace directly. Access to
    > > + * current_driver_name is protected with a write_lock with irqs
    > > + * disabled. Since copy_from_user can fault and may sleep we
    > > + * need to copy to temporary buffer first
    > > + */
    > > + len = min(count, len);
    > > + if (copy_from_user(buf, userbuf, len))
    > > + return -EFAULT;
    > > +
    > > + buf[NAME_MAX_LEN - 1] = 0;
    >
    > Might be able to use strncpy_from_user() here.
    >
    > > + write_lock_irqsave(&driver_name_lock, flags);
    > > + if (!isalnum(buf[0])) {
    > > + if (current_driver_name[0])
    > > + printk(KERN_INFO "DMA-API: switching off dma-debug "
    > > + "driver filter\n");
    > > + current_driver_name[0] = 0;
    > > + current_driver = NULL;
    > > + goto out_unlock;
    > > + }
    > > +
    > > + for (i = 0; i < NAME_MAX_LEN; ++i) {
    > > + current_driver_name[i] = buf[i];
    > > + if (isspace(buf[i]) || buf[i] == ' ' ||
    > > + buf[i] == '\t' || buf[i] == 0)
    > > + break;
    > > + }
    > > + current_driver_name[i] = 0;
    > > + current_driver = NULL;
    > > +
    > > + printk(KERN_INFO "DMA-API: enable driver filter for driver [%s]\n",
    > > + current_driver_name);
    > > +
    > > +out_unlock:
    > > + write_unlock_irqrestore(&driver_name_lock, flags);
    > > +
    > > + return count;
    > > +}
    >
    > It's unobvious what user interface this code is attempting to implement.
    > The change to DMA-API.txt in the fourth patch doesn't illuminate the
    > issue either. Some code comments explaining wtf this is doing and why
    > would help, please.

    It implements a file to which the name of a driver can be written. If
    done, dma-debug will only print errors from that particular driver. I
    will state that also in a comment.

    > I'd have thought that the test for isspace() makes the tests for ' '
    > and '\t' redundant?

    Checking again, this is true for '\t' but not for ' '. The later one
    would be checked by isblank() which is not implemented in the kernel.


    Joerg

    --
    | Advanced Micro Devices GmbH
    Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
    System |
    Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
    Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
    | Registergericht München, HRB Nr. 43632

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2009-06-02 10:59    [W:5.693 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site