Re: [PATCH net 1/3] igb: fix RTNL assertion when resuming from runtime suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] igb: premature netif_set_real_num_queues() call in igb_open()
--

commit bc721a3925f29bdad59ef5c4890bb7278bb44a56
Author: Antonio Quartulli <antonio@mandelbit.com>

igb: fix RTNL assertion when resuming from runtime suspend

This commit moves the TX and RX real queue count updates from __igb_open()
to igb_open() so they are not called without RTNL during runtime resume.

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 131df3e09ba14..028cdd90522cb 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -4178,17 +4178,6 @@ static int __igb_open(struct net_device *netdev, bool resuming)
>  	if (err)
>  		goto err_req_irq;
>  
> -	/* Notify the stack of the actual queue counts. */
> -	err = netif_set_real_num_tx_queues(adapter->netdev,
> -					   adapter->num_tx_queues);
> -	if (err)
> -		goto err_set_queues;
> -
> -	err = netif_set_real_num_rx_queues(adapter->netdev,
> -					   adapter->num_rx_queues);
> -	if (err)
> -		goto err_set_queues;
> -
>  	/* From here on the code is the same as igb_up() */
>  	clear_bit(__IGB_DOWN, &adapter->state);
>  
> @@ -4223,8 +4212,6 @@ static int __igb_open(struct net_device *netdev, bool resuming)
>  
>  	return 0;
>  
> -err_set_queues:
> -	igb_free_irq(adapter);
>  err_req_irq:
>  	igb_release_hw_control(adapter);
>  	igb_power_down_link(adapter);
> @@ -4241,6 +4228,17 @@ static int __igb_open(struct net_device *netdev, bool resuming)
>  
>  int igb_open(struct net_device *netdev)
>  {
> +	struct igb_adapter *adapter = netdev_priv(netdev);
> +	int err;
> +
> +	/* Notify the stack of the actual queue counts. */
> +	err = netif_set_real_num_queues(netdev, adapter->num_tx_queues,
> +					adapter->num_rx_queues);
> +	if (err) {
> +		netdev_err(netdev, "error setting real queue count\n");
> +		return err;
> +	}
> +
>  	return __igb_open(netdev, false);
>  }

[Severity: Medium]
Can calling netif_set_real_num_queues() before __igb_open() leave
netdev->real_num_tx_queues and netdev->real_num_rx_queues out of sync with
adapter->num_tx_queues and adapter->num_rx_queues?

Inside __igb_open(), igb_request_irq() falls back to MSI if
igb_request_msix() fails, which resets adapter->num_tx_queues and
adapter->num_rx_queues to 1 via igb_init_interrupt_scheme(adapter, false):

drivers/net/ethernet/intel/igb/igb_main.c:igb_request_irq() {
    ...
		igb_clear_interrupt_scheme(adapter);
		err = igb_init_interrupt_scheme(adapter, false);
		if (err)
			goto request_done;
    ...
}

Because netif_set_real_num_queues() already ran, the networking stack keeps
the pre-fallback MSI-X subqueue count and transmits on multiple subqueues
under separate _xmit_lock locks, while igb_tx_queue_mapping() folds every
r_idx % adapter->num_tx_queues onto adapter->tx_ring[0] without
synchronization.

Also, if igb_xdp_setup() is called while the interface is up and
runtime-suspended, igb_close() is a no-op because !netif_device_present()
and __igb_shutdown() has cleared adapter->num_{tx,rx}_queues to 0;
igb_open() then passes 0 to netif_set_real_num_queues() and fails with
-EINVAL before __igb_open() can call pm_runtime_get_sync() to resume and
reinitialize the queue counts.

Keeping the queue count update in __igb_open() after igb_request_irq(),
guarded by if (!resuming), would avoid both regressions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113105835.3821433-1-antonio@mandelbit.com?part=1