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

PATCH In review Tony Breeds 9 participants 29 messages Apr 24, 2008 – May 9, 2008 Framebuffer layer

Share & tools

Tony Breeds

author

2008-04-24 04:34 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/video/aty/radeon_base.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

drivers/video/aty/radeon_base.c +6 -2

index 62867cb..a99f8b6 100644
@@ -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

Signature · 7 lines

1.5.5.1


Yours Tony

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

Andrew Morton

2008-05-06 21:40 UTC #1 lore raw

Quoted 7 lines · On Thu, 24 Apr 2008 14:34:01 +1000 [email protected] (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.

David Miller

2008-05-06 21:43 UTC #2 lore raw

From: Andrew Morton <[email protected]>
Date: Tue, 6 May 2008 14:39:36 -0700

Quoted 13 lines

Quoted 7 lines · On Thu, 24 Apr 2008 14:34:01 +1000 [email protected] (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!

Quoted 8 lines

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

Andrew Morton

2008-05-06 21:57 UTC #3 lore raw

Quoted 22 lines · On Tue, 06 May 2008 14:43:01 -0700 (PDT) David Miller <[email protected]>…

From: Andrew Morton <[email protected]>
Date: Tue, 6 May 2008 14:39:36 -0700
Quoted 19 lines
Quoted 7 lines · On Thu, 24 Apr 2008 14:34:01 +1000 [email protected] (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".

Quoted 7 lines

Quoted 2 lines
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.

Arjan van de Ven

2008-05-07 04:01 UTC #4 lore raw

Quoted 64 lines · On Tue, 6 May 2008 14:56:08 -0700 Andrew Morton <[email protected]>…

Quoted 26 lines · On Tue, 06 May 2008 14:43:01 -0700 (PDT) David Miller <[email protected]>…
From: Andrew Morton <[email protected]>
Date: Tue, 6 May 2008 14:39:36 -0700
Quoted 23 lines
Quoted 11 lines · On Thu, 24 Apr 2008 14:34:01 +1000 [email protected] (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".
Quoted 8 lines
Quoted 3 lines
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:
(quoted diff) +4 -2
@@ -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",
drivers/video/aty/radeon_base.c +1 -0
+		           __func__);
 
 	/* save current mode regs before we switch into the new one
 	 * so we can restore this upon __exit
_

Quoted 2 lines

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)

Andrew Morton

2008-05-07 04:13 UTC #5 lore raw

Quoted 19 lines · On Tue, 6 May 2008 21:00:55 -0700 Arjan van de Ven <[email protected]> wrote:

...
Quoted 10 lines
(quoted diff) +2 -0
+	if (err)
+		pr_warning("%s() Creating sysfs files failed,
continuing\n",
drivers/video/aty/radeon_base.c +1 -0
+		           __func__);
 
 	/* save current mode regs before we switch into the new one
 	 * so we can restore this upon __exit
_
Quoted 2 lines
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....

Quoted 2 lines

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

Arjan van de Ven

2008-05-07 04:16 UTC #6 lore raw

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

Quoted 9 lines

Quoted 6 lines
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.

Quoted 3 lines

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 ? :=)

Arjan van de Ven

2008-05-07 04:20 UTC #7 lore raw

Quoted 4 lines

Quoted 3 lines
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

Benjamin Herrenschmidt

2008-05-07 04:38 UTC #8 lore raw

Quoted 6 lines · Arjan van de Ven

Quoted 4 lines
Quoted 3 lines
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.

Andrew Morton

2008-05-07 04:23 UTC #9 lore raw

In reply to Arjan van de Ven

Quoted 6 lines · On Tue, 6 May 2008 21:15:52 -0700 Arjan van de Ven <[email protected]> wrote:

On Tue, 6 May 2008 21:12:33 -0700
Quoted 4 lines
Quoted 4 lines
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....

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
(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.

Quoted 5 lines

Quoted 3 lines
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()!

Andrew Morton

2008-05-07 04:27 UTC #10 lore raw

Quoted 3 lines · On Tue, 6 May 2008 21:23:14 -0700 Andrew Morton <[email protected]>…

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?

Arjan van de Ven

2008-05-07 04:42 UTC #11 lore raw

Quoted 8 lines · On Tue, 6 May 2008 21:26:43 -0700 Andrew Morton <[email protected]>…

Quoted 4 lines · On Tue, 6 May 2008 21:23:14 -0700 Andrew Morton <[email protected]>…
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.

Quoted 5 lines

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)

Arjan van de Ven

2008-05-07 04:37 UTC #12 lore raw

In reply to Andrew Morton

Quoted 12 lines · On Tue, 6 May 2008 21:23:14 -0700 Andrew Morton <[email protected]>…

Quoted 9 lines · On Tue, 6 May 2008 21:15:52 -0700 Arjan van de Ven <[email protected]> wrote:
On Tue, 6 May 2008 21:12:33 -0700
Quoted 7 lines
Quoted 6 lines
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)

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
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)

Quoted 3 lines

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.

Quoted 3 lines

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.

Paul Mackerras

2008-05-07 05:40 UTC #13 lore raw

In reply to Andrew Morton

Quoted 10 lines · Andrew Morton

Quoted 6 lines · On Tue, 6 May 2008 21:00:55 -0700 Arjan van de Ven <[email protected]> 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.

Benjamin Herrenschmidt

2008-05-07 05:50 UTC #14 lore raw

Quoted 10 lines · Paul Mackerras

Quoted 1 line
Quoted 1 line
(rationale: users tend to not read their dmesg much, but
WARN_ON()'s do
Quoted 3 lines
Quoted 1 line
get noticed)
OK by me, although if we're going to do much more of this it might
be time
Quoted 1 line
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.

Benjamin Herrenschmidt

2008-05-07 00:55 UTC #15 lore raw

In reply to David Miller

Quoted 5 lines · David Miller

Quoted 1 line
So I rewrote the title to "drivers/video/aty/radeon_base.c: notify
user if
Quoted 3 lines
sysfs_create_bin_file() failed".
And your fix looks appropriate - if sysfs_create_bin_file() fails we

Quoted 8 lines

will now get
Quoted 1 line
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.

Andrew Morton

2008-05-07 01:21 UTC #16 lore raw

Quoted 23 lines · On Wed, 07 May 2008 10:54:43 +1000 Benjamin Herrenschmidt…

Quoted 5 lines · David Miller
Quoted 1 line
So I rewrote the title to "drivers/video/aty/radeon_base.c: notify
user if
Quoted 3 lines
sysfs_create_bin_file() failed".
And your fix looks appropriate - if sysfs_create_bin_file() fails we
Quoted 8 lines
will now get
Quoted 1 line
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 ?

Quoted 8 lines

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.

Quoted 3 lines

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.

Benjamin Herrenschmidt

2008-05-07 04:34 UTC #17 lore raw

Quoted 9 lines · Andrew Morton

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.

Quoted 5 lines

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

Quoted 4 lines

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.

Cornelia Huck

2008-05-07 08:23 UTC #18 lore raw

Quoted 5 lines · Benjamin Herrenschmidt

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

Quoted 1 line

mostly only compromise the user ABI,

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

Quoted 4 lines

I believe it's
a _LOT_ more efficient to put -one- printk in the function itself, rather than all callers

Quoted 8 lines

Quoted 4 lines
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?

Benjamin Herrenschmidt

2008-05-07 21:45 UTC #19 lore raw

Quoted 8 lines · Cornelia Huck

But they are often an indication that we messed up earlier (e. g. try to add something twice)...
Quoted 1 line
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.

Quoted 6 lines

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.

Cornelia Huck

2008-05-08 07:35 UTC #20 lore raw

Quoted 3 lines · Benjamin Herrenschmidt

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.

Quoted 3 lines

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

Quoted 4 lines

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

Quoted 4 lines

Quoted 2 lines
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?

Benjamin Herrenschmidt

2008-05-08 07:51 UTC #21 lore raw

Quoted 9 lines · Cornelia Huck

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.

Cornelia Huck

2008-05-08 08:36 UTC #22 lore raw

Quoted 12 lines · Benjamin Herrenschmidt

Quoted 9 lines · Cornelia Huck
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.)

Quoted 2 lines

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.

Greg KH

2008-05-08 22:14 UTC #23 lore raw

Quoted 15 lines · Cornelia Huck

Quoted 12 lines · Benjamin Herrenschmidt
Quoted 9 lines · Cornelia Huck
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.)

Quoted 5 lines

Quoted 2 lines
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

Benjamin Herrenschmidt

2008-05-08 23:07 UTC #24 lore raw

Quoted 3 lines · Greg KH

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.

Paul Mackerras

2008-05-08 23:50 UTC #25 lore raw

In reply to Greg KH

Quoted 5 lines · Greg KH

Quoted 2 lines
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.

Harvey Harrison

2008-05-09 00:02 UTC #26 lore raw

Quoted 8 lines · Paul Mackerras

Quoted 5 lines · Greg KH
Quoted 2 lines
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?

Quoted 8 lines

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

Cornelia Huck

2008-05-09 05:34 UTC #27 lore raw

Quoted 5 lines · Harvey Harrison

Quoted 4 lines · Paul Mackerras
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...

Quoted 4 lines

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

Quoted 2 lines

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?

Cornelia Huck

2008-05-09 05:35 UTC #28 lore raw

In reply to Harvey Harrison

Quoted 5 lines · Harvey Harrison

Quoted 4 lines · Paul Mackerras
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...

Quoted 4 lines

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.

Quoted 2 lines

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?