[net,v3,1/5] net: stmmac: Fix incorrect location to set real_num_rx|tx_queues
diff mbox series

Message ID 20200122090936.28555-2-boon.leong.ong@intel.com
State New
Headers show
Series
  • net: stmmac: general fixes for Ethernet functionality
Related show

Commit Message

Ong Boon Leong Jan. 22, 2020, 9:09 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 introduce lock_acquired boolean to control when
to use rtnl_lock|unlock() within stmmac_hw_setup().
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>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

Jose Abreu Jan. 22, 2020, 9:55 a.m. UTC | #1
From: Ong Boon Leong <boon.leong.ong@intel.com>
Date: Jan/22/2020, 09:09:32 (UTC+00:00)

> For driver open(), rtnl_lock is acquired by network stack but not in the
> resume(). Therefore, we introduce lock_acquired boolean to control when
> to use rtnl_lock|unlock() within stmmac_hw_setup().

Why not use rtnl_is_locked() instead of the boolean ?

---
Thanks,
Jose Miguel Abreu
Ong Boon Leong Jan. 24, 2020, 8:56 a.m. UTC | #2
>-----Original Message-----
>From: Jose Abreu <Jose.Abreu@synopsys.com>
>Sent: Wednesday, January 22, 2020 5:56 PM
>To: Ong, Boon Leong <boon.leong.ong@intel.com>; netdev@vger.kernel.org
>Cc: Tan, Tee Min <tee.min.tan@intel.com>; Voon, Weifeng
><weifeng.voon@intel.com>; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
>Alexandre TORGUE <alexandre.torgue@st.com>; David S . Miller
><davem@davemloft.net>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
>Joao Pinto <Joao.Pinto@synopsys.com>; Arnd Bergmann <arnd@arndb.de>;
>Alexandru Ardelean <alexandru.ardelean@analog.com>; linux-stm32@st-md-
>mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
>kernel@vger.kernel.org
>Subject: RE: [PATCH net v3 1/5] net: stmmac: Fix incorrect location to set
>real_num_rx|tx_queues
>
>From: Ong Boon Leong <boon.leong.ong@intel.com>
>Date: Jan/22/2020, 09:09:32 (UTC+00:00)
>
>> For driver open(), rtnl_lock is acquired by network stack but not in the
>> resume(). Therefore, we introduce lock_acquired boolean to control when
>> to use rtnl_lock|unlock() within stmmac_hw_setup().
>
>Why not use rtnl_is_locked() instead of the boolean ?

We know that stmmac_open() is called with rtnl_mutex locked by caller.
And, stmmac_resume() is called without rtnl_mutex is locked by caller.
If we replace the boolean with rtnl_is_locked(), then we will have the
following logics in stmmac_hw_setup():-

     if (!rtnl_is_locked)   ---- (A)
         rtnl_lock();
     netif_set_real_num_rx_queues();
     netif_set_real_num_tx_queues();
     if (!rtnl_is_locked)   ---- (B)
         rtnl_unlock();

For stmmac_open(), (A) is false but (B) is true. 
So, the stmmac_open() exits with rtnl_mutex is released.
Here, the above logic does not perserve the original rtnl_mutex
is locked when stmmac_open() is called.

For stmmac_resume(), (A) is true, and (B) is also true.
So, the stmmac_resume() exits with rtnl_mutex is released.
Here, the above logic works well as the original rtnl_mutex is released
when stmmac_resume() is called.
 
So, as far as I can see, the proposed boolean approach works fine for both
stmmac_open() and stmmac_resume().

Do you agree?
Jose Abreu Jan. 24, 2020, 9:06 a.m. UTC | #3
From: Ong, Boon Leong <boon.leong.ong@intel.com>
Date: Jan/24/2020, 08:56:28 (UTC+00:00)

> >Why not use rtnl_is_locked() instead of the boolean ?
> 
> We know that stmmac_open() is called with rtnl_mutex locked by caller.
> And, stmmac_resume() is called without rtnl_mutex is locked by caller.
> If we replace the boolean with rtnl_is_locked(), then we will have the
> following logics in stmmac_hw_setup():-
> 
>      if (!rtnl_is_locked)   ---- (A)
>          rtnl_lock();
>      netif_set_real_num_rx_queues();
>      netif_set_real_num_tx_queues();
>      if (!rtnl_is_locked)   ---- (B)
>          rtnl_unlock();
> 
> For stmmac_open(), (A) is false but (B) is true. 
> So, the stmmac_open() exits with rtnl_mutex is released.
> Here, the above logic does not perserve the original rtnl_mutex
> is locked when stmmac_open() is called.
> 
> For stmmac_resume(), (A) is true, and (B) is also true.
> So, the stmmac_resume() exits with rtnl_mutex is released.
> Here, the above logic works well as the original rtnl_mutex is released
> when stmmac_resume() is called.
>  
> So, as far as I can see, the proposed boolean approach works fine for both
> stmmac_open() and stmmac_resume().
> 
> Do you agree? 

Can't you just wrap all the HW related logic in stmmac_resume() and 
stmmac_suspend() with the rtnl lock ? Seems like the right thing to do and 
you won't need the boolean.

---
Thanks,
Jose Miguel Abreu

Patch
diff mbox series

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 80d59b775907..417397158d4a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2525,7 +2525,8 @@  static void stmmac_safety_feat_configuration(struct stmmac_priv *priv)
  *  0 on success and an appropriate (-)ve integer as defined in errno.h
  *  file on failure.
  */
-static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
+static int stmmac_hw_setup(struct net_device *dev, bool init_ptp,
+			   bool lock_acquired)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 	u32 rx_cnt = priv->plat->rx_queues_to_use;
@@ -2624,6 +2625,14 @@  static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
 	if (priv->dma_cap.vlins)
 		stmmac_enable_vlan(priv, priv->hw, STMMAC_VLAN_INSERT);
 
+	/* Configure real RX and TX queues */
+	if (!lock_acquired)
+		rtnl_lock();
+	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);
+	if (!lock_acquired)
+		rtnl_unlock();
+
 	/* Start the ball rolling... */
 	stmmac_start_all_dma(priv);
 
@@ -2695,7 +2704,7 @@  static int stmmac_open(struct net_device *dev)
 		goto init_error;
 	}
 
-	ret = stmmac_hw_setup(dev, true);
+	ret = stmmac_hw_setup(dev, true, true);
 	if (ret < 0) {
 		netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
 		goto init_error;
@@ -4622,10 +4631,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 |
@@ -4973,7 +4978,7 @@  int stmmac_resume(struct device *dev)
 
 	stmmac_clear_descriptors(priv);
 
-	stmmac_hw_setup(ndev, false);
+	stmmac_hw_setup(ndev, false, false);
 	stmmac_init_coalesce(priv);
 	stmmac_set_rx_mode(ndev);