[git patches] two warning fixes

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)

-------------------------------------------------------------------------

This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

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

Andi Kleen wrote:

On Thursday 19 July 2007 01:55:04 Jeff Garzik wrote:

quoted

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

On Wed, 2007-07-18 at 20:05 -0400, Jeff Garzik wrote:

Andi Kleen wrote:

quoted

On Thursday 19 July 2007 01:55:04 Jeff Garzik wrote:

quoted

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.



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

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

On Thu, 19 Jul 2007 11:19:05 +1000
Benjamin Herrenschmidt [off-list ref] 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.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

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

On Wed, 2007-07-18 at 18:50 -0700, Linus Torvalds wrote:

quoted

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.



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

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



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

On Wed, 18 Jul 2007 18:50:28 -0700 (PDT) Linus Torvalds [off-list ref] wrote:

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

Look at the reason why the sysfs must_check's were adding in the first
place.

We had a long string of mysterious crashes coming out of the
sysfs/driver/bus/kobject code which appeared to be due to corrupted data
structures and which were believed to be caused by incorrect callers but we
didn't have any idea _which_ callers were incorrect because the crashes
happened long after the error had occurred.

On examination we saw that a very large amount of the driver core was
_internally_ failing to check callee return values and was just proceeding
as if things had succeeded.  Also, many (most) external callers were
failing to check or report upon callee failures as well.

So we were basically left blind without any way to identify where the bugs
were.  We ended up tightening up all of the driver core (largely guided by
the must_check warnings which it emitted) and many callers were fixed as
well.

Now, it could be that we should have (or can now) relax the requirements
upon the callers, but we should still still be told when something has
unexpectedly failed.  Maybe not as a general rule - more of a special-case
for these interfaces because we have such a long history of failures in
there and because of how the lack of error checking caused those failures
to be so hard to fix.

Which is why I proposed new void-returning wrappers which will warn for the
caller when something failed.  If we were to remove the __must_checks then
we'd lose much of our checking ability within the sysfs/driver core, as
well as from callers.

This stuff has got better, and hopefully Tejun's work will make it better
still.  But I don't think it is exactly bulletproof yet.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/