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) 800118001800: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 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
---
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 ");
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 @@
