Re: mem leak in mii ? (fwd)

From: M. Warner Losh (imp_at_bsdimp.com)
Date: 01/22/05

  • Next message: Brooks Davis: "Re: [PATCH] 802.1p priority (fixed)" 
    Date: Sat, 22 Jan 2005 10:57:19 -0700 (MST)
To: bzeeb-lists@lists.zabbadoz.net

    In message: <Pine.BSF.4.53.0501220938580.1337@e0-0.zab2.int.zabbadoz.net>
                "Bjoern A. Zeeb" <bzeeb-lists@lists.zabbadoz.net> 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 "freebsd-net-unsubscribe@freebsd.org"

  • Next message: Brooks Davis: "Re: [PATCH] 802.1p priority (fixed)"