[PATCH] iio: core: print error message on debugfs register failure

[PATCH] iio: core: print error message on debugfs register failure

[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

Re: [PATCH] iio: core: print error message on debugfs register failure

[Greg KH]

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

Re: [PATCH] iio: core: print error message on debugfs register failure

[Ardelean, Alexandru]

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