| Message ID | 20080929021400.GE21560@xi.wantstofly.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jeff Garzik |
| Headers | show |
On Sep 28, 2008, at 21:14, Lennert Buytenhek wrote: > Introduce the mdio_bus class, and give each 'struct mii_bus' its own > 'struct device', so that mii_bus objects are represented in the device > tree and can be found by querying the device tree. > > Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> > --- > drivers/net/phy/mdio_bus.c | 74 +++++++++++++++++++++++++++++++++++ > ++++++-- > include/linux/phy.h | 8 +++++ > 2 files changed, 78 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > index d206691..f9c27ac 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -43,11 +43,35 @@ > */ > struct mii_bus *mdiobus_alloc(void) > { > - return kzalloc(sizeof(struct mii_bus), GFP_KERNEL); > + struct mii_bus *bus; > + > + bus = kzalloc(sizeof(*bus), GFP_KERNEL); > + if (bus != NULL) > + bus->state = MDIOBUS_ALLOCATED; > + > + return bus; > } > EXPORT_SYMBOL(mdiobus_alloc); Do we really need all this state tracking? Doesn't the device layer do this for us? I looked through other driver subsystems, and I didn't see anything like this. Can we rearchitect this so that we're not having to track allocation/registration state? Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 29, 2008 at 05:45:01PM -0500, Andy Fleming wrote: > >Introduce the mdio_bus class, and give each 'struct mii_bus' its own > >'struct device', so that mii_bus objects are represented in the device > >tree and can be found by querying the device tree. > > > >Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> > >--- > >drivers/net/phy/mdio_bus.c | 74 +++++++++++++++++++++++++++++++++++ > >++++++-- > >include/linux/phy.h | 8 +++++ > >2 files changed, 78 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > >index d206691..f9c27ac 100644 > >--- a/drivers/net/phy/mdio_bus.c > >+++ b/drivers/net/phy/mdio_bus.c > >@@ -43,11 +43,35 @@ > > */ > >struct mii_bus *mdiobus_alloc(void) > >{ > >- return kzalloc(sizeof(struct mii_bus), GFP_KERNEL); > >+ struct mii_bus *bus; > >+ > >+ bus = kzalloc(sizeof(*bus), GFP_KERNEL); > >+ if (bus != NULL) > >+ bus->state = MDIOBUS_ALLOCATED; > >+ > >+ return bus; > >} > >EXPORT_SYMBOL(mdiobus_alloc); > > Do we really need all this state tracking? Doesn't the device layer > do this for us? I looked through other driver subsystems, and I > didn't see anything like this. Can we rearchitect this so that we're > not having to track allocation/registration state? I modeled it after struct net_device's state tracking. It's "necessary" for one case, which is mdiobus_alloc() + mdiobus_free() without an intervening mdiobus_register/unregister() pair (e.g. what happens when the mii bus driver's setup function encounters an error halfway through and frees the mii bus without ever registering it). In that case, we need to just call kfree() instead of put_device(), since the embedded struct device will never have been registered. struct net_device has this same issue, and deals with it in the same way. Also, MDIOBUS_ALLOCATED starts at 1 to make sure that any users not using mdiobus_alloc() (i.e. ones that still embed a struct mii_bus in one of their own structs) are caught and dealt with. But that part isn't really necessary. Some of the other states aren't strictly necessary either, but are just there to make sure you call stuff in the right order. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index d206691..f9c27ac 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -43,11 +43,35 @@ */ struct mii_bus *mdiobus_alloc(void) { - return kzalloc(sizeof(struct mii_bus), GFP_KERNEL); + struct mii_bus *bus; + + bus = kzalloc(sizeof(*bus), GFP_KERNEL); + if (bus != NULL) + bus->state = MDIOBUS_ALLOCATED; + + return bus; } EXPORT_SYMBOL(mdiobus_alloc); /** + * mdiobus_release - mii_bus device release callback + * + * Description: called when the last reference to an mii_bus is + * dropped, to free the underlying memory. + */ +static void mdiobus_release(struct device *d) +{ + struct mii_bus *bus = to_mii_bus(d); + BUG_ON(bus->state != MDIOBUS_RELEASED); + kfree(bus); +} + +static struct class mdio_bus_class = { + .name = "mdio_bus", + .dev_release = mdiobus_release, +}; + +/** * mdiobus_register - bring up all the PHYs on a given bus and attach them to bus * @bus: target mii_bus * @@ -66,6 +90,21 @@ int mdiobus_register(struct mii_bus *bus) NULL == bus->write) return -EINVAL; + BUG_ON(bus->state != MDIOBUS_ALLOCATED); + + bus->dev.parent = bus->parent; + bus->dev.class = &mdio_bus_class; + bus->dev.groups = NULL; + memcpy(bus->dev.bus_id, bus->id, MII_BUS_ID_SIZE); + + err = device_register(&bus->dev); + if (err) { + printk(KERN_ERR "mii_bus %s failed to register\n", bus->id); + return -EINVAL; + } + + bus->state = MDIOBUS_REGISTERED; + mutex_init(&bus->mdio_lock); if (bus->reset) @@ -92,6 +131,10 @@ void mdiobus_unregister(struct mii_bus *bus) { int i; + BUG_ON(bus->state != MDIOBUS_REGISTERED); + bus->state = MDIOBUS_UNREGISTERED; + + device_unregister(&bus->dev); for (i = 0; i < PHY_MAX_ADDR; i++) { if (bus->phy_map[i]) device_unregister(&bus->phy_map[i]->dev); @@ -103,11 +146,24 @@ EXPORT_SYMBOL(mdiobus_unregister); * mdiobus_free - free a struct mii_bus * @bus: mii_bus to free * - * This function frees the mii_bus. + * This function releases the reference to the underlying device + * object in the mii_bus. If this is the last reference, the mii_bus + * will be freed. */ void mdiobus_free(struct mii_bus *bus) { - kfree(bus); + /* + * For compatibility with error handling in drivers. + */ + if (bus->state == MDIOBUS_ALLOCATED) { + kfree(bus); + return; + } + + BUG_ON(bus->state != MDIOBUS_UNREGISTERED); + bus->state = MDIOBUS_RELEASED; + + put_device(&bus->dev); } EXPORT_SYMBOL(mdiobus_free); @@ -205,10 +261,20 @@ EXPORT_SYMBOL(mdio_bus_type); int __init mdio_bus_init(void) { - return bus_register(&mdio_bus_type); + int ret; + + ret = class_register(&mdio_bus_class); + if (!ret) { + ret = bus_register(&mdio_bus_type); + if (ret) + class_unregister(&mdio_bus_class); + } + + return ret; } void mdio_bus_exit(void) { + class_unregister(&mdio_bus_class); bus_unregister(&mdio_bus_type); } diff --git a/include/linux/phy.h b/include/linux/phy.h index ca4a83c..891f27f 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -100,6 +100,13 @@ struct mii_bus { struct mutex mdio_lock; struct device *parent; + enum { + MDIOBUS_ALLOCATED = 1, + MDIOBUS_REGISTERED, + MDIOBUS_UNREGISTERED, + MDIOBUS_RELEASED, + } state; + struct device dev; /* list of all PHYs on bus */ struct phy_device *phy_map[PHY_MAX_ADDR]; @@ -113,6 +120,7 @@ struct mii_bus { */ int *irq; }; +#define to_mii_bus(d) container_of(d, struct mii_bus, dev) #define PHY_INTERRUPT_DISABLED 0x0 #define PHY_INTERRUPT_ENABLED 0x80000000
Introduce the mdio_bus class, and give each 'struct mii_bus' its own 'struct device', so that mii_bus objects are represented in the device tree and can be found by querying the device tree. Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> --- drivers/net/phy/mdio_bus.c | 74 +++++++++++++++++++++++++++++++++++++++++-- include/linux/phy.h | 8 +++++ 2 files changed, 78 insertions(+), 4 deletions(-)