Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1239099 > unrolled thread

Re: [char-misc 1/2 4.3 V2] mei: Fix debugfs filename in error output

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [char-misc 1/2 4.3 V2] mei: Fix debugfs filename in error output Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-10-04 13:20 +0200
    RE: [char-misc 1/2 4.3 V2] mei: Fix debugfs filename in error output "Winkler, Tomas" <tomas.winkler@intel.com> - 2015-10-05 16:50 +0200
      Re: [char-misc 1/2 4.3 V2] mei: Fix debugfs filename in error output Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-10-05 17:40 +0200
        RE: [char-misc 1/2 4.3 V2] mei: Fix debugfs filename in error output "Winkler, Tomas" <tomas.winkler@intel.com> - 2015-10-06 10:50 +0200

#1239099 — Re: [char-misc 1/2 4.3 V2] mei: Fix debugfs filename in error output

On Mon, Sep 21, 2015 at 10:58:15AM +0300, Tomas Winkler wrote:
> From: Alexander Kuleshov <kuleshovmail@gmail.com>
> 
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
> V2: fixed author address
> 
>  drivers/misc/mei/debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c
> index 4b469cf9e60f..c157f0ba575c 100644
> --- a/drivers/misc/mei/debugfs.c
> +++ b/drivers/misc/mei/debugfs.c
> @@ -213,7 +213,7 @@ int mei_dbgfs_register(struct mei_device *dev, const char *name)
>  	f = debugfs_create_file("active", S_IRUSR, dir,
>  				dev, &mei_dbgfs_fops_active);
>  	if (!f) {
> -		dev_err(dev->dev, "meclients: registration failed\n");
> +		dev_err(dev->dev, "active: registration failed\n");
>  		goto err;

You should never care if a debugfs call fails or not.  Also, this will
"fail" if you don't have CONFIG_DEBUGFS enabled, which means you are
using the api wrong :(

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1239618

> 
> On Mon, Sep 21, 2015 at 10:58:15AM +0300, Tomas Winkler wrote:
> > From: Alexander Kuleshov <kuleshovmail@gmail.com>
> >
> > Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> > V2: fixed author address
> >
> >  drivers/misc/mei/debugfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c
> > index 4b469cf9e60f..c157f0ba575c 100644
> > --- a/drivers/misc/mei/debugfs.c
> > +++ b/drivers/misc/mei/debugfs.c
> > @@ -213,7 +213,7 @@ int mei_dbgfs_register(struct mei_device *dev, const
> char *name)
> >  	f = debugfs_create_file("active", S_IRUSR, dir,
> >  				dev, &mei_dbgfs_fops_active);
> >  	if (!f) {
> > -		dev_err(dev->dev, "meclients: registration failed\n");
> > +		dev_err(dev->dev, "active: registration failed\n");
> >  		goto err;
> 
> You should never care if a debugfs call fails or not.

The system should not be dependent on the debug feature but, it is always good to know if there our system is failing 

  Also, this will
> "fail" if you don't have CONFIG_DEBUGFS enabled, which means you are
> using the api wrong :(

The whole file is not compiled if CONFIG_DEBUGFS is not set, please see the Makefile 


Thanks
Tomas 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239683

On Mon, Oct 05, 2015 at 02:47:35PM +0000, Winkler, Tomas wrote:
> > 
> > On Mon, Sep 21, 2015 at 10:58:15AM +0300, Tomas Winkler wrote:
> > > From: Alexander Kuleshov <kuleshovmail@gmail.com>
> > >
> > > Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > ---
> > > V2: fixed author address
> > >
> > >  drivers/misc/mei/debugfs.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c
> > > index 4b469cf9e60f..c157f0ba575c 100644
> > > --- a/drivers/misc/mei/debugfs.c
> > > +++ b/drivers/misc/mei/debugfs.c
> > > @@ -213,7 +213,7 @@ int mei_dbgfs_register(struct mei_device *dev, const
> > char *name)
> > >  	f = debugfs_create_file("active", S_IRUSR, dir,
> > >  				dev, &mei_dbgfs_fops_active);
> > >  	if (!f) {
> > > -		dev_err(dev->dev, "meclients: registration failed\n");
> > > +		dev_err(dev->dev, "active: registration failed\n");
> > >  		goto err;
> > 
> > You should never care if a debugfs call fails or not.
> 
> The system should not be dependent on the debug feature but, it is
> always good to know if there our system is failing 

And what can you do if it is "failing"?  Really nothing, so there's
nothing to check here.

>   Also, this will
> > "fail" if you don't have CONFIG_DEBUGFS enabled, which means you are
> > using the api wrong :(
> 
> The whole file is not compiled if CONFIG_DEBUGFS is not set, please see the Makefile 

Ok, then you don't need to check anything.  Debugfs was created to be
dirt-simple, don't add complexity and "must unwind cleanly" type logic
here where it's not needed at all.  That just hurts my sensibilities for
why I made the API the way it is in the first place :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240263

> > > >  		goto err;
> > >
> > > You should never care if a debugfs call fails or not.
> >
> > The system should not be dependent on the debug feature but, it is
> > always good to know if there our system is failing
> 
> And what can you do if it is "failing"?  Really nothing, so there's
> nothing to check here.

As far as I can see the function debugfs_create_file may fail for a few reasons and it return  NULL if this is happening.
I might ignore the error as you suggested, but I'm not sure why not to give a hint in log that this happened. 
Second, as far as I scanned the kernel sources, checking the return value of this function and acting on this is very common.
 
> >   Also, this will
> > > "fail" if you don't have CONFIG_DEBUGFS enabled, which means you are
> > > using the api wrong :(
> >
> > The whole file is not compiled if CONFIG_DEBUGFS is not set, please see the
> Makefile
> 
> Ok, then you don't need to check anything.  Debugfs was created to be
> dirt-simple, don't add complexity and "must unwind cleanly" type logic
> here where it's not needed at all.  That just hurts my sensibilities for
> why I made the API the way it is in the first place :)

I don't see the code much complex, it is pretty much boilerplate code and this is why this c&p error had happened. 
So this patch just fixes a c&p error in an error message and if we wish to change the behavior to match your vision 
I suggest to doing it another patch set... maybe even sweeping the whole kernel. 

Thanks
Tomas 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web