Date: Sat, 13 Jan 2007 12:29:40 -0700 (MST) From: "M. Warner Losh" <imp@bsdimp.com> To: hselasky@freebsd.org Cc: perforce@freebsd.org Subject: Re: PERFORCE change 112835 for review Message-ID: <20070113.122940.420518338.imp@bsdimp.com> In-Reply-To: <200701122132.l0CLWuAG096027@repoman.freebsd.org> References: <200701122132.l0CLWuAG096027@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <200701122132.l0CLWuAG096027@repoman.freebsd.org>
Hans Petter Selasky <hselasky@freebsd.org> writes:
: http://perforce.freebsd.org/chv.cgi?CH=112835
:
: Change 112835 by hselasky@hselasky_mini_itx on 2007/01/12 21:32:24
:
: ether_ifattach() must be called without any mutexes held, hence
: it can sleep. Use generated typedef's for prototypes for exported
: device methods. This prevents invalid parameter passing and return.
Why do the attach methods even have locked mutexes to drop? That was
a common error in the early locking efforts that too many network
drivers inherited. Maybe the right fix is to just remove the
lock/unlock pairs in attach entirely...
: Affected files ...
:
: .. //depot/projects/usb/src/sys/dev/usb/if_aue.c#14 edit
: .. //depot/projects/usb/src/sys/dev/usb/if_axe.c#13 edit
: .. //depot/projects/usb/src/sys/dev/usb/if_cdce.c#9 edit
: .. //depot/projects/usb/src/sys/dev/usb/if_cue.c#11 edit
: .. //depot/projects/usb/src/sys/dev/usb/if_kue.c#13 edit
: .. //depot/projects/usb/src/sys/dev/usb/if_rue.c#12 edit
: .. //depot/projects/usb/src/sys/dev/usb/if_udav.c#12 edit
:
: Differences ...
:
: ==== //depot/projects/usb/src/sys/dev/usb/if_aue.c#14 (text+ko) ====
:
: @@ -215,14 +215,10 @@
: static void
: aue_cfg_read_eeprom(struct aue_softc *sc, u_int8_t *dest,
: u_int16_t off, u_int16_t len);
: -static int
: -aue_cfg_miibus_readreg(device_t dev, int phy, int reg);
:
: -static int
: -aue_cfg_miibus_writereg(device_t dev, int phy, int reg, int data);
: -
: -static void
: -aue_cfg_miibus_statchg(device_t dev);
: +static miibus_readreg_t aue_cfg_miibus_readreg;
: +static miibus_writereg_t aue_cfg_miibus_writereg;
: +static miibus_statchg_t aue_cfg_miibus_statchg;
:
: static void
: aue_cfg_setmulti(struct aue_softc *sc,
: @@ -957,11 +953,15 @@
:
: sc->sc_ifp = ifp;
:
: + mtx_unlock(&(sc->sc_mtx));
: +
: /*
: * Call MI attach routine.
: */
: ether_ifattach(ifp, eaddr);
:
: + mtx_lock(&(sc->sc_mtx));
: +
: done:
: return;
: }
: @@ -1530,7 +1530,6 @@
: ifmr->ifm_status = sc->sc_media_status;
:
: mtx_unlock(&(sc->sc_mtx));
: -
: return;
: }
:
: @@ -1681,3 +1680,4 @@
:
: return 0;
: }
: +
:
: ==== //depot/projects/usb/src/sys/dev/usb/if_axe.c#13 (text+ko) ====
:
: @@ -149,15 +149,11 @@
: static void
: axe_cfg_cmd(struct axe_softc *sc, u_int16_t cmd, u_int16_t index,
: u_int16_t val, void *buf);
: -static int
: -axe_cfg_miibus_readreg(device_t dev, int phy, int reg);
:
: -static int
: -axe_cfg_miibus_writereg(device_t dev, int phy, int reg, int val);
: +static miibus_readreg_t axe_cfg_miibus_readreg;
: +static miibus_writereg_t axe_cfg_miibus_writereg;
: +static miibus_statchg_t axe_cfg_miibus_statchg;
:
: -static void
: -axe_cfg_miibus_statchg(device_t dev);
: -
: static int
: axe_ifmedia_upd_cb(struct ifnet *ifp);
:
: @@ -757,12 +753,16 @@
:
: sc->sc_ifp = ifp;
:
: + mtx_unlock(&(sc->sc_mtx));
: +
: /*
: * Call MI attach routine.
: */
:
: ether_ifattach(ifp, eaddr);
:
: + mtx_lock(&(sc->sc_mtx));
: +
: done:
: return;
: }
:
: ==== //depot/projects/usb/src/sys/dev/usb/if_cdce.c#9 (text+ko) ====
:
: @@ -396,8 +396,12 @@
:
: sc->sc_ifp = ifp;
:
: + mtx_unlock(&(sc->sc_mtx));
: +
: ether_ifattach(ifp, eaddr);
:
: + mtx_lock(&(sc->sc_mtx));
: +
: return 0; /* success */
:
: detach:
:
: ==== //depot/projects/usb/src/sys/dev/usb/if_cue.c#11 (text+ko) ====
:
: @@ -595,7 +595,12 @@
:
: sc->sc_ifp = ifp;
:
: + mtx_unlock(&(sc->sc_mtx));
: +
: ether_ifattach(ifp, eaddr);
: +
: + mtx_lock(&(sc->sc_mtx));
: +
: done:
: return;
: }
:
: ==== //depot/projects/usb/src/sys/dev/usb/if_kue.c#13 (text+ko) ====
:
: @@ -630,7 +630,11 @@
:
: sc->sc_ifp = ifp;
:
: + mtx_unlock(&(sc->sc_mtx));
: +
: ether_ifattach(ifp, sc->sc_desc.kue_macaddr);
: +
: + mtx_lock(&(sc->sc_mtx));
: done:
: return;
: }
:
: ==== //depot/projects/usb/src/sys/dev/usb/if_rue.c#12 (text+ko) ====
:
: @@ -162,16 +162,11 @@
: static void
: rue_cfg_csr_write_4(struct rue_softc *sc, int reg, u_int32_t val);
:
: -static int
: -rue_cfg_miibus_readreg(device_t dev, int phy, int reg);
: -
: -static int
: -rue_cfg_miibus_writereg(device_t dev, int phy, int reg, int data);
: +static miibus_readreg_t rue_cfg_miibus_readreg;
: +static miibus_writereg_t rue_cfg_miibus_writereg;
: +static miibus_statchg_t rue_cfg_miibus_statchg;
:
: static void
: -rue_cfg_miibus_statchg(device_t dev);
: -
: -static void
: rue_config_copy(struct rue_softc *sc,
: struct rue_config_copy *cc, u_int16_t refcount);
: static void
: @@ -857,12 +852,16 @@
:
: sc->sc_ifp = ifp;
:
: + mtx_unlock(&(sc->sc_mtx));
: +
: /*
: * Call MI attach routine.
: */
:
: ether_ifattach(ifp, eaddr);
:
: + mtx_lock(&(sc->sc_mtx));
: +
: done:
: return;
: }
:
: ==== //depot/projects/usb/src/sys/dev/usb/if_udav.c#12 (text+ko) ====
:
: @@ -174,15 +174,10 @@
: static void
: udav_cfg_tick(struct udav_softc *sc,
: struct udav_config_copy *cc, u_int16_t refcount);
: -static int
: -udav_cfg_miibus_readreg(device_t dev, int phy, int reg);
:
: -static void
: -udav_cfg_miibus_writereg(device_t dev, int phy, int reg, int data);
: -
: -static void
: -udav_cfg_miibus_statchg(device_t dev);
: -
: +static miibus_readreg_t udav_cfg_miibus_readreg;
: +static miibus_writereg_t udav_cfg_miibus_writereg;
: +static miibus_statchg_t udav_cfg_miibus_statchg;
:
: static const struct usbd_config udav_config[UDAV_ENDPT_MAX] = {
:
: @@ -464,12 +459,16 @@
:
: sc->sc_ifp = ifp;
:
: + mtx_unlock(&(sc->sc_mtx));
: +
: /*
: * Call MI attach routine.
: */
:
: ether_ifattach(ifp, eaddr);
:
: + mtx_lock(&(sc->sc_mtx));
: +
: done:
: return;
: }
: @@ -1430,7 +1429,7 @@
: return data16;
: }
:
: -static void
: +static int
: udav_cfg_miibus_writereg(device_t dev, int phy, int reg, int data)
: {
: struct udav_softc *sc = device_get_softc(dev);
: @@ -1438,7 +1437,7 @@
:
: /* XXX: one PHY only for the internal PHY */
: if (phy != 0) {
: - return;
: + return 0;
: }
:
: mtx_lock(&(sc->sc_mtx)); /* XXX */
: @@ -1462,7 +1461,7 @@
:
: mtx_unlock(&(sc->sc_mtx)); /* XXX */
:
: - return;
: + return 0;
: }
:
: static void
:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070113.122940.420518338.imp>