Messages in this thread | | | Date | Tue, 2 Jun 2009 10:55:19 +0200 | From | Joerg Roedel <> | Subject | Re: [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/
| |