Discussion:
[PATCH 0/2] introduce devm_mdiobus_alloc/devm_mdiobus_free
Grygorii Strashko
2014-04-04 13:40:23 UTC
Permalink
Introduce a resource managed devm_mdiobus_alloc()/devm_mdiobus_free()
to automatically clean up MDIO bus alocations made by MDIO drivers,
thus leading to simplified MDIO drivers code.

Update Davinci MDIO driver ss example of new devm APIs usage.

Grygorii Strashko (2):
mdio_bus: implement devm_mdiobus_alloc/devm_mdiobus_free
net: davinci_mdio: use devm_* api

Documentation/driver-model/devres.txt | 4 ++
drivers/net/ethernet/ti/davinci_mdio.c | 21 ++--------
drivers/net/phy/mdio_bus.c | 68 ++++++++++++++++++++++++++++++++
include/linux/phy.h | 2 +
4 files changed, 78 insertions(+), 17 deletions(-)
--
1.7.9.5
Grygorii Strashko
2014-04-04 13:40:24 UTC
Permalink
Add a resource managed devm_mdiobus_alloc()/devm_mdiobus_free()
to automatically clean up MDIO bus alocations made by MDIO drivers,
thus leading to simplified MDIO drivers code.

Signed-off-by: Grygorii Strashko <grygorii.strashko at ti.com>
---
Documentation/driver-model/devres.txt | 4 ++
drivers/net/phy/mdio_bus.c | 68 +++++++++++++++++++++++++++++++++
include/linux/phy.h | 2 +
3 files changed, 74 insertions(+)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 4f7897e..31aa066 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -308,3 +308,7 @@ SLAVE DMA ENGINE

SPI
devm_spi_register_master()
+
+MDIO
+ devm_mdiobus_alloc()
+ devm_mdiobus_free()
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 76f54b3..fdcd6d1 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -69,6 +69,74 @@ struct mii_bus *mdiobus_alloc_size(size_t size)
}
EXPORT_SYMBOL(mdiobus_alloc_size);

+static void _devm_mdiobus_free(struct device *dev, void *res)
+{
+ mdiobus_free(*(struct mii_bus **)res);
+}
+
+static int devm_mdiobus_match(struct device *dev, void *res, void *data)
+{
+ struct mii_bus **r = res;
+ if (!r || !*r) {
+ WARN_ON(!r || !*r);
+ return 0;
+ }
+ return *r == data;
+}
+
+/**
+ * devm_mdiobus_alloc - Resource-managed mdiobus_alloc_size()
+ * @dev: Device to allocate mii_bus for
+ * @sizeof_priv: Space to allocate for private structure.
+ *
+ * Managed mdiobus_alloc_size. mii_bus allocated with this function is
+ * automatically freed on driver detach.
+ *
+ * If an mii_bus allocated with this function needs to be freed separately,
+ * devm_mdiobus_free() must be used.
+ *
+ * RETURNS:
+ * Pointer to allocated mii_bus on success, NULL on failure.
+ */
+struct mii_bus *devm_mdiobus_alloc(struct device *dev, int sizeof_priv)
+{
+ struct mii_bus **ptr, *bus;
+
+ ptr = devres_alloc(_devm_mdiobus_free, sizeof(*ptr),
+ GFP_KERNEL);
+ if (!ptr)
+ return NULL;
+
+ /* use raw alloc_dr for kmalloc caller tracing */
+ bus = mdiobus_alloc_size(sizeof_priv);
+ if (bus) {
+ *ptr = bus;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return bus;
+}
+EXPORT_SYMBOL_GPL(devm_mdiobus_alloc);
+
+/**
+ * devm_mdiobus_free - Resource-managed mdiobus_free()
+ * @dev: Device this mii_bus belongs to
+ * @bus: the mii_bus associated with the device
+ *
+ * Free mii_bus allocated with devm_mdiobus_alloc().
+ */
+void devm_mdiobus_free(struct device *dev, struct mii_bus *bus)
+{
+ int rc;
+
+ rc = devres_release(dev, _devm_mdiobus_free,
+ devm_mdiobus_match, bus);
+ WARN_ON(rc);
+}
+EXPORT_SYMBOL_GPL(devm_mdiobus_free);
+
/**
* mdiobus_release - mii_bus device release callback
* @d: the target struct device that contains the mii_bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 24126c4..2238bea 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -195,6 +195,8 @@ static inline struct mii_bus *mdiobus_alloc(void)
int mdiobus_register(struct mii_bus *bus);
void mdiobus_unregister(struct mii_bus *bus);
void mdiobus_free(struct mii_bus *bus);
+struct mii_bus *devm_mdiobus_alloc(struct device *dev, int sizeof_priv);
+void devm_mdiobus_free(struct device *dev, struct mii_bus *bus);
struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
--
1.7.9.5
Sergei Shtylyov
2014-04-04 13:15:35 UTC
Permalink
Hello.
Post by Grygorii Strashko
Add a resource managed devm_mdiobus_alloc()/devm_mdiobus_free()
to automatically clean up MDIO bus alocations made by MDIO drivers,
thus leading to simplified MDIO drivers code.
Signed-off-by: Grygorii Strashko <grygorii.strashko at ti.com>
[...]
Post by Grygorii Strashko
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 76f54b3..fdcd6d1 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -69,6 +69,74 @@ struct mii_bus *mdiobus_alloc_size(size_t size)
}
EXPORT_SYMBOL(mdiobus_alloc_size);
+static void _devm_mdiobus_free(struct device *dev, void *res)
+{
+ mdiobus_free(*(struct mii_bus **)res);
+}
+
+static int devm_mdiobus_match(struct device *dev, void *res, void *data)
+{
+ struct mii_bus **r = res;
Please insert an empty line after declaration.
Post by Grygorii Strashko
+ if (!r || !*r) {
+ WARN_ON(!r || !*r);
Hm, why we need the duplicate check This condition is always true.

[...]
Post by Grygorii Strashko
+/**
+ * devm_mdiobus_free - Resource-managed mdiobus_free()
+ *
+ * Free mii_bus allocated with devm_mdiobus_alloc().
+ */
+void devm_mdiobus_free(struct device *dev, struct mii_bus *bus)
+{
+ int rc;
+
+ rc = devres_release(dev, _devm_mdiobus_free,
+ devm_mdiobus_match, bus);
Please make this line start under 'dev', according to the networking
coding style.

[...]

WBR, Sergei
Grygorii Strashko
2014-04-08 09:52:05 UTC
Permalink
Post by Sergei Shtylyov
Hello.
Post by Grygorii Strashko
Add a resource managed devm_mdiobus_alloc()/devm_mdiobus_free()
to automatically clean up MDIO bus alocations made by MDIO drivers,
thus leading to simplified MDIO drivers code.
Signed-off-by: Grygorii Strashko <grygorii.strashko at ti.com>
[...]
Post by Grygorii Strashko
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 76f54b3..fdcd6d1 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -69,6 +69,74 @@ struct mii_bus *mdiobus_alloc_size(size_t size)
}
EXPORT_SYMBOL(mdiobus_alloc_size);
+static void _devm_mdiobus_free(struct device *dev, void *res)
+{
+ mdiobus_free(*(struct mii_bus **)res);
+}
+
+static int devm_mdiobus_match(struct device *dev, void *res, void *data)
+{
+ struct mii_bus **r = res;
Please insert an empty line after declaration.
Post by Grygorii Strashko
+ if (!r || !*r) {
+ WARN_ON(!r || !*r);
Hm, why we need the duplicate check This condition is always true.
It can be replaced with:
if (WARN_ON(!r || !*r))
return 0;


I will wait for additional comments, then resend.

Regards,
- grygorii
Grygorii Strashko
2014-04-04 13:40:25 UTC
Permalink
Use devm_* API for memory allocation and to get device's clock
to simplify driver's code.

Signed-off-by: Grygorii Strashko <grygorii.strashko at ti.com>
---
drivers/net/ethernet/ti/davinci_mdio.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index 0cca9de..f0f7128 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -321,15 +321,14 @@ static int davinci_mdio_probe(struct platform_device *pdev)
struct phy_device *phy;
int ret, addr;

- data = kzalloc(sizeof(*data), GFP_KERNEL);
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;

- data->bus = mdiobus_alloc();
+ data->bus = devm_mdiobus_alloc(dev, 0);
if (!data->bus) {
dev_err(dev, "failed to alloc mii bus\n");
- ret = -ENOMEM;
- goto bail_out;
+ return -ENOMEM;
}

if (dev->of_node) {
@@ -354,7 +353,7 @@ static int davinci_mdio_probe(struct platform_device *pdev)

pm_runtime_enable(&pdev->dev);
pm_runtime_get_sync(&pdev->dev);
- data->clk = clk_get(&pdev->dev, "fck");
+ data->clk = devm_clk_get(dev, "fck");
if (IS_ERR(data->clk)) {
dev_err(dev, "failed to get device clock\n");
ret = PTR_ERR(data->clk);
@@ -406,16 +405,9 @@ static int davinci_mdio_probe(struct platform_device *pdev)
return 0;

bail_out:
- if (data->bus)
- mdiobus_free(data->bus);
-
- if (data->clk)
- clk_put(data->clk);
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);

- kfree(data);
-
return ret;
}

@@ -425,16 +417,11 @@ static int davinci_mdio_remove(struct platform_device *pdev)

if (data->bus) {
mdiobus_unregister(data->bus);
- mdiobus_free(data->bus);
}

- if (data->clk)
- clk_put(data->clk);
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);

- kfree(data);
-
return 0;
}
--
1.7.9.5
Sergei Shtylyov
2014-04-04 13:18:55 UTC
Permalink
Post by Grygorii Strashko
Use devm_* API for memory allocation and to get device's clock
to simplify driver's code.
Signed-off-by: Grygorii Strashko <grygorii.strashko at ti.com>
---
drivers/net/ethernet/ti/davinci_mdio.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index 0cca9de..f0f7128 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
[...]
Post by Grygorii Strashko
@@ -425,16 +417,11 @@ static int davinci_mdio_remove(struct platform_device *pdev)
if (data->bus) {
mdiobus_unregister(data->bus);
- mdiobus_free(data->bus);
}
Remove {} please, it's not needed anymore, according to
Documentation/CodingStyle.

WBR, Sergei
Florian Fainelli
2014-04-11 04:55:29 UTC
Permalink
Hi Grygorii,
Post by Grygorii Strashko
Introduce a resource managed devm_mdiobus_alloc()/devm_mdiobus_free()
to automatically clean up MDIO bus alocations made by MDIO drivers,
thus leading to simplified MDIO drivers code.
Update Davinci MDIO driver ss example of new devm APIs usage.
This does look good at first glance. net-next is currently closed at
the moment, so this will have to be merged later.

At some point, we might also want to handle the mdio_bus irq array, as
that one is also usually dynamically allocated. Maybe we could just do
a static irq[PHY_MAX_ADDR] allocation, 32 times the size of an integer
might not be worth a potential leak.
Post by Grygorii Strashko
mdio_bus: implement devm_mdiobus_alloc/devm_mdiobus_free
net: davinci_mdio: use devm_* api
Documentation/driver-model/devres.txt | 4 ++
drivers/net/ethernet/ti/davinci_mdio.c | 21 ++--------
drivers/net/phy/mdio_bus.c | 68 ++++++++++++++++++++++++++++++++
include/linux/phy.h | 2 +
4 files changed, 78 insertions(+), 17 deletions(-)
--
1.7.9.5
--
Florian
Grygorii Strashko
2014-04-11 11:05:25 UTC
Permalink
Hi Florian,
Post by Florian Fainelli
Hi Grygorii,
Post by Grygorii Strashko
Introduce a resource managed devm_mdiobus_alloc()/devm_mdiobus_free()
to automatically clean up MDIO bus alocations made by MDIO drivers,
thus leading to simplified MDIO drivers code.
Update Davinci MDIO driver ss example of new devm APIs usage.
This does look good at first glance. net-next is currently closed at
the moment, so this will have to be merged later.
Thanks. It can wait for 3.16, so I'll update & resend after rc1.
I have few more patches for davinci_mdio.c, so
my intention here was to check if I can base them on top of new API or not :)
Post by Florian Fainelli
At some point, we might also want to handle the mdio_bus irq array, as
that one is also usually dynamically allocated. Maybe we could just do
a static irq[PHY_MAX_ADDR] allocation, 32 times the size of an integer
might not be worth a potential leak.
It sounds good, but first of all irq field of mii_bus structure has to be made private.
And drivers have to use getter/setters to access it - then its type can be changes
simply and safely.

By the way, mdiobus_register() can be handled using DEVM approach too, but it
will a bit more complex.

[...]
Regards,
- grygorii

Loading...