| Message ID | bbcdbc1b-44bc-4cf8-86ef-6e6af2b009c3@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] r8169: fix LED-related deadlock on module removal | expand |
On Fri, Apr 05, 2024 at 10:29:15PM +0200, Heiner Kallweit wrote: > Binding devm_led_classdev_register() to the netdev is problematic > because on module removal we get a RTNL-related deadlock. Fix this > by using the parent device instead for device-managed resources. > This is cleaner anyway because then all device-managed resources in > the driver use the same device (the one belonging to the PCI device). I've been thinking a bit about devm for LEDs. At what point do the entries in /sys/class/led disappears? When is the netdev trigger removed? I think it is after the netdev has gone? static int rtl8168_led_hw_control_set(struct led_classdev *led_cdev, unsigned long flags) { struct r8169_led_classdev *ldev = lcdev_to_r8169_ldev(led_cdev); struct rtl8169_private *tp = netdev_priv(ldev->ndev); Is this safe? I think the LED will only be destroyed after rtl_remove_one() has completed. I think to be safe, we cannot use devm_led_classdev_register(). The LEDs need to be added and explicitly removed within the life cycle of the netdev. Andrew
On Fri, Apr 05, 2024 at 10:29:15PM +0200, Heiner Kallweit wrote: > --- a/drivers/net/ethernet/realtek/r8169_leds.c > +++ b/drivers/net/ethernet/realtek/r8169_leds.c > @@ -146,13 +146,12 @@ static void rtl8168_setup_ldev(struct r8169_led_classdev *ldev, > led_cdev->hw_control_get_device = r8169_led_hw_control_get_device; > > /* ignore errors */ > - devm_led_classdev_register(&ndev->dev, led_cdev); > + devm_led_classdev_register(ndev->dev.parent, led_cdev); IIUC, devm_led_classdev_register() uses the first argument both to manage unregistering on unbind, but also as parent of the LED device. While the former is desired, the latter likely is not. I assume the LEDs now appear as children of the PCI device in sysfs, not as children of the netdev. Probably not what you want. The second patch I posted should handle that correctly. Thanks, Lukas
diff --git a/drivers/net/ethernet/realtek/r8169_leds.c b/drivers/net/ethernet/realtek/r8169_leds.c index 7c5dc9d0d..7f2b8a361 100644 --- a/drivers/net/ethernet/realtek/r8169_leds.c +++ b/drivers/net/ethernet/realtek/r8169_leds.c @@ -146,13 +146,12 @@ static void rtl8168_setup_ldev(struct r8169_led_classdev *ldev, led_cdev->hw_control_get_device = r8169_led_hw_control_get_device; /* ignore errors */ - devm_led_classdev_register(&ndev->dev, led_cdev); + devm_led_classdev_register(ndev->dev.parent, led_cdev); } void rtl8168_init_leds(struct net_device *ndev) { - /* bind resource mgmt to netdev */ - struct device *dev = &ndev->dev; + struct device *dev = ndev->dev.parent; struct r8169_led_classdev *leds; int i; @@ -245,13 +244,12 @@ static void rtl8125_setup_led_ldev(struct r8169_led_classdev *ldev, led_cdev->hw_control_get_device = r8169_led_hw_control_get_device; /* ignore errors */ - devm_led_classdev_register(&ndev->dev, led_cdev); + devm_led_classdev_register(ndev->dev.parent, led_cdev); } void rtl8125_init_leds(struct net_device *ndev) { - /* bind resource mgmt to netdev */ - struct device *dev = &ndev->dev; + struct device *dev = ndev->dev.parent; struct r8169_led_classdev *leds; int i;
Binding devm_led_classdev_register() to the netdev is problematic because on module removal we get a RTNL-related deadlock. Fix this by using the parent device instead for device-managed resources. This is cleaner anyway because then all device-managed resources in the driver use the same device (the one belonging to the PCI device). Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") Cc: stable@vger.kernel.org Reported-by: Lukas Wunner <lukas@wunner.de> Suggested-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_leds.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)