Hi,
On Fri, Jun 12, 2015 at 8:16 PM, Fedor Konstantinov <blm...@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 <blm...@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 <j...@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@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
