On Thursday 04 October 2007 07:33, Daniel Drake wrote:
> Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit.
> It will then be reinitialised to the default (ieee80211_subif_start_xmit)
> in ieee80211_if_set_type.
>
Well.. this kinda sucks, but we can clean up the logic here later.
> + BUG_ON(netif_running(dev));
This will never happen, so there's no point.
ACK with that bit removed.
Thanks,
-Michael Wu
On Thu, 2007-10-04 at 10:34 -0400, Michael Wu wrote:
> On Thursday 04 October 2007 07:33, Daniel Drake wrote:
> > Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit.
> > It will then be reinitialised to the default (ieee80211_subif_start_xmit)
> > in ieee80211_if_set_type.
> >
> Well.. this kinda sucks, but we can clean up the logic here later.
I kinda agree, but the hack in 50e1f4d76a9d45c940733f05ccd42c5bbe6ca132
which caused this was is hack too. Somebody really need to rewrite the
whole interface handling, it's a total mess with the master being
specially treated but initialised by the same functions etc. I think we
should start having a IEEE80211_IF_TYPE_MASTER for this, maybe then
things will fall into place nicer.
johannes
On Thursday 04 October 2007 16:34:43 Michael Wu wrote:
> On Thursday 04 October 2007 07:33, Daniel Drake wrote:
> > Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit.
> > It will then be reinitialised to the default (ieee80211_subif_start_xmit)
> > in ieee80211_if_set_type.
> >
> Well.. this kinda sucks, but we can clean up the logic here later.
>
> > + BUG_ON(netif_running(dev));
> This will never happen, so there's no point.
The reason why BUG_ON exists is to catch bugs that happen, although
they Should Never Happen (tm) ;)
--
Greetings Michael.
On Thursday 04 October 2007 11:06, Michael Buesch wrote:
> The reason why BUG_ON exists is to catch bugs that happen, although
> they Should Never Happen (tm) ;)
This is just paranoia. There's plenty of other BUG_ONs which we use to catch
bugs caused by drivers doing silly things. We can verify that this condition
will never occur within the mac80211 layer, so there's no need to have it.
The only thing this can catch is someone deciding to manually invoke
dev->uninit, which only the unregister code should be doing.
-Michael Wu
On Thu, Oct 04, 2007 at 05:06:05PM +0200, Michael Buesch wrote:
> On Thursday 04 October 2007 16:34:43 Michael Wu wrote:
> > On Thursday 04 October 2007 07:33, Daniel Drake wrote:
> > > Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit.
> > > It will then be reinitialised to the default (ieee80211_subif_start_xmit)
> > > in ieee80211_if_set_type.
> > >
> > Well.. this kinda sucks, but we can clean up the logic here later.
> >
> > > + BUG_ON(netif_running(dev));
> > This will never happen, so there's no point.
>
> The reason why BUG_ON exists is to catch bugs that happen, although
> they Should Never Happen (tm) ;)
Precisely.
--
John W. Linville
[email protected]
On Thu, Oct 04, 2007 at 01:11:33PM -0400, Michael Wu wrote:
> On Thursday 04 October 2007 11:19, John W. Linville wrote:
> > > The reason why BUG_ON exists is to catch bugs that happen, although
> > > they Should Never Happen (tm) ;)
> >
> > Precisely.
> No really, this bug will never happen. This is function is merely a helper
> function which is called from interface removal code (where the interface
> *has* to be down) or from changing the interface type (which ensures that the
> interface is down first). There are an unlimited number of bugs which Should
> Never Happen. That doesn't mean we should start adding BUG_ONs for every
> single one of them. That gives some sort of protection against cosmic rays
> flipping bits, but down here on earth, it's bloat.
Falling back on bloat as an argument against a BUG_ON in a
configuration path seems a bit weak. :-)
Programming with assertions (and BUG_ON is a form of that) is
generally a good practice. Almost any book or other source on
good programming practices will agree. Yes, it can be overdone.
But I don't really think that is the case here, since the check is
relatively inexpensive and the consequence should it ever *somehow*
happen could be a something wierd (crash, corruption, etc) w/o any
other indication of what occured.
Anyway, the point is probably moot in this case if there is no great
objection to the alternative patch I proposed.
John
--
John W. Linville
[email protected]
> Programming with assertions (and BUG_ON is a form of that) is
> generally a good practice. Almost any book or other source on
> good programming practices will agree. Yes, it can be overdone.
> But I don't really think that is the case here, since the check is
> relatively inexpensive and the consequence should it ever *somehow*
> happen could be a something wierd (crash, corruption, etc) w/o any
> other indication of what occured.
The problem with BUG_ON is that it kills the whole system. So every
time you add a BUG_ON into code, you have to weigh whether the problem
you detected is so severe that the right response is to panic. For
example, I can see panicking on something fundamental like corrupted
page tables. However I would submit that the wireless stack should
*never* use BUG_ON -- printing a warning and trying to limp on seems
preferable to me.
- R.
On Thursday 04 October 2007 14:15, John W. Linville wrote:
> Falling back on bloat as an argument against a BUG_ON in a
> configuration path seems a bit weak. :-)
>
Seems strong to me. Bloat slows me down and distracts me from what code really
needs to do. Bloat is an indication that one does not understand what the
code really needs to do.
> Programming with assertions (and BUG_ON is a form of that) is
> generally a good practice. Almost any book or other source on
> good programming practices will agree. Yes, it can be overdone.
> But I don't really think that is the case here, since the check is
> relatively inexpensive and the consequence should it ever *somehow*
> happen could be a something wierd (crash, corruption, etc) w/o any
> other indication of what occured.
>
A line has to be drawn somewhere. There's about a billion places where we can
add checks like this because if an assumption should ever break, the code
will break into pieces. However, we don't, and there's no reason to do so
here other than to needlessly add code just because it makes you feel safer.
-Michael Wu
On Thursday 04 October 2007 11:19, John W. Linville wrote:
> > The reason why BUG_ON exists is to catch bugs that happen, although
> > they Should Never Happen (tm) ;)
>
> Precisely.
No really, this bug will never happen. This is function is merely a helper
function which is called from interface removal code (where the interface
*has* to be down) or from changing the interface type (which ensures that the
interface is down first). There are an unlimited number of bugs which Should
Never Happen. That doesn't mean we should start adding BUG_ONs for every
single one of them. That gives some sort of protection against cosmic rays
flipping bits, but down here on earth, it's bloat.
-Michael Wu
On Thu, Oct 04, 2007 at 02:31:26PM -0700, Roland Dreier wrote:
> > Programming with assertions (and BUG_ON is a form of that) is
> > generally a good practice. Almost any book or other source on
> The problem with BUG_ON is that it kills the whole system. So every
> time you add a BUG_ON into code, you have to weigh whether the problem
> you detected is so severe that the right response is to panic. For
> example, I can see panicking on something fundamental like corrupted
> page tables. However I would submit that the wireless stack should
> *never* use BUG_ON -- printing a warning and trying to limp on seems
> preferable to me.
OK, I'll buy that as an argument to use WARN_ON instead of BUG_ON.
But it doesn't invalidate the desire to have some sort of assertion.
John
--
John W. Linville
[email protected]