[OpenWrt-Devel] [PATCH] b53 switch driver memory leak and reset gpio pin initialization fix
blmink
blmink at mink.su
Sat Jun 13 08:31:11 EDT 2015
Hi, Jonas
I inserted trace points (pr_info) at functions in b53_common and figured
out that driver behave wrong.
Please have a look at this:
~~~cut~~~
root@(none):/# uname -a
Linux (none) 4.0.4 #1 SMP Sat Jun 13 02:59:36 MSK 2015 mips64 GNU/Linux
root@(none):/#
root@(none):/# ifconfig eth0 up
[ 15.561304] b53_common: b53_switch_alloc
[ 15.565265] b53_common: b53_switch_register
[ 15.569960] b53_common: b53_switch_init
[ 15.650583] b53_common: found switch: BCM53115, rev 8
[ 15.659538] eth0: 1000 Mbps Full duplex, port 0
[ 15.664439] eth1: 1000 Mbps Full duplex, port 1, queue 1
root@(none):/#
root@(none):/# root@(none):/# ifconfig eth0 down
[ 42.697656] eth0: Link down
[ 42.700742] eth0: 1000 Mbps Full duplex, port 0, queue 0
root@(none):/#
root@(none):/# ifconfig eth0 down
root@(none):/# ifconfig eth0 up
[ 49.145311] b53_common: b53_switch_alloc
[ 49.149267] b53_common: b53_switch_register
[ 49.153972] b53_common: b53_switch_init
[ 49.160932] Broadcom B53 (1) 8001180000001800:1e: failed to register
switch: -16
ifconfig: SIOCSIFFLAGS: No such device
root@(none):/# reboot
~~~cut~~~
We can see that "ifconfig up" calls b53_switch_alloc which calls
devm_kzalloc in sequence.
b53_switch_alloc is called by following sequence:
"ifconfig up" --> b53_mdio driver probe/register --> b53_phy_config_init
--> b53_switch_alloc
Driver removes never (in case of "ifconfig down" either) and it do not
free memory.
So, in our example we have b53_switch_alloc called twice in a row.
As for switch gpio reset.. "failed to register switch: -16" is from
devm_gpio_request_one().
-16 is EBUSY (Device or resource busy).
My system is D-Link DSR-1000 (Cavium Octeon MIPS64), I have added
support for it.
But things described above affect any platform.
Please also see comments below.
13.06.2015 01:57, Jonas Gorski пишет:
> 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.
These affect any platform.
>> 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.
Removal occurs never, unfortunately. But memory may be allocated several
times. :(
In our case "ifconfig down" doesn't remove driver. But "ifconfig up"
allocates memory.
>> +
>> 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.
This prevents registration of already registered switch and allocation
of memory which already allocated.
>> 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.
May be you are right. I didn't test mmap/spi/srab stuff.
But in case of mdio we have switch connected to MDIO bus as pseudo PHY.
And it is initialized on every "ifconfig up".
And every initialization allocates memory.
In case of mmap/spi/srab it is important to know: when driver probe
occurs and does it (or what) happens on "ifconfig up". Unfortunately, I
don't have such hardware and can't test it.
>
>> 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
--
Best regards
Fedor.
_______________________________________________
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