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

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

[Tomas Winkler]

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;
 	}
 	f = debugfs_create_file("devstate", S_IRUSR, dir,
-- 
2.4.3

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

[Greg Kroah-Hartman]

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

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

[Winkler, Tomas]

> 
> 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 

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

[Greg Kroah-Hartman]

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

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

[Winkler, Tomas]

> > > >  		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 

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

[Winkler, Tomas]

> 
> > > > >  		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.


Greg, are you merging this patch? What is your final say?

Thanks
Tomas 

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

['Greg Kroah-Hartman']

On Sun, Oct 25, 2015 at 12:45:30PM +0000, Winkler, Tomas wrote:
> > 
> > > > > >  		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.
> 
> 
> Greg, are you merging this patch? What is your final say?

It's no longer in my todo queue, so there's nothing for me to apply
here, sorry.

Please resend it if you feel it should be added.

thanks,

greg k-h