[cc += Roman Lozko who originally reported the issue] On Sun, Apr 14, 2024 at 09:44:10AM +0200, Kurt Kanzenbach wrote: > unregister_netdev() acquires the RNTL lock and releases the LEDs bound > to that netdevice. However, netdev_trig_deactivate() and later > unregister_netdevice_notifier() try to acquire the RTNL lock again. > > Avoid this situation by not using the device-managed LED class > functions. > > Suggested-by: Lukas Wunner <[email protected]> > Fixes: ea578703b03d ("igc: Add support for LEDs on i225/i226") > Signed-off-by: Kurt Kanzenbach <[email protected]>
This patch is almost a 1:1 copy of the patch I submitted on April 5: https://lore.kernel.org/all/[email protected]/ I think it is mandatory that you include a Signed-off-by with my name in that case. Arguably the commit author ("From:") should also be me. Moreover this is missing a Reported-by tag with Roman Lozko's name. AFAICS the only changes that you made are: - rename igc_led_teardown() to igc_led_free() - rename ret to err - replace devm_kcalloc() with kcalloc() (and you introduced a memory leak while doing so, see below) Honestly I don't see how those small changes justify omitting a Signed-off-by or assuming authorship. I would have been happy to submit a patch myself, I was waiting for a Tested-by from Roman or you. > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -164,6 +164,8 @@ struct igc_ring { > struct xsk_buff_pool *xsk_pool; > } ____cacheline_internodealigned_in_smp; > > +struct igc_led_classdev; Unnecessary forward declaration, this compiled fine for me without it. > int igc_led_setup(struct igc_adapter *adapter) > { > struct net_device *netdev = adapter->netdev; > - struct device *dev = &netdev->dev; > struct igc_led_classdev *leds; > - int i; > + int i, err; > > mutex_init(&adapter->led_mutex); > > - leds = devm_kcalloc(dev, IGC_NUM_LEDS, sizeof(*leds), GFP_KERNEL); > + leds = kcalloc(IGC_NUM_LEDS, sizeof(*leds), GFP_KERNEL); > if (!leds) > return -ENOMEM; > > - for (i = 0; i < IGC_NUM_LEDS; i++) > - igc_setup_ldev(leds + i, netdev, i); > + for (i = 0; i < IGC_NUM_LEDS; i++) { > + err = igc_setup_ldev(leds + i, netdev, i); > + if (err) > + goto err; > + } > + > + adapter->leds = leds; > > return 0; > + > +err: > + for (i--; i >= 0; i--) > + led_classdev_unregister(&((leds + i)->led)); > + > + return err; > +} "leds" allocation is leaked in the error path. This memory leak was not present in my original patch. Not good! Thanks, Lukas
