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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > ---
> >  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