[PATCH net-next 0/3] dpaa2-eth: MAC phylink fixes

[PATCH net-next 0/3] dpaa2-eth: MAC phylink fixes

[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

[PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy()

[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

[PATCH net-next 2/3] dpaa2-eth: add phylink_mac_ops stub callbacks

[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

[PATCH net-next 3/3] dpaa2-eth: return all supported link modes in PHY_INTERFACE_MODE_NA

[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

Re: [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy()

[Andrew Lunn]

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

Re: [PATCH net-next 1/3] dpaa2-eth: do not hold rtnl_lock on phylink_create() or _destroy()

[Russell King - ARM Linux admin]

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

Re: [PATCH net-next 2/3] dpaa2-eth: add phylink_mac_ops stub callbacks

[Russell King - ARM Linux admin]

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

Re: [PATCH net-next 3/3] dpaa2-eth: return all supported link modes in PHY_INTERFACE_MODE_NA

[Russell King - ARM Linux admin]

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