Date: Thu, 19 Nov 2009 14:19:17 -0700 (MST) From: "M. Warner Losh" <imp@bsdimp.com> To: jhb@FreeBSD.org Cc: arm@FreeBSD.org Subject: Re: [PATCH] Fix a few nits in ate(4) Message-ID: <20091119.141917.-1843205663.imp@bsdimp.com> In-Reply-To: <200911191122.02975.jhb@freebsd.org> References: <200911191122.02975.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <200911191122.02975.jhb@freebsd.org>
John Baldwin <jhb@FreeBSD.org> writes:
: I ran into this while working on purging if_watchdog/if_timer use from the
: tree. This patch fixes a few things in ate(4):
:
: - Initialize callout before it is used in atestop() during attach.
: - Fixup detach.
:
: By fixing detach, I mean that it calls ether_ifdetach() to remove outside
: references to the device before shutting down the device. Please test.
OK. I thought the other order was right. See below for one or two
concerns..
Warner
: --- //depot/vendor/freebsd/src/sys/arm/at91/if_ate.c 2009/06/26 11:50:17
: +++ //depot/user/jhb/cleanup/sys/arm/at91/if_ate.c 2009/11/17 18:08:42
: @@ -75,8 +75,7 @@
: /*
: * Driver-specific flags.
: */
: -#define ATE_FLAG_DETACHING 0x01
: -#define ATE_FLAG_MULTICAST 0x02
: +#define ATE_FLAG_MULTICAST 0x01
:
: struct ate_softc
: {
: @@ -196,6 +195,7 @@
: sc = device_get_softc(dev);
: sc->dev = dev;
: ATE_LOCK_INIT(sc);
: + callout_init_mtx(&sc->tick_ch, &sc->sc_mtx, 0);
:
: /*
: * Allocate resources.
: @@ -233,7 +233,6 @@
: ATE_LOCK(sc);
: atestop(sc);
: ATE_UNLOCK(sc);
: - callout_init_mtx(&sc->tick_ch, &sc->sc_mtx, 0);
:
: if ((err = ate_get_mac(sc, eaddr)) != 0) {
: /*
: @@ -311,12 +309,11 @@
: KASSERT(sc != NULL, ("[ate: %d]: sc is NULL", __LINE__));
: ifp = sc->ifp;
: if (device_is_attached(dev)) {
: + ether_ifdetach(ifp);
: ATE_LOCK(sc);
: - sc->flags |= ATE_FLAG_DETACHING;
: - atestop(sc);
: + atestop(sc);
: ATE_UNLOCK(sc);
: callout_drain(&sc->tick_ch);
: - ether_ifdetach(ifp);
: }
: if (sc->miibus != NULL) {
: device_delete_child(dev, sc->miibus);
: @@ -1109,11 +1105,9 @@
: & (IFF_PROMISC | IFF_ALLMULTI)) != 0)
: ate_rxfilter(sc);
: } else {
: - if ((sc->flags & ATE_FLAG_DETACHING) == 0)
: - ateinit_locked(sc);
: + ateinit_locked(sc);
Here we reinitialize the device just before we detach it. I put the
detaching stuff in to prevent that. With this change, are you saying
this routine won't be called, so we don't need this check anymore?
: }
: } else if ((drv_flags & IFF_DRV_RUNNING) != 0) {
: - ifp->if_drv_flags &= ~IFF_DRV_RUNNING;
: atestop(sc);
: }
: sc->if_flags = flags;
Why remove this line? I think it is kinda needed. I don't understand.
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20091119.141917.-1843205663.imp>