* [PATCH] iio: core: print error message on debugfs register failure
@ 2019-12-11 15:16 Alexandru Ardelean
2019-12-17 15:19 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Alexandru Ardelean @ 2019-12-11 15:16 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, Michael Hennerich, Alexandru Ardelean
From: Michael Hennerich <michael.hennerich@analog.com>
If there's a failure when registering a debugfs entry for a device, don't
silently ignore the failure. Instead, print an error message and an error
code signaling the failure.
Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-core.c | 34 +++++++++++++++++++++++++++------
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index dab67cb69fe6..662dabf8b08c 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -364,23 +364,45 @@ static const struct file_operations iio_debugfs_reg_fops = {
static void iio_device_unregister_debugfs(struct iio_dev *indio_dev)
{
debugfs_remove_recursive(indio_dev->debugfs_dentry);
+ indio_dev->debugfs_dentry = NULL;
}
static void iio_device_register_debugfs(struct iio_dev *indio_dev)
{
+ struct dentry *d;
+ int ret;
+
if (indio_dev->info->debugfs_reg_access == NULL)
return;
if (!iio_debugfs_dentry)
return;
- indio_dev->debugfs_dentry =
- debugfs_create_dir(dev_name(&indio_dev->dev),
- iio_debugfs_dentry);
+ d = debugfs_create_dir(dev_name(&indio_dev->dev), iio_debugfs_dentry);
+ if (IS_ERR_OR_NULL(d))
+ goto error;
+
+ indio_dev->debugfs_dentry = d;
+
+ d = debugfs_create_file("direct_reg_access", 0644,
+ indio_dev->debugfs_dentry, indio_dev,
+ &iio_debugfs_reg_fops);
+
+ if (IS_ERR_OR_NULL(d))
+ goto error;
- debugfs_create_file("direct_reg_access", 0644,
- indio_dev->debugfs_dentry, indio_dev,
- &iio_debugfs_reg_fops);
+ return;
+
+error:
+ if (IS_ERR(d))
+ ret = PTR_ERR(d);
+ else
+ ret = -EFAULT;
+
+ dev_err(indio_dev->dev.parent,
+ "Error when trying to register debugfs: %d\n", ret);
+
+ iio_device_unregister_debugfs(indio_dev);
}
#else
static void iio_device_register_debugfs(struct iio_dev *indio_dev)
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: core: print error message on debugfs register failure
2019-12-11 15:16 [PATCH] iio: core: print error message on debugfs register failure Alexandru Ardelean
@ 2019-12-17 15:19 ` Greg KH
2019-12-17 15:23 ` Ardelean, Alexandru
0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2019-12-17 15:19 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel, jic23, Michael Hennerich
On Wed, Dec 11, 2019 at 05:16:36PM +0200, Alexandru Ardelean wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
>
> If there's a failure when registering a debugfs entry for a device, don't
> silently ignore the failure. Instead, print an error message and an error
> code signaling the failure.
No, never do that.
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/iio/industrialio-core.c | 34 +++++++++++++++++++++++++++------
> 1 file changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index dab67cb69fe6..662dabf8b08c 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -364,23 +364,45 @@ static const struct file_operations iio_debugfs_reg_fops = {
> static void iio_device_unregister_debugfs(struct iio_dev *indio_dev)
> {
> debugfs_remove_recursive(indio_dev->debugfs_dentry);
> + indio_dev->debugfs_dentry = NULL;
> }
>
> static void iio_device_register_debugfs(struct iio_dev *indio_dev)
> {
> + struct dentry *d;
> + int ret;
> +
> if (indio_dev->info->debugfs_reg_access == NULL)
> return;
>
> if (!iio_debugfs_dentry)
> return;
>
> - indio_dev->debugfs_dentry =
> - debugfs_create_dir(dev_name(&indio_dev->dev),
> - iio_debugfs_dentry);
> + d = debugfs_create_dir(dev_name(&indio_dev->dev), iio_debugfs_dentry);
> + if (IS_ERR_OR_NULL(d))
> + goto error;
No, don't do that, I spent a lot of time removing all of these pointless
checks.
No kernel code shoudl ever care if debugfs is workign or not, just make
the call and move on. You can always pass the result of a debugfs call
into another one with no problems.
> +
> + indio_dev->debugfs_dentry = d;
> +
> + d = debugfs_create_file("direct_reg_access", 0644,
> + indio_dev->debugfs_dentry, indio_dev,
> + &iio_debugfs_reg_fops);
> +
> + if (IS_ERR_OR_NULL(d))
> + goto error;
This check isn't even correct :)
So this isn't going to work no matter what, sorry.
just don't do this.
The code is just fine as-is.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: core: print error message on debugfs register failure
2019-12-17 15:19 ` Greg KH
@ 2019-12-17 15:23 ` Ardelean, Alexandru
0 siblings, 0 replies; 3+ messages in thread
From: Ardelean, Alexandru @ 2019-12-17 15:23 UTC (permalink / raw)
To: gregkh; +Cc: jic23, Hennerich, Michael, linux-kernel, linux-iio
On Tue, 2019-12-17 at 16:19 +0100, Greg KH wrote:
> [External]
>
> On Wed, Dec 11, 2019 at 05:16:36PM +0200, Alexandru Ardelean wrote:
> > From: Michael Hennerich <michael.hennerich@analog.com>
> >
> > If there's a failure when registering a debugfs entry for a device,
> > don't
> > silently ignore the failure. Instead, print an error message and an
> > error
> > code signaling the failure.
>
> No, never do that.
>
> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > drivers/iio/industrialio-core.c | 34 +++++++++++++++++++++++++++------
> > 1 file changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-core.c
> > b/drivers/iio/industrialio-core.c
> > index dab67cb69fe6..662dabf8b08c 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -364,23 +364,45 @@ static const struct file_operations
> > iio_debugfs_reg_fops = {
> > static void iio_device_unregister_debugfs(struct iio_dev *indio_dev)
> > {
> > debugfs_remove_recursive(indio_dev->debugfs_dentry);
> > + indio_dev->debugfs_dentry = NULL;
> > }
> >
> > static void iio_device_register_debugfs(struct iio_dev *indio_dev)
> > {
> > + struct dentry *d;
> > + int ret;
> > +
> > if (indio_dev->info->debugfs_reg_access == NULL)
> > return;
> >
> > if (!iio_debugfs_dentry)
> > return;
> >
> > - indio_dev->debugfs_dentry =
> > - debugfs_create_dir(dev_name(&indio_dev->dev),
> > - iio_debugfs_dentry);
> > + d = debugfs_create_dir(dev_name(&indio_dev->dev),
> > iio_debugfs_dentry);
> > + if (IS_ERR_OR_NULL(d))
> > + goto error;
>
> No, don't do that, I spent a lot of time removing all of these pointless
> checks.
>
> No kernel code shoudl ever care if debugfs is workign or not, just make
> the call and move on. You can always pass the result of a debugfs call
> into another one with no problems.
>
> > +
> > + indio_dev->debugfs_dentry = d;
> > +
> > + d = debugfs_create_file("direct_reg_access", 0644,
> > + indio_dev->debugfs_dentry, indio_dev,
> > + &iio_debugfs_reg_fops);
> > +
> > + if (IS_ERR_OR_NULL(d))
> > + goto error;
>
> This check isn't even correct :)
>
> So this isn't going to work no matter what, sorry.
>
> just don't do this.
>
> The code is just fine as-is.
I'm fine with this answer.
I'll sync our IIO core code with upstream.
Mostly I just care that the diff between the 2 files is empty.
Thanks
Alex
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-12-17 15:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 15:16 [PATCH] iio: core: print error message on debugfs register failure Alexandru Ardelean
2019-12-17 15:19 ` Greg KH
2019-12-17 15:23 ` Ardelean, Alexandru
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).