On Sat, 2006-12-09 at 16:56 +0100, Jean Delvare wrote:
> Check for error on radeonfb device sysfs files creation. This fixes the
> following warnings:
(Moving to LKML as I think that's a generic issue)
As usual with most of that crap about return values from
sysfs_create_file, I disagree. strongly.
Why would I prevent the framebuffer from initializing (and thus a
console to be displayed at all on many machines) just because for some
reason, I couldn't create a pair of EDID files in sysfs that are not
even very useful anymore ?
I have _plenty_ of cases where the failure to create sysfs files, while
annoying and maybe deserving a warning, certainly doesn't imply
completely preventing the driver from initializing. However, all the
patches I've seen so far to fix the new warnings do just that (make the
driver fail)
I'd really like to have some kind of macro or attribute or whatever I
can put on a function call to say that I'm purposefully ignoring the
error. Is there some gcc magic that can do that ?
Ben.
On Sun, Dec 10, 2006 at 06:59:10AM +1100, Benjamin Herrenschmidt wrote:
> I'd really like to have some kind of macro or attribute or whatever I
> can put on a function call to say that I'm purposefully ignoring the
> error. Is there some gcc magic that can do that ?
(void)bla()?
Cheers,
Muli
On Sun, 10 Dec 2006 06:59:10 +1100
Benjamin Herrenschmidt <[email protected]> wrote:
> On Sat, 2006-12-09 at 16:56 +0100, Jean Delvare wrote:
>
> > Check for error on radeonfb device sysfs files creation. This fixes the
> > following warnings:
>
> (Moving to LKML as I think that's a generic issue)
>
> As usual with most of that crap about return values from
> sysfs_create_file, I disagree. strongly.
Actually, wrongly.
> Why would I prevent the framebuffer from initializing (and thus a
> console to be displayed at all on many machines) just because for some
> reason, I couldn't create a pair of EDID files in sysfs that are not
> even very useful anymore ?
Because there's a bug in your kernel. We don't hide and work around bugs.
> I have _plenty_ of cases where the failure to create sysfs files, while
> annoying and maybe deserving a warning, certainly doesn't imply
> completely preventing the driver from initializing.
Why does it matter? Just fix the bugs and the issue won't arise. If you
hide them and work around them in this manner, they won't get fixed.
> However, all the
> patches I've seen so far to fix the new warnings do just that (make the
> driver fail)
>
> I'd really like to have some kind of macro or attribute or whatever I
> can put on a function call to say that I'm purposefully ignoring the
> error. Is there some gcc magic that can do that ?
#define HIDE_AND_WORK_AROUND_A_BUG(expr) (void)(expr)
but it might meet some resistance.
Just fix the bugs, for heck's sake.
On Sat, 2006-12-09 at 22:22 +0200, Muli Ben-Yehuda wrote:
> On Sun, Dec 10, 2006 at 06:59:10AM +1100, Benjamin Herrenschmidt wrote:
>
> > I'd really like to have some kind of macro or attribute or whatever I
> > can put on a function call to say that I'm purposefully ignoring the
> > error. Is there some gcc magic that can do that ?
>
> (void)bla()?
Do that actually work to silence the warning ? I though it didn't...I'll
try again.
Ben.
> > Why would I prevent the framebuffer from initializing (and thus a
> > console to be displayed at all on many machines) just because for some
> > reason, I couldn't create a pair of EDID files in sysfs that are not
> > even very useful anymore ?
>
> Because there's a bug in your kernel. We don't hide and work around bugs.
But not initializing the fbdev will be much more effective at hiding the
bug than just displaying a warning, which could just be done inside
sysfs_create_file.
Besides, in most cases, there is no bug. That is, there is no bug that
will cause the file creation to fail and it will not fail.
> Just fix the bugs, for heck's sake.
Considering that in 99% of the case, the creation cannot fail unless
some cosmic ray hit your machine or you are running oom... that is,
plenty of cases where there is _no_ bug now gets a useless code bloat
for checking a result code where there is nothing sane you can do about
it anyway.
Ben.
On Sat, Dec 09, 2006 at 12:38:17PM -0800, Andrew Morton wrote:
> On Sun, 10 Dec 2006 06:59:10 +1100
> Benjamin Herrenschmidt <[email protected]> wrote:
> > Why would I prevent the framebuffer from initializing (and thus a
> > console to be displayed at all on many machines) just because for some
> > reason, I couldn't create a pair of EDID files in sysfs that are not
> > even very useful anymore ?
>
> Because there's a bug in your kernel. We don't hide and work around bugs.
Hmmm, I don't understand. Which is the bug, having a sysfs file
creation fail or going on if it happens?
OG.
On Sat, 9 Dec 2006 22:44:53 +0100
Olivier Galibert <[email protected]> wrote:
> On Sat, Dec 09, 2006 at 12:38:17PM -0800, Andrew Morton wrote:
> > On Sun, 10 Dec 2006 06:59:10 +1100
> > Benjamin Herrenschmidt <[email protected]> wrote:
> > > Why would I prevent the framebuffer from initializing (and thus a
> > > console to be displayed at all on many machines) just because for some
> > > reason, I couldn't create a pair of EDID files in sysfs that are not
> > > even very useful anymore ?
> >
> > Because there's a bug in your kernel. We don't hide and work around bugs.
>
> Hmmm, I don't understand. Which is the bug, having a sysfs file
> creation fail or going on if it happens?
Probably the former, probably the latter.
There may be situations in which we want do to "create this sysfs file if
it doesn't already exist", but I'm not aware of any such.
Generally speaking, if sysfs file creation went wrong, it's due to a bug.
The result is that the driver isn't working as intended: tunables or
instrumentation which it is designed to make available are not present. We
want to know about that bug asap so we can get it fixed.
On Sat, Dec 09, 2006 at 01:58:29PM -0800, Andrew Morton wrote:
> On Sat, 9 Dec 2006 22:44:53 +0100
> Olivier Galibert <[email protected]> wrote:
> > Hmmm, I don't understand. Which is the bug, having a sysfs file
> > creation fail or going on if it happens?
>
> Probably the former, probably the latter.
>
> There may be situations in which we want do to "create this sysfs file if
> it doesn't already exist", but I'm not aware of any such.
>
> Generally speaking, if sysfs file creation went wrong, it's due to a bug.
> The result is that the driver isn't working as intended: tunables or
> instrumentation which it is designed to make available are not present. We
> want to know about that bug asap so we can get it fixed.
Hmmm, then why don't you just drop the return value from the creation
function and BUG() in there is something went wrong. That would allow
for better error messages too.
OG.
On Sat, 9 Dec 2006 23:34:19 +0100
Olivier Galibert <[email protected]> wrote:
> On Sat, Dec 09, 2006 at 01:58:29PM -0800, Andrew Morton wrote:
> > On Sat, 9 Dec 2006 22:44:53 +0100
> > Olivier Galibert <[email protected]> wrote:
> > > Hmmm, I don't understand. Which is the bug, having a sysfs file
> > > creation fail or going on if it happens?
> >
> > Probably the former, probably the latter.
> >
> > There may be situations in which we want do to "create this sysfs file if
> > it doesn't already exist", but I'm not aware of any such.
> >
> > Generally speaking, if sysfs file creation went wrong, it's due to a bug.
> > The result is that the driver isn't working as intended: tunables or
> > instrumentation which it is designed to make available are not present. We
> > want to know about that bug asap so we can get it fixed.
>
> Hmmm, then why don't you just drop the return value from the creation
> function and BUG() in there is something went wrong. That would allow
> for better error messages too.
And (ultimately) make the function return void.
Yes, that's probably a valid approach - we've discussed it before but nobody has
taken it further.
> And (ultimately) make the function return void.
>
> Yes, that's probably a valid approach - we've discussed it before but nobody has
> taken it further.
I would have preferred that approach (with a WARN_ON rather than a BUG
though). On the other hand that would make it slightly harder for the
few cases (if any ?) who actually want something like a "create if it
doesn't exist already" semantic.
I'm a bit worried by the amount of code added by systematic checking of
the results for cases that really should never happen. That's why I
prefer a BUG/WARN type semantic.
Maybe the best is to have the examples like radeonfb actually do the
WARN_ON(sysfs_create_file(...));
Ben.
Olivier Galibert writes:
> Hmmm, then why don't you just drop the return value from the creation
> function and BUG() in there is something went wrong. That would allow
> for better error messages too.
In this instance, BUG would mean that the console text would not ever
show up on the screen, and thus the user would never see the message
nor get any indication what wrong beyond "it failed to boot".
Paul.
Andrew Morton wrote:
> Generally speaking, if sysfs file creation went wrong, it's due to a bug.
> The result is that the driver isn't working as intended: tunables or
> instrumentation which it is designed to make available are not present. We
> want to know about that bug asap so we can get it fixed.
>
Failing to init the fb is certainly a good way to make sure the problem
isn't overlooked, but perhaps a bit shy in the area of letting the user
find out what the error was. Perhaps a warning would be better.
--
bill davidsen <[email protected]>
CTO TMR Associates, Inc
Doing interesting things with small computers since 1979
Ben,
On Sun, 10 Dec 2006 11:55:31 +1100, Benjamin Herrenschmidt wrote:
> > And (ultimately) make the function return void.
> >
> > Yes, that's probably a valid approach - we've discussed it before but nobody has
> > taken it further.
>
> I would have preferred that approach (with a WARN_ON rather than a BUG
> though). On the other hand that would make it slightly harder for the
> few cases (if any ?) who actually want something like a "create if it
> doesn't exist already" semantic.
Let's just boldly state that nobody wants that semantic, if it helps.
> I'm a bit worried by the amount of code added by systematic checking of
> the results for cases that really should never happen. That's why I
> prefer a BUG/WARN type semantic.
>
> Maybe the best is to have the examples like radeonfb actually do the
>
> WARN_ON(sysfs_create_file(...));
Beware that sysfs_remove_bin_file() will complain loudly if you later
attempt to delete that file that was never created.
--
Jean Delvare
> Beware that sysfs_remove_bin_file() will complain loudly if you later
> attempt to delete that file that was never created.
That's another problem... what is a driver that creates 15 files
supposed to do ? Have 15 booleans to remember which files where
successfully created and then test all of them on cleanup ? That sounds
like even more bloat to me...
What about making sysfs_remove_bin_file() not complain ? It's common
practice to have disposal things not complain ... like kfree(NULL), or
the changes done recently so that misc_deregister() can be called even
if misc_register() failed etc...
Ben.
On Sat, 16 Dec 2006 07:16:13 +1100
Benjamin Herrenschmidt <[email protected]> wrote:
> > Beware that sysfs_remove_bin_file() will complain loudly if you later
> > attempt to delete that file that was never created.
>
> That's another problem... what is a driver that creates 15 files
> supposed to do ? Have 15 booleans to remember which files where
> successfully created and then test all of them on cleanup ? That sounds
> like even more bloat to me...
That's the sort of thing which should be done inside sysfs_create_group()
and sysfs_remove_group().
On Fri, Dec 15, 2006 at 12:31:03PM -0800, Andrew Morton wrote:
> On Sat, 16 Dec 2006 07:16:13 +1100
> Benjamin Herrenschmidt <[email protected]> wrote:
>
> > > Beware that sysfs_remove_bin_file() will complain loudly if you later
> > > attempt to delete that file that was never created.
> >
> > That's another problem... what is a driver that creates 15 files
> > supposed to do ? Have 15 booleans to remember which files where
> > successfully created and then test all of them on cleanup ? That sounds
> > like even more bloat to me...
>
> That's the sort of thing which should be done inside sysfs_create_group()
> and sysfs_remove_group().
sysfs_create_group() and remove_group() handles this just fine right
now. Or it should, if not, please let me know and I'll fix it.
As for the bin_file stuff, those are very rare. And I'll gladly take
patches that keep bad things from happening if you try to remove a file
that isn't there.
thanks,
greg k-h
> sysfs_create_group() and remove_group() handles this just fine right
> now. Or it should, if not, please let me know and I'll fix it.
Ok, I didn't know about these. I'll have a look. Thanks !
> As for the bin_file stuff, those are very rare. And I'll gladly take
> patches that keep bad things from happening if you try to remove a file
> that isn't there.
Ben.