diff mbox series

[net,v4,1/6] net: stmmac: Fix incorrect location to set real_num_rx|tx_queues

Message ID 20200205085510.32353-2-boon.leong.ong@intel.com (mailing list archive)
State New, archived
Headers show
Series net: stmmac: general fixes for Ethernet functionality | expand

Commit Message

Ong Boon Leong Feb. 5, 2020, 8:55 a.m. UTC 
From: Aashish Verma <aashishx.verma@intel.com>

netif_set_real_num_tx_queues() & netif_set_real_num_rx_queues() should be
used to inform network stack about the real Tx & Rx queue (active) number
in both stmmac_open() and stmmac_resume(), therefore, we move the code
from stmmac_dvr_probe() to stmmac_hw_setup().

For driver open(), rtnl_lock is acquired by network stack but not in the
resume(). Therefore, we need to rtnl_lock() and rtnl_unlock() when
calling stmmac_hw_setup() within resume(). Thanks Jose Abreu for input.

Fixes: c02b7a914551 ("net: stmmac: use netif_set_real_num_{rx,tx}_queues")
Signed-off-by: Aashish Verma <aashishx.verma@intel.com>
Tested-by: Tan, Tee Min <tee.min.tan@intel.com>
Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

David Miller Feb. 5, 2020, 1:39 p.m. UTC | #1 
From: Ong Boon Leong <boon.leong.ong@intel.com>
Date: Wed,  5 Feb 2020 16:55:05 +0800

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 5836b21edd7e..4d9afa13eeb9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2657,6 +2657,10 @@ static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
>  		stmmac_enable_tbs(priv, priv->ioaddr, enable, chan);
>  	}
>  
> +	/* Configure real RX and TX queues */
> +	netif_set_real_num_rx_queues(dev, priv->plat->rx_queues_to_use);
> +	netif_set_real_num_tx_queues(dev, priv->plat->tx_queues_to_use);
> +
>  	/* Start the ball rolling... */
>  	stmmac_start_all_dma(priv);
>  

It is only safe to ignore the return values from
netif_set_real_num_{rx,tx}_queues() if you call them before the
network device is registered.  Because only in that case are these
functions guaranteed to succeed.

But now that you have moved these calls here, they can fail.

Therefore you must check the return value and unwind the state
completely upon failures.

Honestly, I think this change will have several undesirable side effects:

1) Lots of added new code complexity

2) A new failure mode when resuming the device, users will find this
   very hard to diagnose and recover from

What real value do you get from doing these calls after probe?

If you can't come up with a suitable answer to that question, you
should reconsider this change.

Thanks.
Ong Boon Leong Feb. 7, 2020, 7:21 a.m. UTC | #2 
From: David Miller <davem@davemloft.net>
Date: Wednesday, February 5, 2020 9:39 PM

>From: Ong Boon Leong <boon.leong.ong@intel.com>
>Date: Wed,  5 Feb 2020 16:55:05 +0800
>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 5836b21edd7e..4d9afa13eeb9 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -2657,6 +2657,10 @@ static int stmmac_hw_setup(struct net_device
>*dev, bool init_ptp)
>>  >--->-------stmmac_enable_tbs(priv, priv->ioaddr, enable, chan);
>>  >---}
>>
>> +>---/* Configure real RX and TX queues */
>> +>---netif_set_real_num_rx_queues(dev, priv->plat->rx_queues_to_use);
>> +>---netif_set_real_num_tx_queues(dev, priv->plat->tx_queues_to_use);
>> +
>>  >---/* Start the ball rolling... */
>>  >---stmmac_start_all_dma(priv);
>>
>
>It is only safe to ignore the return values from
>netif_set_real_num_{rx,tx}_queues() if you call them before the
>network device is registered.  Because only in that case are these
>functions guaranteed to succeed.
>
>But now that you have moved these calls here, they can fail.
>
>Therefore you must check the return value and unwind the state
>completely upon failures.
>
>Honestly, I think this change will have several undesirable side effects:
>
>1) Lots of added new code complexity
>
>2) A new failure mode when resuming the device, users will find this
>   very hard to diagnose and recover from
>
>What real value do you get from doing these calls after probe?
>
>If you can't come up with a suitable answer to that question, you
>should reconsider this change.
>
>Thanks.

We have patch that implements get|set_channels() that depends on this fix.
Anyway, we understand your insight and perspective now. So, we will drop
this patch in v5 series.

Thanks
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 5836b21edd7e..4d9afa13eeb9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2657,6 +2657,10 @@  static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
 		stmmac_enable_tbs(priv, priv->ioaddr, enable, chan);
 	}
 
+	/* Configure real RX and TX queues */
+	netif_set_real_num_rx_queues(dev, priv->plat->rx_queues_to_use);
+	netif_set_real_num_tx_queues(dev, priv->plat->tx_queues_to_use);
+
 	/* Start the ball rolling... */
 	stmmac_start_all_dma(priv);
 
@@ -4738,10 +4742,6 @@  int stmmac_dvr_probe(struct device *device,
 
 	stmmac_check_ether_addr(priv);
 
-	/* Configure real RX and TX queues */
-	netif_set_real_num_rx_queues(ndev, priv->plat->rx_queues_to_use);
-	netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
-
 	ndev->netdev_ops = &stmmac_netdev_ops;
 
 	ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
@@ -5091,7 +5091,9 @@  int stmmac_resume(struct device *dev)
 
 	stmmac_clear_descriptors(priv);
 
+	rtnl_lock();
 	stmmac_hw_setup(ndev, false);
+	rtnl_unlock();
 	stmmac_init_coalesce(priv);
 	stmmac_set_rx_mode(ndev);