Re: athn@usb: fixup detach panic
On Sat, Nov 10, 2012 at 03:52:37AM +0100, Mike Belopuhov wrote:
> On Tue, Nov 06, 2012 at 12:15 +0100, Stefan Sperling wrote:
> > It seems more appropriate to handle this problem entirely within
> > the USB-specific layer of the driver, rather than within dev/ic/athn.c.
> >
> > If loading the firmware fails, athn_usb_attachhook() doesn't even
> > call athn_attach(). This is what your fix is compensating for. I believe
> > athn_usb_detach() shouldn't be calling athn_detach() unconditionally.
> >
> > I fixed a similar problem for acx(4) cardbus cards once, where ejecting
> > a card that had failed to attach half way through paniced the kernel.
> > This was in r1.12 of dev/cardbus/if_acx_cardbus.c, which looked like this:
> >
>
> works for me. ok?
Yes!
> Index: dev/usb/if_athn_usb.c
> ===
> RCS file: /home/cvs/src/sys/dev/usb/if_athn_usb.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 if_athn_usb.c
> --- dev/usb/if_athn_usb.c 3 Jul 2011 15:47:17 - 1.8
> +++ dev/usb/if_athn_usb.c 10 Nov 2012 02:04:12 -
> @@ -289,7 +289,8 @@ athn_usb_detach(struct device *self, int
> struct athn_usb_softc *usc = (struct athn_usb_softc *)self;
> struct athn_softc *sc = &usc->sc_sc;
>
> - athn_detach(sc);
> + if (usc->sc_athn_attached)
> + athn_detach(sc);
>
> /* Wait for all async commands to complete. */
> athn_usb_wait_async(usc);
> @@ -347,6 +348,7 @@ athn_usb_attachhook(void *xsc)
> splx(s);
> return;
> }
> + usc->sc_athn_attached = 1;
> /* Override some operations for USB. */
> ifp->if_ioctl = athn_usb_ioctl;
> ifp->if_start = athn_usb_start;
> Index: dev/usb/if_athn_usb.h
> ===
> RCS file: /home/cvs/src/sys/dev/usb/if_athn_usb.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 if_athn_usb.h
> --- dev/usb/if_athn_usb.h 8 Jan 2011 15:18:01 - 1.2
> +++ dev/usb/if_athn_usb.h 10 Nov 2012 02:04:33 -
> @@ -417,6 +417,7 @@ struct athn_usb_host_cmd_ring {
> struct athn_usb_softc {
> struct athn_softc sc_sc;
> #define usb_dev sc_sc.sc_dev
> + int sc_athn_attached;
>
> /* USB specific goo. */
> usbd_device_handle sc_udev;
Re: athn@usb: fixup detach panic
On Tue, Nov 06, 2012 at 12:15 +0100, Stefan Sperling wrote:
> On Tue, Nov 06, 2012 at 11:32:19AM +0100, Mike Belopuhov wrote:
> > attach fails early in case there's no firmware, but
> > athn_detach does ieee80211_ifdetach and if_detach
> > regardless of whether ifnet part got setup correctly
> > leading to a free of an unallocated memory and a
> > panic.
> >
> > the following diff follows an established practice
> > in the other drivers and fixes the problem here.
> > tested on this device:
> >
> > athn0 at uhub0 port 3 "NETGEAR WNA WNA1100" rev 2.00/1.08 addr 2
> > athn0: AR9271 rev 1 (1T1R), ROM rev 15, address 20:4e:7f:d9:65:2f
> >
> > ok?
>
> It seems more appropriate to handle this problem entirely within
> the USB-specific layer of the driver, rather than within dev/ic/athn.c.
>
> If loading the firmware fails, athn_usb_attachhook() doesn't even
> call athn_attach(). This is what your fix is compensating for. I believe
> athn_usb_detach() shouldn't be calling athn_detach() unconditionally.
>
> I fixed a similar problem for acx(4) cardbus cards once, where ejecting
> a card that had failed to attach half way through paniced the kernel.
> This was in r1.12 of dev/cardbus/if_acx_cardbus.c, which looked like this:
>
works for me. ok?
Index: dev/usb/if_athn_usb.c
===
RCS file: /home/cvs/src/sys/dev/usb/if_athn_usb.c,v
retrieving revision 1.8
diff -u -p -r1.8 if_athn_usb.c
--- dev/usb/if_athn_usb.c 3 Jul 2011 15:47:17 - 1.8
+++ dev/usb/if_athn_usb.c 10 Nov 2012 02:04:12 -
@@ -289,7 +289,8 @@ athn_usb_detach(struct device *self, int
struct athn_usb_softc *usc = (struct athn_usb_softc *)self;
struct athn_softc *sc = &usc->sc_sc;
- athn_detach(sc);
+ if (usc->sc_athn_attached)
+ athn_detach(sc);
/* Wait for all async commands to complete. */
athn_usb_wait_async(usc);
@@ -347,6 +348,7 @@ athn_usb_attachhook(void *xsc)
splx(s);
return;
}
+ usc->sc_athn_attached = 1;
/* Override some operations for USB. */
ifp->if_ioctl = athn_usb_ioctl;
ifp->if_start = athn_usb_start;
Index: dev/usb/if_athn_usb.h
===
RCS file: /home/cvs/src/sys/dev/usb/if_athn_usb.h,v
retrieving revision 1.2
diff -u -p -r1.2 if_athn_usb.h
--- dev/usb/if_athn_usb.h 8 Jan 2011 15:18:01 - 1.2
+++ dev/usb/if_athn_usb.h 10 Nov 2012 02:04:33 -
@@ -417,6 +417,7 @@ struct athn_usb_host_cmd_ring {
struct athn_usb_softc {
struct athn_softc sc_sc;
#define usb_devsc_sc.sc_dev
+ int sc_athn_attached;
/* USB specific goo. */
usbd_device_handle sc_udev;
Re: athn@usb: fixup detach panic
On Tue, Nov 06, 2012 at 11:32:19AM +0100, Mike Belopuhov wrote:
> attach fails early in case there's no firmware, but
> athn_detach does ieee80211_ifdetach and if_detach
> regardless of whether ifnet part got setup correctly
> leading to a free of an unallocated memory and a
> panic.
>
> the following diff follows an established practice
> in the other drivers and fixes the problem here.
> tested on this device:
>
> athn0 at uhub0 port 3 "NETGEAR WNA WNA1100" rev 2.00/1.08 addr 2
> athn0: AR9271 rev 1 (1T1R), ROM rev 15, address 20:4e:7f:d9:65:2f
>
> ok?
It seems more appropriate to handle this problem entirely within
the USB-specific layer of the driver, rather than within dev/ic/athn.c.
If loading the firmware fails, athn_usb_attachhook() doesn't even
call athn_attach(). This is what your fix is compensating for. I believe
athn_usb_detach() shouldn't be calling athn_detach() unconditionally.
I fixed a similar problem for acx(4) cardbus cards once, where ejecting
a card that had failed to attach half way through paniced the kernel.
This was in r1.12 of dev/cardbus/if_acx_cardbus.c, which looked like this:
Index: if_acx_cardbus.c
===
RCS file: /cvs/src/sys/dev/cardbus/if_acx_cardbus.c,v
retrieving revision 1.11
retrieving revision 1.12
diff -u -p -r1.11 -r1.12
--- if_acx_cardbus.c10 Nov 2006 20:20:04 - 1.11
+++ if_acx_cardbus.c26 Feb 2009 23:03:05 - 1.12
@@ -1,4 +1,4 @@
-/* $OpenBSD: if_acx_cardbus.c,v 1.11 2006/11/10 20:20:04 damien Exp $ */
+/* $OpenBSD: if_acx_cardbus.c,v 1.12 2009/02/26 23:03:05 stsp Exp $ */
/*
* Copyright (c) 2006 Claudio Jeker
@@ -77,6 +77,8 @@ struct acx_cardbus_softc {
bus_space_tag_t sc_io_bt;
bus_space_handle_t sc_io_bh;
bus_size_t sc_iomapsize;
+
+ int sc_acx_attached;
};
intacx_cardbus_match(struct device *, void *, void *);
@@ -171,7 +173,8 @@ acx_cardbus_attach(struct device *parent
else
acx100_set_param(sc);
- acx_attach(sc);
+ error = acx_attach(sc);
+ csc->sc_acx_attached = error == 0;
Cardbus_function_disable(ct);
}
@@ -186,9 +189,11 @@ acx_cardbus_detach(struct device *self,
cardbus_function_tag_t cf = ct->ct_cf;
int error, b1 = CARDBUS_BASE0_REG, b2 = CARDBUS_BASE1_REG;
- error = acx_detach(sc);
- if (error != 0)
- return (error);
+ if (csc->sc_acx_attached) {
+ error = acx_detach(sc);
+ if (error != 0)
+ return (error);
+ }
/* unhook the interrupt handler */
if (csc->sc_ih != NULL) {
>
> Index: athn.c
> ===
> RCS file: /home/cvs/src/sys/dev/ic/athn.c,v
> retrieving revision 1.74
> diff -u -p -u -p -r1.74 athn.c
> --- athn.c20 Oct 2012 09:54:20 - 1.74
> +++ athn.c1 Nov 2012 16:33:53 -
> @@ -396,8 +396,10 @@ athn_detach(struct athn_softc *sc)
> if (sc->eep != NULL)
> free(sc->eep, M_DEVBUF);
>
> - ieee80211_ifdetach(ifp);
> - if_detach(ifp);
> + if (ifp->if_softc != NULL) {
> + ieee80211_ifdetach(ifp);
> + if_detach(ifp);
> + }
> }
>
> #if NBPFILTER > 0
athn@usb: fixup detach panic
attach fails early in case there's no firmware, but
athn_detach does ieee80211_ifdetach and if_detach
regardless of whether ifnet part got setup correctly
leading to a free of an unallocated memory and a
panic.
the following diff follows an established practice
in the other drivers and fixes the problem here.
tested on this device:
athn0 at uhub0 port 3 "NETGEAR WNA WNA1100" rev 2.00/1.08 addr 2
athn0: AR9271 rev 1 (1T1R), ROM rev 15, address 20:4e:7f:d9:65:2f
ok?
Index: athn.c
===
RCS file: /home/cvs/src/sys/dev/ic/athn.c,v
retrieving revision 1.74
diff -u -p -u -p -r1.74 athn.c
--- athn.c 20 Oct 2012 09:54:20 - 1.74
+++ athn.c 1 Nov 2012 16:33:53 -
@@ -396,8 +396,10 @@ athn_detach(struct athn_softc *sc)
if (sc->eep != NULL)
free(sc->eep, M_DEVBUF);
- ieee80211_ifdetach(ifp);
- if_detach(ifp);
+ if (ifp->if_softc != NULL) {
+ ieee80211_ifdetach(ifp);
+ if_detach(ifp);
+ }
}
#if NBPFILTER > 0
