[PATCH] wireless: b43: fix Oops on card eject during transfer

[PATCH] wireless: b43: fix Oops on card eject during transfer

[Guennadi Liakhovetski]

An Oops has once been observed, when the SDIO card had been ejected during
IO. The PC value shows, that the dev pointer in b43_op_stop() was NULL.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Might also be good for stable

 drivers/net/wireless/b43/main.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index 5634d9a..3849c25 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -4834,6 +4834,9 @@ static void b43_op_stop(struct ieee80211_hw *hw)
 	cancel_work_sync(&(wl->beacon_update_trigger));
 
 	mutex_lock(&wl->mutex);
+	if (!dev)
+		goto out_unlock;
+
 	if (b43_status(dev) >= B43_STAT_STARTED) {
 		dev = b43_wireless_core_stop(dev);
 		if (!dev)
-- 
1.7.2.5

Re: [PATCH] wireless: b43: fix Oops on card eject during transfer

[Julian Calaby]

Hi Guennadi,

On Fri, Jan 6, 2012 at 22:58, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> An Oops has once been observed, when the SDIO card had been ejected during
> IO. The PC value shows, that the dev pointer in b43_op_stop() was NULL.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> Might also be good for stable
>
>  drivers/net/wireless/b43/main.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 5634d9a..3849c25 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -4834,6 +4834,9 @@ static void b43_op_stop(struct ieee80211_hw *hw)
>        cancel_work_sync(&(wl->beacon_update_trigger));
>
>        mutex_lock(&wl->mutex);
> +       if (!dev)
> +               goto out_unlock;

Is there any reason to take this mutex, then jump to a label that
releases it immediately?

Would it be better to add a new label after the mutex_unlock() below
and jump to there immediately after the cancel_work_sync() call above?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH] wireless: b43: fix Oops on card eject during transfer

[Guennadi Liakhovetski]

Hi Julian

On Sat, 7 Jan 2012, Julian Calaby wrote:

> Hi Guennadi,
> 
> On Fri, Jan 6, 2012 at 22:58, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > An Oops has once been observed, when the SDIO card had been ejected during
> > IO. The PC value shows, that the dev pointer in b43_op_stop() was NULL.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >
> > Might also be good for stable
> >
> >  drivers/net/wireless/b43/main.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> > index 5634d9a..3849c25 100644
> > --- a/drivers/net/wireless/b43/main.c
> > +++ b/drivers/net/wireless/b43/main.c
> > @@ -4834,6 +4834,9 @@ static void b43_op_stop(struct ieee80211_hw *hw)
> >        cancel_work_sync(&(wl->beacon_update_trigger));
> >
> >        mutex_lock(&wl->mutex);
> > +       if (!dev)
> > +               goto out_unlock;
> 
> Is there any reason to take this mutex, then jump to a label that
> releases it immediately?
> 
> Would it be better to add a new label after the mutex_unlock() below
> and jump to there immediately after the cancel_work_sync() call above?

The reason is exactly that: to avoid adding one more label. It is an 
exceptional case, that that branch would be taken, so, there's no reason 
to optimize for it. Whereas adding one label would clutter the code 
needlessly, IMHO.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] wireless: b43: fix Oops on card eject during transfer

[Guennadi Liakhovetski]

Hi

I haven't seen this patch going into any tree or being included in any 
pull request. What's the status?

Thanks
Guennadi

On Fri, 6 Jan 2012, Guennadi Liakhovetski wrote:

> Hi Julian
> 
> On Sat, 7 Jan 2012, Julian Calaby wrote:
> 
> > Hi Guennadi,
> > 
> > On Fri, Jan 6, 2012 at 22:58, Guennadi Liakhovetski
> > <g.liakhovetski@gmx.de> wrote:
> > > An Oops has once been observed, when the SDIO card had been ejected during
> > > IO. The PC value shows, that the dev pointer in b43_op_stop() was NULL.
> > >
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > >
> > > Might also be good for stable
> > >
> > >  drivers/net/wireless/b43/main.c |    3 +++
> > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> > > index 5634d9a..3849c25 100644
> > > --- a/drivers/net/wireless/b43/main.c
> > > +++ b/drivers/net/wireless/b43/main.c
> > > @@ -4834,6 +4834,9 @@ static void b43_op_stop(struct ieee80211_hw *hw)
> > >        cancel_work_sync(&(wl->beacon_update_trigger));
> > >
> > >        mutex_lock(&wl->mutex);
> > > +       if (!dev)
> > > +               goto out_unlock;
> > 
> > Is there any reason to take this mutex, then jump to a label that
> > releases it immediately?
> > 
> > Would it be better to add a new label after the mutex_unlock() below
> > and jump to there immediately after the cancel_work_sync() call above?
> 
> The reason is exactly that: to avoid adding one more label. It is an 
> exceptional case, that that branch would be taken, so, there's no reason 
> to optimize for it. Whereas adding one label would clutter the code 
> needlessly, IMHO.
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] wireless: b43: fix Oops on card eject during transfer

[John W. Linville]

Well, you weren't interested in making the minor change suggested
by Julian.  So I set it aside with the intent of making the change
myself, then neglected to follow through. :-(  I'll try to get to
that today.  Or, feel free to submit a new version yourself.

John

On Fri, Jan 13, 2012 at 12:29:13AM +0100, Guennadi Liakhovetski wrote:
> Hi
> 
> I haven't seen this patch going into any tree or being included in any 
> pull request. What's the status?
> 
> Thanks
> Guennadi
> 
> On Fri, 6 Jan 2012, Guennadi Liakhovetski wrote:
> 
> > Hi Julian
> > 
> > On Sat, 7 Jan 2012, Julian Calaby wrote:
> > 
> > > Hi Guennadi,
> > > 
> > > On Fri, Jan 6, 2012 at 22:58, Guennadi Liakhovetski
> > > <g.liakhovetski@gmx.de> wrote:
> > > > An Oops has once been observed, when the SDIO card had been ejected during
> > > > IO. The PC value shows, that the dev pointer in b43_op_stop() was NULL.
> > > >
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > ---
> > > >
> > > > Might also be good for stable
> > > >
> > > >  drivers/net/wireless/b43/main.c |    3 +++
> > > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> > > > index 5634d9a..3849c25 100644
> > > > --- a/drivers/net/wireless/b43/main.c
> > > > +++ b/drivers/net/wireless/b43/main.c
> > > > @@ -4834,6 +4834,9 @@ static void b43_op_stop(struct ieee80211_hw *hw)
> > > >        cancel_work_sync(&(wl->beacon_update_trigger));
> > > >
> > > >        mutex_lock(&wl->mutex);
> > > > +       if (!dev)
> > > > +               goto out_unlock;
> > > 
> > > Is there any reason to take this mutex, then jump to a label that
> > > releases it immediately?
> > > 
> > > Would it be better to add a new label after the mutex_unlock() below
> > > and jump to there immediately after the cancel_work_sync() call above?
> > 
> > The reason is exactly that: to avoid adding one more label. It is an 
> > exceptional case, that that branch would be taken, so, there's no reason 
> > to optimize for it. Whereas adding one label would clutter the code 
> > needlessly, IMHO.
> > 
> > Thanks
> > Guennadi
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> > 
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] wireless: b43: fix Oops on card eject during transfer

[Guennadi Liakhovetski]

Hi John

On Fri, 13 Jan 2012, John W. Linville wrote:

> Well, you weren't interested in making the minor change suggested
> by Julian.  So I set it aside with the intent of making the change
> myself, then neglected to follow through. :-(  I'll try to get to
> that today.  Or, feel free to submit a new version yourself.

I didn't neglect, I explained, why I considered his suggestion not 
necessarily an improvement. If there were several votes for that change, 
or if it were a maintainer, who requested the change - I would consider 
obeying, but this time it was just an opinion of one developer against 
that of another.

Thanks
Guennadi

> John
> 
> On Fri, Jan 13, 2012 at 12:29:13AM +0100, Guennadi Liakhovetski wrote:
> > Hi
> > 
> > I haven't seen this patch going into any tree or being included in any 
> > pull request. What's the status?
> > 
> > Thanks
> > Guennadi
> > 
> > On Fri, 6 Jan 2012, Guennadi Liakhovetski wrote:
> > 
> > > Hi Julian
> > > 
> > > On Sat, 7 Jan 2012, Julian Calaby wrote:
> > > 
> > > > Hi Guennadi,
> > > > 
> > > > On Fri, Jan 6, 2012 at 22:58, Guennadi Liakhovetski
> > > > <g.liakhovetski@gmx.de> wrote:
> > > > > An Oops has once been observed, when the SDIO card had been ejected during
> > > > > IO. The PC value shows, that the dev pointer in b43_op_stop() was NULL.
> > > > >
> > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > > ---
> > > > >
> > > > > Might also be good for stable
> > > > >
> > > > >  drivers/net/wireless/b43/main.c |    3 +++
> > > > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> > > > > index 5634d9a..3849c25 100644
> > > > > --- a/drivers/net/wireless/b43/main.c
> > > > > +++ b/drivers/net/wireless/b43/main.c
> > > > > @@ -4834,6 +4834,9 @@ static void b43_op_stop(struct ieee80211_hw *hw)
> > > > >        cancel_work_sync(&(wl->beacon_update_trigger));
> > > > >
> > > > >        mutex_lock(&wl->mutex);
> > > > > +       if (!dev)
> > > > > +               goto out_unlock;
> > > > 
> > > > Is there any reason to take this mutex, then jump to a label that
> > > > releases it immediately?
> > > > 
> > > > Would it be better to add a new label after the mutex_unlock() below
> > > > and jump to there immediately after the cancel_work_sync() call above?
> > > 
> > > The reason is exactly that: to avoid adding one more label. It is an 
> > > exceptional case, that that branch would be taken, so, there's no reason 
> > > to optimize for it. Whereas adding one label would clutter the code 
> > > needlessly, IMHO.
> > > 
> > > Thanks
> > > Guennadi
> > > ---
> > > Guennadi Liakhovetski, Ph.D.
> > > Freelance Open-Source Software Developer
> > > http://www.open-technology.de/
> > > 
> > 
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> -- 
> John W. Linville		Someday the world will need a hero, and you
> linville@tuxdriver.com			might be all we have.  Be ready.
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] wireless: b43: fix Oops on card eject during transfer

[Julian Calaby]

On Sat, Jan 14, 2012 at 02:26, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi John
>
> On Fri, 13 Jan 2012, John W. Linville wrote:
>
>> Well, you weren't interested in making the minor change suggested
>> by Julian.  So I set it aside with the intent of making the change
>> myself, then neglected to follow through. :-(  I'll try to get to
>> that today.  Or, feel free to submit a new version yourself.
>
> I didn't neglect, I explained, why I considered his suggestion not
> necessarily an improvement. If there were several votes for that change,
> or if it were a maintainer, who requested the change - I would consider
> obeying, but this time it was just an opinion of one developer against
> that of another.

I was going to respond by explaining that taking a mutex is an
expensive operation, adding a label is cheap, both in terms of code
complexity and code run-time.

I personally see no change in code complexity by adding a label to
this function. What does concern me is when actions are performed that
perform work for no real benefit - IMHO this increases the code
complexity: when reading it, one will ask one's self "why?" and then
have to search for why someone has done something un-obvious like
this.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/