[PATCH net 0/6] net: bcmasp: Collection of fixes

[PATCH net 0/6] net: bcmasp: Collection of fixes

[Florian Fainelli]

This patch series is a collection of bug fixes accumulated during a LLM
coding session. Thanks to Justin for the internal review and helping
with the last patch.

Florian Fainelli (6):
  net: bcmasp: fix mib counters struct alignment with ethtool stats
  net: bcmasp: unmap previous DMA mappings on TX map failure
  net: bcmasp: validate minimum RX packet size in bcmasp_rx_poll()
  net: bcmasp: fix OF node reference leak for phy_dn
  net: bcmasp: account for offload header in TX short packet padding
  net: bcmasp: fix network filter lookup and wake filter pair allocation

 drivers/net/ethernet/broadcom/asp2/bcmasp.c   | 127 +++++++++++-------
 drivers/net/ethernet/broadcom/asp2/bcmasp.h   |   4 -
 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  |  25 +++-
 3 files changed, 99 insertions(+), 57 deletions(-)

-- 
2.34.1

[PATCH net 1/6] net: bcmasp: fix mib counters struct alignment with ethtool stats

[Florian Fainelli]

When EDPKT stats were removed in commit e9f31435ee7d ("net: bcmasp: Add
support for asp-v3.0"), the corresponding entries were removed from
bcmasp_gstrings_stats in bcmasp_ethtool.c, but the edpkt_* fields were
left at the beginning of struct bcmasp_mib_counters in bcmasp.h.

Because bcmasp_update_mib_counters() and bcmasp_get_ethtool_stats() index
into struct bcmasp_mib_counters sequentially based on the order of
bcmasp_gstrings_stats, this 16-byte offset caused hardware statistics
to be written to the wrong struct members and software counters to be
read from incorrect offsets (and reading out-of-bounds at the end of the
struct).

Remove the obsolete edpkt_* fields from struct bcmasp_mib_counters so
that it aligns with bcmasp_gstrings_stats.

Fixes: e9f31435ee7d ("net: bcmasp: Add support for asp-v3.0")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.h b/drivers/net/ethernet/broadcom/asp2/bcmasp.h
index 8c8ffaeadc79..9c9721da1662 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.h
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.h
@@ -250,10 +250,6 @@ struct bcmasp_intf_stats64 {
 };
 
 struct bcmasp_mib_counters {
-	u32	edpkt_ts;
-	u32	edpkt_rx_pkt_cnt;
-	u32	edpkt_hdr_ext_cnt;
-	u32	edpkt_hdr_out_cnt;
 	u32	umac_frm_cnt;
 	u32	fb_frm_cnt;
 	u32	fb_rx_fifo_depth;
-- 
2.34.1

[PATCH net 2/6] net: bcmasp: unmap previous DMA mappings on TX map failure

[Florian Fainelli]

When mapping an skb with fragments for transmission in bcmasp_xmit(),
if mapping fails on fragment i > 0, the error handler calls
bcmasp_clean_txcb() for previous iterations j < i. However,
bcmasp_clean_txcb() only zeroes the control block fields without
unmapping the DMA buffers, leaking the DMA mappings allocated for the
head and earlier fragments.

Call dma_unmap_single() before calling bcmasp_clean_txcb() in the error
cleanup loop.

Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index f2176ef3a127..9ad5a982542f 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -285,6 +285,11 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
 			intf->mib.tx_dma_failed++;
 			spb_index = intf->tx_spb_index;
 			for (j = 0; j < i; j++) {
+				txcb = &intf->tx_cbs[spb_index];
+				dma_unmap_single(kdev,
+						 dma_unmap_addr(txcb, dma_addr),
+						 dma_unmap_len(txcb, dma_len),
+						 DMA_TO_DEVICE);
 				bcmasp_clean_txcb(intf, spb_index);
 				spb_index = incr_ring(spb_index,
 						      DESC_RING_COUNT);
-- 
2.34.1

[PATCH net 3/6] net: bcmasp: validate minimum RX packet size in bcmasp_rx_poll()

[Florian Fainelli]

In bcmasp_rx_poll(), the driver removes a 2-byte alignment pad and
optionally strips the ETH_FCS_LEN CRC from received packets using
skb_pull(skb, 2) and skb_trim(skb, len - ETH_FCS_LEN).

If the hardware reports a descriptor size smaller than the pad and CRC
lengths (e.g. runt or corrupted frames), len -= 2 or len - ETH_FCS_LEN
underflows u32 len, leading to out-of-bounds trimming and memory
corruption.

Check that desc->size is at least the sum of the 2-byte pad and the CRC
length before proceeding to process the descriptor.

Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index 9ad5a982542f..2ad8a7eac888 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -523,6 +523,12 @@ static int bcmasp_rx_poll(struct napi_struct *napi, int budget)
 					DMA_FROM_DEVICE);
 
 		len = desc->size;
+		if (unlikely(len < 2 + (intf->crc_fwd ? ETH_FCS_LEN : 0))) {
+			u64_stats_update_begin(&stats->syncp);
+			u64_stats_inc(&stats->rx_dropped);
+			u64_stats_update_end(&stats->syncp);
+			goto next;
+		}
 
 		/* Allocate a page pool page as the SKB data area so the
 		 * kernel can recycle it efficiently after the packet is
-- 
2.34.1

[PATCH net 4/6] net: bcmasp: fix OF node reference leak for phy_dn

[Florian Fainelli]

In bcmasp_interface_create(), intf->phy_dn is obtained via
of_parse_phandle() or assigned ndev_dn. of_parse_phandle() returns a
node reference with its refcount incremented, but of_node_put() was
never called on intf->phy_dn in bcmasp_interface_destroy() or the
error unwind path in bcmasp_interface_create().

Acquire a reference on ndev_dn for the fixed-link case as well so that
intf->phy_dn consistently holds a reference, and release it with
of_node_put() on teardown and error.

Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index 2ad8a7eac888..7a63a592f158 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -1298,7 +1298,7 @@ struct bcmasp_intf *bcmasp_interface_create(struct bcmasp_priv *priv,
 				 ndev_dn->name);
 			goto err_free_netdev;
 		}
-		intf->phy_dn = ndev_dn;
+		intf->phy_dn = of_node_get(ndev_dn);
 	}
 
 	/* Map resource */
@@ -1338,6 +1338,7 @@ struct bcmasp_intf *bcmasp_interface_create(struct bcmasp_priv *priv,
 err_deregister_fixed_link:
 	if (of_phy_is_fixed_link(ndev_dn))
 		of_phy_deregister_fixed_link(ndev_dn);
+	of_node_put(intf->phy_dn);
 err_free_netdev:
 	free_netdev(ndev);
 err:
@@ -1350,6 +1351,7 @@ void bcmasp_interface_destroy(struct bcmasp_intf *intf)
 		unregister_netdev(intf->ndev);
 	if (of_phy_is_fixed_link(intf->ndev_dn))
 		of_phy_deregister_fixed_link(intf->ndev_dn);
+	of_node_put(intf->phy_dn);
 	free_netdev(intf->ndev);
 }
 
-- 
2.34.1

[PATCH net 5/6] net: bcmasp: account for offload header in TX short packet padding

[Florian Fainelli]

When hardware checksum offload is enabled for an skb,
bcmasp_csum_offload() prepends a struct bcmasp_pkt_offload header
(20 bytes) to the skb via skb_push(). This increases skb->len and
skb_headlen(skb) by sizeof(struct bcmasp_pkt_offload).

Because the hardware descriptor processor strips this offload header
before transmitting the packet on the wire, the wire packet length is
smaller by sizeof(struct bcmasp_pkt_offload). The padding calculation
did not account for this extra header, causing short frames to skip
padding and be sent on the wire smaller than the minimum Ethernet frame
size (ETH_ZLEN + ETH_FCS_LEN).

Add sizeof(struct bcmasp_pkt_offload) to the minimum padding threshold
when hardware checksum offload is enabled.

Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
Assisted-by: LLM
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index 7a63a592f158..3370cbf1b395 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -236,7 +236,7 @@ static struct sk_buff *bcmasp_csum_offload(struct net_device *dev,
 static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct bcmasp_intf *intf = netdev_priv(dev);
-	unsigned int total_bytes, size;
+	unsigned int total_bytes, size, min_size;
 	int spb_index, nr_frags, i, j;
 	struct bcmasp_tx_cb *txcb;
 	dma_addr_t mapping, valid;
@@ -267,8 +267,12 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
 	for (i = 0; i <= nr_frags; i++) {
 		if (!i) {
 			size = skb_headlen(skb);
-			if (!nr_frags && size < (ETH_ZLEN + ETH_FCS_LEN)) {
-				if (skb_put_padto(skb, ETH_ZLEN + ETH_FCS_LEN))
+			min_size = ETH_ZLEN + ETH_FCS_LEN;
+			if (csum_hw)
+				min_size += sizeof(struct bcmasp_pkt_offload);
+
+			if (!nr_frags && size < min_size) {
+				if (skb_put_padto(skb, min_size))
 					return NETDEV_TX_OK;
 				size = skb->len;
 			}
-- 
2.34.1

[PATCH net 6/6] net: bcmasp: fix network filter lookup and wake filter pair allocation

[Florian Fainelli]

Several issues exist in bcmasp_netfilt_get_init():
1. When looking up an existing filter (!init) with a specified location
   (loc != RX_CLS_LOC_ANY), if the filter at loc was not claimed, the
   loop continued searching higher indices and could return an arbitrary
   unrelated filter belonging to the port. This caused flow get or
   delete operations on an empty rule location to return or delete an
   unintended filter.
2. When allocating an unpositioned wake filter (wake_filter == true and
   loc == RX_CLS_LOC_ANY), if an even index was claimed or its adjacent
   odd index was claimed, the check fell through to the non-wake branch
   "else if (!priv->net_filters[i].claimed)", picking an odd index and
   subsequently claiming the next filter across filter pair boundaries.
3. When allocating a positioned wake filter at loc, only loc was
   checked for being busy, but not loc + 1, which could overwrite an
   existing filter at loc + 1.

Fix these by checking only the requested location on lookup, properly
restricting wake filter pair searches to even boundaries where both
entries are free, checking loc + 1 for positioned wake filters, and
preventing out-of-bounds release.

Fixes: c5d511c49587 ("net: bcmasp: Add support for wake on net filters")
Assisted-by: LLM
Co-developed-by: Justin Chen <justin.chen@broadcom.com>
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
Co-developed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 127 ++++++++++++--------
 1 file changed, 78 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index 972474893a6b..bbd152bc3a8a 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
@@ -511,6 +511,13 @@ static int bcmasp_netfilt_wr_to_hw(struct bcmasp_priv *priv,
 	return 0;
 }
 
+static inline bool bcmasp_netfilt_is_companion(struct bcmasp_priv *priv, int i)
+{
+	return i > 0 && (i % 2) &&
+	       priv->net_filters[i].wake_filter &&
+	       priv->net_filters[i - 1].wake_filter;
+}
+
 void bcmasp_netfilt_suspend(struct bcmasp_intf *intf)
 {
 	struct bcmasp_priv *priv = intf->parent;
@@ -524,9 +531,7 @@ void bcmasp_netfilt_suspend(struct bcmasp_intf *intf)
 		    priv->net_filters[i].port != intf->port)
 			continue;
 
-		if (i > 0 && (i % 2) &&
-		    priv->net_filters[i].wake_filter &&
-		    priv->net_filters[i - 1].wake_filter)
+		if (bcmasp_netfilt_is_companion(priv, i))
 			continue;
 
 		ret = bcmasp_netfilt_wr_to_hw(priv, &priv->net_filters[i]);
@@ -556,9 +561,7 @@ int bcmasp_netfilt_get_all_active(struct bcmasp_intf *intf, u32 *rule_locs,
 		    priv->net_filters[i].port != intf->port)
 			continue;
 
-		if (i > 0 && (i % 2) &&
-		    priv->net_filters[i].wake_filter &&
-		    priv->net_filters[i - 1].wake_filter)
+		if (bcmasp_netfilt_is_companion(priv, i))
 			continue;
 
 		if (j == *rule_cnt)
@@ -583,9 +586,7 @@ int bcmasp_netfilt_get_active(struct bcmasp_intf *intf)
 			continue;
 
 		/* Skip over a wake filter pair */
-		if (i > 0 && (i % 2) &&
-		    priv->net_filters[i].wake_filter &&
-		    priv->net_filters[i - 1].wake_filter)
+		if (bcmasp_netfilt_is_companion(priv, i))
 			continue;
 
 		cnt++;
@@ -607,6 +608,9 @@ bool bcmasp_netfilt_check_dup(struct bcmasp_intf *intf,
 		    priv->net_filters[i].port != intf->port)
 			continue;
 
+		if (bcmasp_netfilt_is_companion(priv, i))
+			continue;
+
 		cur = &priv->net_filters[i].fs;
 
 		if (cur->flow_type != fs->flow_type ||
@@ -659,7 +663,7 @@ bool bcmasp_netfilt_check_dup(struct bcmasp_intf *intf,
 }
 
 /* If no network filter found, return open filter.
- * If no more open filters return NULL
+ * If no more open filters return error.
  */
 struct bcmasp_net_filter *bcmasp_netfilt_get_init(struct bcmasp_intf *intf,
 						  u32 loc, bool wake_filter,
@@ -673,41 +677,61 @@ struct bcmasp_net_filter *bcmasp_netfilt_get_init(struct bcmasp_intf *intf,
 	if (loc != RX_CLS_LOC_ANY && loc >= priv->num_net_filters)
 		return ERR_PTR(-EINVAL);
 
-	/* If the filter location is busy (already claimed) and we are initializing
-	 * the filter (insertion), return a busy error code.
-	 */
-	if (loc != RX_CLS_LOC_ANY && init && priv->net_filters[loc].claimed)
-		return ERR_PTR(-EBUSY);
-
-	/* We need two filters for wake-up, so we cannot use an odd filter */
-	if (wake_filter && loc != RX_CLS_LOC_ANY && (loc % 2))
-		return ERR_PTR(-EINVAL);
+	if (!init) {
+		if (loc != RX_CLS_LOC_ANY) {
+			if (priv->net_filters[loc].claimed &&
+			    priv->net_filters[loc].port == intf->port &&
+			    !bcmasp_netfilt_is_companion(priv, loc))
+				return &priv->net_filters[loc];
+			return ERR_PTR(-ENOENT);
+		}
 
-	/* Initialize the loop index based on the desired location or from 0 */
-	i = loc == RX_CLS_LOC_ANY ? 0 : loc;
+		for (i = 0; i < priv->num_net_filters; i++) {
+			if (bcmasp_netfilt_is_companion(priv, i))
+				continue;
 
-	for ( ; i < priv->num_net_filters; i++) {
-		/* Found matching network filter */
-		if (!init &&
-		    priv->net_filters[i].claimed &&
-		    priv->net_filters[i].hw_index == i &&
-		    priv->net_filters[i].port == intf->port)
-			return &priv->net_filters[i];
+			if (priv->net_filters[i].claimed &&
+			    priv->net_filters[i].port == intf->port)
+				return &priv->net_filters[i];
+		}
 
-		/* If we don't need a new filter or new filter already found */
-		if (!init || open_index >= 0)
-			continue;
+		return ERR_PTR(-ENOENT);
+	}
 
-		/* Wake filter conslidates two filters to cover more bytes
-		 * Wake filter is open if...
-		 * 1. It is an even filter
-		 * 2. The current and next filter is not claimed
-		 */
-		if (wake_filter && !(i % 2) && !priv->net_filters[i].claimed &&
-		    !priv->net_filters[i + 1].claimed)
-			open_index = i;
-		else if (!priv->net_filters[i].claimed)
-			open_index = i;
+	/* If the filter location is busy (already claimed) and we are initializing
+	 * the filter (insertion), return a busy error code.
+	 */
+	if (loc != RX_CLS_LOC_ANY) {
+		if (priv->net_filters[loc].claimed)
+			return ERR_PTR(-EBUSY);
+
+		/* We need two filters for wake-up, so we cannot use an odd filter */
+		if (wake_filter) {
+			if ((loc % 2) || loc + 1 >= priv->num_net_filters)
+				return ERR_PTR(-EINVAL);
+			if (priv->net_filters[loc + 1].claimed)
+				return ERR_PTR(-EBUSY);
+		}
+		open_index = loc;
+	} else {
+		for (i = 0; i < priv->num_net_filters; i++) {
+			/* Wake filter consolidates two filters to cover more bytes.
+			 * Wake filter is open if:
+			 * 1. It is an even filter
+			 * 2. The current and next filter is not claimed
+			 */
+			if (wake_filter) {
+				if (!(i % 2) && (i + 1 < priv->num_net_filters) &&
+				    !priv->net_filters[i].claimed &&
+				    !priv->net_filters[i + 1].claimed) {
+					open_index = i;
+					break;
+				}
+			} else if (!priv->net_filters[i].claimed) {
+				open_index = i;
+				break;
+			}
+		}
 	}
 
 	if (open_index >= 0) {
@@ -716,16 +740,20 @@ struct bcmasp_net_filter *bcmasp_netfilt_get_init(struct bcmasp_intf *intf,
 		nfilter->port = intf->port;
 		nfilter->ch = intf->channel + priv->tx_chan_offset;
 		nfilter->hw_index = open_index;
-	}
 
-	if (wake_filter && open_index >= 0) {
-		/* Claim next filter */
-		priv->net_filters[open_index + 1].claimed = true;
-		priv->net_filters[open_index + 1].wake_filter = true;
-		nfilter->wake_filter = true;
+		if (wake_filter) {
+			/* Claim next filter */
+			priv->net_filters[open_index + 1].claimed = true;
+			priv->net_filters[open_index + 1].wake_filter = true;
+			priv->net_filters[open_index + 1].hw_index = open_index + 1;
+			priv->net_filters[open_index + 1].port = intf->port;
+			priv->net_filters[open_index + 1].ch = intf->channel +
+							       priv->tx_chan_offset;
+			nfilter->wake_filter = true;
+		}
 	}
 
-	return nfilter ? nfilter : ERR_PTR(-EINVAL);
+	return nfilter ? nfilter : ERR_PTR(-ENOSPC);
 }
 
 void bcmasp_netfilt_release(struct bcmasp_intf *intf,
@@ -733,7 +761,8 @@ void bcmasp_netfilt_release(struct bcmasp_intf *intf,
 {
 	struct bcmasp_priv *priv = intf->parent;
 
-	if (nfilt->wake_filter) {
+	if (nfilt->wake_filter && !(nfilt->hw_index % 2) &&
+	    nfilt->hw_index + 1 < priv->num_net_filters) {
 		memset(&priv->net_filters[nfilt->hw_index + 1], 0,
 		       sizeof(struct bcmasp_net_filter));
 	}
-- 
2.34.1

Re: [PATCH net 1/6] net: bcmasp: fix mib counters struct alignment with ethtool stats

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message overstates what the bug did.

--

net: bcmasp: fix mib counters struct alignment with ethtool stats

This commit removes the leftover edpkt_* fields from the start of struct
bcmasp_mib_counters. They were left in place when the matching
bcmasp_gstrings_stats[] entries were removed for asp-v3.0. Since then the
struct layout has not matched the order the ethtool code indexes by.

> Because bcmasp_update_mib_counters() and bcmasp_get_ethtool_stats() index
> into struct bcmasp_mib_counters sequentially based on the order of
> bcmasp_gstrings_stats, this 16-byte offset caused hardware statistics
> to be written to the wrong struct members and software counters to be
> read from incorrect offsets (and reading out-of-bounds at the end of the
> struct).

[Severity: Medium]
Is the out-of-bounds part of this description accurate?

Before this patch, struct bcmasp_mib_counters had 17 u32 members (68
bytes), and bcmasp_gstrings_stats[] has 13 entries. Both loops stop at
BCMASP_STATS_LEN:

drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c:bcmasp_get_ethtool_stats() {
    ...
	for (i = 0; i < BCMASP_STATS_LEN; i++) {
		p = (char *)(&intf->mib) + (i * sizeof(u32));
		data[i] = *(u32 *)p;
	}
}

So the highest offset either loop touches is 12 * 4 = 48 bytes, which is
inside the 68-byte struct.

The hardware values also appear to have reached userspace correctly.
bcmasp_update_mib_counters() writes them to raw slots 0-4, and
bcmasp_get_ethtool_stats() reads them back from the same slots. The only
members overwritten were edpkt_* and umac_frm_cnt, and no code uses those
by name.

The part that looks broken is the software counters. Code increments them
by name (intf->mib.alloc_rx_skb_failed++, tx_dma_failed++, and so on).
They sit in raw slots 9-16, but ethtool read slots 5-12. As a result:

  - "RX SKB Alloc Failed", "TX DMA Failed", "Multicast Filters Full" and
    "Unicast Filters Full" read fb_* members that are never written, so
    they always showed 0.

  - The next four strings showed the values of the first four software
    counters.

  - filters_combine_cnt, promisc_filters_cnt, tx_realloc_offload_failed
    and tx_timeout_cnt were never shown at all.

Could the commit message describe this as shifted or mislabeled software
counters, rather than as an out-of-bounds read at the end of the struct?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922221630.3864427-1-florian.fainelli%40broadcom.com

Re: [PATCH net 3/6] net: bcmasp: validate minimum RX packet size in bcmasp_rx_poll()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 (2) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new minimum-size check in bcmasp_rx_poll()
  (bcmasp_intf.c:526) is `len < 2 + (intf->crc_fwd ?
- [Medium] The commit message overstates the impact.
- [Low] The new runt/invalid-length drop branch in bcmasp_rx_poll()
  (lines 526-530) increments `stats->rx_dropped`.

Pre-existing issues:
- [Medium] bcmasp_rx_poll() never checks desc->size or
  DESC_ADDR(desc->buf) against an upper limit.
- [Medium] One `struct u64_stats_sync` (`intf->stats64.syncp`) is written
  by several contexts that are not mutually exclusive: - `rx_napi`…

--

net: bcmasp: validate minimum RX packet size in bcmasp_rx_poll()

This patch adds a lower bound check on desc->size in bcmasp_rx_poll().
Descriptors smaller than the 2-byte alignment pad plus the optional FCS
are counted in rx_dropped and skipped before an skb is built.

> If the hardware reports a descriptor size smaller than the pad and CRC
> lengths (e.g. runt or corrupted frames), len -= 2 or len - ETH_FCS_LEN
> underflows u32 len, leading to out-of-bounds trimming and memory
> corruption.

[Severity: Medium]
Is "out-of-bounds trimming and memory corruption" accurate here? Both
helpers used by bcmasp_rx_poll() appear to guard against the underflowed
values:

include/linux/skbuff.h:skb_pull_inline() {
	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
}

net/core/skbuff.c:skb_trim() {
	if (skb->len > len)
		__skb_trim(skb, len);
}

If desc->size is below 2, skb_pull() returns NULL and pulls nothing. If
len - ETH_FCS_LEN wraps to a value near 4G, skb_trim() does nothing.

Before this patch, the visible effects seem to be two:

  - a 0-3 byte skb reaches eth_type_trans() and GRO
  - the wrapped len is added to rx_bytes, about 4 GiB per frame:

	u64_stats_add(&stats->rx_bytes, len);

Neither of these is an out-of-bounds write.

The driver also never sets UMC_CMD_RUNT_FILTER_DIS, so the UniMAC runt
filter should stay enabled. That leaves misbehaving or corrupted
descriptors as the only way to reach this path.

This carries a Fixes: tag, so it will go to stable. Could the commit
message describe the actual impact, and say whether such descriptors have
been seen on real hardware?

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index 9ad5a982542f5..2ad8a7eac8888 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> @@ -523,6 +523,12 @@ static int bcmasp_rx_poll(struct napi_struct *napi, int budget)
>  					DMA_FROM_DEVICE);
>  
>  		len = desc->size;
> +		if (unlikely(len < 2 + (intf->crc_fwd ? ETH_FCS_LEN : 0))) {

[Severity: High]
Should this threshold also include ETH_HLEN?

With crc_fwd set, any desc->size from 6 to 19 passes this check. With
crc_fwd clear, the range is 2 to 15. After the pad pull and the FCS trim,
the skb holds 0-13 bytes. With desc->size == 6 and crc_fwd set,
skb->len == 0.

That skb still goes to eth_type_trans(), and eth_skb_pull_mac() ignores
the failed pull:

include/linux/etherdevice.h:eth_skb_pull_mac() {
	struct ethhdr *eth = (struct ethhdr *)skb->data;

	skb_pull_inline(skb, ETH_HLEN);
	return eth;
}

eth_skb_pkt_type() then reads h_dest, and eth_type_trans() reads
h_proto, from bytes past skb->len. The memcpy() never wrote those bytes.
They are stale page_pool page contents, or the stripped CRC, and they
decide skb->pkt_type and skb->protocol.

The frame is then passed to napi_gro_receive(). If a packet socket
receives it, eth_header_parse() copies h_source into sll_addr. Can that
leak 6 stale bytes to userspace?

Sizes just above the new limit still reach this path for the runt or
corrupted descriptors the commit message describes. bcmasp_xmit() already
uses ETH_ZLEN + ETH_FCS_LEN as the minimum frame size.

Would 2 + ETH_HLEN + (crc_fwd ? ETH_FCS_LEN : 0), or a bound based on
ETH_ZLEN, be more appropriate here?

> +			u64_stats_update_begin(&stats->syncp);
> +			u64_stats_inc(&stats->rx_dropped);

[Severity: Low]
Is rx_dropped the right counter for a length error? The page_pool and
napi_build_skb() allocation failures also use it, and
bcmasp_get_stats64() exports it directly:

	stats->rx_dropped = u64_stats_read(&lstats->rx_dropped);
	stats->rx_crc_errors = u64_stats_read(&lstats->rx_crc_errs);
	stats->rx_frame_errors = u64_stats_read(&lstats->rx_sym_errs);
	stats->rx_errors = stats->rx_crc_errors + stats->rx_frame_errors;

include/uapi/linux/if_link.h describes the two counters differently:

  - rx_dropped: "e.g. due to lack of resources or unsupported protocol"
  - rx_length_errors: "Number of packets dropped due to invalid length",
    which is also counted in rx_errors

As written, ip -s link can't tell these drops apart from allocation
failures, and they never show up in rx_errors or rx_length_errors.

> +			u64_stats_update_end(&stats->syncp);

[Severity: Medium]
This is a pre-existing issue, but intf->stats64.syncp seems to have
several writers that are not mutually exclusive:

  - rx_napi, here in bcmasp_rx_poll()
  - tx_napi, via bcmasp_tx_poll()->bcmasp_tx_reclaim()
  - process context, in bcmasp_netif_deinit()

bcmasp_netif_deinit() {
    ...
	bcmasp_tx_reclaim(intf);
    ...
	napi_disable(&intf->rx_napi);
    ...
}

So bcmasp_tx_reclaim() runs while rx_napi is still enabled. The two napi
instances can also run at the same time under threaded napi or on
different CPUs.

On 32-bit kernels, could two concurrent u64_stats_update_begin()/end()
pairs lose a sequence increment and leave the seqcount odd? If so,
bcmasp_get_stats64() would spin forever in its fetch/retry loop, or
return torn values. The process context caller also runs with preemption
enabled.

This patch adds one more RX-side writer that follows the existing
pattern.

> +			goto next;
> +		}
>  
>  		/* Allocate a page pool page as the SKB data area so the
>  		 * kernel can recycle it efficiently after the packet is

[Severity: Medium]
This isn't a bug introduced by this patch, but bcmasp_rx_poll() has no
upper bound on desc->size and no range check on the descriptor address.
Should it have both?

	data = intf->rx_ring_cpu +
		(DESC_ADDR(desc->buf) - intf->rx_ring_dma);
    ...
	skb_put(skb, len);
	memcpy(skb->data, data, len);

The skb is built on a single page. Its tailroom is PAGE_SIZE minus
NET_SKB_PAD minus skb_shared_info, about 3.7 KB with 4K pages.

A desc->size larger than that would hit skb_over_panic(). An address
outside RING_BUFFER_SIZE would make memcpy() copy memory from outside the
RX ring into the skb.

umac_reset_and_init() caps well-behaved hardware at 0x800 through
UMC_FRM_LEN and UMC_RX_MAX_PKT_SZ. So this only matters for the kind of
misbehaving descriptors this patch guards against.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922221630.3864427-1-florian.fainelli%40broadcom.com

Re: [PATCH net 5/6] net: bcmasp: account for offload header in TX short packet padding

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix only works for linear skbs.

--

net: bcmasp: account for offload header in TX short packet padding

This patch makes the TX padding threshold in bcmasp_xmit() include
sizeof(struct bcmasp_pkt_offload) when hardware checksum offload is in
use. bcmasp_csum_offload() pushes that 20-byte header onto the skb, and
the hardware strips it before transmission. Without the adjustment,
short frames could go out below ETH_ZLEN + ETH_FCS_LEN.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index 7a63a592f158d..3370cbf1b395a 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c

[ ... ]

> @@ -267,8 +267,12 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
>  	for (i = 0; i <= nr_frags; i++) {
>  		if (!i) {
>  			size = skb_headlen(skb);
> -			if (!nr_frags && size < (ETH_ZLEN + ETH_FCS_LEN)) {
> -				if (skb_put_padto(skb, ETH_ZLEN + ETH_FCS_LEN))
> +			min_size = ETH_ZLEN + ETH_FCS_LEN;
> +			if (csum_hw)
> +				min_size += sizeof(struct bcmasp_pkt_offload);
> +
> +			if (!nr_frags && size < min_size) {
> +				if (skb_put_padto(skb, min_size))
>  					return NETDEV_TX_OK;
>  				size = skb->len;
>  			}

[Severity: High]
Does this still let short checksum-offloaded frames go out undersized
when the skb is non-linear?

The new min_size is only applied when nr_frags is zero, and it is
compared against skb_headlen() rather than skb->len.

bcmasp_interface_create() advertises scatter-gather together with L4
checksum offload:

	ndev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_SG |
			  NETIF_F_RXCSUM;

tcp_sendmsg_locked() copies payload into page frags through
skb_copy_to_page_nocache(). This means small TCP segments arrive here
as CHECKSUM_PARTIAL skbs with headers in the linear area and
nr_frags >= 1.

As an example, take an IPv4 TCP segment without timestamps carrying 1
to 5 bytes of payload. That is 14 + 20 + 20 + N = 55 to 59 bytes.
bcmasp_csum_offload() pushes the 20-byte header and sets csum_hw, so
skb->len becomes 75 to 79. That is below the new min_size of 84.

Because nr_frags == 1, skb_put_padto() is never called. The hardware
strips the offload header and a 55 to 59 byte frame goes out on the
wire. This looks like the same failure the commit message describes:

  "causing short frames to skip padding and be sent on the wire smaller
  than the minimum Ethernet frame size (ETH_ZLEN + ETH_FCS_LEN)"

For TCP, retransmits of that segment would have the same non-linear
shape, so they could be dropped by the receiver every time.

Would it be better to make the padding decision on skb->len before the
descriptor loop? One way is skb_put_padto() or eth_skb_pad(), which
linearize through __skb_pad(), followed by re-reading nr_frags.

The pad can't safely happen inside the loop. nr_frags is cached before
bcmasp_csum_offload():

	nr_frags = skb_shinfo(skb)->nr_frags;

If the skb were linearized in the middle of the loop, the loop would go
on indexing frags that no longer exist.

As far as I can tell, no later patch in the series changes this code in
bcmasp_intf.c.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922221630.3864427-1-florian.fainelli%40broadcom.com

Re: [PATCH net 6/6] net: bcmasp: fix network filter lookup and wake filter pair allocation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When bcmasp_netfilt_get_init() runs as a lookup (init ==
  false) with loc == RX_CLS_LOC_ANY, it still returns an arbitrary…
- [Low] The commit message presents issues #2 (an unpositioned wake
  allocation picking an odd index and claiming across pair boundaries)…

--

net: bcmasp: fix network filter lookup and wake filter pair allocation

This patch rewrites bcmasp_netfilt_get_init() so that a lookup of an
explicit location only checks that slot. Wake filter pair allocation is
now restricted to even slots where both entries are free, and loc + 1 is
checked for positioned wake filters. It also adds a parity and bounds
check to bcmasp_netfilt_release() and factors the companion slot test
into bcmasp_netfilt_is_companion().

> 2. When allocating an unpositioned wake filter (wake_filter == true and
>    loc == RX_CLS_LOC_ANY), if an even index was claimed or its adjacent
>    odd index was claimed, the check fell through to the non-wake branch
>    "else if (!priv->net_filters[i].claimed)", picking an odd index and
>    subsequently claiming the next filter across filter pair boundaries.
> 3. When allocating a positioned wake filter at loc, only loc was
>    checked for being busy, but not loc + 1, which could overwrite an
>    existing filter at loc + 1.

[ ... ]

> Fix these by checking only the requested location on lookup, properly
> restricting wake filter pair searches to even boundaries where both
> entries are free, checking loc + 1 for positioned wake filters, and
> preventing out-of-bounds release.

[Severity: Low]
Can issues 2 and 3, and the out-of-bounds release, actually happen with
the current driver?

The only allocation caller is bcmasp_flow_insert(). It rejects every
rule that is not a wake filter:

drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c:bcmasp_flow_insert() {
    ...
	/* Currently only supports WAKE filters */
	if (!wake)
		return -EOPNOTSUPP;
    ...
}

This means every allocation in bcmasp_netfilt_get_init() runs with
wake_filter == true. num_net_filters is always even in the platform data
(32, 32 or 16). The only code that writes priv->net_filters[] is
bcmasp_netfilt_get_init() and bcmasp_netfilt_release().

So each pair is claimed and released as a unit, and only even slots are
ever primaries. With that invariant, the old unpositioned loop always
takes the wake branch at the first free even slot and never gets to an
odd slot. An unclaimed even loc also means loc + 1 is unclaimed. And
bcmasp_netfilt_release() is only ever passed an even primary whose
companion is in bounds.

Only issue 1 looks reachable. The other changes look like hardening.
Could the commit message say so? With a Fixes: tag, describing them as
filter table corruption and an out-of-bounds release may lead stable
triage to overestimate the severity.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> index 972474893a6bc..bbd152bc3a8a9 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c

[ ... ]

> @@ -673,41 +677,61 @@ struct bcmasp_net_filter *bcmasp_netfilt_get_init(struct bcmasp_intf *intf,
>  	if (loc != RX_CLS_LOC_ANY && loc >= priv->num_net_filters)
>  		return ERR_PTR(-EINVAL);
>  
> -	/* If the filter location is busy (already claimed) and we are initializing
> -	 * the filter (insertion), return a busy error code.
> -	 */
> -	if (loc != RX_CLS_LOC_ANY && init && priv->net_filters[loc].claimed)
> -		return ERR_PTR(-EBUSY);
> -
> -	/* We need two filters for wake-up, so we cannot use an odd filter */
> -	if (wake_filter && loc != RX_CLS_LOC_ANY && (loc % 2))
> -		return ERR_PTR(-EINVAL);
> +	if (!init) {
> +		if (loc != RX_CLS_LOC_ANY) {
> +			if (priv->net_filters[loc].claimed &&
> +			    priv->net_filters[loc].port == intf->port &&
> +			    !bcmasp_netfilt_is_companion(priv, loc))
> +				return &priv->net_filters[loc];
> +			return ERR_PTR(-ENOENT);
> +		}
>  
> -	/* Initialize the loop index based on the desired location or from 0 */
> -	i = loc == RX_CLS_LOC_ANY ? 0 : loc;
> +		for (i = 0; i < priv->num_net_filters; i++) {
> +			if (bcmasp_netfilt_is_companion(priv, i))
> +				continue;
>  
> -	for ( ; i < priv->num_net_filters; i++) {
> -		/* Found matching network filter */
> -		if (!init &&
> -		    priv->net_filters[i].claimed &&
> -		    priv->net_filters[i].hw_index == i &&
> -		    priv->net_filters[i].port == intf->port)
> -			return &priv->net_filters[i];
> +			if (priv->net_filters[i].claimed &&
> +			    priv->net_filters[i].port == intf->port)
> +				return &priv->net_filters[i];
> +		}
>  
> -		/* If we don't need a new filter or new filter already found */
> -		if (!init || open_index >= 0)
> -			continue;
> +		return ERR_PTR(-ENOENT);
> +	}

[Severity: Medium]
Should the lookup path reject loc == RX_CLS_LOC_ANY rather than return
the first claimed filter owned by the port?

The only lookup callers are bcmasp_flow_delete() and bcmasp_flow_get().
Both pass the user-supplied cmd->fs.location through unchanged:

drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c:bcmasp_flow_delete() {
    ...
	nfilter = bcmasp_netfilt_get_init(intf, cmd->fs.location, false, false);
	if (IS_ERR(nfilter))
		return PTR_ERR(nfilter);

	bcmasp_netfilt_release(intf, nfilter);
    ...
}

ethtool_set_rxnfc() and ethtool_get_rxnfc() in net/ethtool/ioctl.c do
not validate fs.location for ETHTOOL_SRXCLSRLDEL or ETHTOOL_GRXCLSRULE.
The capacity check at the top of bcmasp_netfilt_get_init() also lets
RX_CLS_LOC_ANY through.

So an ETHTOOL_SRXCLSRLDEL with fs.location = RX_CLS_LOC_ANY would
release the port's lowest-index wake filter pair, and silently drop that
wake-on-net rule. An ETHTOOL_GRXCLSRULE with the same location would
copy an unrelated rule's spec back to userspace.

The uAPI documentation in include/uapi/linux/ethtool.h says that for
these commands fs.location names an existing rule. RX_CLS_LOC_ANY is
only documented for ETHTOOL_SRXCLSRLINS.

This rewrite fixes the problem for explicit locations. Could this loop
be replaced with an -EINVAL or -ENOENT return, so a request that names
no rule does not act on an unrelated filter?

>  
> -		/* Wake filter conslidates two filters to cover more bytes
> -		 * Wake filter is open if...
> -		 * 1. It is an even filter
> -		 * 2. The current and next filter is not claimed
> -		 */

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922221630.3864427-1-florian.fainelli%40broadcom.com

Re: [PATCH net 2/6] net: bcmasp: unmap previous DMA mappings on TX map failure

[Nicolai Buchwitz]

On 23.9.2026 00:16, Florian Fainelli wrote:
> When mapping an skb with fragments for transmission in bcmasp_xmit(),
> if mapping fails on fragment i > 0, the error handler calls
> bcmasp_clean_txcb() for previous iterations j < i. However,
> bcmasp_clean_txcb() only zeroes the control block fields without
> unmapping the DMA buffers, leaking the DMA mappings allocated for the
> head and earlier fragments.
> 
> Call dma_unmap_single() before calling bcmasp_clean_txcb() in the error
> cleanup loop.
> 
> Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet 
> controller")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c 
> b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index f2176ef3a127..9ad5a982542f 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> @@ -285,6 +285,11 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff 
> *skb, struct net_device *dev)
>  			intf->mib.tx_dma_failed++;
>  			spb_index = intf->tx_spb_index;
>  			for (j = 0; j < i; j++) {
> +				txcb = &intf->tx_cbs[spb_index];
> +				dma_unmap_single(kdev,
> +						 dma_unmap_addr(txcb, dma_addr),
> +						 dma_unmap_len(txcb, dma_len),
> +						 DMA_TO_DEVICE);
>  				bcmasp_clean_txcb(intf, spb_index);
>  				spb_index = incr_ring(spb_index,
>  						      DESC_RING_COUNT);

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net 4/6] net: bcmasp: fix OF node reference leak for phy_dn

[Nicolai Buchwitz]

On 23.9.2026 00:16, Florian Fainelli wrote:
> In bcmasp_interface_create(), intf->phy_dn is obtained via
> of_parse_phandle() or assigned ndev_dn. of_parse_phandle() returns a
> node reference with its refcount incremented, but of_node_put() was
> never called on intf->phy_dn in bcmasp_interface_destroy() or the
> error unwind path in bcmasp_interface_create().
> 
> Acquire a reference on ndev_dn for the fixed-link case as well so that
> intf->phy_dn consistently holds a reference, and release it with
> of_node_put() on teardown and error.
> 
> Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet 
> controller")
> Assisted-by: LLM
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c 
> b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index 2ad8a7eac888..7a63a592f158 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> @@ -1298,7 +1298,7 @@ struct bcmasp_intf 
> *bcmasp_interface_create(struct bcmasp_priv *priv,
>  				 ndev_dn->name);
>  			goto err_free_netdev;
>  		}
> -		intf->phy_dn = ndev_dn;
> +		intf->phy_dn = of_node_get(ndev_dn);
>  	}
> 
>  	/* Map resource */
> @@ -1338,6 +1338,7 @@ struct bcmasp_intf 
> *bcmasp_interface_create(struct bcmasp_priv *priv,
>  err_deregister_fixed_link:
>  	if (of_phy_is_fixed_link(ndev_dn))
>  		of_phy_deregister_fixed_link(ndev_dn);
> +	of_node_put(intf->phy_dn);
>  err_free_netdev:
>  	free_netdev(ndev);
>  err:
> @@ -1350,6 +1351,7 @@ void bcmasp_interface_destroy(struct bcmasp_intf 
> *intf)
>  		unregister_netdev(intf->ndev);
>  	if (of_phy_is_fixed_link(intf->ndev_dn))
>  		of_phy_deregister_fixed_link(intf->ndev_dn);
> +	of_node_put(intf->phy_dn);
>  	free_netdev(intf->ndev);
>  }

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai