| Message ID | 20200122090936.28555-2-boon.leong.ong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: general fixes for Ethernet functionality | expand |
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
>-----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?
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
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);