Commit 2291982e authored by Vladimir Oltean's avatar Vladimir Oltean Committed by Paolo Abeni
Browse files

net: dpaa2-eth: serialize changes to priv->mac with a mutex 



The dpaa2 architecture permits dynamic connections between objects on
the fsl-mc bus, specifically between a DPNI object (represented by a
struct net_device) and a DPMAC object (represented by a struct phylink).

The DPNI driver is notified when those connections are created/broken
through the dpni_irq0_handler_thread() method. To ensure that ethtool
operations, as well as netdev up/down operations serialize with the
connection/disconnection of the DPNI with a DPMAC,
dpni_irq0_handler_thread() takes the rtnl_lock() to block those other
operations from taking place.

There is code called by dpaa2_mac_connect() which wants to acquire the
rtnl_mutex once again, see phylink_create() -> phylink_register_sfp() ->
sfp_bus_add_upstream() -> rtnl_lock(). So the strategy doesn't quite
work out, even though it's fairly simple.

Create a different strategy, where all code paths in the dpaa2-eth
driver access priv->mac only while they are holding priv->mac_lock.
The phylink instance is not created or connected to the PHY under the
priv->mac_lock, but only assigned to priv->mac then. This will eliminate
the reliance on the rtnl_mutex.

Add lockdep annotations and put comments where holding the lock is not
necessary, and priv->mac can be dereferenced freely.

Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: default avatarIoana Ciornei <ioana.ciornei@nxp.com>
Tested-by: default avatarIoana Ciornei <ioana.ciornei@nxp.com>
Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
parent 55f90a4d

drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c

+38 −5
Original line number Diff line number Diff line
@@ -2147,8 +2147,11 @@ static int dpaa2_eth_link_state_update(struct dpaa2_eth_priv *priv) 


	/* When we manage the MAC/PHY using phylink there is no need

	 * to manually update the netif_carrier.

	 * We can avoid locking because we are called from the "link changed"

	 * IRQ handler, which is the same as the "endpoint changed" IRQ handler

	 * (the writer to priv->mac), so we cannot race with it.

	 */

	if (dpaa2_eth_is_type_phy(priv))

	if (dpaa2_mac_is_type_phy(priv->mac))

		goto out;



	/* Chech link state; speed / duplex changes are not treated yet */
@@ -2179,6 +2182,8 @@ static int dpaa2_eth_open(struct net_device *net_dev) 


	dpaa2_eth_seed_pools(priv);



	mutex_lock(&priv->mac_lock);



	if (!dpaa2_eth_is_type_phy(priv)) {

		/* We'll only start the txqs when the link is actually ready;

		 * make sure we don't race against the link up notification,
@@ -2197,6 +2202,7 @@ static int dpaa2_eth_open(struct net_device *net_dev) 


	err = dpni_enable(priv->mc_io, 0, priv->mc_token);

	if (err < 0) {

		mutex_unlock(&priv->mac_lock);

		netdev_err(net_dev, "dpni_enable() failed\n");

		goto enable_err;

	}
@@ -2204,6 +2210,8 @@ static int dpaa2_eth_open(struct net_device *net_dev) 
	if (dpaa2_eth_is_type_phy(priv))

		dpaa2_mac_start(priv->mac);



	mutex_unlock(&priv->mac_lock);



	return 0;



enable_err:
@@ -2275,6 +2283,8 @@ static int dpaa2_eth_stop(struct net_device *net_dev) 
	int dpni_enabled = 0;

	int retries = 10;



	mutex_lock(&priv->mac_lock);



	if (dpaa2_eth_is_type_phy(priv)) {

		dpaa2_mac_stop(priv->mac);

	} else {
@@ -2282,6 +2292,8 @@ static int dpaa2_eth_stop(struct net_device *net_dev) 
		netif_carrier_off(net_dev);

	}



	mutex_unlock(&priv->mac_lock);



	/* On dpni_disable(), the MC firmware will:

	 * - stop MAC Rx and wait for all Rx frames to be enqueued to software

	 * - cut off WRIOP dequeues from egress FQs and wait until transmission
@@ -2607,12 +2619,20 @@ static int dpaa2_eth_ts_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) 
static int dpaa2_eth_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)

{

	struct dpaa2_eth_priv *priv = netdev_priv(dev);

	int err;



	if (cmd == SIOCSHWTSTAMP)

		return dpaa2_eth_ts_ioctl(dev, rq, cmd);



	if (dpaa2_eth_is_type_phy(priv))

		return phylink_mii_ioctl(priv->mac->phylink, rq, cmd);

	mutex_lock(&priv->mac_lock);



	if (dpaa2_eth_is_type_phy(priv)) {

		err = phylink_mii_ioctl(priv->mac->phylink, rq, cmd);

		mutex_unlock(&priv->mac_lock);

		return err;

	}



	mutex_unlock(&priv->mac_lock);



	return -EOPNOTSUPP;

}
@@ -4639,7 +4659,9 @@ static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv) 
		}

	}



	mutex_lock(&priv->mac_lock);

	priv->mac = mac;

	mutex_unlock(&priv->mac_lock);



	return 0;
@@ -4652,9 +4674,12 @@ static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv) 


static void dpaa2_eth_disconnect_mac(struct dpaa2_eth_priv *priv)

{

	struct dpaa2_mac *mac = priv->mac;

	struct dpaa2_mac *mac;



	mutex_lock(&priv->mac_lock);

	mac = priv->mac;

	priv->mac = NULL;

	mutex_unlock(&priv->mac_lock);



	if (!mac)

		return;
@@ -4673,6 +4698,7 @@ static irqreturn_t dpni_irq0_handler_thread(int irq_num, void *arg) 
	struct fsl_mc_device *dpni_dev = to_fsl_mc_device(dev);

	struct net_device *net_dev = dev_get_drvdata(dev);

	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);

	bool had_mac;

	int err;



	err = dpni_get_irq_status(dpni_dev->mc_io, 0, dpni_dev->mc_handle,
@@ -4690,7 +4716,12 @@ static irqreturn_t dpni_irq0_handler_thread(int irq_num, void *arg) 
		dpaa2_eth_update_tx_fqids(priv);



		rtnl_lock();

		if (dpaa2_eth_has_mac(priv))

		/* We can avoid locking because the "endpoint changed" IRQ

		 * handler is the only one who changes priv->mac at runtime,

		 * so we are not racing with anyone.

		 */

		had_mac = !!priv->mac;

		if (had_mac)

			dpaa2_eth_disconnect_mac(priv);

		else

			dpaa2_eth_connect_mac(priv);
@@ -4792,6 +4823,8 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev) 
	priv->net_dev = net_dev;

	SET_NETDEV_DEVLINK_PORT(net_dev, &priv->devlink_port);



	mutex_init(&priv->mac_lock);



	priv->iommu_domain = iommu_get_domain_for_dev(dev);



	priv->tx_tstamp_type = HWTSTAMP_TX_OFF;

drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h

+6 −0
Original line number Diff line number Diff line
@@ -615,6 +615,8 @@ struct dpaa2_eth_priv { 
#endif



	struct dpaa2_mac *mac;

	/* Serializes changes to priv->mac */

	struct mutex		mac_lock;

	struct workqueue_struct	*dpaa2_ptp_wq;

	struct work_struct	tx_onestep_tstamp;

	struct sk_buff_head	tx_skbs;
@@ -768,11 +770,15 @@ static inline unsigned int dpaa2_eth_rx_head_room(struct dpaa2_eth_priv *priv) 


static inline bool dpaa2_eth_is_type_phy(struct dpaa2_eth_priv *priv)

{

	lockdep_assert_held(&priv->mac_lock);



	return dpaa2_mac_is_type_phy(priv->mac);

}



static inline bool dpaa2_eth_has_mac(struct dpaa2_eth_priv *priv)

{

	lockdep_assert_held(&priv->mac_lock);



	return priv->mac ? true : false;

}

drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c

+47 −11
Original line number Diff line number Diff line
@@ -85,11 +85,16 @@ static void dpaa2_eth_get_drvinfo(struct net_device *net_dev, 
static int dpaa2_eth_nway_reset(struct net_device *net_dev)

{

	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);

	int err = -EOPNOTSUPP;



	mutex_lock(&priv->mac_lock);



	if (dpaa2_eth_is_type_phy(priv))

		return phylink_ethtool_nway_reset(priv->mac->phylink);

		err = phylink_ethtool_nway_reset(priv->mac->phylink);



	return -EOPNOTSUPP;

	mutex_unlock(&priv->mac_lock);



	return err;

}



static int
@@ -97,10 +102,18 @@ dpaa2_eth_get_link_ksettings(struct net_device *net_dev, 
			     struct ethtool_link_ksettings *link_settings)

{

	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);

	int err;



	if (dpaa2_eth_is_type_phy(priv))

		return phylink_ethtool_ksettings_get(priv->mac->phylink,

	mutex_lock(&priv->mac_lock);



	if (dpaa2_eth_is_type_phy(priv)) {

		err = phylink_ethtool_ksettings_get(priv->mac->phylink,

						    link_settings);

		mutex_unlock(&priv->mac_lock);

		return err;

	}



	mutex_unlock(&priv->mac_lock);



	link_settings->base.autoneg = AUTONEG_DISABLE;

	if (!(priv->link_state.options & DPNI_LINK_OPT_HALF_DUPLEX))
@@ -115,11 +128,17 @@ dpaa2_eth_set_link_ksettings(struct net_device *net_dev, 
			     const struct ethtool_link_ksettings *link_settings)

{

	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);

	int err = -EOPNOTSUPP;



	if (!dpaa2_eth_is_type_phy(priv))

		return -EOPNOTSUPP;

	mutex_lock(&priv->mac_lock);



	if (dpaa2_eth_is_type_phy(priv))

		err = phylink_ethtool_ksettings_set(priv->mac->phylink,

						    link_settings);



	mutex_unlock(&priv->mac_lock);



	return phylink_ethtool_ksettings_set(priv->mac->phylink, link_settings);

	return err;

}



static void dpaa2_eth_get_pauseparam(struct net_device *net_dev,
@@ -128,11 +147,16 @@ static void dpaa2_eth_get_pauseparam(struct net_device *net_dev, 
	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);

	u64 link_options = priv->link_state.options;



	mutex_lock(&priv->mac_lock);



	if (dpaa2_eth_is_type_phy(priv)) {

		phylink_ethtool_get_pauseparam(priv->mac->phylink, pause);

		mutex_unlock(&priv->mac_lock);

		return;

	}



	mutex_unlock(&priv->mac_lock);



	pause->rx_pause = dpaa2_eth_rx_pause_enabled(link_options);

	pause->tx_pause = dpaa2_eth_tx_pause_enabled(link_options);

	pause->autoneg = AUTONEG_DISABLE;
@@ -151,9 +175,17 @@ static int dpaa2_eth_set_pauseparam(struct net_device *net_dev, 
		return -EOPNOTSUPP;

	}



	if (dpaa2_eth_is_type_phy(priv))

		return phylink_ethtool_set_pauseparam(priv->mac->phylink,

	mutex_lock(&priv->mac_lock);



	if (dpaa2_eth_is_type_phy(priv)) {

		err = phylink_ethtool_set_pauseparam(priv->mac->phylink,

						     pause);

		mutex_unlock(&priv->mac_lock);

		return err;

	}



	mutex_unlock(&priv->mac_lock);



	if (pause->autoneg)

		return -EOPNOTSUPP;
@@ -309,8 +341,12 @@ static void dpaa2_eth_get_ethtool_stats(struct net_device *net_dev, 
	}

	*(data + i++) = buf_cnt_total;



	mutex_lock(&priv->mac_lock);



	if (dpaa2_eth_has_mac(priv))

		dpaa2_mac_get_ethtool_stats(priv->mac, data + i);



	mutex_unlock(&priv->mac_lock);

}



static int dpaa2_eth_prep_eth_rule(struct ethhdr *eth_value, struct ethhdr *eth_mask,