In message: <[EMAIL PROTECTED]>
"Bjoern A. Zeeb" <[EMAIL PROTECTED]> writes:
: * all PHY drivers currently seem to use mii_phy_detach for
: device_detach. If any implements his own function it will be
: responsible for freeing the ivars allocated in miibus_probe. This
: should perhaps be documented somewhere ?
I think that the current patches are incorrect from a newbus point of
view. They may solve the problem, but just smell wrong...
:
: patch can also be found at
: http://sources.zabbadoz.net/freebsd/patchset/mii-memleaks.diff
:
:
: Index: mii.c
: ===================================================================
: RCS file: /local/mirror/FreeBSD/r/ncvs/src/sys/dev/mii/mii.c,v
: retrieving revision 1.20
: diff -u -p -r1.20 mii.c
: --- mii.c 15 Aug 2004 06:24:40 -0000 1.20
: +++ mii.c 23 Nov 2004 17:08:58 -0000
: @@ -111,7 +111,7 @@ miibus_probe(dev)
: struct mii_attach_args ma, *args;
: struct mii_data *mii;
: device_t child = NULL, parent;
: - int bmsr, capmask = 0xFFFFFFFF;
: + int count = 0, bmsr, capmask = 0xFFFFFFFF;
:
: mii = device_get_softc(dev);
: parent = device_get_parent(dev);
: @@ -145,12 +145,26 @@ miibus_probe(dev)
:
: args = malloc(sizeof(struct mii_attach_args),
: M_DEVBUF, M_NOWAIT);
: + if (args == NULL) {
: + device_printf(dev, "%s: memory allocation failure, "
: + "phyno %d", __func__, ma.mii_phyno);
: + continue;
: + }
: bcopy((char *)&ma, (char *)args, sizeof(ma));
: child = device_add_child(dev, NULL, -1);
: + if (child == NULL) {
: + free(args, M_DEVBUF);
: + device_printf(dev, "%s: device_add_child failed",
: + __func__);
: + continue;
: + }
: device_set_ivars(child, args);
: + count++;
: + /* XXX should we break here or is it really possible
: + * to find more then one PHY ? */
: }
:
: - if (child == NULL)
: + if (count == 0)
: return(ENXIO);
:
: device_set_desc(dev, "MII bus");
: @@ -173,12 +187,15 @@ miibus_attach(dev)
: */
: mii->mii_ifp = device_get_softc(device_get_parent(dev));
: v = device_get_ivars(dev);
: + if (v == NULL)
: + return (ENXIO);
: ifmedia_upd = v[0];
: ifmedia_sts = v[1];
: + device_set_ivars(dev, NULL);
: + free(v, M_DEVBUF);
: ifmedia_init(&mii->mii_media, IFM_IMASK, ifmedia_upd, ifmedia_sts);
: - bus_generic_attach(dev);
:
: - return(0);
: + return (bus_generic_attach(dev));
: }
newbusly, this is bogus. device foo should never set its own ivars.
Nor should it ever get its own ivars to do anything with. parent
accessor functions are needed here.
: int
: @@ -186,8 +203,14 @@ miibus_detach(dev)
: device_t dev;
: {
: struct mii_data *mii;
: + void *v;
:
: bus_generic_detach(dev);
: + v = device_get_ivars(dev);
: + if (v != NULL) {
: + device_set_ivars(dev, NULL);
: + free(v, M_DEVBUF);
: + }
: mii = device_get_softc(dev);
: ifmedia_removeall(&mii->mii_media);
: mii->mii_ifp = NULL;
Newbusly, this is incorrect. The parent bus should be freeing the
ivars, since it is the one that should have put the ivars there in the
first place.
: @@ -305,12 +328,15 @@ mii_phy_probe(dev, child, ifmedia_upd, i
: int bmsr, i;
:
: v = malloc(sizeof(vm_offset_t) * 2, M_DEVBUF, M_NOWAIT);
: - if (v == 0) {
: + if (v == NULL)
: return (ENOMEM);
: - }
: v[0] = ifmedia_upd;
: v[1] = ifmedia_sts;
: *child = device_add_child(dev, "miibus", -1);
: + if (*child == NULL) {
: + free(v, M_DEVBUF);
: + return (ENXIO);
: + }
: device_set_ivars(*child, v);
:
: for (i = 0; i < MII_NPHY; i++) {
This appears to be correct, because the ivars are set on the child
that's added.
: @@ -324,14 +350,22 @@ mii_phy_probe(dev, child, ifmedia_upd, i
: }
:
: if (i == MII_NPHY) {
: + device_set_ivars(dev, NULL);
: + free(v, M_DEVBUF);
: device_delete_child(dev, *child);
: *child = NULL;
: return(ENXIO);
: }
:
: - bus_generic_attach(dev);
: + i = bus_generic_attach(dev);
:
: - return(0);
: + v = device_get_ivars(*child);
: + if (v != NULL) {
: + device_set_ivars(*child, NULL);
: + free(v, M_DEVBUF);
: + }
: +
: + return (i);
: }
This appears to be correct, since it is the bus managing the child's
ivars.
: /*
: Index: mii_physubr.c
: ===================================================================
: RCS file: /local/mirror/FreeBSD/r/ncvs/src/sys/dev/mii/mii_physubr.c,v
: retrieving revision 1.21
: diff -u -p -r1.21 mii_physubr.c
: --- mii_physubr.c 29 May 2004 18:09:10 -0000 1.21
: +++ mii_physubr.c 23 Nov 2004 17:07:30 -0000
: @@ -522,7 +522,13 @@ int
: mii_phy_detach(device_t dev)
: {
: struct mii_softc *sc;
: + void *args;
:
: + args = device_get_ivars(dev);
: + if (args != NULL) {
: + device_set_ivars(dev, NULL);
: + free(args, M_DEVBUF);
: + }
: sc = device_get_softc(dev);
: mii_phy_down(sc);
: sc->mii_dev = NULL;
This looks bogus from a newbus perspective.
Warner
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
