diff mbox series

[v2,net,1/8] net: mscc: ocelot: fix race condition with TX timestamping

Message ID 20200918010730.2911234-2-olteanv@gmail.com
State New
Headers show
Series [v2,net,1/8] net: mscc: ocelot: fix race condition with TX timestamping | expand

Commit Message

Vladimir Oltean Sept. 18, 2020, 1:07 a.m. UTC
From: Vladimir Oltean <vladimir.oltean@nxp.com>

The TX-timestampable skb is added late to the ocelot_port->tx_skbs. It
is in a race with the TX timestamp IRQ, which checks that queue trying
to match the timestamp with the skb by the ts_id. The skb should be
added to the queue before the IRQ can fire.

Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
Changes in v2:
None.

 drivers/net/ethernet/mscc/ocelot_net.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Alexandre Belloni Sept. 18, 2020, 3:27 p.m. UTC | #1
On 18/09/2020 17:26:03+0200, Alexandre Belloni wrote:
> On 18/09/2020 04:07:23+0300, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > The TX-timestampable skb is added late to the ocelot_port->tx_skbs. It

> > is in a race with the TX timestamp IRQ, which checks that queue trying

> > to match the timestamp with the skb by the ts_id. The skb should be

> > added to the queue before the IRQ can fire.

> > 

> > Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> 

> Tested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>


Ok, this misfired, sorry about the noise.

> 

> > ---

> > Changes in v2:

> > None.

> > 

> >  drivers/net/ethernet/mscc/ocelot_net.c | 14 ++++++++------

> >  1 file changed, 8 insertions(+), 6 deletions(-)

> > 

> > diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c

> > index 0668d23cdbfa..cacabc23215a 100644

> > --- a/drivers/net/ethernet/mscc/ocelot_net.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_net.c

> > @@ -330,6 +330,7 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)

> >  	u8 grp = 0; /* Send everything on CPU group 0 */

> >  	unsigned int i, count, last;

> >  	int port = priv->chip_port;

> > +	bool do_tstamp;

> >  

> >  	val = ocelot_read(ocelot, QS_INJ_STATUS);

> >  	if (!(val & QS_INJ_STATUS_FIFO_RDY(BIT(grp))) ||

> > @@ -344,10 +345,14 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)

> >  	info.vid = skb_vlan_tag_get(skb);

> >  

> >  	/* Check if timestamping is needed */

> > +	do_tstamp = (ocelot_port_add_txtstamp_skb(ocelot_port, skb) == 0);

> > +

> >  	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP) {

> >  		info.rew_op = ocelot_port->ptp_cmd;

> > -		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)

> > +		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> >  			info.rew_op |= (ocelot_port->ts_id  % 4) << 3;

> > +			ocelot_port->ts_id++;

> > +		}

> >  	}

> >  

> >  	ocelot_gen_ifh(ifh, &info);

> > @@ -380,12 +385,9 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)

> >  	dev->stats.tx_packets++;

> >  	dev->stats.tx_bytes += skb->len;

> >  

> > -	if (!ocelot_port_add_txtstamp_skb(ocelot_port, skb)) {

> > -		ocelot_port->ts_id++;

> > -		return NETDEV_TX_OK;

> > -	}

> > +	if (!do_tstamp)

> > +		dev_kfree_skb_any(skb);

> >  

> > -	dev_kfree_skb_any(skb);

> >  	return NETDEV_TX_OK;

> >  }

> >  

> > -- 

> > 2.25.1

> > 

> 

> On 18/09/2020 04:07:24+0300, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > The ocelot_port->ts_id is used to:

> > (a) populate skb->cb[0] for matching the TX timestamp in the PTP IRQ

> >     with an skb.

> > (b) populate the REW_OP from the injection header of the ongoing skb.

> > Only then is ocelot_port->ts_id incremented.

> > 

> > This is a problem because, at least theoretically, another timestampable

> > skb might use the same ocelot_port->ts_id before that is incremented.

> > Normally all transmit calls are serialized by the netdev transmit

> > spinlock, but in this case, ocelot_port_add_txtstamp_skb() is also

> > called by DSA, which has started declaring the NETIF_F_LLTX feature

> > since commit 2b86cb829976 ("net: dsa: declare lockless TX feature for

> > slave ports").  So the logic of using and incrementing the timestamp id

> > should be atomic per port.

> > 

> > The solution is to use the global ocelot_port->ts_id only while

> > protected by the associated ocelot_port->ts_id_lock. That's where we

> > populate skb->cb[0]. Note that for ocelot, ocelot_port_add_txtstamp_skb

> > is called for the actual skb, but for felix, it is called for the skb's

> > clone. That is something which will also be changed in the future.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> > ---

> > Changes in v2:

> > Added an extra explanation about NETIF_F_LLTX in commit message.

> > 

> >  drivers/net/ethernet/mscc/ocelot.c     |  8 +++++++-

> >  drivers/net/ethernet/mscc/ocelot_net.c |  6 ++----

> >  include/soc/mscc/ocelot.h              |  1 +

> >  net/dsa/tag_ocelot.c                   | 11 +++++++----

> >  4 files changed, 17 insertions(+), 9 deletions(-)

> > 

> > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c

> > index 5abb7d2b0a9e..83eb7c325061 100644

> > --- a/drivers/net/ethernet/mscc/ocelot.c

> > +++ b/drivers/net/ethernet/mscc/ocelot.c

> > @@ -421,10 +421,15 @@ int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,

> >  

> >  	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&

> >  	    ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> > +		spin_lock(&ocelot_port->ts_id_lock);

> > +

> >  		shinfo->tx_flags |= SKBTX_IN_PROGRESS;

> >  		/* Store timestamp ID in cb[0] of sk_buff */

> > -		skb->cb[0] = ocelot_port->ts_id % 4;

> > +		skb->cb[0] = ocelot_port->ts_id;

> > +		ocelot_port->ts_id = (ocelot_port->ts_id + 1) % 4;

> >  		skb_queue_tail(&ocelot_port->tx_skbs, skb);

> > +

> > +		spin_unlock(&ocelot_port->ts_id_lock);

> >  		return 0;

> >  	}

> >  	return -ENODATA;

> > @@ -1300,6 +1305,7 @@ void ocelot_init_port(struct ocelot *ocelot, int port)

> >  	struct ocelot_port *ocelot_port = ocelot->ports[port];

> >  

> >  	skb_queue_head_init(&ocelot_port->tx_skbs);

> > +	spin_lock_init(&ocelot_port->ts_id_lock);

> >  

> >  	/* Basic L2 initialization */

> >  

> > diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c

> > index cacabc23215a..8490e42e9e2d 100644

> > --- a/drivers/net/ethernet/mscc/ocelot_net.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_net.c

> > @@ -349,10 +349,8 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)

> >  

> >  	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP) {

> >  		info.rew_op = ocelot_port->ptp_cmd;

> > -		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> > -			info.rew_op |= (ocelot_port->ts_id  % 4) << 3;

> > -			ocelot_port->ts_id++;

> > -		}

> > +		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)

> > +			info.rew_op |= skb->cb[0] << 3;

> >  	}

> >  

> >  	ocelot_gen_ifh(ifh, &info);

> > diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h

> > index da369b12005f..4521dd602ddc 100644

> > --- a/include/soc/mscc/ocelot.h

> > +++ b/include/soc/mscc/ocelot.h

> > @@ -566,6 +566,7 @@ struct ocelot_port {

> >  	u8				ptp_cmd;

> >  	struct sk_buff_head		tx_skbs;

> >  	u8				ts_id;

> > +	spinlock_t			ts_id_lock;

> >  

> >  	phy_interface_t			phy_mode;

> >  

> > diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c

> > index 42f327c06dca..b4fc05cafaa6 100644

> > --- a/net/dsa/tag_ocelot.c

> > +++ b/net/dsa/tag_ocelot.c

> > @@ -160,11 +160,14 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,

> >  	packing(injection, &qos_class, 19,  17, OCELOT_TAG_LEN, PACK, 0);

> >  

> >  	if (ocelot->ptp && (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {

> > +		struct sk_buff *clone = DSA_SKB_CB(skb)->clone;

> > +

> >  		rew_op = ocelot_port->ptp_cmd;

> > -		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> > -			rew_op |= (ocelot_port->ts_id  % 4) << 3;

> > -			ocelot_port->ts_id++;

> > -		}

> > +		/* Retrieve timestamp ID populated inside skb->cb[0] of the

> > +		 * clone by ocelot_port_add_txtstamp_skb

> > +		 */

> > +		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)

> > +			rew_op |= clone->cb[0] << 3;

> >  

> >  		packing(injection, &rew_op, 125, 117, OCELOT_TAG_LEN, PACK, 0);

> >  	}

> > -- 

> > 2.25.1

> > 

> 

> On 18/09/2020 04:07:25+0300, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > The VSC9953 Seville switch has 2 megabits of buffer split into 4360

> > words of 60 bytes each.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> > ---

> > Changes in v2:

> > None.

> > 

> >  drivers/net/dsa/ocelot/seville_vsc9953.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c

> > index 2d6a5f5758f8..83a1ab9393e9 100644

> > --- a/drivers/net/dsa/ocelot/seville_vsc9953.c

> > +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c

> > @@ -1018,7 +1018,7 @@ static const struct felix_info seville_info_vsc9953 = {

> >  	.vcap_is2_keys		= vsc9953_vcap_is2_keys,

> >  	.vcap_is2_actions	= vsc9953_vcap_is2_actions,

> >  	.vcap			= vsc9953_vcap_props,

> > -	.shared_queue_sz	= 128 * 1024,

> > +	.shared_queue_sz	= 2048 * 1024,

> >  	.num_mact_rows		= 2048,

> >  	.num_ports		= 10,

> >  	.mdio_bus_alloc		= vsc9953_mdio_bus_alloc,

> > -- 

> > 2.25.1

> > 

> 

> On 18/09/2020 04:07:26+0300, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > Do not proceed probing if we couldn't allocate memory for the ports

> > array, just error out.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> > ---

> > Changes in v2:

> > Stopped leaking the 'ports' OF node.

> > 

> >  drivers/net/ethernet/mscc/ocelot_vsc7514.c | 4 ++++

> >  1 file changed, 4 insertions(+)

> > 

> > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > index 65408bc994c4..904ea299a5e8 100644

> > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > @@ -993,6 +993,10 @@ static int mscc_ocelot_probe(struct platform_device *pdev)

> >  

> >  	ocelot->ports = devm_kcalloc(&pdev->dev, ocelot->num_phys_ports,

> >  				     sizeof(struct ocelot_port *), GFP_KERNEL);

> > +	if (!ocelot->ports) {

> > +		err = -ENOMEM;

> > +		goto out_put_ports;

> > +	}

> >  

> >  	ocelot->vcap_is2_keys = vsc7514_vcap_is2_keys;

> >  	ocelot->vcap_is2_actions = vsc7514_vcap_is2_actions;

> > -- 

> > 2.25.1

> > 

> 

> On 18/09/2020 04:07:27+0300, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > ocelot_init() allocates memory, resets the switch and polls for a status

> > register, things which can fail. Stop probing the driver in that case,

> > and propagate the error result.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> > ---

> > Changes in v2:

> > Stopped leaking the 'ports' OF node in the VSC7514 driver.

> > 

> >  drivers/net/dsa/ocelot/felix.c             | 5 ++++-

> >  drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-

> >  2 files changed, 8 insertions(+), 2 deletions(-)

> > 

> > diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c

> > index a1e1d3824110..f7b43f8d56ed 100644

> > --- a/drivers/net/dsa/ocelot/felix.c

> > +++ b/drivers/net/dsa/ocelot/felix.c

> > @@ -571,7 +571,10 @@ static int felix_setup(struct dsa_switch *ds)

> >  	if (err)

> >  		return err;

> >  

> > -	ocelot_init(ocelot);

> > +	err = ocelot_init(ocelot);

> > +	if (err)

> > +		return err;

> > +

> >  	if (ocelot->ptp) {

> >  		err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info);

> >  		if (err) {

> > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > index 904ea299a5e8..a1cbb20a7757 100644

> > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > @@ -1002,7 +1002,10 @@ static int mscc_ocelot_probe(struct platform_device *pdev)

> >  	ocelot->vcap_is2_actions = vsc7514_vcap_is2_actions;

> >  	ocelot->vcap = vsc7514_vcap_props;

> >  

> > -	ocelot_init(ocelot);

> > +	err = ocelot_init(ocelot);

> > +	if (err)

> > +		goto out_put_ports;

> > +

> >  	if (ocelot->ptp) {

> >  		err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info);

> >  		if (err) {

> > -- 

> > 2.25.1

> > 

> 

> On 18/09/2020 04:07:28+0300, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > mscc_ocelot_probe() is already pretty large and hard to follow. So move

> > the code for parsing ports in a separate function.

> > 

> > This makes it easier for the next patch to just call

> > mscc_ocelot_release_ports from the error path of mscc_ocelot_init_ports.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> > ---

> > Changes in v2:

> > Keep a reference to the 'ports' OF node at caller side, in

> > mscc_ocelot_probe, because we need to populate ocelot->num_phys_ports

> > early. The ocelot_init() function depends on it being set correctly.

> > 

> >  drivers/net/ethernet/mscc/ocelot_vsc7514.c | 209 +++++++++++----------

> >  1 file changed, 110 insertions(+), 99 deletions(-)

> > 

> > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > index a1cbb20a7757..ff4a01424953 100644

> > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > @@ -896,11 +896,115 @@ static struct ptp_clock_info ocelot_ptp_clock_info = {

> >  	.enable		= ocelot_ptp_enable,

> >  };

> >  

> > +static int mscc_ocelot_init_ports(struct platform_device *pdev,

> > +				  struct device_node *ports)

> > +{

> > +	struct ocelot *ocelot = platform_get_drvdata(pdev);

> > +	struct device_node *portnp;

> > +	int err;

> > +

> > +	ocelot->ports = devm_kcalloc(ocelot->dev, ocelot->num_phys_ports,

> > +				     sizeof(struct ocelot_port *), GFP_KERNEL);

> > +	if (!ocelot->ports)

> > +		return -ENOMEM;

> > +

> > +	/* No NPI port */

> > +	ocelot_configure_cpu(ocelot, -1, OCELOT_TAG_PREFIX_NONE,

> > +			     OCELOT_TAG_PREFIX_NONE);

> > +

> > +	for_each_available_child_of_node(ports, portnp) {

> > +		struct ocelot_port_private *priv;

> > +		struct ocelot_port *ocelot_port;

> > +		struct device_node *phy_node;

> > +		phy_interface_t phy_mode;

> > +		struct phy_device *phy;

> > +		struct regmap *target;

> > +		struct resource *res;

> > +		struct phy *serdes;

> > +		char res_name[8];

> > +		u32 port;

> > +

> > +		if (of_property_read_u32(portnp, "reg", &port))

> > +			continue;

> > +

> > +		snprintf(res_name, sizeof(res_name), "port%d", port);

> > +

> > +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

> > +						   res_name);

> > +		target = ocelot_regmap_init(ocelot, res);

> > +		if (IS_ERR(target))

> > +			continue;

> > +

> > +		phy_node = of_parse_phandle(portnp, "phy-handle", 0);

> > +		if (!phy_node)

> > +			continue;

> > +

> > +		phy = of_phy_find_device(phy_node);

> > +		of_node_put(phy_node);

> > +		if (!phy)

> > +			continue;

> > +

> > +		err = ocelot_probe_port(ocelot, port, target, phy);

> > +		if (err) {

> > +			of_node_put(portnp);

> > +			return err;

> > +		}

> > +

> > +		ocelot_port = ocelot->ports[port];

> > +		priv = container_of(ocelot_port, struct ocelot_port_private,

> > +				    port);

> > +

> > +		of_get_phy_mode(portnp, &phy_mode);

> > +

> > +		ocelot_port->phy_mode = phy_mode;

> > +

> > +		switch (ocelot_port->phy_mode) {

> > +		case PHY_INTERFACE_MODE_NA:

> > +			continue;

> > +		case PHY_INTERFACE_MODE_SGMII:

> > +			break;

> > +		case PHY_INTERFACE_MODE_QSGMII:

> > +			/* Ensure clock signals and speed is set on all

> > +			 * QSGMII links

> > +			 */

> > +			ocelot_port_writel(ocelot_port,

> > +					   DEV_CLOCK_CFG_LINK_SPEED

> > +					   (OCELOT_SPEED_1000),

> > +					   DEV_CLOCK_CFG);

> > +			break;

> > +		default:

> > +			dev_err(ocelot->dev,

> > +				"invalid phy mode for port%d, (Q)SGMII only\n",

> > +				port);

> > +			of_node_put(portnp);

> > +			return -EINVAL;

> > +		}

> > +

> > +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);

> > +		if (IS_ERR(serdes)) {

> > +			err = PTR_ERR(serdes);

> > +			if (err == -EPROBE_DEFER)

> > +				dev_dbg(ocelot->dev, "deferring probe\n");

> > +			else

> > +				dev_err(ocelot->dev,

> > +					"missing SerDes phys for port%d\n",

> > +					port);

> > +

> > +			of_node_put(portnp);

> > +			return err;

> > +		}

> > +

> > +		priv->serdes = serdes;

> > +	}

> > +

> > +	return 0;

> > +}

> > +

> >  static int mscc_ocelot_probe(struct platform_device *pdev)

> >  {

> >  	struct device_node *np = pdev->dev.of_node;

> > -	struct device_node *ports, *portnp;

> >  	int err, irq_xtr, irq_ptp_rdy;

> > +	struct device_node *ports;

> >  	struct ocelot *ocelot;

> >  	struct regmap *hsio;

> >  	unsigned int i;

> > @@ -985,19 +1089,12 @@ static int mscc_ocelot_probe(struct platform_device *pdev)

> >  

> >  	ports = of_get_child_by_name(np, "ethernet-ports");

> >  	if (!ports) {

> > -		dev_err(&pdev->dev, "no ethernet-ports child node found\n");

> > +		dev_err(ocelot->dev, "no ethernet-ports child node found\n");

> >  		return -ENODEV;

> >  	}

> >  

> >  	ocelot->num_phys_ports = of_get_child_count(ports);

> >  

> > -	ocelot->ports = devm_kcalloc(&pdev->dev, ocelot->num_phys_ports,

> > -				     sizeof(struct ocelot_port *), GFP_KERNEL);

> > -	if (!ocelot->ports) {

> > -		err = -ENOMEM;

> > -		goto out_put_ports;

> > -	}

> > -

> >  	ocelot->vcap_is2_keys = vsc7514_vcap_is2_keys;

> >  	ocelot->vcap_is2_actions = vsc7514_vcap_is2_actions;

> >  	ocelot->vcap = vsc7514_vcap_props;

> > @@ -1006,6 +1103,10 @@ static int mscc_ocelot_probe(struct platform_device *pdev)

> >  	if (err)

> >  		goto out_put_ports;

> >  

> > +	err = mscc_ocelot_init_ports(pdev, ports);

> > +	if (err)

> > +		goto out_put_ports;

> > +

> >  	if (ocelot->ptp) {

> >  		err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info);

> >  		if (err) {

> > @@ -1015,96 +1116,6 @@ static int mscc_ocelot_probe(struct platform_device *pdev)

> >  		}

> >  	}

> >  

> > -	/* No NPI port */

> > -	ocelot_configure_cpu(ocelot, -1, OCELOT_TAG_PREFIX_NONE,

> > -			     OCELOT_TAG_PREFIX_NONE);

> > -

> > -	for_each_available_child_of_node(ports, portnp) {

> > -		struct ocelot_port_private *priv;

> > -		struct ocelot_port *ocelot_port;

> > -		struct device_node *phy_node;

> > -		phy_interface_t phy_mode;

> > -		struct phy_device *phy;

> > -		struct regmap *target;

> > -		struct resource *res;

> > -		struct phy *serdes;

> > -		char res_name[8];

> > -		u32 port;

> > -

> > -		if (of_property_read_u32(portnp, "reg", &port))

> > -			continue;

> > -

> > -		snprintf(res_name, sizeof(res_name), "port%d", port);

> > -

> > -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

> > -						   res_name);

> > -		target = ocelot_regmap_init(ocelot, res);

> > -		if (IS_ERR(target))

> > -			continue;

> > -

> > -		phy_node = of_parse_phandle(portnp, "phy-handle", 0);

> > -		if (!phy_node)

> > -			continue;

> > -

> > -		phy = of_phy_find_device(phy_node);

> > -		of_node_put(phy_node);

> > -		if (!phy)

> > -			continue;

> > -

> > -		err = ocelot_probe_port(ocelot, port, target, phy);

> > -		if (err) {

> > -			of_node_put(portnp);

> > -			goto out_put_ports;

> > -		}

> > -

> > -		ocelot_port = ocelot->ports[port];

> > -		priv = container_of(ocelot_port, struct ocelot_port_private,

> > -				    port);

> > -

> > -		of_get_phy_mode(portnp, &phy_mode);

> > -

> > -		ocelot_port->phy_mode = phy_mode;

> > -

> > -		switch (ocelot_port->phy_mode) {

> > -		case PHY_INTERFACE_MODE_NA:

> > -			continue;

> > -		case PHY_INTERFACE_MODE_SGMII:

> > -			break;

> > -		case PHY_INTERFACE_MODE_QSGMII:

> > -			/* Ensure clock signals and speed is set on all

> > -			 * QSGMII links

> > -			 */

> > -			ocelot_port_writel(ocelot_port,

> > -					   DEV_CLOCK_CFG_LINK_SPEED

> > -					   (OCELOT_SPEED_1000),

> > -					   DEV_CLOCK_CFG);

> > -			break;

> > -		default:

> > -			dev_err(ocelot->dev,

> > -				"invalid phy mode for port%d, (Q)SGMII only\n",

> > -				port);

> > -			of_node_put(portnp);

> > -			err = -EINVAL;

> > -			goto out_put_ports;

> > -		}

> > -

> > -		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);

> > -		if (IS_ERR(serdes)) {

> > -			err = PTR_ERR(serdes);

> > -			if (err == -EPROBE_DEFER)

> > -				dev_dbg(ocelot->dev, "deferring probe\n");

> > -			else

> > -				dev_err(ocelot->dev,

> > -					"missing SerDes phys for port%d\n",

> > -					port);

> > -

> > -			of_node_put(portnp);

> > -			goto out_put_ports;

> > -		}

> > -

> > -		priv->serdes = serdes;

> > -	}

> > -

> >  	register_netdevice_notifier(&ocelot_netdevice_nb);

> >  	register_switchdev_notifier(&ocelot_switchdev_nb);

> >  	register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb);

> > -- 

> > 2.25.1

> > 

> 

> On 18/09/2020 04:07:29+0300, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > This driver was not unregistering its network interfaces on unbind.

> > Now it is.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> > ---

> > Changes in v2:

> > No longer call mscc_ocelot_release_ports from the regular exit path of

> > mscc_ocelot_init_ports, which was incorrect.

> > 

> >  drivers/net/ethernet/mscc/ocelot_vsc7514.c | 21 +++++++++++++++++++++

> >  1 file changed, 21 insertions(+)

> > 

> > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > index ff4a01424953..252c49b5f22b 100644

> > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > @@ -896,6 +896,26 @@ static struct ptp_clock_info ocelot_ptp_clock_info = {

> >  	.enable		= ocelot_ptp_enable,

> >  };

> >  

> > +static void mscc_ocelot_release_ports(struct ocelot *ocelot)

> > +{

> > +	int port;

> > +

> > +	for (port = 0; port < ocelot->num_phys_ports; port++) {

> > +		struct ocelot_port_private *priv;

> > +		struct ocelot_port *ocelot_port;

> > +

> > +		ocelot_port = ocelot->ports[port];

> > +		if (!ocelot_port)

> > +			continue;

> > +

> > +		priv = container_of(ocelot_port, struct ocelot_port_private,

> > +				    port);

> > +

> > +		unregister_netdev(priv->dev);

> > +		free_netdev(priv->dev);

> > +	}

> > +}

> > +

> >  static int mscc_ocelot_init_ports(struct platform_device *pdev,

> >  				  struct device_node *ports)

> >  {

> > @@ -1132,6 +1152,7 @@ static int mscc_ocelot_remove(struct platform_device *pdev)

> >  	struct ocelot *ocelot = platform_get_drvdata(pdev);

> >  

> >  	ocelot_deinit_timestamp(ocelot);

> > +	mscc_ocelot_release_ports(ocelot);

> >  	ocelot_deinit(ocelot);

> >  	unregister_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb);

> >  	unregister_switchdev_notifier(&ocelot_switchdev_nb);

> > -- 

> > 2.25.1

> > 

> 

> On 18/09/2020 04:07:30+0300, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > Currently mscc_ocelot_init_ports() will skip initializing a port when it

> > doesn't have a phy-handle, so the ocelot->ports[port] pointer will be

> > NULL. Take this into consideration when tearing down the driver, and add

> > a new function ocelot_deinit_port() to the switch library, mirror of

> > ocelot_init_port(), which needs to be called by the driver for all ports

> > it has initialized.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > ---

> > Changes in v2:

> > Patch is new.

> > 

> >  drivers/net/dsa/ocelot/felix.c             |  3 +++

> >  drivers/net/ethernet/mscc/ocelot.c         | 16 ++++++++--------

> >  drivers/net/ethernet/mscc/ocelot_vsc7514.c |  2 ++

> >  include/soc/mscc/ocelot.h                  |  1 +

> >  4 files changed, 14 insertions(+), 8 deletions(-)

> > 

> > diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c

> > index f7b43f8d56ed..64939ee14648 100644

> > --- a/drivers/net/dsa/ocelot/felix.c

> > +++ b/drivers/net/dsa/ocelot/felix.c

> > @@ -624,10 +624,13 @@ static void felix_teardown(struct dsa_switch *ds)

> >  {

> >  	struct ocelot *ocelot = ds->priv;

> >  	struct felix *felix = ocelot_to_felix(ocelot);

> > +	int port;

> >  

> >  	if (felix->info->mdio_bus_free)

> >  		felix->info->mdio_bus_free(ocelot);

> >  

> > +	for (port = 0; port < ocelot->num_phys_ports; port++)

> > +		ocelot_deinit_port(ocelot, port);

> >  	ocelot_deinit_timestamp(ocelot);

> >  	/* stop workqueue thread */

> >  	ocelot_deinit(ocelot);

> > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c

> > index 83eb7c325061..8518e1d60da4 100644

> > --- a/drivers/net/ethernet/mscc/ocelot.c

> > +++ b/drivers/net/ethernet/mscc/ocelot.c

> > @@ -1550,18 +1550,18 @@ EXPORT_SYMBOL(ocelot_init);

> >  

> >  void ocelot_deinit(struct ocelot *ocelot)

> >  {

> > -	struct ocelot_port *port;

> > -	int i;

> > -

> >  	cancel_delayed_work(&ocelot->stats_work);

> >  	destroy_workqueue(ocelot->stats_queue);

> >  	mutex_destroy(&ocelot->stats_lock);

> > -

> > -	for (i = 0; i < ocelot->num_phys_ports; i++) {

> > -		port = ocelot->ports[i];

> > -		skb_queue_purge(&port->tx_skbs);

> > -	}

> >  }

> >  EXPORT_SYMBOL(ocelot_deinit);

> >  

> > +void ocelot_deinit_port(struct ocelot *ocelot, int port)

> > +{

> > +	struct ocelot_port *ocelot_port = ocelot->ports[port];

> > +

> > +	skb_queue_purge(&ocelot_port->tx_skbs);

> > +}

> > +EXPORT_SYMBOL(ocelot_deinit_port);

> > +

> >  MODULE_LICENSE("Dual MIT/GPL");

> > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > index 252c49b5f22b..e02fb8bfab63 100644

> > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > @@ -908,6 +908,8 @@ static void mscc_ocelot_release_ports(struct ocelot *ocelot)

> >  		if (!ocelot_port)

> >  			continue;

> >  

> > +		ocelot_deinit_port(ocelot, port);

> > +

> >  		priv = container_of(ocelot_port, struct ocelot_port_private,

> >  				    port);

> >  

> > diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h

> > index 4521dd602ddc..0ac4e7fba086 100644

> > --- a/include/soc/mscc/ocelot.h

> > +++ b/include/soc/mscc/ocelot.h

> > @@ -678,6 +678,7 @@ void ocelot_configure_cpu(struct ocelot *ocelot, int npi,

> >  int ocelot_init(struct ocelot *ocelot);

> >  void ocelot_deinit(struct ocelot *ocelot);

> >  void ocelot_init_port(struct ocelot *ocelot, int port);

> > +void ocelot_deinit_port(struct ocelot *ocelot, int port);

> >  

> >  /* DSA callbacks */

> >  void ocelot_port_enable(struct ocelot *ocelot, int port,

> > -- 

> > 2.25.1

> > 

> 

> 

> -- 

> Alexandre Belloni, Bootlin

> Embedded Linux and Kernel engineering

> https://bootlin.com


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 0668d23cdbfa..cacabc23215a 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -330,6 +330,7 @@  static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
 	u8 grp = 0; /* Send everything on CPU group 0 */
 	unsigned int i, count, last;
 	int port = priv->chip_port;
+	bool do_tstamp;
 
 	val = ocelot_read(ocelot, QS_INJ_STATUS);
 	if (!(val & QS_INJ_STATUS_FIFO_RDY(BIT(grp))) ||
@@ -344,10 +345,14 @@  static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
 	info.vid = skb_vlan_tag_get(skb);
 
 	/* Check if timestamping is needed */
+	do_tstamp = (ocelot_port_add_txtstamp_skb(ocelot_port, skb) == 0);
+
 	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP) {
 		info.rew_op = ocelot_port->ptp_cmd;
-		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
+		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
 			info.rew_op |= (ocelot_port->ts_id  % 4) << 3;
+			ocelot_port->ts_id++;
+		}
 	}
 
 	ocelot_gen_ifh(ifh, &info);
@@ -380,12 +385,9 @@  static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
 	dev->stats.tx_packets++;
 	dev->stats.tx_bytes += skb->len;
 
-	if (!ocelot_port_add_txtstamp_skb(ocelot_port, skb)) {
-		ocelot_port->ts_id++;
-		return NETDEV_TX_OK;
-	}
+	if (!do_tstamp)
+		dev_kfree_skb_any(skb);
 
-	dev_kfree_skb_any(skb);
 	return NETDEV_TX_OK;
 }