From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: [GIT PATCH] More PCI patches for 2.6.13
Date: Fri, 09 Sep 2005 22:38:03 UTC
Message-ID: <fa.g0qb439.mgu6ip@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.58.0509091535180.3051@g5.osdl.org>

On Fri, 9 Sep 2005, Greg KH wrote:
>
> Dave Jones:
>   must_check attributes for PCI layer.

Why?

This only clutters up the compile, hiding real errors.

I think all those compile warnings are totally bogus. Who really cares?
Are they going to be fixed, or were they added just to irritate people?

We should have a strict rule: anybody who adds things like "must_check"
and "deprecated" had better also be ready and willing to fix all the new
warnings they cause - you're not allowed to just assume that "somebody
else will fix it".

		Linus

From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: [GIT PATCH] More PCI patches for 2.6.13
Date: Fri, 09 Sep 2005 23:23:14 UTC
Message-ID: <fa.g1pv3j7.nga62v@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.58.0509091613310.3051@g5.osdl.org>

On Fri, 9 Sep 2005, Greg KH wrote:
>
> I fixed up all of the PCI core and USB drivers that were flagged by
> these warnings already.  Biggest area left is network drivers that I
> saw.

The reason I really dislike patches like these is that it causes people to
do questionable things.

For example, there may be perfectly valid reasons why somebody doesn't
care about the result. I don't see much point in forcing people to check
the return value of "pci_enable_wake()" for example. There's really no
real reason to ever care, as far as I can tell - if it fails, there's
nothing you can really do about it anyway.

Also, in general, the fact is that things like "pci_set_power_state()"
might fail in _theory_, but we just don't care. A driver that doesn't
check the return value is in practice as good a driver as one that does,
and forcing people to add code that is totally useless in reality - or
look at a warning that is irritating - is just not very productive.

There are functions where it is really _important_ to check the error
return, because they return errors often enough - and the error case is
something you have to do something about - that it's good to force people
to be aware.

But "pci_set_power_state()"?

I don't think so.

		Linus

From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: [GIT PATCH] More PCI patches for 2.6.13
Date: Sat, 10 Sep 2005 00:22:57 UTC
Message-ID: <fa.hck77t1.in61o9@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.58.0509091644050.30958@g5.osdl.org>

On Fri, 9 Sep 2005, Andrew Morton wrote:
>
> If something like a PCI power management function fails then it will likely
> cause suspend or resume to malfunction, and we have a lot of such problems.

No, for several reasons.

First off, some of those functions can't fail in normal usage. Thus
telling people that they have to check the return code is insane.

Secondly, at least some of the suspend failures have historically been
because drivers returned errors for no good reason. Adding yet another
broken reason to return error is not going to help.

		Linus

From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: [GIT PATCH] More PCI patches for 2.6.13
Date: Sat, 10 Sep 2005 21:13:29 UTC
Message-ID: <fa.he3p7ck.j780o4@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.58.0509101410300.30958@g5.osdl.org>

On Sat, 10 Sep 2005, Alan Stern wrote:

> On Fri, 9 Sep 2005, Linus Torvalds wrote:
> >
> > In other words, there's nothing you can or should do about it. Testing the
> > return value is pointless. And thus adding a "must_check" is really really
> > wrong: it might make people do
> >
> > 	if (pci_set_power_state(pdev, 0))
> > 		return -ENODEV
> >
> > which is actually actively the _wrong_ thing to do, and would just cause
> > old revisions of the chip that might not support PM capabilities to no
> > longer work.
>
> Funny you should say this -- exactly that problem _did_ arise.  See
>
> http://marc.theaimsgroup.com/?l=linux-pci&m=112621842604724&w=2
>
> pci_enable_device_bars() would an error when trying to initialize
> devices without PM support, because it started checking the return value
> from pci_set_power_state().

Case closed.

Bogus warnings are a _bad_ thing. They cause people to write buggy code.

That drivers/pci/pci.c code should be simplified to not look at the error
return from pci_set_power_state() at all. Special-casing EIO is just
another bug waiting to happen.

			Linus

From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: [patch 7/7] uml: retry host close() on EINTR
Date: Sat, 10 Sep 2005 19:00:40 UTC
Message-ID: <fa.hd418cr.i7c1o3@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.58.0509101157170.30958@g5.osdl.org>

On Sat, 10 Sep 2005, Paolo 'Blaisorblade' Giarrusso wrote:
>
> When calling close() on the host, we must retry the operation when we get
> EINTR.

Actually, no.

If close() return EINTR, the file descriptor _will_ have been closed. The
error return just tells you that some error happened on the file: for
example, in the case of EINTR, the close() may not have flushed all the
pending data synchronously.

Re-doing the close() is the wrong thing to do, since in a threaded
environment, something else might have opened another file, gotten the
same file descriptor, and you now close _another_ file.

(Normally, re-doing the close will just return EBADF, of course).

I'm going to drop this patch, but in case you've ever seen a case where
EINTR actually means that the fd didn't get closed, please holler, and we
need to fix it.

			Linus

From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: [patch 7/7] uml: retry host close() on EINTR
Date: Sun, 11 Sep 2005 11:49:52 UTC
Message-ID: <fa.g0qf3b4.lgq7qg@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.58.0509110437120.4912@g5.osdl.org>

On Sun, 11 Sep 2005, Paolo Ornati wrote:
>
> So glibc doc is wrong here:

Yes.

> SUSV3:
> -------------------------------------------------------------
> If close() is interrupted by a signal that is to be caught, it shall
> return -1 with errno set to [EINTR] and the state of fildes is
> unspecified
> -------------------------------------------------------------
>
> Unspecified! ;-)

I don't know of any system where re-trying the close() is the right thing
to do, but I guess they exist. I think the Linux behaviour of "hey, it's
closed, live with it" is pretty universal - almost nobody ever tests the
return value of close().

Even the "careful" users that want to hear about IO errors have to really
do an fsync(), so any IO errors should show up there. Of course, checking
the return value of "close()" in addition to the fsync() is always a good
idea anyway, and I suspect they do.

In Linux, another thread may return with the same fd in open() even _long_
before the close() that released it has even finished. The kernel releases
the fd itself early, and then the rest (anything that could return EINTR -
things like TCP linger stuff etc) is all done with just the "struct file".

So retrying is really really wrong. And not just on Linux. I think this is
true on _most_  if not all unix implementations out there.

		Linus

Subject: Re: [git patches] two warning fixes
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 19 Jul 2007 01:38:48 UTC
Message-ID: <fa.mJ8tFyAJNjnhq5E15vYZi5A61n0@ifi.uio.no>
Newsgroups: fa.linux.kernel

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

Subject: Re: [git patches] two warning fixes
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 19 Jul 2007 01:51:26 UTC
Message-ID: <fa.wNkqzlpYTsX+fMwfcq6UV/leloo@ifi.uio.no>
Newsgroups: fa.linux.kernel

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

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [git patches] two warning fixes
Date: Thu, 19 Jul 2007 18:05:51 UTC
Message-ID: <fa.VQX6eUxuNQf8Oq/rUVzh1vD9+90@ifi.uio.no>

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

Subject: Re: [git patches] two warning fixes
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 19 Jul 2007 18:01:38 UTC
Message-ID: <fa.lazKVQYX9tVjZPb69scMJ09zsrA@ifi.uio.no>
Newsgroups: fa.linux.kernel

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 anything 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 might not care. Maybe some internal
sysfs routines really should care. But not a random driver.

			Linus