[PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Tony Breeds]

Current kernel builds warn about:
drivers/video/aty/radeon_base.c: In function 'radeonfb_pci_register':
drivers/video/aty/radeon_base.c:2334: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
drivers/video/aty/radeon_base.c:2336: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result

Do minimal checking of these functions and issue a warning if either
fails.  They don't seem to be critical..

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
---
 drivers/video/aty/radeon_base.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/aty/radeon_base.c b/drivers/video/aty/radeon_base.c
index 62867cb..a99f8b6 100644
--- a/drivers/video/aty/radeon_base.c
+++ b/drivers/video/aty/radeon_base.c
@@ -2158,6 +2158,7 @@ static int __devinit radeonfb_pci_register (struct pci_dev *pdev,
 	struct fb_info *info;
 	struct radeonfb_info *rinfo;
 	int ret;
+	int err = 0;
 
 	RTRACE("radeonfb_pci_register BEGIN\n");
 	
@@ -2331,9 +2332,12 @@ static int __devinit radeonfb_pci_register (struct pci_dev *pdev,
 
 	/* Register some sysfs stuff (should be done better) */
 	if (rinfo->mon1_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
+		err |= sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
 	if (rinfo->mon2_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+		err |= sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+	if (err)
+		pr_warning("%s() Creating sysfs files failed, continuing\n",
+		           __func__);
 
 	/* save current mode regs before we switch into the new one
 	 * so we can restore this upon __exit
-- 
1.5.5.1


Yours Tony

  linux.conf.au    http://www.marchsouth.org/
  Jan 19 - 24 2009 The Australian Linux Technical Conference!

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Andrew Morton]

On Thu, 24 Apr 2008 14:34:01 +1000
tony@bakeyournoodle.com (Tony Breeds) wrote:

> Current kernel builds warn about:
> drivers/video/aty/radeon_base.c: In function 'radeonfb_pci_register':
> drivers/video/aty/radeon_base.c:2334: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
> drivers/video/aty/radeon_base.c:2336: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
> 
> Do minimal checking of these functions and issue a warning if either
> fails.  They don't seem to be critical..

well OK, but I object to the patch title!

The point isn't to silence warnings.  It is to fix the problem which that
warning is drawing our attention to.

So I rewrote the title to "drivers/video/aty/radeon_base.c: notify user if
sysfs_create_bin_file() failed".

And your fix looks appropriate - if sysfs_create_bin_file() fails we will now get
reports of this and we can find out what kernel bug caused this to happen.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 6 May 2008 14:39:36 -0700

> On Thu, 24 Apr 2008 14:34:01 +1000
> tony@bakeyournoodle.com (Tony Breeds) wrote:
> 
> > Current kernel builds warn about:
> > drivers/video/aty/radeon_base.c: In function 'radeonfb_pci_register':
> > drivers/video/aty/radeon_base.c:2334: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
> > drivers/video/aty/radeon_base.c:2336: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
> > 
> > Do minimal checking of these functions and issue a warning if either
> > fails.  They don't seem to be critical..
> 
> well OK, but I object to the patch title!
> 
> The point isn't to silence warnings.  It is to fix the problem which that
> warning is drawing our attention to.
> 
> So I rewrote the title to "drivers/video/aty/radeon_base.c: notify user if
> sysfs_create_bin_file() failed".
> 
> And your fix looks appropriate - if sysfs_create_bin_file() fails we will now get
> reports of this and we can find out what kernel bug caused this to happen.

The last time someone "fixed" this warning in the radeon driver,
people lost their consoles.

Just giving a heads up...

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Andrew Morton]

On Tue, 06 May 2008 14:43:01 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Tue, 6 May 2008 14:39:36 -0700
> 
> > On Thu, 24 Apr 2008 14:34:01 +1000
> > tony@bakeyournoodle.com (Tony Breeds) wrote:
> > 
> > > Current kernel builds warn about:
> > > drivers/video/aty/radeon_base.c: In function 'radeonfb_pci_register':
> > > drivers/video/aty/radeon_base.c:2334: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
> > > drivers/video/aty/radeon_base.c:2336: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
> > > 
> > > Do minimal checking of these functions and issue a warning if either
> > > fails.  They don't seem to be critical..
> > 
> > well OK, but I object to the patch title!
> > 
> > The point isn't to silence warnings.  It is to fix the problem which that
> > warning is drawing our attention to.
> > 
> > So I rewrote the title to "drivers/video/aty/radeon_base.c: notify user if
> > sysfs_create_bin_file() failed".
> > 
> > And your fix looks appropriate - if sysfs_create_bin_file() fails we will now get
> > reports of this and we can find out what kernel bug caused this to happen.
> 
> The last time someone "fixed" this warning in the radeon driver,
> people lost their consoles.
> 
> Just giving a heads up...

heh, thanks.

This one just emits a warning:

@@ -2340,9 +2341,12 @@ static int __devinit radeonfb_pci_regist
 
 	/* Register some sysfs stuff (should be done better) */
 	if (rinfo->mon1_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
+		err |= sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
 	if (rinfo->mon2_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+		err |= sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+	if (err)
+		pr_warning("%s() Creating sysfs files failed, continuing\n",
+		           __func__);
 
 	/* save current mode regs before we switch into the new one
 	 * so we can restore this upon __exit
_


So from what you say, it sounds like we will be seeing that warning.  I
wonder why.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Arjan van de Ven]

On Tue, 6 May 2008 14:56:08 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 06 May 2008 14:43:01 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Andrew Morton <akpm@linux-foundation.org>
> > Date: Tue, 6 May 2008 14:39:36 -0700
> > 
> > > On Thu, 24 Apr 2008 14:34:01 +1000
> > > tony@bakeyournoodle.com (Tony Breeds) wrote:
> > > 
> > > > Current kernel builds warn about:
> > > > drivers/video/aty/radeon_base.c: In function
> > > > 'radeonfb_pci_register': drivers/video/aty/radeon_base.c:2334:
> > > > warning: ignoring return value of 'sysfs_create_bin_file',
> > > > declared with attribute warn_unused_result
> > > > drivers/video/aty/radeon_base.c:2336: warning: ignoring return
> > > > value of 'sysfs_create_bin_file', declared with attribute
> > > > warn_unused_result
> > > > 
> > > > Do minimal checking of these functions and issue a warning if
> > > > either fails.  They don't seem to be critical..
> > > 
> > > well OK, but I object to the patch title!
> > > 
> > > The point isn't to silence warnings.  It is to fix the problem
> > > which that warning is drawing our attention to.
> > > 
> > > So I rewrote the title to "drivers/video/aty/radeon_base.c:
> > > notify user if sysfs_create_bin_file() failed".
> > > 
> > > And your fix looks appropriate - if sysfs_create_bin_file() fails
> > > we will now get reports of this and we can find out what kernel
> > > bug caused this to happen.
> > 
> > The last time someone "fixed" this warning in the radeon driver,
> > people lost their consoles.
> > 
> > Just giving a heads up...
> 
> heh, thanks.
> 
> This one just emits a warning:
> 
> @@ -2340,9 +2341,12 @@ static int __devinit radeonfb_pci_regist
>  
>  	/* Register some sysfs stuff (should be done better) */
>  	if (rinfo->mon1_EDID)
> -		sysfs_create_bin_file(&rinfo->pdev->dev.kobj,
> &edid1_attr);
> +		err |= sysfs_create_bin_file(&rinfo->pdev->dev.kobj,
> &edid1_attr); if (rinfo->mon2_EDID)
> -		sysfs_create_bin_file(&rinfo->pdev->dev.kobj,
> &edid2_attr);
> +		err |= sysfs_create_bin_file(&rinfo->pdev->dev.kobj,
> &edid2_attr);
> +	if (err)
> +		pr_warning("%s() Creating sysfs files failed,
> continuing\n",
> +		           __func__);
>  
>  	/* save current mode regs before we switch into the new one
>  	 * so we can restore this upon __exit
> _
> 
> 
> So from what you say, it sounds like we will be seeing that warning.
> I wonder why.

can we make it a WARN_ON() as well? that way we'll see it in various
kerneloops.org stats etc etc.. and we also get a nice backtrace for
free to go with it....

(rationale: users tend to not read their dmesg much, but WARN_ON()'s do
get noticed)

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Andrew Morton]

On Tue, 6 May 2008 21:00:55 -0700 Arjan van de Ven <arjan@infradead.org> wrote:

> ...
>
> > +	if (err)
> > +		pr_warning("%s() Creating sysfs files failed,
> > continuing\n",
> > +		           __func__);
> >  
> >  	/* save current mode regs before we switch into the new one
> >  	 * so we can restore this upon __exit
> > _
> > 
> > 
> > So from what you say, it sounds like we will be seeing that warning.
> > I wonder why.
> 
> can we make it a WARN_ON() as well? that way we'll see it in various
> kerneloops.org stats etc etc.. and we also get a nice backtrace for
> free to go with it....
> 
> (rationale: users tend to not read their dmesg much, but WARN_ON()'s do
> get noticed)

OK by me, although if we're going to do much more of this it might be time
to add a WARN_ON which takes (fmt, args...).

Which should be called WARN, but of course 12,000,000 drivers have gone and
screwed that up with indiscriminate namespace poaching.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Arjan van de Ven]

On Tue, 6 May 2008 21:12:33 -0700

> > can we make it a WARN_ON() as well? that way we'll see it in various
> > kerneloops.org stats etc etc.. and we also get a nice backtrace for
> > free to go with it....
> > 
> > (rationale: users tend to not read their dmesg much, but
> > WARN_ON()'s do get noticed)
> 
> OK by me, although if we're going to do much more of this it might be
> time to add a WARN_ON which takes (fmt, args...).

totally; I was just talking to some others about doing just this.
> 
> Which should be called WARN, but of course 12,000,000 drivers have
> gone and screwed that up with indiscriminate namespace poaching.

how about this deal: I implement it, you pick the name ? :=)
 

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Arjan van de Ven]

> 
> > Which should be called WARN, but of course 12,000,000 drivers have
> > gone and screwed that up with indiscriminate namespace poaching.
> 

seems to be only 5 of them; we can move those aside I suppose

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Benjamin Herrenschmidt]

On Tue, 2008-05-06 at 21:20 -0700, Arjan van de Ven wrote:
> > 
> > > Which should be called WARN, but of course 12,000,000 drivers have
> > > gone and screwed that up with indiscriminate namespace poaching.
> > 
> 
> seems to be only 5 of them; we can move those aside I suppose

In the case of radeonfb, don't bother. Those EDID files aren't used
by anything afaik.

Ben.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Andrew Morton]

On Tue, 6 May 2008 21:15:52 -0700 Arjan van de Ven <arjan@infradead.org> wrote:

> On Tue, 6 May 2008 21:12:33 -0700
> 
> > > can we make it a WARN_ON() as well? that way we'll see it in various
> > > kerneloops.org stats etc etc.. and we also get a nice backtrace for
> > > free to go with it....
> > > 
> > > (rationale: users tend to not read their dmesg much, but
> > > WARN_ON()'s do get noticed)
> > 
> > OK by me, although if we're going to do much more of this it might be
> > time to add a WARN_ON which takes (fmt, args...).
> 
> totally; I was just talking to some others about doing just this.

The challenge will be to minimise the code footprint.

otcompletelyoh, perhaps we could generate a backtrace from within printk()
itself for when it sees messages which have KERN_ERR or some other
suitably-chosen (and probably configurable) facility level.

That'll generate false positives and will reveal dubious choices, but we
can fix those up.

> > 
> > Which should be called WARN, but of course 12,000,000 drivers have
> > gone and screwed that up with indiscriminate namespace poaching.
> 
> how about this deal: I implement it, you pick the name ? :=)

AKPM()!

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Andrew Morton]

On Tue, 6 May 2008 21:23:14 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> otcompletelyoh, perhaps we could generate a backtrace from within printk()
> itself for when it sees messages which have KERN_ERR or some other
> suitably-chosen (and probably configurable) facility level.

and if that is impractical, perhaps create a new printk facility
(KERN_TRACE?) which gets rewritten to KERN_ERR, but tells printk()
to generate a trace.

Do we really need the file-n-line info?

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Arjan van de Ven]

On Tue, 6 May 2008 21:26:43 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 6 May 2008 21:23:14 -0700 Andrew Morton
> <akpm@linux-foundation.org> wrote:
> 
> > otcompletelyoh, perhaps we could generate a backtrace from within
> > printk() itself for when it sees messages which have KERN_ERR or
> > some other suitably-chosen (and probably configurable) facility
> > level.
> 
> and if that is impractical, perhaps create a new printk facility
> (KERN_TRACE?) which gets rewritten to KERN_ERR, but tells printk()
> to generate a trace.
> 
> Do we really need the file-n-line info?

for me, it really does help to have that; it saves me and others time
to grep where exactly it happens. it also allows me to tie things to
gitweb automatically
(see  http://www.kerneloops.org/search.php?search=write_msg for an
example of how this works; this feature is very appreciated by several
developers and was done on their request)

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Arjan van de Ven]

On Tue, 6 May 2008 21:23:14 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 6 May 2008 21:15:52 -0700 Arjan van de Ven
> <arjan@infradead.org> wrote:
> 
> > On Tue, 6 May 2008 21:12:33 -0700
> > 
> > > > can we make it a WARN_ON() as well? that way we'll see it in
> > > > various kerneloops.org stats etc etc.. and we also get a nice
> > > > backtrace for free to go with it....
> > > > 
> > > > (rationale: users tend to not read their dmesg much, but
> > > > WARN_ON()'s do get noticed)
> > > 
> > > OK by me, although if we're going to do much more of this it
> > > might be time to add a WARN_ON which takes (fmt, args...).
> > 
> > totally; I was just talking to some others about doing just this.
> 
> The challenge will be to minimise the code footprint.

the meat of WARN_ON() is already out of line; so all the vararg stuff
is just going to be one function out of line, if we really care about
the last 20 bytes we can even implement WARN_ON() using an empty string.

I suspect it's not all that bad.
(but gcc is still churning, so no hard data yet)
 
> otcompletelyoh, perhaps we could generate a backtrace from within
> printk() itself for when it sees messages which have KERN_ERR or some
> other suitably-chosen (and probably configurable) facility level.

interesting thought. Only thing is that this won't give the option to
have a condition there, or to compile it out for those folks who care
about squeezing the last bytes out by disabling such debug messages.


> 
> That'll generate false positives and will reveal dubious choices, but
> we can fix those up.

I'm not all that worried about that; if we introduce a new flag that is
it'll be explicit and fine.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Paul Mackerras]

Andrew Morton writes:

> On Tue, 6 May 2008 21:00:55 -0700 Arjan van de Ven <arjan@infradead.org> wrote:
> > can we make it a WARN_ON() as well? that way we'll see it in various
> > kerneloops.org stats etc etc.. and we also get a nice backtrace for
> > free to go with it....
> > 
> > (rationale: users tend to not read their dmesg much, but WARN_ON()'s do
> > get noticed)
> 
> OK by me, although if we're going to do much more of this it might be time
> to add a WARN_ON which takes (fmt, args...).

It would make a lot more sense to put the printk and/or WARN_ON inside
sysfs_create_bin_file rather than in every single caller.

Paul.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Benjamin Herrenschmidt]

On Wed, 2008-05-07 at 15:40 +1000, Paul Mackerras wrote:
> > > (rationale: users tend to not read their dmesg much, but
> WARN_ON()'s do
> > > get noticed)
> > 
> > OK by me, although if we're going to do much more of this it might
> be time
> > to add a WARN_ON which takes (fmt, args...).
> 
> It would make a lot more sense to put the printk and/or WARN_ON inside
> sysfs_create_bin_file rather than in every single caller.

Amen !

Ben.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Benjamin Herrenschmidt]

On Tue, 2008-05-06 at 14:43 -0700, David Miller wrote:
> > So I rewrote the title to "drivers/video/aty/radeon_base.c: notify
> user if
> > sysfs_create_bin_file() failed".
> > 
> > And your fix looks appropriate - if sysfs_create_bin_file() fails we
> will now get
> > reports of this and we can find out what kernel bug caused this to
> happen.
> 
> The last time someone "fixed" this warning in the radeon driver,
> people lost their consoles.
> 
> Just giving a heads up...

I asked Tony to only warn. I still don't like it tho. As I (and paulus)
have explained several times in the past, but I'm not going to veto the
patch because I'm tired of that argument.
 
We have something like 99% of users of sysfs_create_file not supposed to
fail right ?

So what we are effectively doing is adding -hundreds- of printk's all
over the place (bloat bloat bloat) while instead we could have warned
inside sysfs_create_file itself, and provided a __sysfs_create_file or
sysfs_create_file_nowarn, or whatever you want to call it for the
handful of users that actually want to explicitely deal with failures.

And it's the same for a whole bunch of those must check things. We are
just adding bloat often for nothing useful. In some case, even harmful
as we prevent entire modules from initializing due to what is often just
a minor failure.

Ben.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Andrew Morton]

On Wed, 07 May 2008 10:54:43 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> On Tue, 2008-05-06 at 14:43 -0700, David Miller wrote:
> > > So I rewrote the title to "drivers/video/aty/radeon_base.c: notify
> > user if
> > > sysfs_create_bin_file() failed".
> > > 
> > > And your fix looks appropriate - if sysfs_create_bin_file() fails we
> > will now get
> > > reports of this and we can find out what kernel bug caused this to
> > happen.
> > 
> > The last time someone "fixed" this warning in the radeon driver,
> > people lost their consoles.
> > 
> > Just giving a heads up...
> 
> I asked Tony to only warn. I still don't like it tho. As I (and paulus)
> have explained several times in the past, but I'm not going to veto the
> patch because I'm tired of that argument.
>  
> We have something like 99% of users of sysfs_create_file not supposed to
> fail right ?
> 
> So what we are effectively doing is adding -hundreds- of printk's all
> over the place (bloat bloat bloat) while instead we could have warned
> inside sysfs_create_file itself, and provided a __sysfs_create_file or
> sysfs_create_file_nowarn, or whatever you want to call it for the
> handful of users that actually want to explicitely deal with failures.
> 
> And it's the same for a whole bunch of those must check things. We are
> just adding bloat often for nothing useful.

You said "not useful".  But you're just wrong.

Look at the history of this.  A couple of years ago we were seeing a huge
number of mysterious crashes and warnings in and around sysfs and driver
code and we just didn't have a clue what was causing it, because those
crashes were happening long, long after the buggy code had done its work.

So I went in there and found a tremendous amount of code in driver core,
sysfs and in callers of both which was just ignoring error returns and
blundering on.

It was a comnplete undebuggable unmaintainable mess.  And the reason why it
was undebuggable was because the code was failing to detect errors *when
they occurred*.  So we (me, Cornelia, Greg, others) set about fixing all of
that.  And to support that effort we marked all the things which should be
checked with __must_check.

Now you come along and cherrypick a few callsites where you'd rather not
bother checking and assert that the entire effort was wrong-headed.  Well
sorry, no, it wasn't.  Sure, there's a little bit of undesirable fallout
but the whole thing had.  to.  be.  done.

> In some case, even harmful
> as we prevent entire modules from initializing due to what is often just
> a minor failure.

There is no such thing as a "minor failure".  Code either works as
designed or it doesn't.  If it doesn't work as designed then we cannot
state how serious the problem is until we've understood its causes.

If you have sysfs files which aren't needed then remove the damn things. 
If they _are_ needed then they should be available to all users of the
driver.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Benjamin Herrenschmidt]

On Tue, 2008-05-06 at 18:20 -0700, Andrew Morton wrote:
> Look at the history of this.  A couple of years ago we were seeing a huge
> number of mysterious crashes and warnings in and around sysfs and driver
> code and we just didn't have a clue what was causing it, because those
> crashes were happening long, long after the buggy code had done its work.
> 
> So I went in there and found a tremendous amount of code in driver core,
> sysfs and in callers of both which was just ignoring error returns and
> blundering on.
> 
> It was a comnplete undebuggable unmaintainable mess.  And the reason why it
> was undebuggable was because the code was failing to detect errors *when
> they occurred*.  So we (me, Cornelia, Greg, others) set about fixing all of
> that.  And to support that effort we marked all the things which should be
> checked with __must_check.

You haven't read me properly. I'm not advocating completely ignoring
those errors. In fact, I'm all about keeping must check on things like
allocations. However, in cases like sysfs_create_file() like many
similar things where failure will -not- prevent proper operations of the
driver or subsystem, mostly only compromise the user ABI, I believe it's
a _LOT_ more efficient to put -one- printk in the function itself,
rather than all callers

> Now you come along and cherrypick a few callsites where you'd rather not
> bother checking and assert that the entire effort was wrong-headed.  Well
> sorry, no, it wasn't.  Sure, there's a little bit of undesirable fallout
> but the whole thing had.  to.  be.  done.

Of course the whole effort was not wrong headed. I'm really only
complaining about all those stupid sysfs_create_file() and maybe a
handful of similar ones.

Ben.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Cornelia Huck]

On Wed, 07 May 2008 14:33:24 +1000,
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> You haven't read me properly. I'm not advocating completely ignoring
> those errors. In fact, I'm all about keeping must check on things like
> allocations. However, in cases like sysfs_create_file() like many
> similar things where failure will -not- prevent proper operations of the
> driver or subsystem, 

But they are often an indication that we messed up earlier (e. g. try
to add something twice)...

> mostly only compromise the user ABI, 

Which is bad enough in itself. Most people will want to avoid a
crippled ABI.

> I believe it's
> a _LOT_ more efficient to put -one- printk in the function itself,
> rather than all callers
> 
> > Now you come along and cherrypick a few callsites where you'd rather not
> > bother checking and assert that the entire effort was wrong-headed.  Well
> > sorry, no, it wasn't.  Sure, there's a little bit of undesirable fallout
> > but the whole thing had.  to.  be.  done.
> 
> Of course the whole effort was not wrong headed. I'm really only
> complaining about all those stupid sysfs_create_file() and maybe a
> handful of similar ones.

Hm, just took a look at the code:

- for "entry already exists", sysfs will already spit a warning.
- for "argh, we can't get a dirent", sysfs won't say anything.

The first one is the one we really want to yell about, since we've
messed up somewhere. The second one is not as likely, maybe we want to
warn about it when we activate debug options?

Which of the current __must_check functions do you think should have
the __must_check removed?

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Benjamin Herrenschmidt]

On Wed, 2008-05-07 at 10:23 +0200, Cornelia Huck wrote:
> 
> But they are often an indication that we messed up earlier (e. g. try
> to add something twice)...
>
> > mostly only compromise the user ABI, 
> 
> Which is bad enough in itself. Most people will want to avoid a
> crippled ABI.

You prefer a crippled ABI or a machine that doesn't boot with no console
at all to see what happened because the console driver refused to
initialize due to such a sysfs file conflict ?

Now, again, that's only part of what I'm talking about anyway.

What I'm saying is basically that rather than have a test & printk in
every bloody driver, we are better off having it once in the function
itself (bloat ?)

In addition, in most cases, failure of initializing the driver is -not-
the right solution, so the driver should probably just warn, which can
as well be done by ... having sysfs_create_file() itself do the warning.

> The first one is the one we really want to yell about, since we've
> messed up somewhere. The second one is not as likely, maybe we want to
> warn about it when we activate debug options?
> 
> Which of the current __must_check functions do you think should have
> the __must_check removed?

sysfs_create_file is a good candidate imho :-)

Ben.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Cornelia Huck]

On Thu, 08 May 2008 07:43:41 +1000,
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> You prefer a crippled ABI or a machine that doesn't boot with no console
> at all to see what happened because the console driver refused to
> initialize due to such a sysfs file conflict ?

So it really depends on the driver, doesn't it?

If the file isn't vital, ignore the return code (sysfs will already
complain). If things will break, fail the device registration.

> What I'm saying is basically that rather than have a test & printk in
> every bloody driver, we are better off having it once in the function
> itself (bloat ?)

A centralized printk makes sense, sure (and it is already in place for
duplicate entries).

> 
> In addition, in most cases, failure of initializing the driver is -not-
> the right solution, so the driver should probably just warn, which can
> as well be done by ... having sysfs_create_file() itself do the warning.

sysfs_add_one() does it :)

I was under the impression that failing the initialization was usually
the right thing to do, since we end up with an un-configurable,
un-usable device. But that may be coloured by my experience with s390
devices, where we rely on sysfs attributes extensively. If indeed most
sysfs files are non-vital, removing the __must_check and relying on a
scary warning in the core may be fine. (OTOH, adding checks to the core
has helped us to find some lurking bugs.)

> > Which of the current __must_check functions do you think should have
> > the __must_check removed?
> 
> sysfs_create_file is a good candidate imho :-)

:)

I was thinking more about functions like bus_rescan_devices(). Any
others?

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Benjamin Herrenschmidt]

On Thu, 2008-05-08 at 09:34 +0200, Cornelia Huck wrote:
> 
> I was under the impression that failing the initialization was usually
> the right thing to do, since we end up with an un-configurable,
> un-usable device. But that may be coloured by my experience with s390
> devices, where we rely on sysfs attributes extensively. If indeed most
> sysfs files are non-vital, removing the __must_check and relying on a
> scary warning in the core may be fine. (OTOH, adding checks to the
> core
> has helped us to find some lurking bugs.)

The driver can still fail initialization if it wants... I'm just
objecting to the __must_check.

Ben.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Cornelia Huck]

On Thu, 08 May 2008 17:49:44 +1000,
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> On Thu, 2008-05-08 at 09:34 +0200, Cornelia Huck wrote:
> > 
> > I was under the impression that failing the initialization was usually
> > the right thing to do, since we end up with an un-configurable,
> > un-usable device. But that may be coloured by my experience with s390
> > devices, where we rely on sysfs attributes extensively. If indeed most
> > sysfs files are non-vital, removing the __must_check and relying on a
> > scary warning in the core may be fine. (OTOH, adding checks to the
> > core
> > has helped us to find some lurking bugs.)
> 
> The driver can still fail initialization if it wants... I'm just
> objecting to the __must_check.

I hear you :) I found it useful, but it seems we should get rid of it
for _create_file() now.

<Adding Greg to cc: for his opinion on this>

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Greg KH]

On Thu, May 08, 2008 at 10:36:21AM +0200, Cornelia Huck wrote:
> On Thu, 08 May 2008 17:49:44 +1000,
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > 
> > On Thu, 2008-05-08 at 09:34 +0200, Cornelia Huck wrote:
> > > 
> > > I was under the impression that failing the initialization was usually
> > > the right thing to do, since we end up with an un-configurable,
> > > un-usable device. But that may be coloured by my experience with s390
> > > devices, where we rely on sysfs attributes extensively. If indeed most
> > > sysfs files are non-vital, removing the __must_check and relying on a
> > > scary warning in the core may be fine. (OTOH, adding checks to the
> > > core
> > > has helped us to find some lurking bugs.)
> > 
> > The driver can still fail initialization if it wants... I'm just
> > objecting to the __must_check.
> 
> I hear you :) I found it useful, but it seems we should get rid of it
> for _create_file() now.

Why?  You point out it found some real bugs, should we just assume that
no new bugs of this same problem will happen again in the future?

thanks,

greg k-h

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Benjamin Herrenschmidt]

On Thu, 2008-05-08 at 15:03 -0700, Greg KH wrote:
> Why?  You point out it found some real bugs, should we just assume
> that
> no new bugs of this same problem will happen again in the future?

Making sure it warns/printk's on error would probably find the same
bugs...

Ben.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Paul Mackerras]

Greg KH writes:

> > I hear you :) I found it useful, but it seems we should get rid of it
> > for _create_file() now.
> 
> Why?  You point out it found some real bugs, should we just assume that
> no new bugs of this same problem will happen again in the future?

Because it causes warnings for the callers which don't really care
whether the file gets created or not, and getting rid of those
warnings adds unnecessary bloat.

I think the best solution is to make a new sysfs_maybe_create_file()
which isn't marked must_check, and then move suitable callers (such as
radeonfb) over to that.  That will make it obvious in the callers that
the file creation isn't guaranteed.

Paul.

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Harvey Harrison]

On Fri, 2008-05-09 at 09:50 +1000, Paul Mackerras wrote:
> Greg KH writes:
> 
> > > I hear you :) I found it useful, but it seems we should get rid of it
> > > for _create_file() now.
> > 
> > Why?  You point out it found some real bugs, should we just assume that
> > no new bugs of this same problem will happen again in the future?
> 
> Because it causes warnings for the callers which don't really care
> whether the file gets created or not, and getting rid of those
> warnings adds unnecessary bloat.
> 
> I think the best solution is to make a new sysfs_maybe_create_file()
> which isn't marked must_check, and then move suitable callers (such as
> radeonfb) over to that.  That will make it obvious in the callers that
> the file creation isn't guaranteed.

Or just a a flag parameter to the existing one that says whether failure
is allowed or not.  In a case that fails, a WARN_ON can be printed from
the common create_file rather than putting printks all over the kernel.

This would also document which files are necessary vs. optional.

Harvey

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Cornelia Huck]

On Thu, 08 May 2008 17:02:44 -0700,
Harvey Harrison <harvey.harrison@gmail.com> wrote:

> On Fri, 2008-05-09 at 09:50 +1000, Paul Mackerras wrote:
> > I think the best solution is to make a new sysfs_maybe_create_file()
> > which isn't marked must_check, and then move suitable callers (such as
> > radeonfb) over to that.  That will make it obvious in the callers that
> > the file creation isn't guaranteed.

Either that...

> 
> Or just a a flag parameter to the existing one that says whether failure
> is allowed or not.  In a case that fails, a WARN_ON can be printed from
> the common create_file rather than putting printks all over the kernel.

...or that. But the core should still warn about duplicate files in all
cases, since that indicates a bug. (And that will catch the problems we
found before; IIRC a core warning didn't exist back then.)

> 
> This would also document which files are necessary vs. optional.

Shouldn't that rather go into Documentation/ABI/, so that people can
easily find out what they may rely on?

Re: [PATCH] Silence 'ignoring return value' warnings in drivers/video/aty/radeon_base.c

[Cornelia Huck]

On Thu, 08 May 2008 17:02:44 -0700,
Harvey Harrison <harvey.harrison@gmail.com> wrote:

> On Fri, 2008-05-09 at 09:50 +1000, Paul Mackerras wrote:
> > I think the best solution is to make a new sysfs_maybe_create_file()
> > which isn't marked must_check, and then move suitable callers (such as
> > radeonfb) over to that.  That will make it obvious in the callers that
> > the file creation isn't guaranteed.

Either that...

> 
> Or just a a flag parameter to the existing one that says whether failure
> is allowed or not.  In a case that fails, a WARN_ON can be printed from
> the common create_file rather than putting printks all over the kernel.

...or that. But the core should still warn about duplicate files in all
cases, since that indicates a bug. (And that will catch the problems we
found before; IIRC a core warning didn't exist back then.)

> 
> This would also document which files are necessary vs. optional.

Shouldn't that rather go into Documentation/ABI/, so that people can
easily find out what they may rely on?