Re: [git patches] two warning fixes
On Jul 19, 2007, at 14:04:29, Linus Torvalds wrote: On Thu, 19 Jul 2007, Krzysztof Halasa wrote: Jeff Garzik <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
On Jul 19, 2007, at 14:04:29, Linus Torvalds wrote: On Thu, 19 Jul 2007, Krzysztof Halasa wrote: Jeff Garzik [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
On Fri, 2007-07-20 at 20:34 +0200, Krzysztof Halasa wrote: > Linus Torvalds <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
Linus Torvalds <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
Linus Torvalds [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
On Fri, 2007-07-20 at 20:34 +0200, Krzysztof Halasa wrote: Linus Torvalds [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
On Thu, 19 Jul 2007, Krzysztof Halasa wrote: > > Jeff Garzik <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
Jeff Garzik <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
Linus Torvalds <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
Linus Torvalds [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
Jeff Garzik [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
On Thu, 19 Jul 2007, Krzysztof Halasa wrote: Jeff Garzik [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
On Thu, 19 Jul 2007 11:19:05 +1000 Benjamin Herrenschmidt <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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(>pdev->dev.kobj, _attr);
- if (rinfo->mon2_EDID)
- sysfs_create_bin_file(>pdev->dev.kobj, _attr);
+ if (rinfo->mon1_EDID) {
+ ret = sysfs_create_bin_file(>pdev->dev.kobj,_attr);
+ if (ret)
+ goto err_unmap_fb;
+ }
+ if (rinfo->mon2_EDID) {
+ ret = sysfs_create_bin_file(>pdev->dev.kobj,_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(>pdev->dev.kobj, _attr);
+err_free_mon1:
+ if (rinfo->mon1_EDID)
+ sysfs_remove_bin_file(>pdev->dev.kobj, _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)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
On Thu, 19 Jul 2007 11:19:05 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git patches] two warning fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
