* [PATCH net-next 0/3] dpaa2-eth: MAC phylink fixes
@ 2019-11-21 19:15 Ioana Ciornei
2019-11-21 19:15 ` [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy() Ioana Ciornei
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Ioana Ciornei @ 2019-11-21 19:15 UTC (permalink / raw)
To: davem, netdev; +Cc: linux, andrew, Ioana Ciornei
This patch set adds a couple of fixes on top of the
initial MAC support in dpaa2-eth.
Patch 2/3 depends on the phylink callback rename:
http://patchwork.ozlabs.org/patch/1198615/
Ioana Ciornei (3):
dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy()
dpaa2-eth: add phylink_mac_ops stub callbacks
dpaa2-eth: return all supported link modes in PHY_INTERFACE_MODE_NA
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 4 ----
drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 18 ++++++++++++++++++
2 files changed, 18 insertions(+), 4 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy() 2019-11-21 19:15 [PATCH net-next 0/3] dpaa2-eth: MAC phylink fixes Ioana Ciornei @ 2019-11-21 19:15 ` Ioana Ciornei 2019-11-21 19:50 ` Andrew Lunn 2019-11-21 23:48 ` Russell King - ARM Linux admin 2019-11-21 19:15 ` [PATCH net-next 2/3] dpaa2-eth: add phylink_mac_ops stub callbacks Ioana Ciornei 2019-11-21 19:15 ` [PATCH net-next 3/3] dpaa2-eth: return all supported link modes in PHY_INTERFACE_MODE_NA Ioana Ciornei 2 siblings, 2 replies; 8+ messages in thread From: Ioana Ciornei @ 2019-11-21 19:15 UTC (permalink / raw) To: davem, netdev; +Cc: linux, andrew, Ioana Ciornei The rtnl_lock should not be held when calling phylink_create() or phylink_destroy() since it leads to the deadlock listed below: [ 18.656576] rtnl_lock+0x18/0x20 [ 18.659798] sfp_bus_add_upstream+0x28/0x90 [ 18.663974] phylink_create+0x2cc/0x828 [ 18.667803] dpaa2_mac_connect+0x14c/0x2a8 [ 18.671890] dpaa2_eth_connect_mac+0x94/0xd8 Fix this by moving the _lock() and _unlock() calls just outside of phylink_of_phy_connect() and phylink_disconnect_phy(). Fixes: 719479230893 ("dpaa2-eth: add MAC/PHY support through phylink") Reported-by: Russell King <linux@armlinux.org.uk> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 4 ---- drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c index 7ff147e89426..40290fea9e36 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c @@ -3431,12 +3431,10 @@ static irqreturn_t dpni_irq0_handler_thread(int irq_num, void *arg) set_mac_addr(netdev_priv(net_dev)); update_tx_fqids(priv); - rtnl_lock(); if (priv->mac) dpaa2_eth_disconnect_mac(priv); else dpaa2_eth_connect_mac(priv); - rtnl_unlock(); } return IRQ_HANDLED; @@ -3675,9 +3673,7 @@ static int dpaa2_eth_remove(struct fsl_mc_device *ls_dev) #ifdef CONFIG_DEBUG_FS dpaa2_dbg_remove(priv); #endif - rtnl_lock(); dpaa2_eth_disconnect_mac(priv); - rtnl_unlock(); unregister_netdev(net_dev); diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c index 84233e467ed1..0200308d1bc7 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c @@ -277,7 +277,9 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) } mac->phylink = phylink; + rtnl_lock(); err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0); + rtnl_unlock(); if (err) { netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err); goto err_phylink_destroy; @@ -301,7 +303,9 @@ void dpaa2_mac_disconnect(struct dpaa2_mac *mac) if (!mac->phylink) return; + rtnl_lock(); phylink_disconnect_phy(mac->phylink); + rtnl_unlock(); phylink_destroy(mac->phylink); dpmac_close(mac->mc_io, 0, mac->mc_dev->mc_handle); } -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy() 2019-11-21 19:15 ` [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy() Ioana Ciornei @ 2019-11-21 19:50 ` Andrew Lunn 2019-11-21 23:48 ` Russell King - ARM Linux admin 1 sibling, 0 replies; 8+ messages in thread From: Andrew Lunn @ 2019-11-21 19:50 UTC (permalink / raw) To: Ioana Ciornei; +Cc: davem, netdev, linux On Thu, Nov 21, 2019 at 09:15:25PM +0200, Ioana Ciornei wrote: > The rtnl_lock should not be held when calling phylink_create() or > phylink_destroy() since it leads to the deadlock listed below: > > [ 18.656576] rtnl_lock+0x18/0x20 > [ 18.659798] sfp_bus_add_upstream+0x28/0x90 > [ 18.663974] phylink_create+0x2cc/0x828 > [ 18.667803] dpaa2_mac_connect+0x14c/0x2a8 > [ 18.671890] dpaa2_eth_connect_mac+0x94/0xd8 Hi Ioana Have you done any testing with CONFIG_PROVE_LOCKING enabled. It should find this sort of problem if the code paths get exercised. Andrew ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy() 2019-11-21 19:15 ` [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy() Ioana Ciornei 2019-11-21 19:50 ` Andrew Lunn @ 2019-11-21 23:48 ` Russell King - ARM Linux admin 1 sibling, 0 replies; 8+ messages in thread From: Russell King - ARM Linux admin @ 2019-11-21 23:48 UTC (permalink / raw) To: Ioana Ciornei; +Cc: davem, netdev, andrew On Thu, Nov 21, 2019 at 09:15:25PM +0200, Ioana Ciornei wrote: > The rtnl_lock should not be held when calling phylink_create() or > phylink_destroy() since it leads to the deadlock listed below: > > [ 18.656576] rtnl_lock+0x18/0x20 > [ 18.659798] sfp_bus_add_upstream+0x28/0x90 > [ 18.663974] phylink_create+0x2cc/0x828 > [ 18.667803] dpaa2_mac_connect+0x14c/0x2a8 > [ 18.671890] dpaa2_eth_connect_mac+0x94/0xd8 > > Fix this by moving the _lock() and _unlock() calls just outside of > phylink_of_phy_connect() and phylink_disconnect_phy(). > > Fixes: 719479230893 ("dpaa2-eth: add MAC/PHY support through phylink") > Reported-by: Russell King <linux@armlinux.org.uk> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 4 ---- > drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 4 ++++ > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > index 7ff147e89426..40290fea9e36 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > @@ -3431,12 +3431,10 @@ static irqreturn_t dpni_irq0_handler_thread(int irq_num, void *arg) > set_mac_addr(netdev_priv(net_dev)); > update_tx_fqids(priv); > > - rtnl_lock(); > if (priv->mac) > dpaa2_eth_disconnect_mac(priv); > else > dpaa2_eth_connect_mac(priv); > - rtnl_unlock(); As I said previously, this will not be safe if net_dev is actively published to userspace, and I'm very disappointed that you have not taken on board what I wrote. Thread 1 Thread 2 rtnl_lock() dpaa2_eth_disconnect_mac(); dpaa2_mac_disconnect(priv->mac) phylink_disconnect_phy(mac->phylink); dpaa2_eth_get_link_ksettings() phylink_destroy(mac->phylink); phylink_ethtool_ksettings_get(priv->mac->phylink, ...) kfree(pl); phylink_ethtool_ksettings_get() now dereferences freed memory kfree(priv->mac); priv->mac = NULL; Similar can happen with priv->mac. As long as the netdev is published, paths such as those in thread 2 are possible while thread 1 is running concurrently. So, your patch is at best a band-aid around the deadlock issue I pointed out, but in doing so exposes another problem. I think there is a fundamental design problem, and merely tweaking some locks will not fix it. As I've already stated, the phylink is not designed to be created and destroyed on a published network device. > } > > return IRQ_HANDLED; > @@ -3675,9 +3673,7 @@ static int dpaa2_eth_remove(struct fsl_mc_device *ls_dev) > #ifdef CONFIG_DEBUG_FS > dpaa2_dbg_remove(priv); > #endif > - rtnl_lock(); > dpaa2_eth_disconnect_mac(priv); > - rtnl_unlock(); > > unregister_netdev(net_dev); > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > index 84233e467ed1..0200308d1bc7 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > @@ -277,7 +277,9 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > } > mac->phylink = phylink; > > + rtnl_lock(); > err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0); > + rtnl_unlock(); > if (err) { > netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err); > goto err_phylink_destroy; > @@ -301,7 +303,9 @@ void dpaa2_mac_disconnect(struct dpaa2_mac *mac) > if (!mac->phylink) > return; > > + rtnl_lock(); > phylink_disconnect_phy(mac->phylink); > + rtnl_unlock(); > phylink_destroy(mac->phylink); > dpmac_close(mac->mc_io, 0, mac->mc_dev->mc_handle); > } > -- > 1.9.1 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 2/3] dpaa2-eth: add phylink_mac_ops stub callbacks 2019-11-21 19:15 [PATCH net-next 0/3] dpaa2-eth: MAC phylink fixes Ioana Ciornei 2019-11-21 19:15 ` [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy() Ioana Ciornei @ 2019-11-21 19:15 ` Ioana Ciornei 2019-11-22 0:27 ` Russell King - ARM Linux admin 2019-11-21 19:15 ` [PATCH net-next 3/3] dpaa2-eth: return all supported link modes in PHY_INTERFACE_MODE_NA Ioana Ciornei 2 siblings, 1 reply; 8+ messages in thread From: Ioana Ciornei @ 2019-11-21 19:15 UTC (permalink / raw) To: davem, netdev; +Cc: linux, andrew, Ioana Ciornei For the moment, we do not have a way to query the PCS link state or to restart aneg on it. Add stub functions for both of the callbacks since phylink can provoke an oops when an SFP module has been inserted. Fixes: 719479230893 ("dpaa2-eth: add MAC/PHY support through phylink") Reported-by: Russell King <linux@armlinux.org.uk> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c index 0200308d1bc7..efc587515661 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c @@ -183,11 +183,24 @@ static void dpaa2_mac_link_down(struct phylink_config *config, netdev_err(mac->net_dev, "dpmac_set_link_state() = %d\n", err); } +static void dpaa2_mac_an_restart(struct phylink_config *config) +{ + /* Not supported */ +} + +static void dpaa2_mac_pcs_get_state(struct phylink_config *config, + struct phylink_link_state *state) +{ + /* Not supported */ +} + static const struct phylink_mac_ops dpaa2_mac_phylink_ops = { .validate = dpaa2_mac_validate, .mac_config = dpaa2_mac_config, .mac_link_up = dpaa2_mac_link_up, .mac_link_down = dpaa2_mac_link_down, + .mac_an_restart = dpaa2_mac_an_restart, + .mac_pcs_get_state = dpaa2_mac_pcs_get_state, }; bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev, -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/3] dpaa2-eth: add phylink_mac_ops stub callbacks 2019-11-21 19:15 ` [PATCH net-next 2/3] dpaa2-eth: add phylink_mac_ops stub callbacks Ioana Ciornei @ 2019-11-22 0:27 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 8+ messages in thread From: Russell King - ARM Linux admin @ 2019-11-22 0:27 UTC (permalink / raw) To: Ioana Ciornei; +Cc: davem, netdev, andrew On Thu, Nov 21, 2019 at 09:15:26PM +0200, Ioana Ciornei wrote: > For the moment, we do not have a way to query the PCS link state > or to restart aneg on it. Add stub functions for both of the callbacks > since phylink can provoke an oops when an SFP module has been inserted. > > Fixes: 719479230893 ("dpaa2-eth: add MAC/PHY support through phylink") > Reported-by: Russell King <linux@armlinux.org.uk> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> This is independent of patch 1. Acked-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > index 0200308d1bc7..efc587515661 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > @@ -183,11 +183,24 @@ static void dpaa2_mac_link_down(struct phylink_config *config, > netdev_err(mac->net_dev, "dpmac_set_link_state() = %d\n", err); > } > > +static void dpaa2_mac_an_restart(struct phylink_config *config) > +{ > + /* Not supported */ > +} > + > +static void dpaa2_mac_pcs_get_state(struct phylink_config *config, > + struct phylink_link_state *state) > +{ > + /* Not supported */ > +} > + > static const struct phylink_mac_ops dpaa2_mac_phylink_ops = { > .validate = dpaa2_mac_validate, > .mac_config = dpaa2_mac_config, > .mac_link_up = dpaa2_mac_link_up, > .mac_link_down = dpaa2_mac_link_down, > + .mac_an_restart = dpaa2_mac_an_restart, > + .mac_pcs_get_state = dpaa2_mac_pcs_get_state, > }; > > bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev, > -- > 1.9.1 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 3/3] dpaa2-eth: return all supported link modes in PHY_INTERFACE_MODE_NA 2019-11-21 19:15 [PATCH net-next 0/3] dpaa2-eth: MAC phylink fixes Ioana Ciornei 2019-11-21 19:15 ` [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy() Ioana Ciornei 2019-11-21 19:15 ` [PATCH net-next 2/3] dpaa2-eth: add phylink_mac_ops stub callbacks Ioana Ciornei @ 2019-11-21 19:15 ` Ioana Ciornei 2019-11-22 0:27 ` Russell King - ARM Linux admin 2 siblings, 1 reply; 8+ messages in thread From: Ioana Ciornei @ 2019-11-21 19:15 UTC (permalink / raw) To: davem, netdev; +Cc: linux, andrew, Ioana Ciornei The include/linux/phylink.h states: * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink expects * MAC driver to return all supported link modes. Make the necessary adjustment to meet the requirements. Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c index efc587515661..d93d71724e5a 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c @@ -95,6 +95,7 @@ static void dpaa2_mac_validate(struct phylink_config *config, phylink_set(mask, Asym_Pause); switch (state->interface) { + case PHY_INTERFACE_MODE_NA: case PHY_INTERFACE_MODE_RGMII: case PHY_INTERFACE_MODE_RGMII_ID: case PHY_INTERFACE_MODE_RGMII_RXID: -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 3/3] dpaa2-eth: return all supported link modes in PHY_INTERFACE_MODE_NA 2019-11-21 19:15 ` [PATCH net-next 3/3] dpaa2-eth: return all supported link modes in PHY_INTERFACE_MODE_NA Ioana Ciornei @ 2019-11-22 0:27 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 8+ messages in thread From: Russell King - ARM Linux admin @ 2019-11-22 0:27 UTC (permalink / raw) To: Ioana Ciornei; +Cc: davem, netdev, andrew On Thu, Nov 21, 2019 at 09:15:27PM +0200, Ioana Ciornei wrote: > The include/linux/phylink.h states: > * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink expects > * MAC driver to return all supported link modes. > > Make the necessary adjustment to meet the requirements. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> This is independent of patch 1. Acked-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > index efc587515661..d93d71724e5a 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > @@ -95,6 +95,7 @@ static void dpaa2_mac_validate(struct phylink_config *config, > phylink_set(mask, Asym_Pause); > > switch (state->interface) { > + case PHY_INTERFACE_MODE_NA: > case PHY_INTERFACE_MODE_RGMII: > case PHY_INTERFACE_MODE_RGMII_ID: > case PHY_INTERFACE_MODE_RGMII_RXID: > -- > 1.9.1 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-11-22 0:27 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-21 19:15 [PATCH net-next 0/3] dpaa2-eth: MAC phylink fixes Ioana Ciornei 2019-11-21 19:15 ` [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy() Ioana Ciornei 2019-11-21 19:50 ` Andrew Lunn 2019-11-21 23:48 ` Russell King - ARM Linux admin 2019-11-21 19:15 ` [PATCH net-next 2/3] dpaa2-eth: add phylink_mac_ops stub callbacks Ioana Ciornei 2019-11-22 0:27 ` Russell King - ARM Linux admin 2019-11-21 19:15 ` [PATCH net-next 3/3] dpaa2-eth: return all supported link modes in PHY_INTERFACE_MODE_NA Ioana Ciornei 2019-11-22 0:27 ` Russell King - ARM Linux admin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).