Subject: Re: [PATCH net] r8169: fix LED-related deadlock on module removal

From: Lukas Wunner <[email-masked]>
Date: Mon, 15 Apr 2024 10:49:11 +0200

On Mon, Apr 15, 2024 at 08:44:35AM +0200, Heiner Kallweit wrote:
> Binding devm_led_classdev_register() to the netdev is problematic
> because on module removal we get a RTNL-related deadlock.

More precisely the issue is triggered on driver unbind.

Module unload as well as device unplug imply driver unbinding.

> The original change was introduced with 6.8, 6.9 added support for
> LEDs on RTL8125. Therefore the first version of the fix applied on
> 6.9-rc only. This is the modified version for 6.8.

I guess the recipient of this patch should have been the stable
maintainers then, not netdev maintainers.

> --- a/drivers/net/ethernet/realtek/r8169.h
> +++ b/drivers/net/ethernet/realtek/r8169.h
> @@ -72,6 +72,7 @@ enum mac_version {
>  };
>  
>  struct rtl8169_private;
> +struct r8169_led_classdev;

Normally these forward declarations are not needed if you're just
referencing the struct name in a pointer.  Usage of the struct name
in a pointer implies a forward declaration.

> +struct r8169_led_classdev *rtl8168_init_leds(struct net_device *ndev)
>  {
> -	/* bind resource mgmt to netdev */
> -	struct device *dev = &ndev->dev;
>  	struct r8169_led_classdev *leds;
>  	int i;
>  
> -	leds = devm_kcalloc(dev, RTL8168_NUM_LEDS, sizeof(*leds), GFP_KERNEL);
> +	leds = kcalloc(RTL8168_NUM_LEDS + 1, sizeof(*leds), GFP_KERNEL);
>  	if (!leds)
> -		return;
> +		return NULL;
>  
>  	for (i = 0; i < RTL8168_NUM_LEDS; i++)
>  		rtl8168_setup_ldev(leds + i, ndev, i);
> +
> +	return leds;
> +}

If registration of some LEDs fails, you seem to continue driver binding.
So the leds allocation may stick around even if it's not used at all.
Not a big deal, but not super pretty either.

Thanks,

Lukas