On hardware busy the scan request pointer should be cleared, as higher levels will release. This avoids a crash when that pointer is erroneously used later.
Signed-off-by: Joseph J. Gunn <[email protected]>
---
When the hardware is busy the error is propagated to higher levels on the stack. Those layers release the buffer. Therefore the copy of the pointer must be erased. Otherwise subsequent events checking this pointer ma crash.
---
diff --git a/drivers/net/wireless/orinoco/cfg.c b/drivers/net/wireless/orinoco/cfg.c
index 09fae2f..2022815 100644
--- a/drivers/net/wireless/orinoco/cfg.c
+++ b/drivers/net/wireless/orinoco/cfg.c
@@ -151,8 +151,17 @@ static int orinoco_scan(struct wiphy *wiphy, struct net_device *dev,
return -EBUSY;
priv->scan_request = request;
+ DEBUG(3, "orinoco_scan():"
+ " scan_request %p wiphy %p, dev %p\n",
+ priv->scan_request,
+ priv->scan_request->wiphy,
+ priv->scan_request->dev
+ );
err = orinoco_hw_trigger_scan(priv, request->ssids);
+ /* On EBUSY the hardware is busy. We aren't processing the request */
+ if (err == -EBUSY)
+ priv->scan_request = NULL;
return err;
}
diff --git a/drivers/net/wireless/orinoco/scan.c b/drivers/net/wireless/orinoco/scan.c
index e99ca1c..698e9ff 100644
--- a/drivers/net/wireless/orinoco/scan.c
+++ b/drivers/net/wireless/orinoco/scan.c
@@ -230,6 +230,12 @@ void orinoco_add_hostscan_results(struct orinoco_private *priv,
scan_abort:
if (priv->scan_request) {
+ DEBUG(3, "orinoco_add_hostscan_results():"
+ " scan_request %p wiphy %p, dev %p\n",
+ priv->scan_request,
+ priv->scan_request->wiphy,
+ priv->scan_request->dev
+ );
cfg80211_scan_done(priv->scan_request, abort);
priv->scan_request = NULL;
}
@@ -238,6 +244,12 @@ void orinoco_add_hostscan_results(struct orinoco_private *priv,
void orinoco_scan_done(struct orinoco_private *priv, bool abort)
{
if (priv->scan_request) {
+ DEBUG(3, "orinoco_scan_done():"
+ " scan_request %p, wiphy %p, dev %p\n",
+ priv->scan_request,
+ priv->scan_request->wiphy,
+ priv->scan_request->dev
+ );
cfg80211_scan_done(priv->scan_request, abort);
priv->scan_request = NULL;
}
Thanks for your response(s).
--- On Mon, 3/21/11, Dave Kilroy <[email protected]> wrote:
> From: Dave Kilroy <[email protected]>
> Subject: Re: [Orinoco-devel] [PATCH 2.6.38-rc8-wl RESEND] orinoco: Clear dangling pointer on hardware busy
> To: [email protected]
> Cc: [email protected], [email protected]
> Date: Monday, March 21, 2011, 6:40 PM
> On 21/03/2011 08:21, [email protected]
> wrote:
> > On hardware busy the scan request pointer should be
> cleared, as higher levels will release. This avoids a crash
> when that pointer is erroneously used later.
>
> I think you need to add line breaks for the git log
>
Sorry i am new to this.
logs on git don't have a column length restriction. Is there a "80", or
so, column restriction suggested for this?
> > Signed-off-by: Joseph J. Gunn<[email protected]>
> > ---
> > When the hardware is busy the error is propagated to
> higher levels on the stack. Those layers release the buffer.
> Therefore the copy of the pointer must be erased. Otherwise
> subsequent events checking this pointer ma crash.
> > ---
> > diff --git a/drivers/net/wireless/orinoco/cfg.c
> b/drivers/net/wireless/orinoco/cfg.c
> > index 09fae2f..2022815 100644
> > --- a/drivers/net/wireless/orinoco/cfg.c
> > +++ b/drivers/net/wireless/orinoco/cfg.c
> > @@ -151,8 +151,17 @@ static int orinoco_scan(struct
> wiphy *wiphy, struct net_device *dev,
> >?????? ???
> return -EBUSY;
> >
> >??????
> priv->scan_request = request;
> > +??? DEBUG(3, "orinoco_scan():"
> > +??? ??? " scan_request
> %p wiphy %p, dev %p\n",
> > +??? ???
> priv->scan_request,
> > +??? ???
> priv->scan_request->wiphy,
> > +??? ???
> priv->scan_request->dev
> > +??? ??? );
> >
> >?????? err =
> orinoco_hw_trigger_scan(priv, request->ssids);
> > +??? /* On EBUSY the hardware is busy.
> We aren't processing the request */
> > +??? if (err == -EBUSY)
>
> We should reset priv->scan_request on all errors, not
> just -EBUSY. Were
> you getting this in a particular situation? If so,
> highlighting it in
> the commit log is useful.
>
> I notice -EBUSY is returned when we can't get the
> orinoco_lock - are you
> having an issue with lock cotention on a particular
> device?
The calls for orinoco_lock seem to be well placed from a software point.
There are other cases where the low level hardware routines return EBUSY. When the hardware can not accept the command. This is the situation i can
reproduce.
It is possible, probable even, that all cases of error do not propagate
the scan request. I don't know the driver that well yet.
I chose to limit the effect of the patch to the case i could prove.
>
> > +??? ???
> priv->scan_request = NULL;
> >
> >?????? return err;
> >???}
> > diff --git a/drivers/net/wireless/orinoco/scan.c
> b/drivers/net/wireless/orinoco/scan.c
> > index e99ca1c..698e9ff 100644
> > --- a/drivers/net/wireless/orinoco/scan.c
> > +++ b/drivers/net/wireless/orinoco/scan.c
> > @@ -230,6 +230,12 @@ void
> orinoco_add_hostscan_results(struct orinoco_private *priv,
> >
> >? ? scan_abort:
> >?????? if
> (priv->scan_request) {
> > +??? ??? DEBUG(3,
> "orinoco_add_hostscan_results():"
> > +??? ???
> ??? " scan_request %p wiphy %p, dev %p\n",
> > +??? ???
> ??? priv->scan_request,
> > +??? ???
> ??? priv->scan_request->wiphy,
> > +??? ???
> ??? priv->scan_request->dev
> > +??? ???
> ??? );
>
> I'm not a big fan of scattering DEBUG statements about, but
> if we're
> going to, use __FUNCTION__ (or whatever the C99 incarnation
> is) rather
> than explicitly naming the functions.
>
As per the above the patch only fixes one possible scenario. I left the
messages there because they could be used to help prove/disprove that another set of events could produce a similar error.
It appears that some recent changes in user space code make it more likely
for the driver to hit this condition.
>
> Dave.
>
>
Thanks
Joe
On 21/03/2011 08:21, [email protected] wrote:
> On hardware busy the scan request pointer should be cleared, as higher levels will release. This avoids a crash when that pointer is erroneously used later.
I think you need to add line breaks for the git log
> Signed-off-by: Joseph J. Gunn<[email protected]>
> ---
> When the hardware is busy the error is propagated to higher levels on the stack. Those layers release the buffer. Therefore the copy of the pointer must be erased. Otherwise subsequent events checking this pointer ma crash.
> ---
> diff --git a/drivers/net/wireless/orinoco/cfg.c b/drivers/net/wireless/orinoco/cfg.c
> index 09fae2f..2022815 100644
> --- a/drivers/net/wireless/orinoco/cfg.c
> +++ b/drivers/net/wireless/orinoco/cfg.c
> @@ -151,8 +151,17 @@ static int orinoco_scan(struct wiphy *wiphy, struct net_device *dev,
> return -EBUSY;
>
> priv->scan_request = request;
> + DEBUG(3, "orinoco_scan():"
> + " scan_request %p wiphy %p, dev %p\n",
> + priv->scan_request,
> + priv->scan_request->wiphy,
> + priv->scan_request->dev
> + );
>
> err = orinoco_hw_trigger_scan(priv, request->ssids);
> + /* On EBUSY the hardware is busy. We aren't processing the request */
> + if (err == -EBUSY)
We should reset priv->scan_request on all errors, not just -EBUSY. Were
you getting this in a particular situation? If so, highlighting it in
the commit log is useful.
I notice -EBUSY is returned when we can't get the orinoco_lock - are you
having an issue with lock cotention on a particular device?
> + priv->scan_request = NULL;
>
> return err;
> }
> diff --git a/drivers/net/wireless/orinoco/scan.c b/drivers/net/wireless/orinoco/scan.c
> index e99ca1c..698e9ff 100644
> --- a/drivers/net/wireless/orinoco/scan.c
> +++ b/drivers/net/wireless/orinoco/scan.c
> @@ -230,6 +230,12 @@ void orinoco_add_hostscan_results(struct orinoco_private *priv,
>
> scan_abort:
> if (priv->scan_request) {
> + DEBUG(3, "orinoco_add_hostscan_results():"
> + " scan_request %p wiphy %p, dev %p\n",
> + priv->scan_request,
> + priv->scan_request->wiphy,
> + priv->scan_request->dev
> + );
I'm not a big fan of scattering DEBUG statements about, but if we're
going to, use __FUNCTION__ (or whatever the C99 incarnation is) rather
than explicitly naming the functions.
Dave.
Joe Gunn <[email protected]> writes:
>> I think you need to add line breaks for the git log
>
> Sorry i am new to this. logs on git don't have a column length
> restriction. Is there a "80", or so, column restriction suggested
> for this?
I think following an old rule from usenet, max 72 chars per line, is
still valid. At least replies still look nice.
--
Kalle Valo