RE: [PATCH net-next v4 4/5] dpaa2-eth: add MAC/PHY support through phylink

[Ioana Ciornei <ioana.ciornei@nxp.com>]

> Subject: Re: [PATCH net-next v4 4/5] dpaa2-eth: add MAC/PHY support
> through phylink
> 
> On Thu, Oct 31, 2019 at 01:18:31AM +0200, Ioana Ciornei wrote:
> > The dpaa2-eth driver now has support for connecting to its associated
> > PHY device found through standard OF bindings.
> >
> > This happens when the DPNI object (that the driver probes on) gets
> > connected to a DPMAC. When that happens, the device tree is looked up
> > by the DPMAC ID, and the associated PHY bindings are found.
> >
> > The old logic of handling the net device's link state by hand still
> > needs to be kept, as the DPNI can be connected to other devices on the
> > bus than a DPMAC: other DPNI, DPSW ports, etc. This logic is only
> > engaged when there is no DPMAC (and therefore no phylink instance)
> > attached.
> >
> > The MC firmware support multiple type of DPMAC links: TYPE_FIXED,
> > TYPE_PHY. The TYPE_FIXED mode does not require any DPMAC
> management
> > from Linux side, and as such, the driver will not handle such a DPMAC.
> >
> > Although PHYLINK typically handles SFP cages and in-band AN modes, for
> > the moment the driver only supports the RGMII interfaces found on the
> > LX2160A. Support for other modes will come later.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> ...
> 
> > @@ -3363,6 +3425,13 @@ static irqreturn_t dpni_irq0_handler_thread(int
> irq_num, void *arg)
> >  	if (status & DPNI_IRQ_EVENT_ENDPOINT_CHANGED) {
> >  		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();
> 
> Sorry, but this locking is deadlock prone.
> 
> You're taking rtnl_lock().
> dpaa2_eth_connect_mac() calls dpaa2_mac_connect().
> dpaa2_mac_connect() calls phylink_create().
> phylink_create() calls phylink_register_sfp().
> phylink_register_sfp() calls sfp_bus_add_upstream().
> sfp_bus_add_upstream() calls rtnl_lock() *** DEADLOCK ***.

I recently discovered this also when working on adding support for SFPs.

> 
> Neither phylink_create() nor phylink_destroy() may be called holding the rtnl
> lock.
> 

Just to summarise:

* phylink_create() and phylink_destroy() should NOT be called with the rtnl lock held
* phylink_disconnect_phy() should be called with the lock
* depending on when the netdev is registered the phylink_of_phy_connect()
may be called with or without the rtnl lock

I'll have to move the lock and unlock around the actual _connect() and _disconnect_phy()
calls so that even the case where the DPMAC is connected at runtime
(the EVENT_ENDPOINT_CHANGED referred above) is treated correctly.

Thanks,
Ioana