Patchwork [3/3] phylib: give mdio buses a device tree presence

login
register
mail settings
Submitter Lennert Buytenhek
Date Sept. 29, 2008, 2:14 a.m.
Message ID <20080929021400.GE21560@xi.wantstofly.org>
Download mbox | patch
Permalink /patch/1850/
State Superseded
Delegated to: Jeff Garzik
Headers show

Comments

Lennert Buytenhek - Sept. 29, 2008, 2:14 a.m.
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(-)
Andy Fleming - Sept. 29, 2008, 10:45 p.m.
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
Lennert Buytenhek - Sept. 30, 2008, 3:04 a.m.
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

Patch

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