* [PATCH] coresight: cpu-debug: no need to check return value of debugfs_create functions @ 2019-06-18 15:52 Greg Kroah-Hartman 2019-06-18 17:23 ` Mathieu Poirier 0 siblings, 1 reply; 6+ messages in thread From: Greg Kroah-Hartman @ 2019-06-18 15:52 UTC (permalink / raw) To: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin; +Cc: linux-arm-kernel 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: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/hwtracing/coresight/coresight-cpu-debug.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c index e8819d750938..6446ed69ab2f 100644 --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c @@ -525,23 +525,12 @@ static const struct file_operations debug_func_knob_fops = { static int debug_func_init(void) { - struct dentry *file; int ret; /* Create debugfs node */ debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL); - if (!debug_debugfs_dir) { - pr_err("%s: unable to create debugfs directory\n", __func__); - return -ENOMEM; - } - - file = debugfs_create_file("enable", 0644, debug_debugfs_dir, NULL, - &debug_func_knob_fops); - if (!file) { - pr_err("%s: unable to create enable knob file\n", __func__); - ret = -ENOMEM; - goto err; - } + debugfs_create_file("enable", 0644, debug_debugfs_dir, NULL, + &debug_func_knob_fops); /* Register function to be called for panic */ ret = atomic_notifier_chain_register(&panic_notifier_list, -- 2.22.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: cpu-debug: no need to check return value of debugfs_create functions 2019-06-18 15:52 [PATCH] coresight: cpu-debug: no need to check return value of debugfs_create functions Greg Kroah-Hartman @ 2019-06-18 17:23 ` Mathieu Poirier 2019-06-18 17:46 ` Greg Kroah-Hartman 0 siblings, 1 reply; 6+ messages in thread From: Mathieu Poirier @ 2019-06-18 17:23 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Alexander Shishkin, linux-arm-kernel, Suzuki K Poulose Hi Greg, On Tue, 18 Jun 2019 at 09:52, 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. Looking around in the kernel there is no shortage of instances where the return value of debugfs functions are checked and the logic altered based on these values. But there are also just as many that don't... It also seems counter intuitive to ignore the return value of any function, something that in most case is guaranteed to raise admonition. That being said I am sure there is a good reason to support your position - would you mind expanding a little so that I can follow? Thanks, Mathieu > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/hwtracing/coresight/coresight-cpu-debug.c | 15 ++------------- > 1 file changed, 2 insertions(+), 13 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c > index e8819d750938..6446ed69ab2f 100644 > --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c > +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c > @@ -525,23 +525,12 @@ static const struct file_operations debug_func_knob_fops = { > > static int debug_func_init(void) > { > - struct dentry *file; > int ret; > > /* Create debugfs node */ > debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL); > - if (!debug_debugfs_dir) { > - pr_err("%s: unable to create debugfs directory\n", __func__); > - return -ENOMEM; > - } > - > - file = debugfs_create_file("enable", 0644, debug_debugfs_dir, NULL, > - &debug_func_knob_fops); > - if (!file) { > - pr_err("%s: unable to create enable knob file\n", __func__); > - ret = -ENOMEM; > - goto err; > - } > + debugfs_create_file("enable", 0644, debug_debugfs_dir, NULL, > + &debug_func_knob_fops); > > /* Register function to be called for panic */ > ret = atomic_notifier_chain_register(&panic_notifier_list, > -- > 2.22.0 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: cpu-debug: no need to check return value of debugfs_create functions 2019-06-18 17:23 ` Mathieu Poirier @ 2019-06-18 17:46 ` Greg Kroah-Hartman 2019-06-18 17:52 ` Greg Kroah-Hartman 2019-06-18 19:26 ` Mathieu Poirier 0 siblings, 2 replies; 6+ messages in thread From: Greg Kroah-Hartman @ 2019-06-18 17:46 UTC (permalink / raw) To: Mathieu Poirier; +Cc: Alexander Shishkin, linux-arm-kernel, Suzuki K Poulose On Tue, Jun 18, 2019 at 11:23:25AM -0600, Mathieu Poirier wrote: > Hi Greg, > > On Tue, 18 Jun 2019 at 09:52, 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. > > Looking around in the kernel there is no shortage of instances where > the return value of debugfs functions are checked and the logic > altered based on these values. But there are also just as many that > don't... It also seems counter intuitive to ignore the return value > of any function, something that in most case is guaranteed to raise > admonition. In my tree, those instances are almost all gone. I've also posted over 100+ patches in the past few weeks to clean this up. > That being said I am sure there is a good reason to support your > position - would you mind expanding a little so that I can follow? No kernel code should ever care if debugfs works or not. No user code should ever require it for normal operation either. debugfs was written to be simple and easy to use, no need to check any return values at all. Any return value of a debugfs call can be fed back into another call with no issues at all. Also, due to some debugfs core changes a few kernel releases ago, the checks: if (!debug_debugfs_dir) { ... if (!file) { can never trigger as debugfs_create_dir() or debugfs_create_file() can never return NULL (and in the past, it almost never would either). So as it is, that code isn't correct anyway (my fault, I know, hey, I'm trying to fix it!) I'm trying to make things simple, and easy, and impossible to get wrong. I know it goes against the normal "robust" kernel development mentality, but there is no need to ever care about debugfs at all. The reason I started all of this is that we have found places where userspace, and the kernel, was depending on the proper operation of debugfs. In one horrid example, a device would not display the batter level if debugfs was disabled. In another case, the kernel was actually relying on a debugfs call to fail in order to handle some logic the subsystem should have been doing on its own. All of that has now been cleaned up, and I am working on making debugfs just not return any values at all to prevent this type of mess happening again. And hey, I am removing code, here's my current tree as a diff from what is not already merged into linux-next: 301 files changed, 1394 insertions(+), 4637 deletions(-) that's always a good thing :) Hopefully this helps explain things better. thanks, greg k-h _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: cpu-debug: no need to check return value of debugfs_create functions 2019-06-18 17:46 ` Greg Kroah-Hartman @ 2019-06-18 17:52 ` Greg Kroah-Hartman 2019-06-18 19:26 ` Mathieu Poirier 1 sibling, 0 replies; 6+ messages in thread From: Greg Kroah-Hartman @ 2019-06-18 17:52 UTC (permalink / raw) To: Mathieu Poirier; +Cc: Alexander Shishkin, linux-arm-kernel, Suzuki K Poulose On Tue, Jun 18, 2019 at 07:46:37PM +0200, Greg Kroah-Hartman wrote: > On Tue, Jun 18, 2019 at 11:23:25AM -0600, Mathieu Poirier wrote: > > Hi Greg, > > > > On Tue, 18 Jun 2019 at 09:52, 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. > > > > Looking around in the kernel there is no shortage of instances where > > the return value of debugfs functions are checked and the logic > > altered based on these values. But there are also just as many that > > don't... It also seems counter intuitive to ignore the return value > > of any function, something that in most case is guaranteed to raise > > admonition. > > In my tree, those instances are almost all gone. I've also posted over > 100+ patches in the past few weeks to clean this up. > > > That being said I am sure there is a good reason to support your > > position - would you mind expanding a little so that I can follow? > > No kernel code should ever care if debugfs works or not. No user code > should ever require it for normal operation either. debugfs was written > to be simple and easy to use, no need to check any return values at all. > > Any return value of a debugfs call can be fed back into another call > with no issues at all. > > Also, due to some debugfs core changes a few kernel releases ago, the > checks: > if (!debug_debugfs_dir) { > ... > if (!file) { > can never trigger as debugfs_create_dir() or debugfs_create_file() can > never return NULL (and in the past, it almost never would either). So > as it is, that code isn't correct anyway (my fault, I know, hey, I'm > trying to fix it!) > > I'm trying to make things simple, and easy, and impossible to get wrong. > I know it goes against the normal "robust" kernel development mentality, > but there is no need to ever care about debugfs at all. > > The reason I started all of this is that we have found places where > userspace, and the kernel, was depending on the proper operation of > debugfs. In one horrid example, a device would not display the batter > level if debugfs was disabled. In another case, the kernel was actually > relying on a debugfs call to fail in order to handle some logic the > subsystem should have been doing on its own. All of that has now been > cleaned up, and I am working on making debugfs just not return any > values at all to prevent this type of mess happening again. > > And hey, I am removing code, here's my current tree as a diff from > what is not already merged into linux-next: > 301 files changed, 1394 insertions(+), 4637 deletions(-) > that's always a good thing :) Oh, forgot the pointer to the tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/log/?h=debugfs_cleanup this is all public, just not in linux-next as it's being fed through 50+ different subsystem trees. thanks, greg k-h _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: cpu-debug: no need to check return value of debugfs_create functions 2019-06-18 17:46 ` Greg Kroah-Hartman 2019-06-18 17:52 ` Greg Kroah-Hartman @ 2019-06-18 19:26 ` Mathieu Poirier 2019-06-19 15:27 ` Greg Kroah-Hartman 1 sibling, 1 reply; 6+ messages in thread From: Mathieu Poirier @ 2019-06-18 19:26 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Alexander Shishkin, linux-arm-kernel, Suzuki K Poulose On Tue, 18 Jun 2019 at 11:46, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jun 18, 2019 at 11:23:25AM -0600, Mathieu Poirier wrote: > > Hi Greg, > > > > On Tue, 18 Jun 2019 at 09:52, 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. > > > > Looking around in the kernel there is no shortage of instances where > > the return value of debugfs functions are checked and the logic > > altered based on these values. But there are also just as many that > > don't... It also seems counter intuitive to ignore the return value > > of any function, something that in most case is guaranteed to raise > > admonition. > > In my tree, those instances are almost all gone. I've also posted over > 100+ patches in the past few weeks to clean this up. > > > That being said I am sure there is a good reason to support your > > position - would you mind expanding a little so that I can follow? > > No kernel code should ever care if debugfs works or not. No user code > should ever require it for normal operation either. debugfs was written > to be simple and easy to use, no need to check any return values at all. > > Any return value of a debugfs call can be fed back into another call > with no issues at all. > > Also, due to some debugfs core changes a few kernel releases ago, the > checks: > if (!debug_debugfs_dir) { > ... > if (!file) { > can never trigger as debugfs_create_dir() or debugfs_create_file() can > never return NULL (and in the past, it almost never would either). So That is the rational I was looking for. > as it is, that code isn't correct anyway (my fault, I know, hey, I'm > trying to fix it!) > > I'm trying to make things simple, and easy, and impossible to get wrong. > I know it goes against the normal "robust" kernel development mentality, > but there is no need to ever care about debugfs at all. > > The reason I started all of this is that we have found places where > userspace, and the kernel, was depending on the proper operation of > debugfs. In one horrid example, a device would not display the batter > level if debugfs was disabled. In another case, the kernel was actually > relying on a debugfs call to fail in order to handle some logic the > subsystem should have been doing on its own. All of that has now been > cleaned up, and I am working on making debugfs just not return any > values at all to prevent this type of mess happening again. > > And hey, I am removing code, here's my current tree as a diff from > what is not already merged into linux-next: > 301 files changed, 1394 insertions(+), 4637 deletions(-) > that's always a good thing :) > > Hopefully this helps explain things better. It does - thanks for taking the time to write all this. Do you want me to take the patch through my tree (only to see it coming back to you later this week) or you'll add it directly to yours? In the latter case: Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> Tested-by: Mathieu Poirier <mathieu.poirier@linaro.org> Regards, Mathieu > > thanks, > > greg k-h _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: cpu-debug: no need to check return value of debugfs_create functions 2019-06-18 19:26 ` Mathieu Poirier @ 2019-06-19 15:27 ` Greg Kroah-Hartman 0 siblings, 0 replies; 6+ messages in thread From: Greg Kroah-Hartman @ 2019-06-19 15:27 UTC (permalink / raw) To: Mathieu Poirier; +Cc: Alexander Shishkin, linux-arm-kernel, Suzuki K Poulose On Tue, Jun 18, 2019 at 01:26:12PM -0600, Mathieu Poirier wrote: > On Tue, 18 Jun 2019 at 11:46, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Jun 18, 2019 at 11:23:25AM -0600, Mathieu Poirier wrote: > > > Hi Greg, > > > > > > On Tue, 18 Jun 2019 at 09:52, 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. > > > > > > Looking around in the kernel there is no shortage of instances where > > > the return value of debugfs functions are checked and the logic > > > altered based on these values. But there are also just as many that > > > don't... It also seems counter intuitive to ignore the return value > > > of any function, something that in most case is guaranteed to raise > > > admonition. > > > > In my tree, those instances are almost all gone. I've also posted over > > 100+ patches in the past few weeks to clean this up. > > > > > That being said I am sure there is a good reason to support your > > > position - would you mind expanding a little so that I can follow? > > > > No kernel code should ever care if debugfs works or not. No user code > > should ever require it for normal operation either. debugfs was written > > to be simple and easy to use, no need to check any return values at all. > > > > Any return value of a debugfs call can be fed back into another call > > with no issues at all. > > > > Also, due to some debugfs core changes a few kernel releases ago, the > > checks: > > if (!debug_debugfs_dir) { > > ... > > if (!file) { > > can never trigger as debugfs_create_dir() or debugfs_create_file() can > > never return NULL (and in the past, it almost never would either). So > > That is the rational I was looking for. > > > as it is, that code isn't correct anyway (my fault, I know, hey, I'm > > trying to fix it!) > > > > I'm trying to make things simple, and easy, and impossible to get wrong. > > I know it goes against the normal "robust" kernel development mentality, > > but there is no need to ever care about debugfs at all. > > > > The reason I started all of this is that we have found places where > > userspace, and the kernel, was depending on the proper operation of > > debugfs. In one horrid example, a device would not display the batter > > level if debugfs was disabled. In another case, the kernel was actually > > relying on a debugfs call to fail in order to handle some logic the > > subsystem should have been doing on its own. All of that has now been > > cleaned up, and I am working on making debugfs just not return any > > values at all to prevent this type of mess happening again. > > > > And hey, I am removing code, here's my current tree as a diff from > > what is not already merged into linux-next: > > 301 files changed, 1394 insertions(+), 4637 deletions(-) > > that's always a good thing :) > > > > Hopefully this helps explain things better. > > It does - thanks for taking the time to write all this. > > Do you want me to take the patch through my tree (only to see it > coming back to you later this week) or you'll add it directly to > yours? In the latter case: > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Tested-by: Mathieu Poirier <mathieu.poirier@linaro.org> I can just take it directly, thanks! greg k-h _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-19 15:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-18 15:52 [PATCH] coresight: cpu-debug: no need to check return value of debugfs_create functions Greg Kroah-Hartman 2019-06-18 17:23 ` Mathieu Poirier 2019-06-18 17:46 ` Greg Kroah-Hartman 2019-06-18 17:52 ` Greg Kroah-Hartman 2019-06-18 19:26 ` Mathieu Poirier 2019-06-19 15:27 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).