[OpenWrt-Devel] [PATCH] b53 switch driver memory leak and reset gpio pin initialization fix

Jonas Gorski jogo at openwrt.org
Fri Jun 12 18:57:41 EDT 2015


Hi,

On Fri, Jun 12, 2015 at 8:16 PM, Fedor Konstantinov <blmink at mink.su> wrote:
> Memory and switch reset gpio pin must be allocated on switch/module init and freed on removal. At least they should not be allocated 2 or more times in a row.
>
> Signed-off-by: Fedor Konstantinov <blmink at mink.su>
> ---
> Comments:
>
> Following cmd sequence calls b53_switch_init() twice, so causes leak of memory.
> Last ifconfig will fail also on targets which support switch reset gpio pin, because devm_gpio_request_one() will be called twice in a row.
> ifconfig eth0 up
> ifconfig eth0 down
> ifconfig eth0 up

On what platform? This also requires a better explanation why this is
the correct fix.

> mmap/spi/srab drivers were not tested yet because I don't have such hardware.
> ---
>  .../generic/files/drivers/net/phy/b53/b53_common.c | 19 +++++++++++++++
>  .../generic/files/drivers/net/phy/b53/b53_mdio.c   |  6 +++++
>  .../generic/files/drivers/net/phy/b53/b53_mmap.c   | 28 +++++++++++++++++-----
>  .../generic/files/drivers/net/phy/b53/b53_priv.h   |  7 +++---
>  .../generic/files/drivers/net/phy/b53/b53_spi.c    | 20 ++++++++++++----
>  .../generic/files/drivers/net/phy/b53/b53_srab.c   | 28 +++++++++++++++++-----
>  6 files changed, 88 insertions(+), 20 deletions(-)
>
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_common.c b/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> index 47b5a8b..2e2f6aa 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> @@ -1362,6 +1362,12 @@ struct b53_device *b53_switch_alloc(struct device *base, struct b53_io_ops *ops,
>  }
>  EXPORT_SYMBOL(b53_switch_alloc);
>
> +void b53_switch_free(struct device *dev, struct b53_device *priv)
> +{
> +       devm_kfree(dev, priv);
> +}
> +EXPORT_SYMBOL(b53_switch_free);
> +
>  int b53_switch_detect(struct b53_device *dev)
>  {
>         u32 id32;
> @@ -1452,6 +1458,19 @@ int b53_switch_register(struct b53_device *dev)
>  }
>  EXPORT_SYMBOL(b53_switch_register);
>
> +void b53_switch_remove(struct b53_device *dev)
> +{
> +       unregister_switch(&dev->sw_dev);
> +
> +       if (dev->reset_gpio >= 0)
> +               devm_gpio_free(dev->dev, dev->reset_gpio);
> +
> +       devm_kfree(dev->dev, dev->buf);
> +       devm_kfree(dev->dev, dev->vlans);
> +       devm_kfree(dev->dev, dev->ports);
> +}
> +EXPORT_SYMBOL(b53_switch_remove);

These look wrong, the whole point of using devm_* is that you do *not*
need to free the resources manually and it will be automatically done
on removal or probe failure.

> +
>  MODULE_AUTHOR("Jonas Gorski <jogo at openwrt.org>");
>  MODULE_DESCRIPTION("B53 switch library");
>  MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c b/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> index 3c25f0e..9a5f058 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> @@ -285,6 +285,10 @@ static int b53_phy_config_init(struct phy_device *phydev)
>         struct b53_device *dev;
>         int ret;
>
> +       /* check if already initialized */
> +       if (phydev->priv)
> +               return 0;
> +

This is the only thing that looks somewhat valid, but needs a better
explanation why this might happen.

>         dev = b53_switch_alloc(&phydev->dev, &b53_mdio_ops, phydev->bus);
>         if (!dev)
>                 return -ENOMEM;
> @@ -314,6 +318,8 @@ static void b53_phy_remove(struct phy_device *phydev)
>
>         b53_switch_remove(priv);
>
> +       b53_switch_free(&phydev->dev, priv);
> +

See the above comment regarding devm_*free

>         phydev->priv = NULL;
>  }
>
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_mmap.c b/target/linux/generic/files/drivers/net/phy/b53/b53_mmap.c
> index ab1895e..7c83758 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_mmap.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_mmap.c
> @@ -200,7 +200,12 @@ static struct b53_io_ops b53_mmap_ops = {
>  static int b53_mmap_probe(struct platform_device *pdev)
>  {
>         struct b53_platform_data *pdata = pdev->dev.platform_data;
> -       struct b53_device *dev;
> +       struct b53_device *dev = platform_get_drvdata(pdev);
> +       int ret;
> +
> +       /* check if already initialized */
> +       if (dev)
> +               return 0;

This shouldn't be possible. The probe shouldn't have been run twice,
and a remove should have unregistered the switch.

>
>         if (!pdata)
>                 return -EINVAL;
> @@ -209,20 +214,31 @@ static int b53_mmap_probe(struct platform_device *pdev)
>         if (!dev)
>                 return -ENOMEM;
>
> -       if (pdata)
> -               dev->pdata = pdata;
> +       dev->pdata = pdata;
> +
> +       ret = b53_switch_register(dev);
> +       if (ret) {
> +               dev_err(dev->dev, "failed to register switch: %i\n", ret);

If anything b53_switch_register should complain if it fails, not each
and every user of it.

> +               return ret;
> +       }
>
>         platform_set_drvdata(pdev, dev);
>
> -       return b53_switch_register(dev);
> +       return 0;

Undocumented, mostly unrelated code churn.

>  }
>
>  static int b53_mmap_remove(struct platform_device *pdev)
>  {
>         struct b53_device *dev = platform_get_drvdata(pdev);
>
> -       if (dev)
> -               b53_switch_remove(dev);
> +       if (!dev)
> +               return 0;
> +
> +       b53_switch_remove(dev);
> +
> +       b53_switch_free(&pdev->dev, dev);
> +
> +       platform_set_drvdata(pdev, NULL);

This is superfluous, the driver core already NULLs the drvdata on
remove / probe failure since 3.6.

>
>         return 0;
>  }
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h b/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> index 4336fdb..8ef68a1 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> @@ -176,14 +176,13 @@ static inline struct b53_device *sw_to_b53(struct switch_dev *sw)
>  struct b53_device *b53_switch_alloc(struct device *base, struct b53_io_ops *ops,
>                                     void *priv);
>
> +void b53_switch_free(struct device *base, struct b53_device *priv);
> +
>  int b53_switch_detect(struct b53_device *dev);
>
>  int b53_switch_register(struct b53_device *dev);
>
> -static inline void b53_switch_remove(struct b53_device *dev)
> -{
> -       unregister_switch(&dev->sw_dev);
> -}
> +void b53_switch_remove(struct b53_device *dev);
>
>  static inline int b53_read8(struct b53_device *dev, u8 page, u8 reg, u8 *val)
>  {
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_spi.c b/target/linux/generic/files/drivers/net/phy/b53/b53_spi.c
> index 469a8dd..7cfb120 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_spi.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_spi.c
> @@ -284,9 +284,13 @@ static struct b53_io_ops b53_spi_ops = {
>
>  static int b53_spi_probe(struct spi_device *spi)
>  {
> -       struct b53_device *dev;
> +       struct b53_device *dev = spi_get_drvdata(spi);
>         int ret;
>
> +       /* check if already initialized */
> +       if (dev)
> +               return 0;

This should be impossible.

> +
>         dev = b53_switch_alloc(&spi->dev, &b53_spi_ops, spi);
>         if (!dev)
>                 return -ENOMEM;
> @@ -295,8 +299,10 @@ static int b53_spi_probe(struct spi_device *spi)
>                 dev->pdata = spi->dev.platform_data;
>
>         ret = b53_switch_register(dev);
> -       if (ret)
> +       if (ret) {
> +               dev_err(dev->dev, "failed to register switch: %i\n", ret);
>                 return ret;
> +       }
>
>         spi_set_drvdata(spi, dev);
>
> @@ -307,8 +313,14 @@ static int b53_spi_remove(struct spi_device *spi)
>  {
>         struct b53_device *dev = spi_get_drvdata(spi);
>
> -       if (dev)
> -               b53_switch_remove(dev);
> +       if (!dev)
> +               return 0;
> +
> +       b53_switch_remove(dev);
> +
> +       b53_switch_free(&spi->dev, dev);
> +
> +       spi_set_drvdata(spi, NULL);

This is superfluous, the driver core already NULLs the drvdata on
remove / probe failure since 3.6.

>
>         return 0;
>  }
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_srab.c b/target/linux/generic/files/drivers/net/phy/b53/b53_srab.c
> index 012daa3..00eb0fe 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_srab.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_srab.c
> @@ -337,7 +337,12 @@ static struct b53_io_ops b53_srab_ops = {
>  static int b53_srab_probe(struct platform_device *pdev)
>  {
>         struct b53_platform_data *pdata = pdev->dev.platform_data;
> -       struct b53_device *dev;
> +       struct b53_device *dev = platform_get_drvdata(pdev);
> +       int ret;
> +
> +       /* check if already initialized */
> +       if (dev)
> +               return 0;

Should be impossible etc etc.

>
>         if (!pdata)
>                 return -EINVAL;
> @@ -346,20 +351,31 @@ static int b53_srab_probe(struct platform_device *pdev)
>         if (!dev)
>                 return -ENOMEM;
>
> -       if (pdata)
> -               dev->pdata = pdata;
> +       dev->pdata = pdata;
> +
> +       ret = b53_switch_register(dev);
> +       if (ret) {
> +               dev_err(dev->dev, "failed to register switch: %i\n", ret);
> +               return ret;
> +       }
>
>         platform_set_drvdata(pdev, dev);
>
> -       return b53_switch_register(dev);
> +       return 0;

Undocumented, mostly unrelated code churn.

>  }
>
>  static int b53_srab_remove(struct platform_device *pdev)
>  {
>         struct b53_device *dev = platform_get_drvdata(pdev);
>
> -       if (dev)
> -               b53_switch_remove(dev);
> +       if (!dev)
> +               return 0;
> +
> +       b53_switch_remove(dev);
> +
> +       b53_switch_free(&pdev->dev, dev);
> +
> +       platform_set_drvdata(pdev, NULL);

This is superfluous, the driver core already NULLs the drvdata on
remove / probe failure since 3.6.

>
>         return 0;
>  }


Regards
Jonas
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel



More information about the openwrt-devel mailing list