| Message ID | E1ZcsNb-0001Bf-U5@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
From: Russell King <rmk+kernel@arm.linux.org.uk> Date: Fri, 18 Sep 2015 10:54:55 +0100 > Update the comment, and arrange for the only user of this function > to drop this refcount when disposing of a reference to it. mdio_mux is not the only user of of_mdio_find_bus(), DSA uses it as well. So if anything this commit message is inaccurate. I also wonder about this refcounting scheme. If you are going to drop the inner device reference, then we take the mdio bus returned from of_mdio_find_bus() what holds onto it and keeps it from disappearing on us? Don't we have to hold onto some reference count of some kind here?
On Mon, Sep 21, 2015 at 12:01:59PM -0700, David Miller wrote: > From: Russell King <rmk+kernel@arm.linux.org.uk> > Date: Fri, 18 Sep 2015 10:54:55 +0100 > > > Update the comment, and arrange for the only user of this function > > to drop this refcount when disposing of a reference to it. > > mdio_mux is not the only user of of_mdio_find_bus(), DSA uses it as > well. > > So if anything this commit message is inaccurate. Yes, I missed that as it wasn't under drivers/net. It doesn't change the validity of this patch, the existing code is wrong and I'm not introducing anything that makes the code any more wrong than it is. I'll fix the commit message, and I'll fix the DSA code too but in a separate patch. Thanks for pointing it out. > I also wonder about this refcounting scheme. It's the standard driver model refcounting rules that we've lived with for about a decade, ever since the driver model was introduced by Patrick Mochel. > If you are going to drop the inner device reference, then we take the > mdio bus returned from of_mdio_find_bus() what holds onto it and keeps > it from disappearing on us? > > Don't we have to hold onto some reference count of some kind here? In the case of the mdio mux code, I'm dropping the reference when either (a) we've encountered an error during initialisation and we're cleaning up, or (b) when the mdio mux code is being torn down after the mdiomux bus has been unregistered and freed. In both cases, we're done with the mdio bus that was returned from of_mdio_find_bus(). In case (a), the devres code will release the kmalloc'd memory when mdio_mux_gpio_probe() or mdio_mux_mmioreg_probe() propagates the error out of their probe() function. I'm not sure why you think anything is wrong here - maybe it's the odd code structure to the success path at the bottom of mdio_mux_init()?
From: Russell King - ARM Linux <linux@arm.linux.org.uk> Date: Mon, 21 Sep 2015 20:32:07 +0100 > In the case of the mdio mux code, I'm dropping the reference when > either (a) we've encountered an error during initialisation and > we're cleaning up, or (b) when the mdio mux code is being torn down > after the mdiomux bus has been unregistered and freed. In both > cases, we're done with the mdio bus that was returned from > of_mdio_find_bus(). > > In case (a), the devres code will release the kmalloc'd memory when > mdio_mux_gpio_probe() or mdio_mux_mmioreg_probe() propagates the error > out of their probe() function. > > I'm not sure why you think anything is wrong here - maybe it's the odd > code structure to the success path at the bottom of mdio_mux_init()? Ok I may have misread your change. I'll restudy it when you respin the series with the commit message fixed and the DSA change added. Thanks.
diff --git a/drivers/net/phy/mdio-mux.c b/drivers/net/phy/mdio-mux.c index 4d4d25efc1e1..280c7c311f72 100644 --- a/drivers/net/phy/mdio-mux.c +++ b/drivers/net/phy/mdio-mux.c @@ -113,18 +113,18 @@ int mdio_mux_init(struct device *dev, if (!parent_bus_node) return -ENODEV; - parent_bus = of_mdio_find_bus(parent_bus_node); - if (parent_bus == NULL) { - ret_val = -EPROBE_DEFER; - goto err_parent_bus; - } - pb = devm_kzalloc(dev, sizeof(*pb), GFP_KERNEL); if (pb == NULL) { ret_val = -ENOMEM; goto err_parent_bus; } + parent_bus = of_mdio_find_bus(parent_bus_node); + if (parent_bus == NULL) { + ret_val = -EPROBE_DEFER; + goto err_parent_bus; + } + pb->switch_data = data; pb->switch_fn = switch_fn; pb->current_child = -1; @@ -173,6 +173,10 @@ int mdio_mux_init(struct device *dev, dev_info(dev, "Version " DRV_VERSION "\n"); return 0; } + + /* balance the reference of_mdio_find_bus() took */ + put_device(&pb->mii_bus->dev); + err_parent_bus: of_node_put(parent_bus_node); return ret_val; @@ -189,6 +193,9 @@ void mdio_mux_uninit(void *mux_handle) mdiobus_free(cb->mii_bus); cb = cb->next; } + + /* balance the reference of_mdio_find_bus() in mdio_mux_init() took */ + put_device(&pb->mii_bus->dev); } EXPORT_SYMBOL_GPL(mdio_mux_uninit); diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 02a4615b65f8..67553e13bd36 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -167,7 +167,9 @@ static int of_mdio_bus_match(struct device *dev, const void *mdio_bus_np) * of_mdio_find_bus - Given an mii_bus node, find the mii_bus. * @mdio_bus_np: Pointer to the mii_bus. * - * Returns a pointer to the mii_bus, or NULL if none found. + * Returns a reference to the mii_bus, or NULL if none found. The + * embedded struct device will have its reference count incremented, + * and this must be put once the bus is finished with. * * Because the association of a device_node and mii_bus is made via * of_mdiobus_register(), the mii_bus cannot be found before it is
of_mdio_find_bus() leaks a struct device refcount, caused by using class_find_device() and not realising that the device reference has its refcount incremented: * Note, you will need to drop the reference with put_device() after use. ... while ((dev = class_dev_iter_next(&iter))) { if (match(dev, data)) { get_device(dev); break; } Update the comment, and arrange for the only user of this function to drop this refcount when disposing of a reference to it. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/net/phy/mdio-mux.c | 19 +++++++++++++------ drivers/net/phy/mdio_bus.c | 4 +++- 2 files changed, 16 insertions(+), 7 deletions(-)