[PATCH net 0/7] Bugfixes in Microsemi Ocelot switch driver

[PATCH net 0/7] Bugfixes in Microsemi Ocelot switch driver

[Vladimir Oltean]

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

This is a series of 7 assorted patches for "net", on the drivers for the
VSC7514 MIPS switch (Ocelot-1), the VSC9953 PowerPC (Seville), and a few
more that are common to all supported devices since they are in the
common library portion.

Vladimir Oltean (7):
  net: mscc: ocelot: fix race condition with TX timestamping
  net: mscc: ocelot: add locking for the port TX timestamp ID
  net: dsa: seville: fix buffer size of the queue system
  net: mscc: ocelot: check for errors on memory allocation of ports
  net: mscc: ocelot: error checking when calling ocelot_init()
  net: mscc: ocelot: refactor ports parsing code into a dedicated
    function
  net: mscc: ocelot: unregister net devices on unbind

 drivers/net/dsa/ocelot/felix.c             |   5 +-
 drivers/net/dsa/ocelot/seville_vsc9953.c   |   2 +-
 drivers/net/ethernet/mscc/ocelot.c         |  13 +-
 drivers/net/ethernet/mscc/ocelot_net.c     |  12 +-
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 234 ++++++++++++---------
 include/soc/mscc/ocelot.h                  |   1 +
 net/dsa/tag_ocelot.c                       |  11 +-
 7 files changed, 168 insertions(+), 110 deletions(-)

-- 
2.25.1

[PATCH net 1/7] net: mscc: ocelot: fix race condition with TX timestamping

[Vladimir Oltean]

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

[PATCH net 2/7] net: mscc: ocelot: add locking for the port TX timestamp ID

[Vladimir Oltean]

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

The ocelot_port->ts_id is used to:
- populate skb->cb[0] for matching the TX timestamp in the PTP IRQ with
  an skb.
- 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. 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.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c     | 13 ++++++++++++-
 drivers/net/ethernet/mscc/ocelot_net.c |  6 ++----
 include/soc/mscc/ocelot.h              |  1 +
 net/dsa/tag_ocelot.c                   | 11 +++++++----
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 5abb7d2b0a9e..8caf3afd464d 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;
@@ -1443,6 +1448,12 @@ int ocelot_init(struct ocelot *ocelot)
 	if (!ocelot->stats_queue)
 		return -ENOMEM;
 
+	for (port = 0; port < ocelot->num_phys_ports; port++) {
+		struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+		spin_lock_init(&ocelot_port->ts_id_lock);
+	}
+
 	INIT_LIST_HEAD(&ocelot->multicast);
 	ocelot_mact_init(ocelot);
 	ocelot_vlan_init(ocelot);
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

Re: [PATCH net 2/7] net: mscc: ocelot: add locking for the port TX timestamp ID

[Alexandre Belloni]

On 15/09/2020 21:22:24+0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The ocelot_port->ts_id is used to:
> - populate skb->cb[0] for matching the TX timestamp in the PTP IRQ with
>   an skb.
> - 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. 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.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/mscc/ocelot.c     | 13 ++++++++++++-
>  drivers/net/ethernet/mscc/ocelot_net.c |  6 ++----
>  include/soc/mscc/ocelot.h              |  1 +
>  net/dsa/tag_ocelot.c                   | 11 +++++++----
>  4 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 5abb7d2b0a9e..8caf3afd464d 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;
> @@ -1443,6 +1448,12 @@ int ocelot_init(struct ocelot *ocelot)
>  	if (!ocelot->stats_queue)
>  		return -ENOMEM;
>  
> +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
> +

At this point, ocelot_port is NULL because ocelot_init is called before
the port structures are allocated in mscc_ocelot_probe

> +		spin_lock_init(&ocelot_port->ts_id_lock);
> +	}
> +
>  	INIT_LIST_HEAD(&ocelot->multicast);
>  	ocelot_mact_init(ocelot);
>  	ocelot_vlan_init(ocelot);
> 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
> 

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

Re: [PATCH net 2/7] net: mscc: ocelot: add locking for the port TX timestamp ID

[Vladimir Oltean]

On Wed, Sep 16, 2020 at 01:12:04PM +0200, Alexandre Belloni wrote:
> > +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> > +		struct ocelot_port *ocelot_port = ocelot->ports[port];
> > +
> 
> At this point, ocelot_port is NULL because ocelot_init is called before
> the port structures are allocated in mscc_ocelot_probe

Yikes, you're right, thanks for testing!

-Vladimir

Re: [PATCH net 2/7] net: mscc: ocelot: add locking for the port TX timestamp ID

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Tue, 15 Sep 2020 21:22:24 +0300

> This is a problem because, at least theoretically, another timestampable
> skb might use the same ocelot_port->ts_id before that is incremented. So
> the logic of using and incrementing the timestamp id should be atomic
> per port.

Have you actually observed this race in practice?

All transmit calls are serialized by the netdev transmit spinlock.

Let's not add locking if it is not actually necessary.

Re: [PATCH net 2/7] net: mscc: ocelot: add locking for the port TX timestamp ID

[Vladimir Oltean]

On Wed, Sep 16, 2020 at 05:19:26PM -0700, David Miller wrote:
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Tue, 15 Sep 2020 21:22:24 +0300
>
> > This is a problem because, at least theoretically, another timestampable
> > skb might use the same ocelot_port->ts_id before that is incremented. So
> > the logic of using and incrementing the timestamp id should be atomic
> > per port.
>
> Have you actually observed this race in practice?
>
> All transmit calls are serialized by the netdev transmit spinlock.
>
> Let's not add locking if it is not actually necessary.

It's a bit more complicated.
This code is also used from DSA, and DSA now declares NETIF_F_LLTX.

Re: [PATCH net 2/7] net: mscc: ocelot: add locking for the port TX timestamp ID

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Fri, 18 Sep 2020 02:43:40 +0300

> On Wed, Sep 16, 2020 at 05:19:26PM -0700, David Miller wrote:
>> From: Vladimir Oltean <olteanv@gmail.com>
>> Date: Tue, 15 Sep 2020 21:22:24 +0300
>>
>> > This is a problem because, at least theoretically, another timestampable
>> > skb might use the same ocelot_port->ts_id before that is incremented. So
>> > the logic of using and incrementing the timestamp id should be atomic
>> > per port.
>>
>> Have you actually observed this race in practice?
>>
>> All transmit calls are serialized by the netdev transmit spinlock.
>>
>> Let's not add locking if it is not actually necessary.
> 
> It's a bit more complicated.
> This code is also used from DSA, and DSA now declares NETIF_F_LLTX.

Please document that in the commit log message, thank you.

[PATCH net 3/7] net: dsa: seville: fix buffer size of the queue system

[Vladimir Oltean]

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

[PATCH net 4/7] net: mscc: ocelot: check for errors on memory allocation of ports

[Vladimir Oltean]

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>
---
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 65408bc994c4..99872f1b7460 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -993,6 +993,8 @@ 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)
+		return -ENOMEM;
 
 	ocelot->vcap_is2_keys = vsc7514_vcap_is2_keys;
 	ocelot->vcap_is2_actions = vsc7514_vcap_is2_actions;
-- 
2.25.1

Re: [PATCH net 4/7] net: mscc: ocelot: check for errors on memory allocation of ports

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Tue, 15 Sep 2020 21:22:26 +0300

> @@ -993,6 +993,8 @@ 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)
> +		return -ENOMEM;

This leaks the reference to OF node 'ports'.

[PATCH net 5/7] net: mscc: ocelot: error checking when calling ocelot_init()

[Vladimir Oltean]

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>
---
 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 99872f1b7460..91a915d0693f 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -1000,7 +1000,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)
+		return err;
+
 	if (ocelot->ptp) {
 		err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info);
 		if (err) {
-- 
2.25.1

Re: [PATCH net 5/7] net: mscc: ocelot: error checking when calling ocelot_init()

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Tue, 15 Sep 2020 21:22:27 +0300

> diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> index 99872f1b7460..91a915d0693f 100644
> --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> @@ -1000,7 +1000,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)
> +		return err;
> +

This also leaks the OF device 'ports' object.

[PATCH net 6/7] net: mscc: ocelot: refactor ports parsing code into a dedicated function

[Vladimir Oltean]

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>
---
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 223 +++++++++++----------
 1 file changed, 118 insertions(+), 105 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 91a915d0693f..851e79e11aed 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -896,123 +896,26 @@ static struct ptp_clock_info ocelot_ptp_clock_info = {
 	.enable		= ocelot_ptp_enable,
 };
 
-static int mscc_ocelot_probe(struct platform_device *pdev)
+static int mscc_ocelot_init_ports(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
+	struct ocelot *ocelot = platform_get_drvdata(pdev);
+	struct device_node *np = ocelot->dev->of_node;
 	struct device_node *ports, *portnp;
-	int err, irq_xtr, irq_ptp_rdy;
-	struct ocelot *ocelot;
-	struct regmap *hsio;
-	unsigned int i;
-
-	struct {
-		enum ocelot_target id;
-		char *name;
-		u8 optional:1;
-	} io_target[] = {
-		{ SYS, "sys" },
-		{ REW, "rew" },
-		{ QSYS, "qsys" },
-		{ ANA, "ana" },
-		{ QS, "qs" },
-		{ S2, "s2" },
-		{ PTP, "ptp", 1 },
-	};
-
-	if (!np && !pdev->dev.platform_data)
-		return -ENODEV;
-
-	ocelot = devm_kzalloc(&pdev->dev, sizeof(*ocelot), GFP_KERNEL);
-	if (!ocelot)
-		return -ENOMEM;
-
-	platform_set_drvdata(pdev, ocelot);
-	ocelot->dev = &pdev->dev;
-
-	for (i = 0; i < ARRAY_SIZE(io_target); i++) {
-		struct regmap *target;
-		struct resource *res;
-
-		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-						   io_target[i].name);
-
-		target = ocelot_regmap_init(ocelot, res);
-		if (IS_ERR(target)) {
-			if (io_target[i].optional) {
-				ocelot->targets[io_target[i].id] = NULL;
-				continue;
-			}
-			return PTR_ERR(target);
-		}
-
-		ocelot->targets[io_target[i].id] = target;
-	}
-
-	hsio = syscon_regmap_lookup_by_compatible("mscc,ocelot-hsio");
-	if (IS_ERR(hsio)) {
-		dev_err(&pdev->dev, "missing hsio syscon\n");
-		return PTR_ERR(hsio);
-	}
-
-	ocelot->targets[HSIO] = hsio;
-
-	err = ocelot_chip_init(ocelot, &ocelot_ops);
-	if (err)
-		return err;
-
-	irq_xtr = platform_get_irq_byname(pdev, "xtr");
-	if (irq_xtr < 0)
-		return -ENODEV;
-
-	err = devm_request_threaded_irq(&pdev->dev, irq_xtr, NULL,
-					ocelot_xtr_irq_handler, IRQF_ONESHOT,
-					"frame extraction", ocelot);
-	if (err)
-		return err;
-
-	irq_ptp_rdy = platform_get_irq_byname(pdev, "ptp_rdy");
-	if (irq_ptp_rdy > 0 && ocelot->targets[PTP]) {
-		err = devm_request_threaded_irq(&pdev->dev, irq_ptp_rdy, NULL,
-						ocelot_ptp_rdy_irq_handler,
-						IRQF_ONESHOT, "ptp ready",
-						ocelot);
-		if (err)
-			return err;
-
-		/* Both the PTP interrupt and the PTP bank are available */
-		ocelot->ptp = 1;
-	}
+	int err;
 
 	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,
+	ocelot->ports = devm_kcalloc(ocelot->dev, ocelot->num_phys_ports,
 				     sizeof(struct ocelot_port *), GFP_KERNEL);
 	if (!ocelot->ports)
 		return -ENOMEM;
 
-	ocelot->vcap_is2_keys = vsc7514_vcap_is2_keys;
-	ocelot->vcap_is2_actions = vsc7514_vcap_is2_actions;
-	ocelot->vcap = vsc7514_vcap_props;
-
-	err = ocelot_init(ocelot);
-	if (err)
-		return err;
-
-	if (ocelot->ptp) {
-		err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info);
-		if (err) {
-			dev_err(ocelot->dev,
-				"Timestamp initialization failed\n");
-			ocelot->ptp = 0;
-		}
-	}
-
 	/* No NPI port */
 	ocelot_configure_cpu(ocelot, -1, OCELOT_TAG_PREFIX_NONE,
 			     OCELOT_TAG_PREFIX_NONE);
@@ -1103,14 +1006,124 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
 		priv->serdes = serdes;
 	}
 
+out_put_ports:
+	of_node_put(ports);
+	return err;
+}
+
+static int mscc_ocelot_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int err, irq_xtr, irq_ptp_rdy;
+	struct ocelot *ocelot;
+	struct regmap *hsio;
+	unsigned int i;
+
+	struct {
+		enum ocelot_target id;
+		char *name;
+		u8 optional:1;
+	} io_target[] = {
+		{ SYS, "sys" },
+		{ REW, "rew" },
+		{ QSYS, "qsys" },
+		{ ANA, "ana" },
+		{ QS, "qs" },
+		{ S2, "s2" },
+		{ PTP, "ptp", 1 },
+	};
+
+	if (!np && !pdev->dev.platform_data)
+		return -ENODEV;
+
+	ocelot = devm_kzalloc(&pdev->dev, sizeof(*ocelot), GFP_KERNEL);
+	if (!ocelot)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ocelot);
+	ocelot->dev = &pdev->dev;
+
+	for (i = 0; i < ARRAY_SIZE(io_target); i++) {
+		struct regmap *target;
+		struct resource *res;
+
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   io_target[i].name);
+
+		target = ocelot_regmap_init(ocelot, res);
+		if (IS_ERR(target)) {
+			if (io_target[i].optional) {
+				ocelot->targets[io_target[i].id] = NULL;
+				continue;
+			}
+			return PTR_ERR(target);
+		}
+
+		ocelot->targets[io_target[i].id] = target;
+	}
+
+	hsio = syscon_regmap_lookup_by_compatible("mscc,ocelot-hsio");
+	if (IS_ERR(hsio)) {
+		dev_err(&pdev->dev, "missing hsio syscon\n");
+		return PTR_ERR(hsio);
+	}
+
+	ocelot->targets[HSIO] = hsio;
+
+	err = ocelot_chip_init(ocelot, &ocelot_ops);
+	if (err)
+		return err;
+
+	irq_xtr = platform_get_irq_byname(pdev, "xtr");
+	if (irq_xtr < 0)
+		return -ENODEV;
+
+	err = devm_request_threaded_irq(&pdev->dev, irq_xtr, NULL,
+					ocelot_xtr_irq_handler, IRQF_ONESHOT,
+					"frame extraction", ocelot);
+	if (err)
+		return err;
+
+	irq_ptp_rdy = platform_get_irq_byname(pdev, "ptp_rdy");
+	if (irq_ptp_rdy > 0 && ocelot->targets[PTP]) {
+		err = devm_request_threaded_irq(&pdev->dev, irq_ptp_rdy, NULL,
+						ocelot_ptp_rdy_irq_handler,
+						IRQF_ONESHOT, "ptp ready",
+						ocelot);
+		if (err)
+			return err;
+
+		/* Both the PTP interrupt and the PTP bank are available */
+		ocelot->ptp = 1;
+	}
+
+	ocelot->vcap_is2_keys = vsc7514_vcap_is2_keys;
+	ocelot->vcap_is2_actions = vsc7514_vcap_is2_actions;
+	ocelot->vcap = vsc7514_vcap_props;
+
+	err = ocelot_init(ocelot);
+	if (err)
+		return err;
+
+	err = mscc_ocelot_init_ports(pdev);
+	if (err)
+		return err;
+
+	if (ocelot->ptp) {
+		err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info);
+		if (err) {
+			dev_err(ocelot->dev,
+				"Timestamp initialization failed\n");
+			ocelot->ptp = 0;
+		}
+	}
+
 	register_netdevice_notifier(&ocelot_netdevice_nb);
 	register_switchdev_notifier(&ocelot_switchdev_nb);
 	register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb);
 
 	dev_info(&pdev->dev, "Ocelot switch probed\n");
 
-out_put_ports:
-	of_node_put(ports);
 	return err;
 }
 
-- 
2.25.1

[PATCH net 7/7] net: mscc: ocelot: unregister net devices on unbind

[Vladimir Oltean]

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>
---
 drivers/net/ethernet/mscc/ocelot_vsc7514.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index 851e79e11aed..b8f895483653 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 ocelot *ocelot = platform_get_drvdata(pdev);
@@ -1008,6 +1028,7 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev)
 
 out_put_ports:
 	of_node_put(ports);
+	mscc_ocelot_release_ports(ocelot);
 	return err;
 }
 
@@ -1132,6 +1153,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

Re: [PATCH net 0/7] Bugfixes in Microsemi Ocelot switch driver

[Horatiu Vultur]

The 09/15/2020 21:22, Vladimir Oltean wrote:
> 
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> This is a series of 7 assorted patches for "net", on the drivers for the
> VSC7514 MIPS switch (Ocelot-1), the VSC9953 PowerPC (Seville), and a few
> more that are common to all supported devices since they are in the
> common library portion.

I have looked over ocelot changes and they look fine to me.

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

> 
> Vladimir Oltean (7):
>   net: mscc: ocelot: fix race condition with TX timestamping
>   net: mscc: ocelot: add locking for the port TX timestamp ID
>   net: dsa: seville: fix buffer size of the queue system
>   net: mscc: ocelot: check for errors on memory allocation of ports
>   net: mscc: ocelot: error checking when calling ocelot_init()
>   net: mscc: ocelot: refactor ports parsing code into a dedicated
>     function
>   net: mscc: ocelot: unregister net devices on unbind
> 
>  drivers/net/dsa/ocelot/felix.c             |   5 +-
>  drivers/net/dsa/ocelot/seville_vsc9953.c   |   2 +-
>  drivers/net/ethernet/mscc/ocelot.c         |  13 +-
>  drivers/net/ethernet/mscc/ocelot_net.c     |  12 +-
>  drivers/net/ethernet/mscc/ocelot_vsc7514.c | 234 ++++++++++++---------
>  include/soc/mscc/ocelot.h                  |   1 +
>  net/dsa/tag_ocelot.c                       |  11 +-
>  7 files changed, 168 insertions(+), 110 deletions(-)
> 
> --
> 2.25.1
> 

-- 
/Horatiu

Re: [PATCH net 0/7] Bugfixes in Microsemi Ocelot switch driver

[Alexandre Belloni]

Hi,

On 15/09/2020 21:22:22+0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> This is a series of 7 assorted patches for "net", on the drivers for the
> VSC7514 MIPS switch (Ocelot-1), the VSC9953 PowerPC (Seville), and a few
> more that are common to all supported devices since they are in the
> common library portion.
> 
> Vladimir Oltean (7):
>   net: mscc: ocelot: fix race condition with TX timestamping
>   net: mscc: ocelot: add locking for the port TX timestamp ID
>   net: dsa: seville: fix buffer size of the queue system
>   net: mscc: ocelot: check for errors on memory allocation of ports
>   net: mscc: ocelot: error checking when calling ocelot_init()
>   net: mscc: ocelot: refactor ports parsing code into a dedicated
>     function
>   net: mscc: ocelot: unregister net devices on unbind
> 
>  drivers/net/dsa/ocelot/felix.c             |   5 +-
>  drivers/net/dsa/ocelot/seville_vsc9953.c   |   2 +-
>  drivers/net/ethernet/mscc/ocelot.c         |  13 +-
>  drivers/net/ethernet/mscc/ocelot_net.c     |  12 +-
>  drivers/net/ethernet/mscc/ocelot_vsc7514.c | 234 ++++++++++++---------
>  include/soc/mscc/ocelot.h                  |   1 +
>  net/dsa/tag_ocelot.c                       |  11 +-
>  7 files changed, 168 insertions(+), 110 deletions(-)
> 

This series is leading to multiple crashes on my vc7524 board. I'm
trying to pinpoint the problematic commits


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