Message ID | 20200818133608.462514-7-gregkh@linuxfoundation.org (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | Laurent Pinchart |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1k81hf-007eMI-1c; Tue, 18 Aug 2020 13:31:04 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726820AbgHRNgd (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 18 Aug 2020 09:36:33 -0400 Received: from mail.kernel.org ([198.145.29.99]:45610 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726660AbgHRNgJ (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 18 Aug 2020 09:36:09 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id DD6D22076D; Tue, 18 Aug 2020 13:36:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597757769; bh=qKMe8yvEZAbay72M2tJtP0TdwY4VnJuiqPFS8BMNa0I=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=0/CpW8GIgXJ+2e+UUMPBIshWolNDL2nz14zrE9S5cvkhP5srhs5CEWVWhY3nV5XHv Yk2FF9E1kWK/QPZWhzfCzdhVj2RhcDvXD2OSz1K3LfHOZdt9GyvFh+Ns6ZPRtPXgai jby3SMBKDzcyew0UrS65uiFTfpNE7q59jTNi4MXk= From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> To: linux-media@vger.kernel.org Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, linux-kernel@vger.kernel.org Subject: [PATCH 7/7] media: usb: uvc: no need to check return value of debugfs_create functions Date: Tue, 18 Aug 2020 15:36:08 +0200 Message-Id: <20200818133608.462514-7-gregkh@linuxfoundation.org> X-Mailer: git-send-email 2.28.0 In-Reply-To: <20200818133608.462514-1-gregkh@linuxfoundation.org> References: <20200818133608.462514-1-gregkh@linuxfoundation.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIMWL_WL_HIGH=0.001,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_EF=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
[1/7] media: cec: no need to check return value of debugfs_create functions
|
|
Commit Message
Greg KH
Aug. 18, 2020, 1:36 p.m. UTC
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.
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/media/usb/uvc/uvc_debugfs.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
Comments
Hi Greg, Thank you for the patch. On Tue, Aug 18, 2020 at 03:36:08PM +0200, Greg Kroah-Hartman 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. Is there no value in warning the user that something went wrong ? Silent failures are harder to debug. > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: linux-media@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/media/usb/uvc/uvc_debugfs.c | 20 ++++---------------- > 1 file changed, 4 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_debugfs.c b/drivers/media/usb/uvc/uvc_debugfs.c > index 2b8af4b54117..1a1258d4ffca 100644 > --- a/drivers/media/usb/uvc/uvc_debugfs.c > +++ b/drivers/media/usb/uvc/uvc_debugfs.c > @@ -73,7 +73,6 @@ static struct dentry *uvc_debugfs_root_dir; > void uvc_debugfs_init_stream(struct uvc_streaming *stream) > { > struct usb_device *udev = stream->dev->udev; > - struct dentry *dent; > char dir_name[33]; > > if (uvc_debugfs_root_dir == NULL) > @@ -82,22 +81,11 @@ void uvc_debugfs_init_stream(struct uvc_streaming *stream) > snprintf(dir_name, sizeof(dir_name), "%u-%u-%u", udev->bus->busnum, > udev->devnum, stream->intfnum); > > - dent = debugfs_create_dir(dir_name, uvc_debugfs_root_dir); > - if (IS_ERR_OR_NULL(dent)) { > - uvc_printk(KERN_INFO, "Unable to create debugfs %s " > - "directory.\n", dir_name); > - return; > - } > - > - stream->debugfs_dir = dent; > + stream->debugfs_dir = debugfs_create_dir(dir_name, > + uvc_debugfs_root_dir); > > - dent = debugfs_create_file("stats", 0444, stream->debugfs_dir, > - stream, &uvc_debugfs_stats_fops); > - if (IS_ERR_OR_NULL(dent)) { > - uvc_printk(KERN_INFO, "Unable to create debugfs stats file.\n"); > - uvc_debugfs_cleanup_stream(stream); > - return; > - } > + debugfs_create_file("stats", 0444, stream->debugfs_dir, stream, > + &uvc_debugfs_stats_fops); > } > > void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream)
Hi Greg, On Wed, Aug 19, 2020 at 02:47:19AM +0300, Laurent Pinchart wrote: > Hi Greg, > > Thank you for the patch. > > On Tue, Aug 18, 2020 at 03:36:08PM +0200, Greg Kroah-Hartman 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. > > Is there no value in warning the user that something went wrong ? Silent > failures are harder to debug. Could yous share your opinion about this ? > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > > Cc: linux-media@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > drivers/media/usb/uvc/uvc_debugfs.c | 20 ++++---------------- > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_debugfs.c b/drivers/media/usb/uvc/uvc_debugfs.c > > index 2b8af4b54117..1a1258d4ffca 100644 > > --- a/drivers/media/usb/uvc/uvc_debugfs.c > > +++ b/drivers/media/usb/uvc/uvc_debugfs.c > > @@ -73,7 +73,6 @@ static struct dentry *uvc_debugfs_root_dir; > > void uvc_debugfs_init_stream(struct uvc_streaming *stream) > > { > > struct usb_device *udev = stream->dev->udev; > > - struct dentry *dent; > > char dir_name[33]; > > > > if (uvc_debugfs_root_dir == NULL) > > @@ -82,22 +81,11 @@ void uvc_debugfs_init_stream(struct uvc_streaming *stream) > > snprintf(dir_name, sizeof(dir_name), "%u-%u-%u", udev->bus->busnum, > > udev->devnum, stream->intfnum); > > > > - dent = debugfs_create_dir(dir_name, uvc_debugfs_root_dir); > > - if (IS_ERR_OR_NULL(dent)) { > > - uvc_printk(KERN_INFO, "Unable to create debugfs %s " > > - "directory.\n", dir_name); > > - return; > > - } > > - > > - stream->debugfs_dir = dent; > > + stream->debugfs_dir = debugfs_create_dir(dir_name, > > + uvc_debugfs_root_dir); > > > > - dent = debugfs_create_file("stats", 0444, stream->debugfs_dir, > > - stream, &uvc_debugfs_stats_fops); > > - if (IS_ERR_OR_NULL(dent)) { > > - uvc_printk(KERN_INFO, "Unable to create debugfs stats file.\n"); > > - uvc_debugfs_cleanup_stream(stream); > > - return; > > - } > > + debugfs_create_file("stats", 0444, stream->debugfs_dir, stream, > > + &uvc_debugfs_stats_fops); > > } > > > > void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream)
On Thu, Sep 17, 2020 at 03:25:50PM +0300, Laurent Pinchart wrote: > Hi Greg, > > On Wed, Aug 19, 2020 at 02:47:19AM +0300, Laurent Pinchart wrote: > > Hi Greg, > > > > Thank you for the patch. > > > > On Tue, Aug 18, 2020 at 03:36:08PM +0200, Greg Kroah-Hartman 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. > > > > Is there no value in warning the user that something went wrong ? Silent > > failures are harder to debug. > > Could yous share your opinion about this ? For debugfs, this isn't an issue, what can a user do with something like "debugfs isn't working? What does that mean???" And if we _really_ want warnings like this, it should go into the debugfs core, not require this to be done for every debugfs user, right? debugfs is just there for kernel developers to help debug things, it's not a dependancy on any userspace functionality, so if it works or not should not be an issue for any user. Unless that user is a kernel developer of course :) thanks, greg k-h
Hi Greg, On Thu, Sep 17, 2020 at 02:34:26PM +0200, Greg Kroah-Hartman wrote: > On Thu, Sep 17, 2020 at 03:25:50PM +0300, Laurent Pinchart wrote: > > On Wed, Aug 19, 2020 at 02:47:19AM +0300, Laurent Pinchart wrote: > > > On Tue, Aug 18, 2020 at 03:36:08PM +0200, Greg Kroah-Hartman 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. > > > > > > Is there no value in warning the user that something went wrong ? Silent > > > failures are harder to debug. > > > > Could yous share your opinion about this ? > > For debugfs, this isn't an issue, what can a user do with something like > "debugfs isn't working? What does that mean???" > > And if we _really_ want warnings like this, it should go into the > debugfs core, not require this to be done for every debugfs user, right? > > debugfs is just there for kernel developers to help debug things, it's > not a dependancy on any userspace functionality, so if it works or not > should not be an issue for any user. > > Unless that user is a kernel developer of course :) Exactly my point :-) I'm fine moving the error message to the debugfs core itself instead of duplicating it in drivers. Maybe it's already there though, I haven't checked. Not printing any message isn't a great idea in my opinion, it makes debugging more difficult. I can't count the number of times where I've had to add printk's and recompile the kernel to debug issues that really should have generated at least a dev_dbg().
On Thu, Sep 17, 2020 at 03:37:57PM +0300, Laurent Pinchart wrote: > Hi Greg, > > On Thu, Sep 17, 2020 at 02:34:26PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Sep 17, 2020 at 03:25:50PM +0300, Laurent Pinchart wrote: > > > On Wed, Aug 19, 2020 at 02:47:19AM +0300, Laurent Pinchart wrote: > > > > On Tue, Aug 18, 2020 at 03:36:08PM +0200, Greg Kroah-Hartman 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. > > > > > > > > Is there no value in warning the user that something went wrong ? Silent > > > > failures are harder to debug. > > > > > > Could yous share your opinion about this ? > > > > For debugfs, this isn't an issue, what can a user do with something like > > "debugfs isn't working? What does that mean???" > > > > And if we _really_ want warnings like this, it should go into the > > debugfs core, not require this to be done for every debugfs user, right? > > > > debugfs is just there for kernel developers to help debug things, it's > > not a dependancy on any userspace functionality, so if it works or not > > should not be an issue for any user. > > > > Unless that user is a kernel developer of course :) > > Exactly my point :-) > > I'm fine moving the error message to the debugfs core itself instead of > duplicating it in drivers. Maybe it's already there though, I haven't > checked. Not printing any message isn't a great idea in my opinion, it > makes debugging more difficult. I can't count the number of times where > I've had to add printk's and recompile the kernel to debug issues that > really should have generated at least a dev_dbg(). There are a lot of error messages that debugfs will print out if it can not create a file: inode.c 329 pr_err("Unable to pin filesystem for file '%s'\n", name); inode.c 348 pr_err("Directory '%s' with parent '%s' already present!\n", inode.c 351 pr_err("File '%s' in directory '%s' already present!\n", inode.c 402 pr_err("out of free dentries, can not create file '%s'\n", inode.c 563 pr_err("out of free dentries, can not create directory '%s'\n", inode.c 610 pr_err("out of free dentries, can not create automount '%s'\n", inode.c 668 pr_err("out of free dentries, can not create symlink '%s'\n", So I think you are safe here. If we are missing any, I'll gladly add them. Also given that you've never noticed this being a real error, means it's probably not an issue :) thanks, greg k-h
diff --git a/drivers/media/usb/uvc/uvc_debugfs.c b/drivers/media/usb/uvc/uvc_debugfs.c index 2b8af4b54117..1a1258d4ffca 100644 --- a/drivers/media/usb/uvc/uvc_debugfs.c +++ b/drivers/media/usb/uvc/uvc_debugfs.c @@ -73,7 +73,6 @@ static struct dentry *uvc_debugfs_root_dir; void uvc_debugfs_init_stream(struct uvc_streaming *stream) { struct usb_device *udev = stream->dev->udev; - struct dentry *dent; char dir_name[33]; if (uvc_debugfs_root_dir == NULL) @@ -82,22 +81,11 @@ void uvc_debugfs_init_stream(struct uvc_streaming *stream) snprintf(dir_name, sizeof(dir_name), "%u-%u-%u", udev->bus->busnum, udev->devnum, stream->intfnum); - dent = debugfs_create_dir(dir_name, uvc_debugfs_root_dir); - if (IS_ERR_OR_NULL(dent)) { - uvc_printk(KERN_INFO, "Unable to create debugfs %s " - "directory.\n", dir_name); - return; - } - - stream->debugfs_dir = dent; + stream->debugfs_dir = debugfs_create_dir(dir_name, + uvc_debugfs_root_dir); - dent = debugfs_create_file("stats", 0444, stream->debugfs_dir, - stream, &uvc_debugfs_stats_fops); - if (IS_ERR_OR_NULL(dent)) { - uvc_printk(KERN_INFO, "Unable to create debugfs stats file.\n"); - uvc_debugfs_cleanup_stream(stream); - return; - } + debugfs_create_file("stats", 0444, stream->debugfs_dir, stream, + &uvc_debugfs_stats_fops); } void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream)