Re: [PATCH net-next 3/4] dpaa2-eth: add MAC/PHY support through phylink

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Oct 22, 2019 at 03:06:49AM +0200, Andrew Lunn wrote:
> Hi Ioana
> 
> > +static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv)
> > +{
> > +	struct fsl_mc_device *dpni_dev, *dpmac_dev;
> > +	struct dpaa2_mac *mac;
> > +	int err;
> > +
> > +	dpni_dev = to_fsl_mc_device(priv->net_dev->dev.parent);
> > +	dpmac_dev = fsl_mc_get_endpoint(dpni_dev);
> > +	if (!dpmac_dev || dpmac_dev->dev.type != &fsl_mc_bus_dpmac_type)
> > +		return 0;
> > +
> > +	if (dpaa2_mac_is_type_fixed(dpmac_dev, priv->mc_io))
> > +		return 0;
> > +
> > +	mac = kzalloc(sizeof(struct dpaa2_mac), GFP_KERNEL);
> > +	if (!mac)
> > +		return -ENOMEM;
> > +
> > +	mac->mc_dev = dpmac_dev;
> > +	mac->mc_io = priv->mc_io;
> > +	mac->net_dev = priv->net_dev;
> > +
> > +	err = dpaa2_mac_connect(mac);
> > +	if (err) {
> > +		netdev_err(priv->net_dev, "Error connecting to the MAC endpoint\n");
> > +		kfree(mac);
> > +		return err;
> > +	}
> > +	priv->mac = mac;
> > +
> > +	return 0;
> > +}
> > +
> > +static void dpaa2_eth_disconnect_mac(struct dpaa2_eth_priv *priv)
> > +{
> > +	if (!priv->mac)
> > +		return;
> > +
> > +	rtnl_lock();
> > +	dpaa2_mac_disconnect(priv->mac);
> > +	kfree(priv->mac);
> > +	priv->mac = NULL;
> > +	rtnl_unlock();
> > +}
> 
> dpaa2_eth_connect_mac() does not take the rtnl lock.
> dpaa2_eth_disconnect_mac() does. This asymmetry makes me think
> something is wrong. But it could be correct....

The way the driver is written, it's fine.

dpaa2_eth_connect_mac() is called prior to the netdev being registered.
At that point, nothing is published.

dpaa2_eth_disconnect_mac() is called _prior_ to the netdev being
unregistered, so there could be live accesses happening to the phy
and phylink.

-- 
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