| Message ID | 20180529153107.12791-5-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 29 May 2018 17:30:50 +0200 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > When calling debugfs functions, there is no need to ever check the > return value. The function can work or not, but the code logic should > never do something different based on this. Okay, fair enough. And the code works, surprisingly, even when the debugfs is disabled (well, according to my review). I just have one question - wouldn't it be cleaner to deprecate and remove the text API altogether? -- Pete -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 29, 2018 at 11:34:34AM -0500, Pete Zaitcev wrote: > On Tue, 29 May 2018 17:30:50 +0200 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > When calling debugfs functions, there is no need to ever check the > > return value. The function can work or not, but the code logic should > > never do something different based on this. > > Okay, fair enough. And the code works, surprisingly, even when the > debugfs is disabled (well, according to my review). I just have one > question - wouldn't it be cleaner to deprecate and remove the text > API altogether? If you think we can do that, sure! But what's the odds that someone still uses it? This patch doesn't change any real functionality. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, On Tue, May 29, 2018 at 06:51:54PM +0200, Greg Kroah-Hartman wrote: > On Tue, May 29, 2018 at 11:34:34AM -0500, Pete Zaitcev wrote: > > On Tue, 29 May 2018 17:30:50 +0200 > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > When calling debugfs functions, there is no need to ever check the > > > return value. The function can work or not, but the code logic should > > > never do something different based on this. > > > > Okay, fair enough. And the code works, surprisingly, even when the > > debugfs is disabled (well, according to my review). I just have one > > question - wouldn't it be cleaner to deprecate and remove the text > > API altogether? > > If you think we can do that, sure! But what's the odds that someone > still uses it? This patch doesn't change any real functionality. I had some use of this text API last year, when I also discovered and submitted and fix for its data corruption problems described in commit a5f596830e27e "usb: usbmon: Read text within supplied buffer size". A significant disadvantage with the text API is, as far as I understand it, that it can drop events (due to its buffering limit) without informing API readers (via event sequence numbers or some such). I've found a couple of USB bugs/problems (commit d6c931ea32dc0 "USB: OHCI: Fix NULL dereference in HCDs using HCD_LOCAL_MEM" for instance) and I'm still investigating at least one bug possibly related to lost interrupts where some kind of probing method could be helpful. However, I would explore other kernel probing methods for that. Not only because I'm curious but also because I have the impression that there are more powerful methods available than this text API. Would you recommend any alternatives? Fredrik -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/mon/mon_text.c b/drivers/usb/mon/mon_text.c index 984f7e12a6a5..bc5ecd5ff565 100644 --- a/drivers/usb/mon/mon_text.c +++ b/drivers/usb/mon/mon_text.c @@ -700,7 +700,6 @@ static const struct file_operations mon_fops_text_u = { int mon_text_add(struct mon_bus *mbus, const struct usb_bus *ubus) { - struct dentry *d; enum { NAMESZ = 10 }; char name[NAMESZ]; int busnum = ubus? ubus->busnum: 0; @@ -713,42 +712,32 @@ int mon_text_add(struct mon_bus *mbus, const struct usb_bus *ubus) rc = snprintf(name, NAMESZ, "%dt", busnum); if (rc <= 0 || rc >= NAMESZ) goto err_print_t; - d = debugfs_create_file(name, 0600, mon_dir, mbus, + mbus->dent_t = debugfs_create_file(name, 0600, mon_dir, mbus, &mon_fops_text_t); - if (d == NULL) - goto err_create_t; - mbus->dent_t = d; } rc = snprintf(name, NAMESZ, "%du", busnum); if (rc <= 0 || rc >= NAMESZ) goto err_print_u; - d = debugfs_create_file(name, 0600, mon_dir, mbus, &mon_fops_text_u); - if (d == NULL) - goto err_create_u; - mbus->dent_u = d; + mbus->dent_u = debugfs_create_file(name, 0600, mon_dir, mbus, + &mon_fops_text_u); rc = snprintf(name, NAMESZ, "%ds", busnum); if (rc <= 0 || rc >= NAMESZ) goto err_print_s; - d = debugfs_create_file(name, 0600, mon_dir, mbus, &mon_fops_stat); - if (d == NULL) - goto err_create_s; - mbus->dent_s = d; + mbus->dent_s = debugfs_create_file(name, 0600, mon_dir, mbus, + &mon_fops_stat); return 1; -err_create_s: err_print_s: debugfs_remove(mbus->dent_u); mbus->dent_u = NULL; -err_create_u: err_print_u: if (ubus != NULL) { debugfs_remove(mbus->dent_t); mbus->dent_t = NULL; } -err_create_t: err_print_t: return 0; } @@ -756,8 +745,7 @@ int mon_text_add(struct mon_bus *mbus, const struct usb_bus *ubus) void mon_text_del(struct mon_bus *mbus) { debugfs_remove(mbus->dent_u); - if (mbus->dent_t != NULL) - debugfs_remove(mbus->dent_t); + debugfs_remove(mbus->dent_t); debugfs_remove(mbus->dent_s); } @@ -775,18 +763,7 @@ static void mon_text_ctor(void *mem) int __init mon_text_init(void) { - struct dentry *mondir; - - mondir = debugfs_create_dir("usbmon", usb_debug_root); - if (IS_ERR(mondir)) { - /* debugfs not available, but we can use usbmon without it */ - return 0; - } - if (mondir == NULL) { - printk(KERN_NOTICE TAG ": unable to create usbmon directory\n"); - return -ENOMEM; - } - mon_dir = mondir; + mon_dir = debugfs_create_dir("usbmon", usb_debug_root); return 0; }
When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this. We do need to save the dentries for these files, so keep them around, but no need to check if they are "valid" or not, as the code works just as well either way. Cc: Pete Zaitcev <zaitcev@kotori.zaitcev.us> Cc: Philippe Ombredanne <pombredanne@nexb.com> Cc: Fredrik Noring <noring@nocrew.org> Cc: Kate Stewart <kstewart@linuxfoundation.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/usb/mon/mon_text.c | 37 +++++++------------------------------ 1 file changed, 7 insertions(+), 30 deletions(-)