[git patches] two warning fixes

[git patches] two warning fixes

[Jeff Garzik]

Please pull from 'warnings' branch of
master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git warnings

to receive the following updates:

 drivers/video/aty/radeon_base.c |   23 ++++++++++++++++++-----
 include/asm-x86_64/tlbflush.h   |    6 +++++-
 2 files changed, 23 insertions(+), 6 deletions(-)

Jeff Garzik (2):
      drivers/video/aty/radeon_base: fix radeonfb_pci_register() err handling
      [X86-64] make flush_tlb_kernel_range() a static inline function

diff --git a/drivers/video/aty/radeon_base.c b/drivers/video/aty/radeon_base.c
index 47ca62f..5a5458b 100644
--- a/drivers/video/aty/radeon_base.c
+++ b/drivers/video/aty/radeon_base.c
@@ -2326,10 +2326,16 @@ static int __devinit radeonfb_pci_register (struct pci_dev *pdev,
 	radeon_check_modes(rinfo, mode_option);
 
 	/* Register some sysfs stuff (should be done better) */
-	if (rinfo->mon1_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
-	if (rinfo->mon2_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+	if (rinfo->mon1_EDID) {
+		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj,&edid1_attr);
+		if (ret)
+			goto err_unmap_fb;
+	}
+	if (rinfo->mon2_EDID) {
+		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj,&edid2_attr);
+		if (ret)
+			goto err_free_mon1;
+	}
 
 	/* save current mode regs before we switch into the new one
 	 * so we can restore this upon __exit
@@ -2353,7 +2359,7 @@ static int __devinit radeonfb_pci_register (struct pci_dev *pdev,
 	if (ret < 0) {
 		printk (KERN_ERR "radeonfb (%s): could not register framebuffer\n",
 			pci_name(rinfo->pdev));
-		goto err_unmap_fb;
+		goto err_free_mon2;
 	}
 
 #ifdef CONFIG_MTRR
@@ -2372,6 +2378,13 @@ static int __devinit radeonfb_pci_register (struct pci_dev *pdev,
 	RTRACE("radeonfb_pci_register END\n");
 
 	return 0;
+
+err_free_mon2:
+	if (rinfo->mon2_EDID)
+		sysfs_remove_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+err_free_mon1:
+	if (rinfo->mon1_EDID)
+		sysfs_remove_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
 err_unmap_fb:
 	iounmap(rinfo->fb_base);
 err_unmap_rom:
diff --git a/include/asm-x86_64/tlbflush.h b/include/asm-x86_64/tlbflush.h
index 8516225..a82464c 100644
--- a/include/asm-x86_64/tlbflush.h
+++ b/include/asm-x86_64/tlbflush.h
@@ -92,7 +92,11 @@ static inline void flush_tlb_range(struct vm_area_struct * vma, unsigned long st
 
 #endif
 
-#define flush_tlb_kernel_range(start, end) flush_tlb_all()
+static inline void flush_tlb_kernel_range(unsigned long start,
+					  unsigned long end)
+{
+	flush_tlb_all();
+}
 
 static inline void flush_tlb_pgtables(struct mm_struct *mm,
 				      unsigned long start, unsigned long end)

Re: [git patches] two warning fixes

[Andi Kleen]

On Thursday 19 July 2007 01:55:04 Jeff Garzik wrote:
> 
> Please pull from 'warnings' branch of
> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git warnings
> 
> to receive the following updates:
> 
>  drivers/video/aty/radeon_base.c |   23 ++++++++++++++++++-----
>  include/asm-x86_64/tlbflush.h   |    6 +++++-
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> Jeff Garzik (2):
>       drivers/video/aty/radeon_base: fix radeonfb_pci_register() err handling
>       [X86-64] make flush_tlb_kernel_range() a static inline function

I already got that patch queued. Why don't you send them through the maintainers?

-Andi

Re: [git patches] two warning fixes

[Jeff Garzik]

Andi Kleen wrote:
> On Thursday 19 July 2007 01:55:04 Jeff Garzik wrote:
>> Please pull from 'warnings' branch of
>> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git warnings
>>
>> to receive the following updates:
>>
>>  drivers/video/aty/radeon_base.c |   23 ++++++++++++++++++-----
>>  include/asm-x86_64/tlbflush.h   |    6 +++++-
>>  2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> Jeff Garzik (2):
>>       drivers/video/aty/radeon_base: fix radeonfb_pci_register() err handling
>>       [X86-64] make flush_tlb_kernel_range() a static inline function
> 
> I already got that patch queued. Why don't you send them through the maintainers?

Because in both cases the maintainers never responded to me, indicating 
they were queued?

Also, you haven't pushed anything upstream during this merge window 
AFAICS, and I didn't want to miss it because you were being slow.

	Jeff

Re: [git patches] two warning fixes

[Benjamin Herrenschmidt]

On Wed, 2007-07-18 at 20:05 -0400, Jeff Garzik wrote:
> Andi Kleen wrote:
> > On Thursday 19 July 2007 01:55:04 Jeff Garzik wrote:
> >> Please pull from 'warnings' branch of
> >> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git warnings
> >>
> >> to receive the following updates:
> >>
> >>  drivers/video/aty/radeon_base.c |   23 ++++++++++++++++++-----
> >>  include/asm-x86_64/tlbflush.h   |    6 +++++-
> >>  2 files changed, 23 insertions(+), 6 deletions(-)
> >>
> >> Jeff Garzik (2):
> >>       drivers/video/aty/radeon_base: fix radeonfb_pci_register() err handling
> >>       [X86-64] make flush_tlb_kernel_range() a static inline function
> > 
> > I already got that patch queued. Why don't you send them through the maintainers?
> 
> Because in both cases the maintainers never responded to me, indicating 
> they were queued?

I suppose I should have acked the radeonfb one... I'm a bit of a slacker
with radeonfb maintainership lately.

However, in this case, I think I'll NACK it. I don't think it's fair to
fail the fb initialization because it couldn't create the EDID files. A
warning in dmesg is enough. For lots of machines, failing the fb init
means no console at all...

In general, I share paulus point of view here that forcing us to test
all those result code from sysfs file creation functions is just a major
PITA and adds bloat all over the kernel. There are many many cases where
the "obvious" thing of erroring out is actually not good policy. In many
cases, the failure to create some random sysfs file shouldn't prevent
the driver from operating, because the consequences of doing the later
are worse than the consequences of not having that sysfs file in the
first place. Thus, warnings are a better thing to do. But multiply the
number of sysfs_* calls by the code size of adding a test & printk and
you'll get the direct non-configurable-out bloat to the kernel.

Ben.

Re: [git patches] two warning fixes

[Linus Torvalds]

On Wed, 18 Jul 2007, Jeff Garzik wrote:
> 
> Please pull from 'warnings' branch of
> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git warnings
> 
> to receive the following updates:

Quite frankly, I think a *lot* better fix for warnings would be to remove 
those damn broken "must_check" things on functions that don't at all need 
checking!

I'm pretty fed up with random "must_check" and "deprecated". They have 
never *ever* helped anybody, afaik. There are some very few functions that 
really do need to have their error returns checked (because not checking 
it is a security issue), but people seem to think "must_check" is a good 
approximation of "I think most of the time it makes sense to check".

So let's make a new rule:

  We absolutely NEVER add things like "must_check" unless not checking 
  causes a real and obvious SECURITY ISSUE.

  And we absolutely *never* add crap like "deprecated", where the only 
  point of the warning is to effectively hide *real* problems.

So realistically, the only thing that needs must_check is pretty much 
things like "get_user()" and quite frankly, I'm not sure even about that 
one.

Ok?

			Linus

Re: [git patches] two warning fixes

[Andrew Morton]

On Thu, 19 Jul 2007 11:19:05 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> In general, I share paulus point of view here that forcing us to test
> all those result code from sysfs file creation functions is just a major
> PITA and adds bloat all over the kernel. There are many many cases where
> the "obvious" thing of erroring out is actually not good policy. In many
> cases, the failure to create some random sysfs file shouldn't prevent
> the driver from operating, because the consequences of doing the later
> are worse than the consequences of not having that sysfs file in the
> first place. 

The only reason why the sysfs creation would fail is a kernel bug,
so the consequence of your proposal is in fact unfixed kernel bugs.

Plus, of course, a driver which doesn't offer the interfaces which
it is supposed to offer.

Now, we can talk about making those sysfs core functions generate warnings
themselves, and we can talk about generating new wrappers around them which
generate warnings and which return void, then migrating code over to use
those.

And we can also talk about blithely ignoring these errors and not telling
anyone about our bugs, but nobody should listen to such scandalous ideas.

Re: [git patches] two warning fixes

[Linus Torvalds]

On Wed, 18 Jul 2007, Andrew Morton wrote:
> 
> The only reason why the sysfs creation would fail is a kernel bug,
> so the consequence of your proposal is in fact unfixed kernel bugs.

Well, the thing is, I suspect we have created way more bugs by having that 
stupid "you must check the return value even if you don't care", than by 
just letting it go.

> Now, we can talk about making those sysfs core functions generate warnings
> themselves, and we can talk about generating new wrappers around them which
> generate warnings and which return void, then migrating code over to use
> those.

If the only valid reason to fail is a kernel bug, it damn well should be 
that sysfs function itself that should complain. It's the only thing that 
knows and cares.

> And we can also talk about blithely ignoring these errors and not telling
> anyone about our bugs, but nobody should listen to such scandalous ideas.

Here's a question: do you always check the return value of "printf()"?

Nobody does. It's not worth it. Trying to do so just creates messy code, 
and MORE BUGS.

So yes, I think we should ignore return values when they have absolutely 
zero interest level to us.

		Linus

Re: [git patches] two warning fixes

[Benjamin Herrenschmidt]

On Wed, 2007-07-18 at 18:50 -0700, Linus Torvalds wrote:
> > Now, we can talk about making those sysfs core functions generate warnings
> > themselves, and we can talk about generating new wrappers around them which
> > generate warnings and which return void, then migrating code over to use
> > those.
> 
> If the only valid reason to fail is a kernel bug, it damn well should be 
> that sysfs function itself that should complain. It's the only thing that 
> knows and cares.

That's pretty much what Paulus and I have been advocating all along.

There -might- be a couple of cases where something has a good reason to
do a call that may fail and want to test the result code. For those few
rare cases (though none comes to mind at the moment), then I suppose we
could provide some kind of _try version of the function (or whatever you
want to call it) that doesn't warn and just returns an error.

But as I said, I can't see any such case out of the blue.

Cheers,
Ben.

Re: [git patches] two warning fixes

[Jeff Garzik]

Linus Torvalds wrote:
> So let's make a new rule:
> 
>   We absolutely NEVER add things like "must_check" unless not checking 
>   causes a real and obvious SECURITY ISSUE.
> 
>   And we absolutely *never* add crap like "deprecated", where the only 
>   point of the warning is to effectively hide *real* problems.
> 
> So realistically, the only thing that needs must_check is pretty much 
> things like "get_user()" and quite frankly, I'm not sure even about that 
> one.
> 
> Ok?


Sounds great to me...  My overall goal is killing useless warnings that 
continually obscure real ones.

	Jeff

Re: [git patches] two warning fixes

[Krzysztof Halasa]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> So let's make a new rule:
>
>   We absolutely NEVER add things like "must_check" unless not checking 
>   causes a real and obvious SECURITY ISSUE.

Oh, come on, almost every kernel bug is a potential security issue.

IMHO, if the function can only fail due to a kernel bug, it should
return void and, in case of bug, explode with BUG_ON() or something
like that. Sure, must_check doesn't apply too well to void.

But, if I have functions which can fail for legitimate (not kernel
bug) reasons, and I know ignoring their return values would always
be a bug, then must_check seems an obvious best and simple defense
against that.
-- 
Krzysztof Halasa

Re: [git patches] two warning fixes

[Krzysztof Halasa]

Jeff Garzik <jeff@garzik.org> writes:

> My overall goal is killing useless warnings
> that continually obscure real ones.

Precisely, the goal should be to make must_check (and similar things)
warn only in real cases.
-- 
Krzysztof Halasa

Re: [git patches] two warning fixes

[Linus Torvalds]

On Thu, 19 Jul 2007, Krzysztof Halasa wrote:
> >
> >   We absolutely NEVER add things like "must_check" unless not checking 
> >   causes a real and obvious SECURITY ISSUE.
> 
> Oh, come on, almost every kernel bug is a potential security issue.

Sure. And adding unnecessary checking that doesn't make sense makes bugs 
*more* likely rather than less.

> IMHO, if the function can only fail due to a kernel bug, it should
> return void and, in case of bug, explode with BUG_ON() or something
> like that. Sure, must_check doesn't apply too well to void.

There are absolutely tons of functions that can return errors (or other 
values), and where many users MAY SIMPLY NOT CARE.

I think "must_check" is an abomination. It makes the callee dictate what 
the caller has to do, but dammit, if the callee really "knows" its errors 
are that serious, it should damn well handle them itself.

The whole "sysfs_create_file()" thing is an example of that. If it fails, 
it fails. The caller can't do anythign about it anyway, except perhaps 
print a message.  Why the hell does such a function have the "right" to 
dictate what the user should do?

That doesn't mean that *all* callers migth not care. Maybe some internal 
sysfs routines really should care. But not a random driver.

			Linus

Re: [git patches] two warning fixes

[Linus Torvalds]

On Thu, 19 Jul 2007, Krzysztof Halasa wrote:
>
> Jeff Garzik <jeff@garzik.org> writes:
> 
> > My overall goal is killing useless warnings
> > that continually obscure real ones.
> 
> Precisely, the goal should be to make must_check (and similar things)
> warn only in real cases.

.. the problem with that mentality is that it's not how people work.

People shut up warnings by adding code.

Adding code tends to add bugs.

People don't generally think "maybe that warning was bogus".

More people *should* generally ask themselves: "was the warning worth it?" 
and then, if the answer is "no", they shouldn't add code, they should 
remove the thing that causes the warning in the first place.

For example, for compiler options, the correct thign is often to just say 
"that option was broken", and not use "-fsign-warning", for example. We've 
literally have had bugs *added* because people "fixed" a sign warning. 
More than once, in fact.

Every time you see a warning, you should ask yourself: is the warning 
interesting, correct and valid? And if it isn't all three, then the 
problem is whatever *causes* the warning, not the code itself.

			Linus

Re: [git patches] two warning fixes

[Tim Tassonis]

Linus Torvalds wrote:

> 
> I think "must_check" is an abomination. It makes the callee dictate what 
> the caller has to do, but dammit, if the callee really "knows" its errors 
> are that serious, it should damn well handle them itself.
> 
> The whole "sysfs_create_file()" thing is an example of that. If it fails, 
> it fails. The caller can't do anythign about it anyway, except perhaps 
> print a message.  Why the hell does such a function have the "right" to 
> dictate what the user should do?

Well, that's just how OO fascists think. An object dictates to the user 
what he/she can do with it, as opposed to the user can do what he 
wants/needs.


Tim

Re: [git patches] two warning fixes

[Krzysztof Halasa]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> More people *should* generally ask themselves: "was the warning worth it?" 
> and then, if the answer is "no", they shouldn't add code, they should 
> remove the thing that causes the warning in the first place.

Sure. If a routine uses must_check yet its return value may be
safely ignored then that must_check is simply misplaced and should
be removed. It does not mean all must_checks are bad - each of them
isn't bad unless one can demonstrate it is.

Back to sysfs_create_bin_file() - if one can demonstrate a caller
can safely ignore the return value (which, it seems, is the
case), then exactly this very must_check should be removed.
-- 
Krzysztof Halasa

Re: [git patches] two warning fixes

[Benjamin Herrenschmidt]

On Fri, 2007-07-20 at 20:34 +0200, Krzysztof Halasa wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > More people *should* generally ask themselves: "was the warning worth it?" 
> > and then, if the answer is "no", they shouldn't add code, they should 
> > remove the thing that causes the warning in the first place.
> 
> Sure. If a routine uses must_check yet its return value may be
> safely ignored then that must_check is simply misplaced and should
> be removed. It does not mean all must_checks are bad - each of them
> isn't bad unless one can demonstrate it is.
> 
> Back to sysfs_create_bin_file() - if one can demonstrate a caller
> can safely ignore the return value (which, it seems, is the
> case), then exactly this very must_check should be removed

Typically, the EDID creation in radeonfb :-)

In fact, I'm not even sure there's -any- user of those sysfs files. I
added them back then to allow distros to extract the EDID infos that
were probed by radeonfb to properly configure the X server (because on
some machines, the EDID is coming from the firmware/BIOS, not from DDC,
and X can't get at it). I don't know if they ever used them.

In any case, it doesn't make sense to abort initialization of the driver
if for some reasons those files can't be created (for example, the core
fbdev starts exposing EDID files, radeonfb isn't properly updated, name
clash, error). Aborting the initialization will make sure that on some
machines such as powermacs with radeon, whatever error is displayed will
never be seen by the user.

That's a typical, but I have plenty more.

For example, the powermac thermal control drivers. They work pretty well
by themselves. They also expose via sysfs all the current values, fan
speeds, temps ,etc... for the sake of whoever wants to do a GUI or
"monitor" what's going on, but that is not critical to the operation of
the driver. Thus, failure to create those files is not critical.

I have plenty other examples.

Thus, we have two choices here:

 - The simple one: sysfs_create_blah() displays a warning when it fails
and has no must_check

 - The one that adds code everywhere (the current one):
sysfs_create_blah() returns an error, has much_check, and thus all
callers like I described abvoe need to add code to test it and print a
warning. Lots of added .text and .data for little benefit.

Cheers,
Ben.

Re: [git patches] two warning fixes

[Jeff Garzik]

Benjamin Herrenschmidt wrote:
> Thus, we have two choices here:
> 
>  - The simple one: sysfs_create_blah() displays a warning when it fails
> and has no must_check
> 
>  - The one that adds code everywhere (the current one):
> sysfs_create_blah() returns an error, has much_check, and thus all
> callers like I described abvoe need to add code to test it and print a
> warning. Lots of added .text and .data for little benefit.


Not necessarily as simple as that -- you need to make sure you don't 
pass something bogus to a sysfs_remove_blah() function at 
unregister/unload time, if sysfs_create_blah() failed.

Certainly sysfs_foo() failure is often ignorable in the sense that you 
want the driver to keep loading... but that does not imply that it is 
strictly ignorable, if you also consider the associated cleanup code.

	Jeff

Re: [git patches] two warning fixes

[Benjamin Herrenschmidt]

> Not necessarily as simple as that -- you need to make sure you don't 
> pass something bogus to a sysfs_remove_blah() function at 
> unregister/unload time, if sysfs_create_blah() failed.
> 
> Certainly sysfs_foo() failure is often ignorable in the sense that you 
> want the driver to keep loading... but that does not imply that it is 
> strictly ignorable, if you also consider the associated cleanup code.

It should be trivial enough to have sysfs_create_blah() do enough
initializations before it can fail so that sysfs_remove_blah() do the
right thing regardless.

It's actually a major PITA for a driver that creates a whole bunch of
sysfs files to have to track precisely which ones were created
successfully for the error path. If it's a single function, goto does
the trick but if for some reason it's not, it's really annoying.

Ben.

Re: [git patches] two warning fixes

[Kyle Moffett]

On Jul 19, 2007, at 14:04:29, Linus Torvalds wrote:
> On Thu, 19 Jul 2007, Krzysztof Halasa wrote:
>> Jeff Garzik <jeff@garzik.org> writes:
>>> My overall goal is killing useless warnings that continually  
>>> obscure real ones.
>>
>> Precisely, the goal should be to make must_check (and similar  
>> things) warn only in real cases.
>
> .. the problem with that mentality is that it's not how people work.
>
> People shut up warnings by adding code.
>
> Adding code tends to add bugs.
>
> People don't generally think "maybe that warning was bogus".
>
> More people *should* generally ask themselves: "was the warning  
> worth it?" and then, if the answer is "no", they shouldn't add  
> code, they should remove the thing that causes the warning in the  
> first place.
>
> For example, for compiler options, the correct thign is often to  
> just say "that option was broken", and not use "-fsign-warning",  
> for example. We've literally have had bugs *added* because people  
> "fixed" a sign warning.  More than once, in fact.
>
> Every time you see a warning, you should ask yourself: is the  
> warning interesting, correct and valid? And if it isn't all three,  
> then the problem is whatever *causes* the warning, not the code  
> itself.

I agree that there are a fair number of things (like the sysfs calls)  
that should just WARN() when they hit an error, but I also think that  
we're currently missing a *lot* of __must_check's that we should  
have.  For example a friend of mine was having problems with an HDAPS  
patch where it just kind of hung.  Turns out the problem was that the  
code blithely called scsi_execute_async() and then put itself to  
sleep on a completion... except scsi_execute_async() returned failure  
and the completion would never complete.

For instance, I would bet that a fair number of the other int- 
returning functions in include/scsi/scsi_device.h want __must_check  
on them.  That said, the person adding the __must_check should be  
REQUIRED to do at least a superficial audit of the code.

I'd propose a few simple rules:
   (1) If it can return the only pointer to freshly-allocated pointer  
then it's __must_check
   (2) If it can return a hard error which the caller must handle  
specially, then it's __must_check
   (3) If the only possible error is a kernel bug then make the damn  
thing return void and give it a big fat WARN() when it fails.
   (4) For any other case (or if you are unsure), don't flag it.

And of course the burden of proof is on the person trying to add the  
__must_check.

Cheers,
Kyle Moffett