Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752050AbdHCQBl (ORCPT ); Thu, 3 Aug 2017 12:01:41 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:50148 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751244AbdHCQBi (ORCPT ); Thu, 3 Aug 2017 12:01:38 -0400 Date: Thu, 3 Aug 2017 09:01:35 -0700 From: Greg Kroah-Hartman To: Chunfeng Yun Cc: Felipe Balbi , Mathias Nyman , Matthias Brugger , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org Subject: Re: [PATCH 1/2] usb: mtu3: add a vbus debugfs interface Message-ID: <20170803160135.GC20736@kroah.com> References: <1501749439-8837-1-git-send-email-chunfeng.yun@mediatek.com> <20170803160057.GB20736@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170803160057.GB20736@kroah.com> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2299 Lines: 67 On Thu, Aug 03, 2017 at 09:00:57AM -0700, Greg Kroah-Hartman wrote: > On Thu, Aug 03, 2017 at 04:37:18PM +0800, Chunfeng Yun wrote: > > +static ssize_t ssusb_vbus_write(struct file *file, > > + const char __user *ubuf, size_t count, loff_t *ppos) > > +{ > > + struct seq_file *sf = file->private_data; > > + struct ssusb_mtk *ssusb = sf->private; > > + struct otg_switch_mtk *otg_sx = &ssusb->otg_switch; > > + char buf[16]; > > + > > + if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count))) > > + return -EFAULT; > > + > > + if (!strncmp(buf, "on", 2)) { > > + ssusb_set_vbus(otg_sx, 1); > > + } else if (!strncmp(buf, "off", 3)) { > > + ssusb_set_vbus(otg_sx, 0); > > kstrtobool() is better to use here. > > > +static int ssusb_debugfs_init(struct ssusb_mtk *ssusb) > > { > > struct dentry *root; > > struct dentry *file; > > > > root = debugfs_create_dir(dev_name(ssusb->dev), usb_debug_root); > > if (IS_ERR_OR_NULL(root)) { > > That test is wrong, you should never need to care about the return value > of a debugfs call. You can always just use it in your next call (if it > is a dentry *), or just ignore it. > > > - if (!root) > > - dev_err(ssusb->dev, "create debugfs root failed\n"); > > - return; > > + dev_err(ssusb->dev, "create debugfs root failed\n"); > > + goto err_dbgfs; > > } > > ssusb->dbgfs_root = root; > > > > - file = debugfs_create_file("mode", S_IRUGO | S_IWUSR, root, > > - ssusb, &ssusb_mode_fops); > > - if (!file) > > - dev_dbg(ssusb->dev, "create debugfs mode failed\n"); > > + file = debugfs_create_file("mode", 0644, root, ssusb, &ssusb_mode_fops); > > + if (!file) { > > + dev_err(ssusb->dev, "create debugfs mode failed\n"); > > Same here, do not care, as it does not matter. Actually, you don't even > need to keep the file pointer around, right? > > > + goto err_dbgfs; > > + } > > + > > + file = debugfs_create_file("vbus", 0644, root, ssusb, &ssusb_vbus_fops); > > + if (!file) { > > Same here, you don't care if debugfs fails or not, it should never > affect your code flow. > > And don't you need to remove the directory when your module/hardware is > removed? I don't see that happening here as you are not saving root off > anywhere. Oh nevermind, you do save it, sorry for the noise... greg k-h