Commit c04d1b9e authored by Lukas Wunner's avatar Lukas Wunner Committed by Jakub Kicinski
Browse files

igc: Fix LED-related deadlock on driver unbind 



Roman reports a deadlock on unplug of a Thunderbolt docking station
containing an Intel I225 Ethernet adapter.

The root cause is that led_classdev's for LEDs on the adapter are
registered such that they're device-managed by the netdev.  That
results in recursive acquisition of the rtnl_lock() mutex on unplug:

When the driver calls unregister_netdev(), it acquires rtnl_lock(),
then frees the device-managed resources.  Upon unregistering the LEDs,
netdev_trig_deactivate() invokes unregister_netdevice_notifier(),
which tries to acquire rtnl_lock() again.

Avoid by using non-device-managed LED registration.

Stack trace for posterity:

  schedule+0x6e/0xf0
  schedule_preempt_disabled+0x15/0x20
  __mutex_lock+0x2a0/0x750
  unregister_netdevice_notifier+0x40/0x150
  netdev_trig_deactivate+0x1f/0x60 [ledtrig_netdev]
  led_trigger_set+0x102/0x330
  led_classdev_unregister+0x4b/0x110
  release_nodes+0x3d/0xb0
  devres_release_all+0x8b/0xc0
  device_del+0x34f/0x3c0
  unregister_netdevice_many_notify+0x80b/0xaf0
  unregister_netdev+0x7c/0xd0
  igc_remove+0xd8/0x1e0 [igc]
  pci_device_remove+0x3f/0xb0

Fixes: ea578703 ("igc: Add support for LEDs on i225/i226")
Reported-by: default avatarRoman Lozko <lozko.roma@gmail.com>
Closes: https://lore.kernel.org/r/CAEhC_B=ksywxCG_+aQqXUrGEgKq+4mqnSV8EBHOKbC3-Obj9+Q@mail.gmail.com/


Reported-by: default avatar"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Closes: https://lore.kernel.org/r/ZhRD3cOtz5i-61PB@mail-itl/


Signed-off-by: default avatarKurt Kanzenbach <kurt@linutronix.de>
Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: default avatarSimon Horman <horms@kernel.org>
Reviewed-by: default avatarKurt Kanzenbach <kurt@linutronix.de>
Tested-by: Kurt Kanzenbach <kurt@linutronix.de> # Intel i225
Tested-by: default avatarNaama Meir <naamax.meir@linux.intel.com>
Signed-off-by: default avatarTony Nguyen <anthony.l.nguyen@intel.com>
Link: https://lore.kernel.org/r/20240422204503.225448-1-anthony.l.nguyen@intel.com


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parent edd2d250

drivers/net/ethernet/intel/igc/igc.h

+2 −0
Original line number Diff line number Diff line
@@ -298,6 +298,7 @@ struct igc_adapter { 


	/* LEDs */

	struct mutex led_mutex;

	struct igc_led_classdev *leds;

};



void igc_up(struct igc_adapter *adapter);
@@ -723,6 +724,7 @@ void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts); 
void igc_ptp_tx_tstamp_event(struct igc_adapter *adapter);



int igc_led_setup(struct igc_adapter *adapter);

void igc_led_free(struct igc_adapter *adapter);



#define igc_rx_pg_size(_ring) (PAGE_SIZE << igc_rx_pg_order(_ring))

drivers/net/ethernet/intel/igc/igc_leds.c

+30 −8
Original line number Diff line number Diff line
@@ -236,7 +236,7 @@ static void igc_led_get_name(struct igc_adapter *adapter, int index, char *buf, 
		 pci_dev_id(adapter->pdev), index);

}



static void igc_setup_ldev(struct igc_led_classdev *ldev,

static int igc_setup_ldev(struct igc_led_classdev *ldev,

			  struct net_device *netdev, int index)

{

	struct igc_adapter *adapter = netdev_priv(netdev);
@@ -257,24 +257,46 @@ static void igc_setup_ldev(struct igc_led_classdev *ldev, 
	led_cdev->hw_control_get = igc_led_hw_control_get;

	led_cdev->hw_control_get_device = igc_led_hw_control_get_device;



	devm_led_classdev_register(&netdev->dev, led_cdev);

	return led_classdev_register(&netdev->dev, led_cdev);

}



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));



	kfree(leds);

	return err;

}



void igc_led_free(struct igc_adapter *adapter)

{

	struct igc_led_classdev *leds = adapter->leds;

	int i;



	for (i = 0; i < IGC_NUM_LEDS; i++)

		led_classdev_unregister(&((leds + i)->led));



	kfree(leds);

}

drivers/net/ethernet/intel/igc/igc_main.c

+3 −0
Original line number Diff line number Diff line
@@ -7021,6 +7021,9 @@ static void igc_remove(struct pci_dev *pdev) 
	cancel_work_sync(&adapter->watchdog_task);

	hrtimer_cancel(&adapter->hrtimer);



	if (IS_ENABLED(CONFIG_IGC_LEDS))

		igc_led_free(adapter);



	/* Release control of h/w to f/w.  If f/w is AMT enabled, this

	 * would have already happened in close and is redundant.

	 */