Re: [PATCH net] bnxt_en: bring back rtnl_lock() in bnxt_fw_reset_task()

[Stanislav Fomichev <stfomichev@gmail.com>]

On 05/11, Michael Chan wrote:
> RTNL assertion failed in netif_set_real_num_tx_queues() in the
> error recovery path:
> 
> RTNL: assertion failed at net/core/dev.c (3178)
> WARNING: CPU: 3 PID: 3392 at net/core/dev.c:3178 netif_set_real_num_tx_queues+0x1fd/0x210
> 
> Call Trace:
>  <TASK>
>  ? __pfx_bnxt_msix+0x10/0x10 [bnxt_en]
>  __bnxt_open_nic+0x1ef/0xb20 [bnxt_en]
>  bnxt_open+0xda/0x130 [bnxt_en]
>  bnxt_fw_reset_task+0x21f/0x780 [bnxt_en]
>  process_scheduled_works+0x9d/0x400
> 
> Bring back the rtnl_lock() for now in bnxt_fw_reset_task().
> 
> Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 86a5de44b6f3..8df602663e0d 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14960,15 +14960,17 @@ static void bnxt_fw_reset_task(struct work_struct *work)
>  		bp->fw_reset_state = BNXT_FW_RESET_STATE_OPENING;
>  		fallthrough;
>  	case BNXT_FW_RESET_STATE_OPENING:
> -		while (!netdev_trylock(bp->dev)) {
> +		while (!rtnl_trylock()) {
>  			bnxt_queue_fw_reset_work(bp, HZ / 10);
>  			return;
>  		}
> +		netdev_lock(bp->dev);
>  		rc = bnxt_open(bp->dev);
>  		if (rc) {
>  			netdev_err(bp->dev, "bnxt_open() failed during FW reset\n");
>  			bnxt_fw_reset_abort(bp, rc);
>  			netdev_unlock(bp->dev);
> +			rtnl_unlock();
>  			goto ulp_start;
>  		}
>  
> @@ -14988,6 +14990,7 @@ static void bnxt_fw_reset_task(struct work_struct *work)
>  			bnxt_dl_health_fw_status_update(bp, true);
>  		}
>  		netdev_unlock(bp->dev);
> +		rtnl_unlock();
>  		bnxt_ulp_start(bp, 0);
>  		bnxt_reenable_sriov(bp);
>  		netdev_lock(bp->dev);
> -- 
> 2.30.1
> 

Will the following work instead? netdev_ops_assert_locked should take
care of asserting either ops lock or rtnl lock depending on the device
properties.

diff --git a/net/core/dev.c b/net/core/dev.c
index c9013632296f..d8d29729c685 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3177,7 +3177,6 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq)
 
 	if (dev->reg_state == NETREG_REGISTERED ||
 	    dev->reg_state == NETREG_UNREGISTERING) {
-		ASSERT_RTNL();
 		netdev_ops_assert_locked(dev);
 
 		rc = netdev_queue_update_kobjects(dev, dev->real_num_tx_queues,
@@ -3227,7 +3226,6 @@ int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq)
 		return -EINVAL;
 
 	if (dev->reg_state == NETREG_REGISTERED) {
-		ASSERT_RTNL();
 		netdev_ops_assert_locked(dev);
 
 		rc = net_rx_queue_update_kobjects(dev, dev->real_num_rx_queues,