This patch series fixes a system hang I got each time i tried to shutdown
or reboot a system that uses a KSZ9897 as a DSA switch with a broadcom
GENET network device as the DSA master device. At the time the system hangs
the message "unregister_netdevice: waiting for eth0 to become free. Usage
count = 2." is dumped periodically to the console.
After some investigation I found the reason to be unreleased references to
the master device which are still held by the slave devices at the time the
system is shut down (I have two slave devices in use).
While these references are supposed to be released in ksz_switch_remove()
this function never gets the chance to be called due to the system hang at
the master device deregistration which happens before ksz_switch_remove()
is called.
The fix is to make sure that the master device references are already
released when the device is unregistered. For this reason PATCH1 provides
a new function dsa_tree_shutdown() that can be called by DSA drivers to
untear the DSA switch at shutdown. PATCH2 uses this function in a new
helper function for KSZ switches to properly shutdown the KSZ switch.
PATCH 3 uses the new helper function in the KSZ9477 shutdown handler.
Theses patches have been tested on a Raspberry PI 5.10 kernel with a
KSZ9897. The patches have been adjusted to apply against net-next and are
compile tested with next-next.
Lino Sanfilippo (3):
net: dsa: introduce function dsa_tree_shutdown()
net: dsa: microchip: provide the function ksz_switch_shutdown()
net: dsa: microchip: tear down DSA tree at system shutdown
drivers/net/dsa/microchip/ksz9477.c | 12 +++++++++++-
drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++++
drivers/net/dsa/microchip/ksz_common.h | 1 +
include/net/dsa.h | 1 +
net/dsa/dsa2.c | 8 ++++++++
5 files changed, 34 insertions(+), 1 deletion(-)
base-commit: 626bf91a292e2035af5b9d9cce35c5c138dfe06d
--
2.33.0
Provide the function dsa_tree_shutdown() that can be called by DSA drivers
to tear down the DSA tree.
This is particularly useful for shutdown handlers to make sure that the DSA
tree is torn down (and thus all references to the master device are
released) before the master device is deregistered.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
include/net/dsa.h | 1 +
net/dsa/dsa2.c | 8 ++++++++
2 files changed, 9 insertions(+)
@@ -1066,6 +1066,14 @@ static void dsa_tree_teardown(struct dsa_switch_tree *dst)dst->setup=false;}+voiddsa_tree_shutdown(structdsa_switch_tree*dst)+{+mutex_lock(&dsa2_mutex);+dsa_tree_teardown(dst);+mutex_unlock(&dsa2_mutex);+}+EXPORT_SYMBOL_GPL(dsa_tree_shutdown);+/* Since the dsa/tagging sysfs device attribute is per master, the assumption*isthatallDSAswitcheswithinatreesharethesametagger,otherwise*theywouldhaveformeddisjointtrees(different"dsa,member"values).
Provide a function ksz_switch_shutdown() which properly shuts down the KSZ
switch by stopping the mib_read worker thread and then tearing down the DSA
tree.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++++
drivers/net/dsa/microchip/ksz_common.h | 1 +
2 files changed, 14 insertions(+)
@@ -446,6 +446,19 @@ int ksz_switch_register(struct ksz_device *dev,}EXPORT_SYMBOL(ksz_switch_register);+voidksz_switch_shutdown(structksz_device*dev)+{+structdsa_switch*ds=dev->ds;++/* timer started */+if(dev->mib_read_interval){+cancel_delayed_work_sync(&dev->mib_read);+dev->mib_read_interval=0;+}+dsa_tree_shutdown(ds->dst);+}+EXPORT_SYMBOL(ksz_switch_shutdown);+voidksz_switch_remove(structksz_device*dev){/* timer started */
Shutting down the system with registered KSZ9477 slave devices results in a
system hang. Additionally the message "unregister_netdevice: waiting for
ETH to become free. Usage count = X" is dumped into the kernel log (with
ETH being the DSA master device and X the number of registered slave
devices).
The reason for this issue are pending references to the DSA master device
which are still held by the slave devices at the time master device is
unregistered.
While these references are supposed to be released in ksz_switch_remove()
this function never gets the chance to be called due to the earlier system
hang at the master device deregistration.
Fix this deadlock situation by deregistering the switch (and thus releasing
the master device references) in the KSZ9477 shutdown handler. Since this
handler is called before the master device deregistration all references to
the master device are released at the time of deregistration and thus the
deadlock does not occur.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/net/dsa/microchip/ksz9477.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-09-09 10:15:01
On Thu, Sep 09, 2021 at 11:53:21AM +0200, Lino Sanfilippo wrote:
This patch series fixes a system hang I got each time i tried to shutdown
or reboot a system that uses a KSZ9897 as a DSA switch with a broadcom
GENET network device as the DSA master device. At the time the system hangs
the message "unregister_netdevice: waiting for eth0 to become free. Usage
count = 2." is dumped periodically to the console.
After some investigation I found the reason to be unreleased references to
the master device which are still held by the slave devices at the time the
system is shut down (I have two slave devices in use).
While these references are supposed to be released in ksz_switch_remove()
this function never gets the chance to be called due to the system hang at
the master device deregistration which happens before ksz_switch_remove()
is called.
The fix is to make sure that the master device references are already
released when the device is unregistered. For this reason PATCH1 provides
a new function dsa_tree_shutdown() that can be called by DSA drivers to
untear the DSA switch at shutdown. PATCH2 uses this function in a new
helper function for KSZ switches to properly shutdown the KSZ switch.
PATCH 3 uses the new helper function in the KSZ9477 shutdown handler.
Theses patches have been tested on a Raspberry PI 5.10 kernel with a
KSZ9897. The patches have been adjusted to apply against net-next and are
compile tested with next-next.
Can you try this patch
commit 07b90056cb15ff9877dca0d8f1b6583d1051f724
Author: Vladimir Oltean [off-list ref]
Date: Tue Jan 12 01:09:43 2021 +0200
net: dsa: unbind all switches from tree when DSA master unbinds
Currently the following happens when a DSA master driver unbinds while
there are DSA switches attached to it:
Can you try this patch
commit 07b90056cb15ff9877dca0d8f1b6583d1051f724
Author: Vladimir Oltean [off-list ref]
Date: Tue Jan 12 01:09:43 2021 +0200
net: dsa: unbind all switches from tree when DSA master unbinds
Currently the following happens when a DSA master driver unbinds while
there are DSA switches attached to it:
This patch is already part of the kernel which shows the described shutdown issues.
Regards,
Lino
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-09-09 11:54:22
On Thu, Sep 09, 2021 at 01:08:26PM +0200, Lino Sanfilippo wrote:
Hi,
On 09.09.21 at 12:14, Vladimir Oltean wrote:
quoted
Can you try this patch
commit 07b90056cb15ff9877dca0d8f1b6583d1051f724
Author: Vladimir Oltean [off-list ref]
Date: Tue Jan 12 01:09:43 2021 +0200
net: dsa: unbind all switches from tree when DSA master unbinds
Currently the following happens when a DSA master driver unbinds while
there are DSA switches attached to it:
This patch is already part of the kernel which shows the described shutdown issues.
How can I reproduce this issue?
When I test with sysrq-o, I do see the various DSA trees getting torn
down:
[ 16.731468] sysrq: HELP : loglevel(0-9) reboot(b) crash(c) show-all-locks(d) terminate-all-tasks(e) memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j) sak(k) show-backtrace-all-active-cpus(l) show-memory-usage(m) nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q) unraw(r) sync(s) show-task-states(t) unmount(u) show-blocked-tasks(w) dump-ftrace-buffer(z)
[ 29.912535] sysrq: Power Off
[ 29.917806] kvm: exiting hardware virtualization
[ 29.988036] device swp0 left promiscuous mode
[ 30.370424] sja1105 spi2.0: Link is Down
[ 30.402790] DSA: tree 1 torn down
[ 30.495096] device swp2 left promiscuous mode
[ 31.011576] sja1105 spi2.1: Link is Down
[ 31.032925] DSA: tree 2 torn down
[ 31.074226] reboot: Power down
I feel that something is missing in your system. Is the device link
created? Is it deleted before going into effect on shutdown?
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-09-09 14:23:50
On Thu, Sep 09, 2021 at 02:42:48PM +0300, Vladimir Oltean wrote:
I feel that something is missing in your system. Is the device link
created? Is it deleted before going into effect on shutdown?
So in case my questions were confusing, you can check the presence of
the device links via sysfs.
On my board, eno2 is the top-level DSA master, there is a switch which
is PCIe PF 0000:00:00.5 which is its consumer:
ls -la /sys/class/net/eno2/device/consumer\:pci\:0000\:00\:00.5
lrwxrwxrwx 1 root root 0 Jan 1 00:00 /sys/class/net/eno2/device/consumer:pci:0000:00:00.5 -> ../../../../../virtual/devlink/pci:0000:00:00.2--pci:0000:00:00.5
In turn, that switch is a DSA master on two ports for SPI-attached switches:
ls -la /sys/class/net/swp0/device/consumer\:spi\:spi2.*
lrwxrwxrwx 1 root root 0 Jan 1 00:04 /sys/class/net/swp0/device/consumer:spi:spi2.0 -> ../../../../../virtual/devlink/pci:0000:00:00.5--spi:spi2.0
lrwxrwxrwx 1 root root 0 Jan 1 00:04 /sys/class/net/swp0/device/consumer:spi:spi2.1 -> ../../../../../virtual/devlink/pci:0000:00:00.5--spi:spi2.1
Do you see similar things on your 5.10 kernel?
Please note that I don't think that particular patch with device links
was backported to v5.10, at least I don't see it when I run:
git tag --contains 07b90056cb15f
So how did it reach your tree?
Gesendet: Donnerstag, 09. September 2021 um 14:56 Uhr
Von: "Vladimir Oltean" [off-list ref]
An: "Lino Sanfilippo" [off-list ref]
Cc: p.rosenberger@kunbus.com, woojung.huh@microchip.com, UNGLinuxDriver@microchip.com, andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com, davem@davemloft.net, kuba@kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Betreff: Re: [PATCH 0/3] Fix for KSZ DSA switch shutdown
On Thu, Sep 09, 2021 at 02:42:48PM +0300, Vladimir Oltean wrote:
quoted
I feel that something is missing in your system. Is the device link
created? Is it deleted before going into effect on shutdown?
So in case my questions were confusing, you can check the presence of
the device links via sysfs.
On my board, eno2 is the top-level DSA master, there is a switch which
is PCIe PF 0000:00:00.5 which is its consumer:
ls -la /sys/class/net/eno2/device/consumer\:pci\:0000\:00\:00.5
lrwxrwxrwx 1 root root 0 Jan 1 00:00 /sys/class/net/eno2/device/consumer:pci:0000:00:00.5 -> ../../../../../virtual/devlink/pci:0000:00:00.2--pci:0000:00:00.5
In turn, that switch is a DSA master on two ports for SPI-attached switches:
ls -la /sys/class/net/swp0/device/consumer\:spi\:spi2.*
lrwxrwxrwx 1 root root 0 Jan 1 00:04 /sys/class/net/swp0/device/consumer:spi:spi2.0 -> ../../../../../virtual/devlink/pci:0000:00:00.5--spi:spi2.0
lrwxrwxrwx 1 root root 0 Jan 1 00:04 /sys/class/net/swp0/device/consumer:spi:spi2.1 -> ../../../../../virtual/devlink/pci:0000:00:00.5--spi:spi2.1
Do you see similar things on your 5.10 kernel?
For the master device is see
lrwxrwxrwx 1 root root 0 Sep 9 14:10 /sys/class/net/eth0/device/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.ethernet--spi:spi3.0
Please note that I don't think that particular patch with device links
was backported to v5.10, at least I don't see it when I run:
git tag --contains 07b90056cb15f
So how did it reach your tree?
The kernel I use is the Raspberry Pi 5.10 kernel. The commit number in this kernel is d0b97c8cd63e37e6d4dc9fefd6381b09f6c31a67
Andrew: the switch is not on a hat, the device tree part I use is:
spi@7e204c00 {
cs-gpios = <0x0000000f 0x00000010 0x00000001 0x0000000f 0x00000012 0x00000001>;
pinctrl-0 = <0x000000e5 0x000000e6>;
pinctrl-names = "default";
compatible = "brcm,bcm2835-spi";
reg = <0x7e204c00 0x00000200>;
interrupts = <0x00000000 0x00000076 0x00000004>;
clocks = <0x00000007 0x00000014>;
#address-cells = <0x00000001>;
#size-cells = <0x00000000>;
status = "okay";
phandle = <0x000000be>;
tpm@1 {
phandle = <0x000000ed>;
status = "okay";
interrupts = <0x0000000a 0x00000008>;
#interrupt-cells = <0x00000002>;
interrupt-parent = <0x0000000f>;
spi-max-frequency = <0x000f4240>;
reg = <0x00000001>;
pinctrl-0 = <0x000000e7>;
pinctrl-names = "default";
compatible = "infineon,slb9670";
};
ksz9897@0 {
phandle = <0x000000ec>;
status = "okay";
reset-gpios = <0x000000e1 0x0000000d 0x00000001>;
spi-cpol;
spi-cpha;
spi-max-frequency = <0x01f78a40>;
reg = <0x00000000>;
compatible = "microchip,ksz9897";
ports {
#size-cells = <0x00000000>;
#address-cells = <0x00000001>;
port@2 {
label = "piright";
reg = <0x00000002>;
};
port@1 {
label = "pileft";
reg = <0x00000001>;
};
port@0 {
ethernet = <0x000000d7>;
label = "cpu";
reg = <0x00000000>;
};
};
};
Regards,
Lino
The kernel I use is the Raspberry Pi 5.10 kernel. The commit number in this kernel is d0b97c8cd63e37e6d4dc9fefd6381b09f6c31a67
This is not correct. The kernel I use right now is based on Gregs stable linux-5.10.y.
The commit number is correct here. Sorry for the confusion.
Regards,
Lino
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-09-09 15:17:26
This is not correct. The kernel I use right now is based on Gregs stable linux-5.10.y.
The commit number is correct here. Sorry for the confusion.
Can you use 5.14.2?
When we understand the problem, the fixes will need to be for
net-next, which will be based on 5.15-rcX. They will then be
backported to 5.10. So you need to do some testing on a newer
kernel. Such testing will also help us figure out if it is a new
problem, or a backporting problem.
Andrew
This is not correct. The kernel I use right now is based on Gregs stable linux-5.10.y.
The commit number is correct here. Sorry for the confusion.
Can you use 5.14.2?
When we understand the problem, the fixes will need to be for
net-next, which will be based on 5.15-rcX. They will then be
backported to 5.10. So you need to do some testing on a newer
kernel. Such testing will also help us figure out if it is a new
problem, or a backporting problem.
Andrew
You are right, I will try to switch to a newer kernel to test future DSA issue like that
(I have the impression that DSA is an area in which a lot of progress is done from
one kernel version to the next).
Regards,
Lino
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-09-09 15:12:02
Andrew: the switch is not on a hat, the device tree part I use is:
And this is not an overlay. It is all there at boot?
I was just thinking that maybe the Ethernet interface gets opened at
boot, and overlay is loaded, and the interface is opened a second
time. I don't know of anybody using DSA with overlays, so that could
of been the key difference which breaks it for you.
Your decompiled DT blob looks O.K.
Andrew
Andrew: the switch is not on a hat, the device tree part I use is:
And this is not an overlay. It is all there at boot?
Well actually we DO use an overlay. The dev tree snipped I posted was an excerpt form
fdtdump. The concerning fragment looks like this in the overlay file:
fragment@4 {
target = <&spi6>;
__overlay__ {
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&spi6_pins>, <&spi6_cs_pins>;
cs-gpios = <&gpio 16 GPIO_ACTIVE_LOW>,
<&gpio 18 GPIO_ACTIVE_LOW>;
status = "okay";
ksz9897: ksz9897@0 {
compatible = "microchip,ksz9897";
reg = <0>;
spi-max-frequency = <50000000>;
spi-cpha;
spi-cpol;
reset-gpios = <&expander_core 13 GPIO_ACTIVE_LOW>;
status = "okay";
ports {
#address-cells = <1>;
#size-cells = <0>;
/* PORT 1 */
port@0 {
reg = <0>;
label = "cpu";
ethernet = <&genet>;
};
/* PORT 2 */
port@1 {
reg = <1>;
label = "pileft";
};
/* PORT 3 */
port@2 {
reg = <2>;
label = "piright";
};
/*
* PORT 4-7 unused
*/
};
};
tpm: tpm@1 {
compatible = "infineon,slb9670";
pinctrl-names = "default";
pinctrl-0 = <&tpm_pins>;
reg = <1>;
spi-max-frequency = <1000000>;
interrupt-parent = <&gpio>;
#interrupt-cells = <2>;
interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
status = "okay";
};
};
};
But probably this does not matter any more now
that Vladimir was able to reproduce the issue.
I was just thinking that maybe the Ethernet interface gets opened at
boot, and overlay is loaded, and the interface is opened a second
time. I don't know of anybody using DSA with overlays, so that could
of been the key difference which breaks it for you.
Your decompiled DT blob looks O.K.
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-09-09 17:55:16
On Thu, Sep 09, 2021 at 06:46:49PM +0200, Lino Sanfilippo wrote:
On 09.09.21 at 17:11, Andrew Lunn wrote:
quoted
quoted
Andrew: the switch is not on a hat, the device tree part I use is:
And this is not an overlay. It is all there at boot?
Well actually we DO use an overlay. The dev tree snipped I posted was an excerpt form
fdtdump. The concerning fragment looks like this in the overlay file:
Thanks for the information. Good to know somebody is using DSA like
this. The device tree description can be quite complex, especially for
some of the other switches.
But probably this does not matter any more now that Vladimir was
able to reproduce the issue.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-09-09 15:47:41
On Thu, Sep 09, 2021 at 03:19:52PM +0200, Lino Sanfilippo wrote:
quoted
Do you see similar things on your 5.10 kernel?
For the master device is see
lrwxrwxrwx 1 root root 0 Sep 9 14:10 /sys/class/net/eth0/device/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.ethernet--spi:spi3.0
So this is the worst of the worst, we have a device link but it doesn't help.
Where the device link helps is here:
__device_release_driver
while (device_links_busy(dev))
device_links_unbind_consumers(dev);
but during dev_shutdown, device_links_unbind_consumers does not get called
(actually I am not even sure whether it should).
I've reproduced your issue by making this very simple change:
on my DSA master driver. This is what the genet driver has "special".
I was led into grave error by Documentation/driver-api/device_link.rst,
which I've based my patch on, where it clearly says that device links
are supposed to help with shutdown ordering (how?!).
So the question is, why did my DSA trees get torn down on shutdown?
Basically the short answer is that my SPI controller driver does
implement .shutdown, and calls the same code path as the .remove code,
which calls spi_unregister_controller which removes all SPI children..
When I added this device link, one of the main objectives was to not
modify all DSA drivers. I was certain based on the documentation that
device links would help, now I'm not so sure anymore.
So what happens is that the DSA master attempts to unregister its net
device on .shutdown, but DSA does not implement .shutdown, so it just
sits there holding a reference (supposedly via dev_hold, but where from?!)
to the master, which makes netdev_wait_allrefs to wait and wait.
I need more time for the denial phase to pass, and to understand what
can actually be done. I will also be away from the keyboard for the next
few days, so it might take a while. Your patches obviously offer a
solution only for KSZ switches, we need something more general. If I
understand your solution, it works not by virtue of there being any
shutdown ordering guarantee at all, but simply due to the fact that
DSA's .shutdown hook gets called eventually, and the reference to the
master gets freed eventually, which unblocks the unregister_netdevice
call from the master. I don't yet understand why DSA holds a long-term
reference to the master, that's one thing I need to figure out.
+Saravana,
On 9/9/2021 8:47 AM, Vladimir Oltean wrote:
quoted hunk
On Thu, Sep 09, 2021 at 03:19:52PM +0200, Lino Sanfilippo wrote:
quoted
quoted
Do you see similar things on your 5.10 kernel?
For the master device is see
lrwxrwxrwx 1 root root 0 Sep 9 14:10 /sys/class/net/eth0/device/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.ethernet--spi:spi3.0
So this is the worst of the worst, we have a device link but it doesn't help.
Where the device link helps is here:
__device_release_driver
while (device_links_busy(dev))
device_links_unbind_consumers(dev);
but during dev_shutdown, device_links_unbind_consumers does not get called
(actually I am not even sure whether it should).
I've reproduced your issue by making this very simple change:
on my DSA master driver. This is what the genet driver has "special".
I was led into grave error by Documentation/driver-api/device_link.rst,
which I've based my patch on, where it clearly says that device links
are supposed to help with shutdown ordering (how?!).
I was also under the impression that device links were supposed to help
with shutdown ordering, because it does matter a lot. One thing that I
had to work before (and seems like it came back recently) is the
shutdown ordering between gpio_keys.c and the GPIO controller. If you
suspend the GPIO controller first, gpio_keys.c never gets a chance to
keep the GPIO pin configured for a wake-up interrupt, therefore no
wake-up event happens on key presses, whoops.
So the question is, why did my DSA trees get torn down on shutdown?
Basically the short answer is that my SPI controller driver does
implement .shutdown, and calls the same code path as the .remove code,
which calls spi_unregister_controller which removes all SPI children..
When I added this device link, one of the main objectives was to not
modify all DSA drivers. I was certain based on the documentation that
device links would help, now I'm not so sure anymore.
So what happens is that the DSA master attempts to unregister its net
device on .shutdown, but DSA does not implement .shutdown, so it just
sits there holding a reference (supposedly via dev_hold, but where from?!)
to the master, which makes netdev_wait_allrefs to wait and wait.
It's not coming from of_find_net_device_by_node() that's for sure and
with OF we don't go through the code path calling
dsa_dev_to_net_device() which does call dev_hold() and then shortly
thereafter the caller calls dev_put() anyway.
I need more time for the denial phase to pass, and to understand what
can actually be done. I will also be away from the keyboard for the next
few days, so it might take a while. Your patches obviously offer a
solution only for KSZ switches, we need something more general. If I
understand your solution, it works not by virtue of there being any
shutdown ordering guarantee at all, but simply due to the fact that
DSA's .shutdown hook gets called eventually, and the reference to the
master gets freed eventually, which unblocks the unregister_netdevice
call from the master. I don't yet understand why DSA holds a long-term
reference to the master, that's one thing I need to figure out.
On Thu, Sep 9, 2021 at 9:00 AM Florian Fainelli [off-list ref] wrote:
+Saravana,
On 9/9/2021 8:47 AM, Vladimir Oltean wrote:
quoted
On Thu, Sep 09, 2021 at 03:19:52PM +0200, Lino Sanfilippo wrote:
quoted
quoted
Do you see similar things on your 5.10 kernel?
For the master device is see
lrwxrwxrwx 1 root root 0 Sep 9 14:10 /sys/class/net/eth0/device/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.ethernet--spi:spi3.0
So this is the worst of the worst, we have a device link but it doesn't help.
Where the device link helps is here:
__device_release_driver
while (device_links_busy(dev))
device_links_unbind_consumers(dev);
but during dev_shutdown, device_links_unbind_consumers does not get called
(actually I am not even sure whether it should).
I've reproduced your issue by making this very simple change:
on my DSA master driver. This is what the genet driver has "special".
I was led into grave error by Documentation/driver-api/device_link.rst,
which I've based my patch on, where it clearly says that device links
are supposed to help with shutdown ordering (how?!).
I was also under the impression that device links were supposed to help
with shutdown ordering, because it does matter a lot. One thing that I
had to work before (and seems like it came back recently) is the
shutdown ordering between gpio_keys.c and the GPIO controller. If you
suspend the GPIO controller first, gpio_keys.c never gets a chance to
keep the GPIO pin configured for a wake-up interrupt, therefore no
wake-up event happens on key presses, whoops.
This is more of a Rafael question. Adding him. I haven't looked too
closely at device links and shutdown.
-Saravana
quoted
So the question is, why did my DSA trees get torn down on shutdown?
Basically the short answer is that my SPI controller driver does
implement .shutdown, and calls the same code path as the .remove code,
which calls spi_unregister_controller which removes all SPI children..
When I added this device link, one of the main objectives was to not
modify all DSA drivers. I was certain based on the documentation that
device links would help, now I'm not so sure anymore.
So what happens is that the DSA master attempts to unregister its net
device on .shutdown, but DSA does not implement .shutdown, so it just
sits there holding a reference (supposedly via dev_hold, but where from?!)
to the master, which makes netdev_wait_allrefs to wait and wait.
It's not coming from of_find_net_device_by_node() that's for sure and
with OF we don't go through the code path calling
dsa_dev_to_net_device() which does call dev_hold() and then shortly
thereafter the caller calls dev_put() anyway.
quoted
I need more time for the denial phase to pass, and to understand what
can actually be done. I will also be away from the keyboard for the next
few days, so it might take a while. Your patches obviously offer a
solution only for KSZ switches, we need something more general. If I
understand your solution, it works not by virtue of there being any
shutdown ordering guarantee at all, but simply due to the fact that
DSA's .shutdown hook gets called eventually, and the reference to the
master gets freed eventually, which unblocks the unregister_netdevice
call from the master. I don't yet understand why DSA holds a long-term
reference to the master, that's one thing I need to figure out.
On Thu, Sep 09, 2021 at 03:19:52PM +0200, Lino Sanfilippo wrote:
quoted
quoted
Do you see similar things on your 5.10 kernel?
For the master device is see
lrwxrwxrwx 1 root root 0 Sep 9 14:10 /sys/class/net/eth0/device/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.ethernet--spi:spi3.0
So this is the worst of the worst, we have a device link but it doesn't help.
Where the device link helps is here:
__device_release_driver
while (device_links_busy(dev))
device_links_unbind_consumers(dev);
but during dev_shutdown, device_links_unbind_consumers does not get called
(actually I am not even sure whether it should).
I've reproduced your issue by making this very simple change:
on my DSA master driver. This is what the genet driver has "special".
Ah, that is interesting.
I was led into grave error by Documentation/driver-api/device_link.rst,
which I've based my patch on, where it clearly says that device links
are supposed to help with shutdown ordering (how?!).
So the question is, why did my DSA trees get torn down on shutdown?
Basically the short answer is that my SPI controller driver does
implement .shutdown, and calls the same code path as the .remove code,
which calls spi_unregister_controller which removes all SPI children..
When I added this device link, one of the main objectives was to not
modify all DSA drivers. I was certain based on the documentation that
device links would help, now I'm not so sure anymore.
So what happens is that the DSA master attempts to unregister its net
device on .shutdown, but DSA does not implement .shutdown, so it just
sits there holding a reference (supposedly via dev_hold, but where from?!)
to the master, which makes netdev_wait_allrefs to wait and wait.
Right, that was also my conclusion.
I need more time for the denial phase to pass, and to understand what
can actually be done. I will also be away from the keyboard for the next
few days, so it might take a while. Your patches obviously offer a
solution only for KSZ switches, we need something more general. If I
understand your solution, it works not by virtue of there being any
shutdown ordering guarantee at all, but simply due to the fact that
DSA's .shutdown hook gets called eventually, and the reference to the
master gets freed eventually, which unblocks the unregister_netdevice
call from the master.
Well actually the SPI shutdown hook gets called which then calls ksz9477_shutdown
(formerly ksz9477_reset_switch) which then shuts down the switch by
stopping the worker thread and tearing down the DSA tree (via dsa_tree_shutdown()).
While it is right that the patch series only fixes the KSZ case for now, the idea was that
other drivers could use a similar approach in by calling the new function dsa_tree_shutdown()
in their shutdown handler to make sure that all refs to the master device are released.
Regards,
Lino
On Thu, Sep 09, 2021 at 03:19:52PM +0200, Lino Sanfilippo wrote:
quoted
quoted
Do you see similar things on your 5.10 kernel?
For the master device is see
lrwxrwxrwx 1 root root 0 Sep 9 14:10 /sys/class/net/eth0/device/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.ethernet--spi:spi3.0
So this is the worst of the worst, we have a device link but it doesn't help.
Where the device link helps is here:
__device_release_driver
while (device_links_busy(dev))
device_links_unbind_consumers(dev);
but during dev_shutdown, device_links_unbind_consumers does not get called
(actually I am not even sure whether it should).
I've reproduced your issue by making this very simple change:
on my DSA master driver. This is what the genet driver has "special".
Ah, that is interesting.
quoted
I was led into grave error by Documentation/driver-api/device_link.rst,
which I've based my patch on, where it clearly says that device links
are supposed to help with shutdown ordering (how?!).
So the question is, why did my DSA trees get torn down on shutdown?
Basically the short answer is that my SPI controller driver does
implement .shutdown, and calls the same code path as the .remove code,
which calls spi_unregister_controller which removes all SPI children..
When I added this device link, one of the main objectives was to not
modify all DSA drivers. I was certain based on the documentation that
device links would help, now I'm not so sure anymore.
So what happens is that the DSA master attempts to unregister its net
device on .shutdown, but DSA does not implement .shutdown, so it just
sits there holding a reference (supposedly via dev_hold, but where from?!)
to the master, which makes netdev_wait_allrefs to wait and wait.
Right, that was also my conclusion.
quoted
I need more time for the denial phase to pass, and to understand what
can actually be done. I will also be away from the keyboard for the next
few days, so it might take a while. Your patches obviously offer a
solution only for KSZ switches, we need something more general. If I
understand your solution, it works not by virtue of there being any
shutdown ordering guarantee at all, but simply due to the fact that
DSA's .shutdown hook gets called eventually, and the reference to the
master gets freed eventually, which unblocks the unregister_netdevice
call from the master.
Well actually the SPI shutdown hook gets called which then calls ksz9477_shutdown
(formerly ksz9477_reset_switch) which then shuts down the switch by
stopping the worker thread and tearing down the DSA tree (via dsa_tree_shutdown()).
While it is right that the patch series only fixes the KSZ case for now, the idea was that
other drivers could use a similar approach in by calling the new function dsa_tree_shutdown()
in their shutdown handler to make sure that all refs to the master device are released.
It does not scale really well to have individual drivers call
dsa_tree_shutdown() in their respective .shutdown callback, and in a
multi-switch configuration, I am not sure what the results would look like.
In premise, each driver ought to be able to call
dsa_unregister_switch(), along with all of the driver specific shutdown
and eventually, given proper device ordering the DSA tree would get
automatically torn down, and then the DSA master's .shutdown() callback
would be called.
FWIW, the reason why we call .shutdown() in bcmgenet is to turn off DMA
and clocks, which matters for kexec (DMA) as well as power savings (S5
mode).
--
Florian
On Thu, Sep 09, 2021 at 03:19:52PM +0200, Lino Sanfilippo wrote:
quoted
quoted
Do you see similar things on your 5.10 kernel?
For the master device is see
lrwxrwxrwx 1 root root 0 Sep 9 14:10 /sys/class/net/eth0/device/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.ethernet--spi:spi3.0
So this is the worst of the worst, we have a device link but it doesn't help.
Where the device link helps is here:
__device_release_driver
while (device_links_busy(dev))
device_links_unbind_consumers(dev);
but during dev_shutdown, device_links_unbind_consumers does not get called
(actually I am not even sure whether it should).
I've reproduced your issue by making this very simple change:
.id_table = enetc_pf_id_table,
.probe = enetc_pf_probe,
.remove = enetc_pf_remove,
+ .shutdown = enetc_pf_remove,
#ifdef CONFIG_PCI_IOV
.sriov_configure = enetc_sriov_configure,
#endif
on my DSA master driver. This is what the genet driver has "special".
Ah, that is interesting.
quoted
I was led into grave error by Documentation/driver-api/device_link.rst,
which I've based my patch on, where it clearly says that device links
are supposed to help with shutdown ordering (how?!).
So the question is, why did my DSA trees get torn down on shutdown?
Basically the short answer is that my SPI controller driver does
implement .shutdown, and calls the same code path as the .remove code,
which calls spi_unregister_controller which removes all SPI children..
When I added this device link, one of the main objectives was to not
modify all DSA drivers. I was certain based on the documentation that
device links would help, now I'm not so sure anymore.
So what happens is that the DSA master attempts to unregister its net
device on .shutdown, but DSA does not implement .shutdown, so it just
sits there holding a reference (supposedly via dev_hold, but where from?!)
to the master, which makes netdev_wait_allrefs to wait and wait.
Right, that was also my conclusion.
quoted
I need more time for the denial phase to pass, and to understand what
can actually be done. I will also be away from the keyboard for the next
few days, so it might take a while. Your patches obviously offer a
solution only for KSZ switches, we need something more general. If I
understand your solution, it works not by virtue of there being any
shutdown ordering guarantee at all, but simply due to the fact that
DSA's .shutdown hook gets called eventually, and the reference to the
master gets freed eventually, which unblocks the unregister_netdevice
call from the master.
Well actually the SPI shutdown hook gets called which then calls ksz9477_shutdown
(formerly ksz9477_reset_switch) which then shuts down the switch by
stopping the worker thread and tearing down the DSA tree (via dsa_tree_shutdown()).
While it is right that the patch series only fixes the KSZ case for now, the idea was that
other drivers could use a similar approach in by calling the new function dsa_tree_shutdown()
in their shutdown handler to make sure that all refs to the master device are released.
It does not scale really well to have individual drivers call dsa_tree_shutdown() in their respective .shutdown callback, and in a multi-switch configuration, I am not sure what the results would look like.
In premise, each driver ought to be able to call dsa_unregister_switch(), along with all of the driver specific shutdown and eventually, given proper device ordering the DSA tree would get automatically torn down, and then the DSA master's .shutdown() callback would be called.
FWIW, the reason why we call .shutdown() in bcmgenet is to turn off DMA and clocks, which matters for kexec (DMA) as well as power savings (S5 mode).
I agree with the scalability. Concerning the multi-switch case I dont know about the possible issues (I am quite new to working with DSA).
So lets wait for Vladimirs solution.
Regards,
Lino
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-09-09 22:55:06
On Thu, Sep 09, 2021 at 07:07:33PM +0200, Lino Sanfilippo wrote:
quoted
It does not scale really well to have individual drivers call
dsa_tree_shutdown() in their respective .shutdown callback, and in a
multi-switch configuration, I am not sure what the results would
look like.
In premise, each driver ought to be able to call
dsa_unregister_switch(), along with all of the driver specific
shutdown and eventually, given proper device ordering the DSA tree
would get automatically torn down, and then the DSA master's
.shutdown() callback would be called.
FWIW, the reason why we call .shutdown() in bcmgenet is to turn off
DMA and clocks, which matters for kexec (DMA) as well as power
savings (S5 mode).
I agree with the scalability. Concerning the multi-switch case I dont
know about the possible issues (I am quite new to working with DSA).
So lets wait for Vladimirs solution.
I'm back for now and was able to spend a bit more time and understand
what is happening.
So first things first: why does DSA call dev_hold long-term on the
master, and where from?
Answer: it does so since commit 2f1e8ea726e9 ("net: dsa: link interfaces
with the DSA master to get rid of lockdep warnings"), see this call path:
dsa_slave_create
-> netdev_upper_dev_link
-> __netdev_upper_dev_link
-> __netdev_adjacent_dev_insert
-> dev_hold
Ok, so since DSA holds a reference to the master interface, it is
natural that unregister_netdevice() will not finish, and it will hang
the system.
Question 2: why does bcmgenet need to unregister the net device on
shutdown?
See Florian's answer, it doesn't, strictly speaking, it just needs to
turn off the DMA and some clocks.
Question 3: can we revert commit 2f1e8ea726e9?
Answer: not so easily, we are looking at >10 commits to revert, and find
other solutions to some problems. We have built in the meantime on top
of the fact that there is an upper/lower relationship between DSA user
ports and the DSA master.
Question 4: how do other stacked interfaces deal with this?
Answer: as I said in the commit message of 2f1e8ea726e9, DSA is not
VLAN, DSA has unique challenges of its own, like a tree of struct
devices to manage, with their own lifetime. So what other drivers do is
not really relevant. Anyway, to entertain the question: VLAN watches the
NETDEV_UNREGISTER event emitted on the netdev notifier chain for its
real_dev, and effectively unregisters itself. Now this is exactly why it
is irrelevant, we can watch for NETDEV_UNREGISTER on the DSA master, but
then what? There is nothing sensible to do. Consider that in the master
unbind case (not shutdown), both the NETDEV_UNREGISTER code path will
execute, and the unbind of the DSA switch itself, due to that device
link. But let's say we delete the device link and leave only the
NETDEV_UNREGISTER code path to do something. What?
device_release_driver(ds->dev), most probably. That would effectively
force the DSA unbind path. But surprise: the DSA unbind path takes the
rtnl_mutex from quite a couple of places, and we are already under the
rtnl_lock (held by the netdev notifier chain). So, unless we schedule
the DSA device driver detach, there is an impending deadlock.
Ok, let's entertain even that: detach the DSA driver in a scheduled work
item, with the rtnl_lock not held. First off, we will trigger again the
WARN_ON solved by commit 2f1e8ea726e9 (because the unregistering of the
DSA master has "completed", but it still has an upper interface - us),
and secondly, the unregister_netdev function will have already deleted
stuff belonging to the DSA master, namely its sysfs entries. But DSA
also touches the master's sysfs, namely the "tagging" file. So NULL
pointer dereference on the master's sysfs.
So very simply put, DSA cannot unbind itself from the switch device when
the master net device unregisters. The best case scenario would be for
DSA to unbind _before_ the net device even unregisters. That was the
whole point of my attempt with the device links, to ensure shutdown
_ordering_.
Question 5: can the device core actually be patched to call
device_links_unbind_consumers() from device_shutdown()? This would
actually simplify DSA's options, and make the device links live up to
their documented expectations.
Answer: yes and no, technically it can, but it is an invasive change
which will certainly introduce regressions. See the answer to question 2
for an example. Technically .shutdown exists so that drivers can do
something lightweight to quiesce the hardware, without really caring too
much about data structure integrity (hey, the kernel is going to die
soon anyway). But some drivers, like bcmgenet, do the same thing in
.resume and .shutdown, which blurs the lines quite a lot. If the device
links were to start calling .remove at shutdown time, potentially after
.shutdown was already called, bcmgenet would effectively unregister its
net device twice. Yikes.
Question 6: How about a patch on the device core that is more lightweight?
Wouldn't it be sensible for device_shutdown() to just call ->remove if
the device's bus has no ->shutdown, and the device's driver doesn't have
a ->shutdown either?
Answer: This would sometimes work, the vast majority of DSA switch
drivers, and Ethernet controllers (in this case used as DSA masters) do
not have a .shutdown method implemented. But their bus does: PCI does,
SPI controllers do, most of the time. So it would work for limited
scenarios, but would be ineffective in the general sense.
Question 7: I said that .shutdown, as opposed to .remove, doesn't really
care so much about the integrity of data structures. So how far should
we really go to fix this issue? Should we even bother to unbind the
whole DSA tree, when the sole problem is that we are the DSA master's
upper, and that is keeping a reference on it?
Answer: Well, any solution that does unnecessary data structure teardown
only delays the reboot for nothing. Lino's patch just bluntly calls
dsa_tree_teardown() from the switch .shutdown method, and this leaks
memory, namely dst->ports. But does this really matter? Nope, so let's
extrapolate. In this case, IMO, the simplest possible solution would be
to patch bcmgenet to not unregister the net device. Then treat every
other DSA master driver in the same way as they come, one by one.
Do you need to unregister_netdevice() at shutdown? No. Then don't.
Is it nice? Probably not, but I'm not seeing alternatives.
Also, unless I'm missing something, Lino probably still sees the WARN_ON
in bcmgenet's unregister_netdevice() about eth0 getting unregistered
while having an upper interface. If not, it's by sheer luck that the DSA
switch's ->shutdown gets called before bcmgenet's ->shutdown. But for
this reason, it isn't a great solution either. If the device links can't
guarantee us some sort of shutdown ordering (what we ideally want, as
mentioned, is for the DSA switch driver to get _unbound_ (->remove)
before the DSA master gets unbound or shut down).
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-09-09 23:24:09
On Fri, Sep 10, 2021 at 01:54:57AM +0300, Vladimir Oltean wrote:
On Thu, Sep 09, 2021 at 07:07:33PM +0200, Lino Sanfilippo wrote:
quoted
quoted
It does not scale really well to have individual drivers call
dsa_tree_shutdown() in their respective .shutdown callback, and in a
multi-switch configuration, I am not sure what the results would
look like.
In premise, each driver ought to be able to call
dsa_unregister_switch(), along with all of the driver specific
shutdown and eventually, given proper device ordering the DSA tree
would get automatically torn down, and then the DSA master's
.shutdown() callback would be called.
FWIW, the reason why we call .shutdown() in bcmgenet is to turn off
DMA and clocks, which matters for kexec (DMA) as well as power
savings (S5 mode).
I agree with the scalability. Concerning the multi-switch case I dont
know about the possible issues (I am quite new to working with DSA).
So lets wait for Vladimirs solution.
I'm back for now and was able to spend a bit more time and understand
what is happening.
So first things first: why does DSA call dev_hold long-term on the
master, and where from?
Answer: it does so since commit 2f1e8ea726e9 ("net: dsa: link interfaces
with the DSA master to get rid of lockdep warnings"), see this call path:
dsa_slave_create
-> netdev_upper_dev_link
-> __netdev_upper_dev_link
-> __netdev_adjacent_dev_insert
-> dev_hold
Ok, so since DSA holds a reference to the master interface, it is
natural that unregister_netdevice() will not finish, and it will hang
the system.
Question 2: why does bcmgenet need to unregister the net device on
shutdown?
See Florian's answer, it doesn't, strictly speaking, it just needs to
turn off the DMA and some clocks.
Question 3: can we revert commit 2f1e8ea726e9?
Answer: not so easily, we are looking at >10 commits to revert, and find
other solutions to some problems. We have built in the meantime on top
of the fact that there is an upper/lower relationship between DSA user
ports and the DSA master.
Question 4: how do other stacked interfaces deal with this?
Answer: as I said in the commit message of 2f1e8ea726e9, DSA is not
VLAN, DSA has unique challenges of its own, like a tree of struct
devices to manage, with their own lifetime. So what other drivers do is
not really relevant. Anyway, to entertain the question: VLAN watches the
NETDEV_UNREGISTER event emitted on the netdev notifier chain for its
real_dev, and effectively unregisters itself. Now this is exactly why it
is irrelevant, we can watch for NETDEV_UNREGISTER on the DSA master, but
then what? There is nothing sensible to do. Consider that in the master
unbind case (not shutdown), both the NETDEV_UNREGISTER code path will
execute, and the unbind of the DSA switch itself, due to that device
link. But let's say we delete the device link and leave only the
NETDEV_UNREGISTER code path to do something. What?
device_release_driver(ds->dev), most probably. That would effectively
force the DSA unbind path. But surprise: the DSA unbind path takes the
rtnl_mutex from quite a couple of places, and we are already under the
rtnl_lock (held by the netdev notifier chain). So, unless we schedule
the DSA device driver detach, there is an impending deadlock.
Ok, let's entertain even that: detach the DSA driver in a scheduled work
item, with the rtnl_lock not held. First off, we will trigger again the
WARN_ON solved by commit 2f1e8ea726e9 (because the unregistering of the
DSA master has "completed", but it still has an upper interface - us),
and secondly, the unregister_netdev function will have already deleted
stuff belonging to the DSA master, namely its sysfs entries. But DSA
also touches the master's sysfs, namely the "tagging" file. So NULL
pointer dereference on the master's sysfs.
So very simply put, DSA cannot unbind itself from the switch device when
the master net device unregisters. The best case scenario would be for
DSA to unbind _before_ the net device even unregisters. That was the
whole point of my attempt with the device links, to ensure shutdown
_ordering_.
Question 5: can the device core actually be patched to call
device_links_unbind_consumers() from device_shutdown()? This would
actually simplify DSA's options, and make the device links live up to
their documented expectations.
Answer: yes and no, technically it can, but it is an invasive change
which will certainly introduce regressions. See the answer to question 2
for an example. Technically .shutdown exists so that drivers can do
something lightweight to quiesce the hardware, without really caring too
much about data structure integrity (hey, the kernel is going to die
soon anyway). But some drivers, like bcmgenet, do the same thing in
.resume and .shutdown, which blurs the lines quite a lot. If the device
links were to start calling .remove at shutdown time, potentially after
.shutdown was already called, bcmgenet would effectively unregister its
net device twice. Yikes.
Question 6: How about a patch on the device core that is more lightweight?
Wouldn't it be sensible for device_shutdown() to just call ->remove if
the device's bus has no ->shutdown, and the device's driver doesn't have
a ->shutdown either?
Answer: This would sometimes work, the vast majority of DSA switch
drivers, and Ethernet controllers (in this case used as DSA masters) do
not have a .shutdown method implemented. But their bus does: PCI does,
SPI controllers do, most of the time. So it would work for limited
scenarios, but would be ineffective in the general sense.
Question 7: I said that .shutdown, as opposed to .remove, doesn't really
care so much about the integrity of data structures. So how far should
we really go to fix this issue? Should we even bother to unbind the
whole DSA tree, when the sole problem is that we are the DSA master's
upper, and that is keeping a reference on it?
Answer: Well, any solution that does unnecessary data structure teardown
only delays the reboot for nothing. Lino's patch just bluntly calls
dsa_tree_teardown() from the switch .shutdown method, and this leaks
memory, namely dst->ports. But does this really matter? Nope, so let's
extrapolate. In this case, IMO, the simplest possible solution would be
to patch bcmgenet to not unregister the net device. Then treat every
other DSA master driver in the same way as they come, one by one.
Do you need to unregister_netdevice() at shutdown? No. Then don't.
Is it nice? Probably not, but I'm not seeing alternatives.
Also, unless I'm missing something, Lino probably still sees the WARN_ON
in bcmgenet's unregister_netdevice() about eth0 getting unregistered
while having an upper interface. If not, it's by sheer luck that the DSA
switch's ->shutdown gets called before bcmgenet's ->shutdown. But for
this reason, it isn't a great solution either. If the device links can't
guarantee us some sort of shutdown ordering (what we ideally want, as
mentioned, is for the DSA switch driver to get _unbound_ (->remove)
before the DSA master gets unbound or shut down).
I forgot about this, for completeness:
Question 8: Ok, so this is an even more lightweight variant of question 6.
To patch device_shutdown here:
if (dev->bus && dev->bus->shutdown) {
if (initcall_debug)
dev_info(dev, "shutdown\n");
dev->bus->shutdown(dev);
} else if (dev->driver && dev->driver->shutdown) {
if (initcall_debug)
dev_info(dev, "shutdown\n");
dev->driver->shutdown(dev);
+ } else {
+ __device_release_driver(dev, parent);
}
would go towards helping DSA in general, but it wouldn't help the situation at hand,
and it would introduce regressions.
So what about patching bcmgenet (and other drivers) to implement .shutdown in the following way:
device_release_driver(&pdev->dev);
basically this should force-unbind the driver from the device, which
would quite nicely make the device link go into action and make DSA
unbind too.
Answer: device_release_driver calls device_lock(dev), and device_shutdown
also holds that lock when it calls our ->shutdown method. So unless the
device core would be so nice so as to provide a generic shutdown method
that just unbinds the device using an unlocked version of device_release_driver,
we are back to square one with this solution. Anything that needs to patch
the device core is more or less disqualified, especially for a bug fix.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-09-10 01:21:27
On Fri, Sep 10, 2021 at 02:23:58AM +0300, Vladimir Oltean wrote:
On Fri, Sep 10, 2021 at 01:54:57AM +0300, Vladimir Oltean wrote:
quoted
On Thu, Sep 09, 2021 at 07:07:33PM +0200, Lino Sanfilippo wrote:
quoted
quoted
It does not scale really well to have individual drivers call
dsa_tree_shutdown() in their respective .shutdown callback, and in a
multi-switch configuration, I am not sure what the results would
look like.
In premise, each driver ought to be able to call
dsa_unregister_switch(), along with all of the driver specific
shutdown and eventually, given proper device ordering the DSA tree
would get automatically torn down, and then the DSA master's
.shutdown() callback would be called.
FWIW, the reason why we call .shutdown() in bcmgenet is to turn off
DMA and clocks, which matters for kexec (DMA) as well as power
savings (S5 mode).
I agree with the scalability. Concerning the multi-switch case I dont
know about the possible issues (I am quite new to working with DSA).
So lets wait for Vladimirs solution.
I'm back for now and was able to spend a bit more time and understand
what is happening.
So first things first: why does DSA call dev_hold long-term on the
master, and where from?
Answer: it does so since commit 2f1e8ea726e9 ("net: dsa: link interfaces
with the DSA master to get rid of lockdep warnings"), see this call path:
dsa_slave_create
-> netdev_upper_dev_link
-> __netdev_upper_dev_link
-> __netdev_adjacent_dev_insert
-> dev_hold
Ok, so since DSA holds a reference to the master interface, it is
natural that unregister_netdevice() will not finish, and it will hang
the system.
Question 2: why does bcmgenet need to unregister the net device on
shutdown?
See Florian's answer, it doesn't, strictly speaking, it just needs to
turn off the DMA and some clocks.
Question 3: can we revert commit 2f1e8ea726e9?
Answer: not so easily, we are looking at >10 commits to revert, and find
other solutions to some problems. We have built in the meantime on top
of the fact that there is an upper/lower relationship between DSA user
ports and the DSA master.
Question 4: how do other stacked interfaces deal with this?
Answer: as I said in the commit message of 2f1e8ea726e9, DSA is not
VLAN, DSA has unique challenges of its own, like a tree of struct
devices to manage, with their own lifetime. So what other drivers do is
not really relevant. Anyway, to entertain the question: VLAN watches the
NETDEV_UNREGISTER event emitted on the netdev notifier chain for its
real_dev, and effectively unregisters itself. Now this is exactly why it
is irrelevant, we can watch for NETDEV_UNREGISTER on the DSA master, but
then what? There is nothing sensible to do. Consider that in the master
unbind case (not shutdown), both the NETDEV_UNREGISTER code path will
execute, and the unbind of the DSA switch itself, due to that device
link. But let's say we delete the device link and leave only the
NETDEV_UNREGISTER code path to do something. What?
device_release_driver(ds->dev), most probably. That would effectively
force the DSA unbind path. But surprise: the DSA unbind path takes the
rtnl_mutex from quite a couple of places, and we are already under the
rtnl_lock (held by the netdev notifier chain). So, unless we schedule
the DSA device driver detach, there is an impending deadlock.
Ok, let's entertain even that: detach the DSA driver in a scheduled work
item, with the rtnl_lock not held. First off, we will trigger again the
WARN_ON solved by commit 2f1e8ea726e9 (because the unregistering of the
DSA master has "completed", but it still has an upper interface - us),
and secondly, the unregister_netdev function will have already deleted
stuff belonging to the DSA master, namely its sysfs entries. But DSA
also touches the master's sysfs, namely the "tagging" file. So NULL
pointer dereference on the master's sysfs.
So very simply put, DSA cannot unbind itself from the switch device when
the master net device unregisters. The best case scenario would be for
DSA to unbind _before_ the net device even unregisters. That was the
whole point of my attempt with the device links, to ensure shutdown
_ordering_.
Question 5: can the device core actually be patched to call
device_links_unbind_consumers() from device_shutdown()? This would
actually simplify DSA's options, and make the device links live up to
their documented expectations.
Answer: yes and no, technically it can, but it is an invasive change
which will certainly introduce regressions. See the answer to question 2
for an example. Technically .shutdown exists so that drivers can do
something lightweight to quiesce the hardware, without really caring too
much about data structure integrity (hey, the kernel is going to die
soon anyway). But some drivers, like bcmgenet, do the same thing in
.resume and .shutdown, which blurs the lines quite a lot. If the device
links were to start calling .remove at shutdown time, potentially after
.shutdown was already called, bcmgenet would effectively unregister its
net device twice. Yikes.
Question 6: How about a patch on the device core that is more lightweight?
Wouldn't it be sensible for device_shutdown() to just call ->remove if
the device's bus has no ->shutdown, and the device's driver doesn't have
a ->shutdown either?
Answer: This would sometimes work, the vast majority of DSA switch
drivers, and Ethernet controllers (in this case used as DSA masters) do
not have a .shutdown method implemented. But their bus does: PCI does,
SPI controllers do, most of the time. So it would work for limited
scenarios, but would be ineffective in the general sense.
Question 7: I said that .shutdown, as opposed to .remove, doesn't really
care so much about the integrity of data structures. So how far should
we really go to fix this issue? Should we even bother to unbind the
whole DSA tree, when the sole problem is that we are the DSA master's
upper, and that is keeping a reference on it?
Answer: Well, any solution that does unnecessary data structure teardown
only delays the reboot for nothing. Lino's patch just bluntly calls
dsa_tree_teardown() from the switch .shutdown method, and this leaks
memory, namely dst->ports. But does this really matter? Nope, so let's
extrapolate. In this case, IMO, the simplest possible solution would be
to patch bcmgenet to not unregister the net device. Then treat every
other DSA master driver in the same way as they come, one by one.
Do you need to unregister_netdevice() at shutdown? No. Then don't.
Is it nice? Probably not, but I'm not seeing alternatives.
Also, unless I'm missing something, Lino probably still sees the WARN_ON
in bcmgenet's unregister_netdevice() about eth0 getting unregistered
while having an upper interface. If not, it's by sheer luck that the DSA
switch's ->shutdown gets called before bcmgenet's ->shutdown. But for
this reason, it isn't a great solution either. If the device links can't
guarantee us some sort of shutdown ordering (what we ideally want, as
mentioned, is for the DSA switch driver to get _unbound_ (->remove)
before the DSA master gets unbound or shut down).
I forgot about this, for completeness:
Question 8: Ok, so this is an even more lightweight variant of question 6.
To patch device_shutdown here:
if (dev->bus && dev->bus->shutdown) {
if (initcall_debug)
dev_info(dev, "shutdown\n");
dev->bus->shutdown(dev);
} else if (dev->driver && dev->driver->shutdown) {
if (initcall_debug)
dev_info(dev, "shutdown\n");
dev->driver->shutdown(dev);
+ } else {
+ __device_release_driver(dev, parent);
}
would go towards helping DSA in general, but it wouldn't help the situation at hand,
and it would introduce regressions.
So what about patching bcmgenet (and other drivers) to implement .shutdown in the following way:
device_release_driver(&pdev->dev);
basically this should force-unbind the driver from the device, which
would quite nicely make the device link go into action and make DSA
unbind too.
Answer: device_release_driver calls device_lock(dev), and device_shutdown
also holds that lock when it calls our ->shutdown method. So unless the
device core would be so nice so as to provide a generic shutdown method
that just unbinds the device using an unlocked version of device_release_driver,
we are back to square one with this solution. Anything that needs to patch
the device core is more or less disqualified, especially for a bug fix.
Question 9: can Lino's patch set be generalized to all DSA switches,
i.e. can all DSA drivers redirect their ->shutdown method to their
->remove method?
Answer: Apart from the fact that mdio_driver_register() does not provide
for a ->shutdown() method, which can be trivially addressed, we still
have issues. The fundamental problem is that some bus device drivers
implement ->shutdown as ->remove for their own device, see dspi_shutdown()
and dspi_remove() for an example.
When that happens, the DSA switch device attached to that bus will be
(a) once unbound from its driver (by dspi_shutdown -> dspi_remove ->
spi_unregister_controller -> device_for_each_child(...unregister), and
(b) once shut down (by its own ->shutdown method).
With the "ds" structure being naturally destroyed by the first call to
the ->remove function, a second call to ->remove is impossible to
succeed without tripping some NULL pointer, from the device's ->shutdown path.
Simply said, DSA is not designed to support an unbalanced number of
calls to dsa_register_switch and dsa_unregister_switch.
In fact, if Lino's ksz9897 switch was attached to the dspi driver for a
SPI controller, even his own patches would not work and result in this
unbalance of calls that I mentioned earlier. If we're going to fix it,
let's think of something that covers all cases at least.
Simply put, it looks like we need some guidance (again) from driver core
maintainers as to what should a ->suspend method _not_ do. Specifically,
if it is okay to redirect ->suspend to ->remove. It looks like in the
case of buses doing that, and child devices doing that too, the results
are not quite sane. And maybe that would give us some clue as to what to
do about the genet driver which does the same thing.
On 9/9/2021 3:54 PM, Vladimir Oltean wrote:
[snip]
Question 6: How about a patch on the device core that is more lightweight?
Wouldn't it be sensible for device_shutdown() to just call ->remove if
the device's bus has no ->shutdown, and the device's driver doesn't have
a ->shutdown either?
Answer: This would sometimes work, the vast majority of DSA switch
drivers, and Ethernet controllers (in this case used as DSA masters) do
not have a .shutdown method implemented. But their bus does: PCI does,
SPI controllers do, most of the time. So it would work for limited
scenarios, but would be ineffective in the general sense.
Having wondered about that question as well, I don't really see a
compelling reason as to why we do not default to calling .remove() when
.shutdown() is not implemented. In almost all of the cases the semantics
of .remove() are superior to those required by .shutdown().
Question 7: I said that .shutdown, as opposed to .remove, doesn't really
care so much about the integrity of data structures. So how far should
we really go to fix this issue? Should we even bother to unbind the
whole DSA tree, when the sole problem is that we are the DSA master's
upper, and that is keeping a reference on it?
Answer: Well, any solution that does unnecessary data structure teardown
only delays the reboot for nothing. Lino's patch just bluntly calls
dsa_tree_teardown() from the switch .shutdown method, and this leaks
memory, namely dst->ports. But does this really matter? Nope, so let's
extrapolate. In this case, IMO, the simplest possible solution would be
to patch bcmgenet to not unregister the net device. Then treat every
other DSA master driver in the same way as they come, one by one.
Do you need to unregister_netdevice() at shutdown? No. Then don't.
Is it nice? Probably not, but I'm not seeing alternatives.
It does not really scale but we also don't have that many DSA masters to
support, I believe I can name them all: bcmgenet, stmmac, bcmsysport,
enetc, mv643xx_eth, cpsw, macb. If you want me to patch bcmgenet, give
me a few days to test and make sure there is no power management
regression, that's the primary concern I have.
Also, unless I'm missing something, Lino probably still sees the WARN_ON
in bcmgenet's unregister_netdevice() about eth0 getting unregistered
while having an upper interface. If not, it's by sheer luck that the DSA
switch's ->shutdown gets called before bcmgenet's ->shutdown. But for
this reason, it isn't a great solution either. If the device links can't
guarantee us some sort of shutdown ordering (what we ideally want, as
mentioned, is for the DSA switch driver to get _unbound_ (->remove)
before the DSA master gets unbound or shut down).
All of your questions are good and I don't have answers to any of them,
however I would like you and others to reason about .shutdown() not just
in the context of a reboot, or kexec'd kernel but also in the context of
putting the system into ACPI S5 (via poweroff). In that case the goal is
not only to quiesce the device, the goal is also to put it in a low
power mode.
For bcmgenet specifically the code path that leads to a driver remove is
well tested and is guaranteeing the network device registration, thus
putting the PHY into suspend, shutting down DMAs, turning off clocks.
This is a big hammer, but it gets the job done and does not introduce
yet another code path to test, it's the same as the module removal.
--
Florian
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-09-10 11:52:15
It does not really scale but we also don't have that many DSA masters to
support, I believe I can name them all: bcmgenet, stmmac, bcmsysport, enetc,
mv643xx_eth, cpsw, macb.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-09-10 14:59:00
On Fri, Sep 10, 2021 at 01:51:56PM +0200, Andrew Lunn wrote:
quoted
It does not really scale but we also don't have that many DSA masters to
support, I believe I can name them all: bcmgenet, stmmac, bcmsysport, enetc,
mv643xx_eth, cpsw, macb.
fec, mvneta, mvpp2, i210/igb.
I can probably double that list only with Freescale/NXP Ethernet
drivers, some of which are not even submitted to mainline. To name some
mainline drivers: gianfar, dpaa-eth, dpaa2-eth, dpaa2-switch, ucc_geth.
Also consider that DSA/switchdev drivers can also be DSA masters of
their own, we have boards doing that too.
Anyway, I've decided to at least try and accept the fact that DSA
masters will unregister their net_device on shutdown, and attempt to do
something sane for all DSA switches in that case.
Attached are two patches (they are fairly big so I won't paste them
inline, and I would like initial feedback before posting them to the
list).
As mentioned in those patches, the shutdown ordering guarantee is still
very important, I still have no clue what goes on there, what we need to
do, etc.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-09-11 11:44:38
On Fri, Sep 10, 2021 at 05:58:52PM +0300, Vladimir Oltean wrote:
On Fri, Sep 10, 2021 at 01:51:56PM +0200, Andrew Lunn wrote:
quoted
quoted
It does not really scale but we also don't have that many DSA masters to
support, I believe I can name them all: bcmgenet, stmmac, bcmsysport, enetc,
mv643xx_eth, cpsw, macb.
fec, mvneta, mvpp2, i210/igb.
I can probably double that list only with Freescale/NXP Ethernet
drivers, some of which are not even submitted to mainline. To name some
mainline drivers: gianfar, dpaa-eth, dpaa2-eth, dpaa2-switch, ucc_geth.
Also consider that DSA/switchdev drivers can also be DSA masters of
their own, we have boards doing that too.
Anyway, I've decided to at least try and accept the fact that DSA
masters will unregister their net_device on shutdown, and attempt to do
something sane for all DSA switches in that case.
Attached are two patches (they are fairly big so I won't paste them
inline, and I would like initial feedback before posting them to the
list).
As mentioned in those patches, the shutdown ordering guarantee is still
very important, I still have no clue what goes on there, what we need to
do, etc.
So to answer my own question, there is a comment above device_link_add:
* A side effect of the link creation is re-ordering of dpm_list and the
* devices_kset list by moving the consumer device and all devices depending
* on it to the ends of these lists (that does not happen to devices that have
* not been registered when this function is called).
so the fact that DSA uses device_link_add towards its master is not
exactly for nothing. device_shutdown() walks devices_kset from the back,
so this is our guarantee that DSA's shutdown happens before the master's
shutdown.
So these patches should be okay. Any other comments? If not, I will
formally submit them tomorrow towards the net tree.
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-09-09 12:55:13
On Thu, Sep 09, 2021 at 11:53:21AM +0200, Lino Sanfilippo wrote:
This patch series fixes a system hang I got each time i tried to shutdown
or reboot a system that uses a KSZ9897 as a DSA switch with a broadcom
GENET network device as the DSA master device. At the time the system hangs
the message "unregister_netdevice: waiting for eth0 to become free. Usage
count = 2." is dumped periodically to the console.
After some investigation I found the reason to be unreleased references to
the master device which are still held by the slave devices at the time the
system is shut down (I have two slave devices in use).
While these references are supposed to be released in ksz_switch_remove()
this function never gets the chance to be called due to the system hang at
the master device deregistration which happens before ksz_switch_remove()
is called.
The fix is to make sure that the master device references are already
released when the device is unregistered. For this reason PATCH1 provides
a new function dsa_tree_shutdown() that can be called by DSA drivers to
untear the DSA switch at shutdown. PATCH2 uses this function in a new
helper function for KSZ switches to properly shutdown the KSZ switch.
PATCH 3 uses the new helper function in the KSZ9477 shutdown handler.
I agree with Vladimir here. Shutdown works without issue on mv88e6xxx,
i do it frequently. I'm sure other developers shutdown there devices
at the end of the edit/compile/test cycle. If there was a generic
problem, we would probably know about it. So it seems like there is
something specific to your system which breaks the reference
counting. We need to understand that first, then we can see how we fix
it.
Theses patches have been tested on a Raspberry PI 5.10 kernel with a
KSZ9897. The patches have been adjusted to apply against net-next and are
compile tested with next-next.
Is the switch on a hat? Are you using DT overlays?
Andrew