[PATCH 0/2] introduce devm_mdiobus_alloc/devm_mdiobus_free

[PATCH 0/2] introduce devm_mdiobus_alloc/devm_mdiobus_free

[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.

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

[PATCH 1/2] mdio_bus: implement devm_mdiobus_alloc/devm_mdiobus_free

[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@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

Re: [PATCH 1/2] mdio_bus: implement devm_mdiobus_alloc/devm_mdiobus_free

[Sergei Shtylyov]

Hello.

On 04-04-2014 17:40, Grygorii Strashko wrote:

> 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@ti.com>
[...]

> 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.

> +	if (!r || !*r) {
> +		WARN_ON(!r || !*r);

    Hm, why we need the duplicate check This condition is always true.

[...]
> +/**
> + * 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);

    Please make this line start under 'dev', according to the networking 
coding style.

[...]

WBR, Sergei

Re: [PATCH 1/2] mdio_bus: implement devm_mdiobus_alloc/devm_mdiobus_free

[Grygorii Strashko]

On 04/04/2014 04:15 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 04-04-2014 17:40, Grygorii Strashko wrote:
>
>> 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@ti.com>
> [...]
>
>> 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.
>
>> +    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

[PATCH 2/2] net: davinci_mdio: use devm_* api

[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@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

Re: [PATCH 2/2] net: davinci_mdio: use devm_* api

[Sergei Shtylyov]

On 04-04-2014 17:40, Grygorii Strashko wrote:

> Use devm_* API for memory allocation and to get device's clock
> to simplify driver's code.

> Signed-off-by: Grygorii Strashko <grygorii.strashko@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
[...]
> @@ -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

Re: [PATCH 0/2] introduce devm_mdiobus_alloc/devm_mdiobus_free

[Florian Fainelli]

Hi Grygorii,

2014-04-04 6:40 GMT-07:00 Grygorii Strashko <grygorii.strashko@ti.com>:
> 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.

>
> 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
>



-- 
Florian

Re: [PATCH 0/2] introduce devm_mdiobus_alloc/devm_mdiobus_free

[Grygorii Strashko]

Hi Florian,
On 04/11/2014 07:55 AM, Florian Fainelli wrote:
> Hi Grygorii,
> 
> 2014-04-04 6:40 GMT-07:00 Grygorii Strashko <grygorii.strashko@ti.com>:
>> 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 :)

> 
> 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