mem leak in mii ? (fwd)

[M. Warner Losh]

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