[PATCH net v2 0/4] net: sb1250-mac: ethtool link updates

[PATCH net v2 0/4] net: sb1250-mac: ethtool link updates

[Florian Fainelli]

This patch series addresses a few minor issues:

- Running ethtool against an sb1250-mac backed interface would not
  return any link information, although all of it is there
- There was no support for restarting auto-negotiation, also there for
  "free" with PHYLIB
- The kernel driver reports enabling RX Checksum offload but does not
  report it through the NETIF_F_* feature bits

Changes in v2:

- collected tags for patches 1 through 3
- added additional change to address RX checksum status and header
  offset setup based upon Sashiko's review and checked against the
  datasheet to be correct

Florian Fainelli (4):
  net: sb1250-mac: Add ethtool link settings support
  net: sb1250-mac: Advertise and honor RX checksum offload
  net: sb1250-mac: Add ethtool autonegotiation restart support
  net: sb1250-mac: Fix RX checksum status and header offset setup

 drivers/net/ethernet/broadcom/sb1250-mac.c | 27 ++++++++++++++++++----
 1 file changed, 22 insertions(+), 5 deletions(-)

-- 
2.53.0

[PATCH net v2 1/4] net: sb1250-mac: Add ethtool link settings support

[Florian Fainelli]

The driver connects to its PHY through phylib, but does not provide
ethtool operations. As a result, ethtool cannot report or configure the
link settings.

Use the phylib get_link_ksettings and set_link_ksettings helpers, and
provide ethtool_op_get_link for carrier reporting. The helpers return
-ENODEV when the PHY is not attached, as the driver attaches it when
the interface is opened and disconnects it when the interface is closed.

Acked-by: Maciej W. Rozycki <macro@orcam.me.uk>
Reviewed-by: Daniel Machon <daniel.machon@microchip.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/sb1250-mac.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/sb1250-mac.c b/drivers/net/ethernet/broadcom/sb1250-mac.c
index c839dd9804d3..1843b52e96f5 100644
--- a/drivers/net/ethernet/broadcom/sb1250-mac.c
+++ b/drivers/net/ethernet/broadcom/sb1250-mac.c
@@ -2128,6 +2128,12 @@ static void sbmac_setmulti(struct sbmac_softc *sc)
 	}
 }
 
+static const struct ethtool_ops sbmac_ethtool_ops = {
+	.get_link		= ethtool_op_get_link,
+	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
+	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
+};
+
 static const struct net_device_ops sbmac_netdev_ops = {
 	.ndo_open		= sbmac_open,
 	.ndo_stop		= sbmac_close,
@@ -2197,6 +2203,7 @@ static int sbmac_init(struct platform_device *pldev, long long base)
 	spin_lock_init(&(sc->sbm_lock));
 
 	dev->netdev_ops = &sbmac_netdev_ops;
+	dev->ethtool_ops = &sbmac_ethtool_ops;
 	dev->watchdog_timeo = TX_TIMEOUT;
 	dev->min_mtu = 0;
 	dev->max_mtu = ENET_PACKET_SIZE;
-- 
2.53.0

[PATCH net v2 2/4] net: sb1250-mac: Advertise and honor RX checksum offload

[Florian Fainelli]

The driver already trusts hardware RX checksum status on supported
chip revisions, but does not advertise NETIF_F_RXCSUM. Consequently,
ethtool does not report the offload or allow it to be disabled.

Advertise RX checksum offload in hw_features and enable it by default
when the existing hardware revision check reports support. Test the
active feature bit in the receive path before marking a packet as
CHECKSUM_UNNECESSARY, so disabling RX checksum offload restores software
verification. Hardware can continue computing checksums while their
results are ignored.

BCM1250 pass 1 does not support the offload and retains the existing
software checksum behavior.

Acked-by: Maciej W. Rozycki <macro@orcam.me.uk>
Reviewed-by: Daniel Machon <daniel.machon@microchip.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/sb1250-mac.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/sb1250-mac.c b/drivers/net/ethernet/broadcom/sb1250-mac.c
index 1843b52e96f5..546415d3134c 100644
--- a/drivers/net/ethernet/broadcom/sb1250-mac.c
+++ b/drivers/net/ethernet/broadcom/sb1250-mac.c
@@ -1142,8 +1142,8 @@ static int sbdma_rx_process(struct sbmac_softc *sc, struct sbmacdma *d,
 				 * the kernel
 				 */
 				sb->protocol = eth_type_trans(sb,d->sbdma_eth->sbm_dev);
-				/* Check hw IPv4/TCP checksum if supported */
-				if (sc->rx_hw_checksum == ENABLE) {
+				/* Honor the RX checksum offload feature setting. */
+				if (dev->features & NETIF_F_RXCSUM) {
 					if (!((dsc->dscr_a) & M_DMA_ETHRX_BADIP4CS) &&
 					    !((dsc->dscr_a) & M_DMA_ETHRX_BADTCPCS)) {
 						sb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -2214,6 +2214,10 @@ static int sbmac_init(struct platform_device *pldev, long long base)
 
 	/* This is needed for PASS2 for Rx H/W checksum feature */
 	sbmac_set_iphdr_offset(sc);
+	if (sc->rx_hw_checksum == ENABLE) {
+		dev->hw_features |= NETIF_F_RXCSUM;
+		dev->features |= NETIF_F_RXCSUM;
+	}
 
 	sc->mii_bus = mdiobus_alloc();
 	if (sc->mii_bus == NULL) {
-- 
2.53.0

[PATCH net v2 3/4] net: sb1250-mac: Add ethtool autonegotiation restart support

[Florian Fainelli]

Wire up the nway_reset callback to phy_ethtool_nway_reset() so that
ethtool -r can restart PHY autonegotiation. The phylib helper handles
PHY locking and returns -ENODEV when the interface has no attached PHY.

Acked-by: Maciej W. Rozycki <macro@orcam.me.uk>
Reviewed-by: Daniel Machon <daniel.machon@microchip.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/sb1250-mac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/sb1250-mac.c b/drivers/net/ethernet/broadcom/sb1250-mac.c
index 546415d3134c..433aa1e24439 100644
--- a/drivers/net/ethernet/broadcom/sb1250-mac.c
+++ b/drivers/net/ethernet/broadcom/sb1250-mac.c
@@ -2129,6 +2129,7 @@ static void sbmac_setmulti(struct sbmac_softc *sc)
 }
 
 static const struct ethtool_ops sbmac_ethtool_ops = {
+	.nway_reset		= phy_ethtool_nway_reset,
 	.get_link		= ethtool_op_get_link,
 	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
 	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
-- 
2.53.0

[PATCH net v2 4/4] net: sb1250-mac: Fix RX checksum status and header offset setup

[Florian Fainelli]

The receive path tests M_DMA_ETHRX_BADTCPCS in descriptor A, but the
hardware reports it in descriptor B. Bit zero of descriptor A belongs
to the buffer address and is clear for the aligned receive buffers.
Consequently, a frame with a valid IPv4 checksum and a bad TCP checksum
can be marked CHECKSUM_UNNECESSARY. Read the TCP status from descriptor B.

The IP header offset setup also only masks the old register value due
to the combined AND/OR expression. Split the mask and field assignment
so that the intended offset of 15 is actually programmed.

Channel startup clears the receive filter register, losing any offset
set during probe. Program it again before enabling reception on every
channel start, including link changes, and preserve it when enabling
unicast and broadcast reception.

Both bugs predate the RX checksum feature advertisement. Sashiko AI
reported them during review of that change.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/sb1250-mac.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/sb1250-mac.c b/drivers/net/ethernet/broadcom/sb1250-mac.c
index 433aa1e24439..7998ab9fc46c 100644
--- a/drivers/net/ethernet/broadcom/sb1250-mac.c
+++ b/drivers/net/ethernet/broadcom/sb1250-mac.c
@@ -280,6 +280,7 @@ static void sbdma_tx_process(struct sbmac_softc *sc, struct sbmacdma *d,
 			     int poll);
 static int sbmac_initctx(struct sbmac_softc *s);
 static void sbmac_channel_start(struct sbmac_softc *s);
+static void sbmac_set_iphdr_offset(struct sbmac_softc *sc);
 static void sbmac_channel_stop(struct sbmac_softc *s);
 static enum sbmac_state sbmac_set_channel_state(struct sbmac_softc *,
 						enum sbmac_state);
@@ -1145,7 +1146,7 @@ static int sbdma_rx_process(struct sbmac_softc *sc, struct sbmacdma *d,
 				/* Honor the RX checksum offload feature setting. */
 				if (dev->features & NETIF_F_RXCSUM) {
 					if (!((dsc->dscr_a) & M_DMA_ETHRX_BADIP4CS) &&
-					    !((dsc->dscr_a) & M_DMA_ETHRX_BADTCPCS)) {
+					    !((dsc->dscr_b) & M_DMA_ETHRX_BADTCPCS)) {
 						sb->ip_summed = CHECKSUM_UNNECESSARY;
 						/* don't need to set sb->csum */
 					} else {
@@ -1496,6 +1497,7 @@ static void sbmac_channel_start(struct sbmac_softc *s)
 	 */
 
 	__raw_writeq(0, s->sbm_rxfilter);
+	sbmac_set_iphdr_offset(s);
 	__raw_writeq(0, s->sbm_imr);
 	__raw_writeq(framecfg, s->sbm_framecfg);
 	__raw_writeq(fifo, s->sbm_fifocfg);
@@ -1549,7 +1551,9 @@ static void sbmac_channel_start(struct sbmac_softc *s)
 	 * Enable receiving unicasts and broadcasts
 	 */
 
-	__raw_writeq(M_MAC_UCAST_EN | M_MAC_BCAST_EN, s->sbm_rxfilter);
+	reg = __raw_readq(s->sbm_rxfilter);
+	reg |= M_MAC_UCAST_EN | M_MAC_BCAST_EN;
+	__raw_writeq(reg, s->sbm_rxfilter);
 
 	/*
 	 * we're running now.
@@ -1717,7 +1721,8 @@ static void sbmac_set_iphdr_offset(struct sbmac_softc *sc)
 
 	/* Hard code the off set to 15 for now */
 	reg = __raw_readq(sc->sbm_rxfilter);
-	reg &= ~M_MAC_IPHDR_OFFSET | V_MAC_IPHDR_OFFSET(15);
+	reg &= ~M_MAC_IPHDR_OFFSET;
+	reg |= V_MAC_IPHDR_OFFSET(15);
 	__raw_writeq(reg, sc->sbm_rxfilter);
 
 	/* BCM1250 pass1 didn't have hardware checksum.  Everything
-- 
2.53.0

Re: [PATCH net v2 0/4] net: sb1250-mac: ethtool link updates

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2 0/4] net: sb1250-mac: ethtool link updates

[Florian Fainelli]

On 11/10/26 15:52, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>   - Whether the issue was actually triggered, or is only theoretical
>     (e.g. found by LLM and not proven at runtime). If it was triggered
>     please include the symptoms, like the stack trace or kernel messages.

These issues were all triggered by running ethool and not obtaining what 
was expected: link information, checksum offload indication, auto 
negotiation restart.

> 
>   - What hardware the change was tested on. For driver fixes please
>     mention the device (and if relevant firmware version) used for
>     testing, or say that the change was not tested on real hardware.

This was tested on a BCM91250E (Sentosa) reference board.
-- 
Florian

Re: [PATCH net v2 0/4] net: sb1250-mac: ethtool link updates

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 10 Nov 2026 15:02:56 -0800 you wrote:
> This patch series addresses a few minor issues:
> 
> - Running ethtool against an sb1250-mac backed interface would not
>   return any link information, although all of it is there
> - There was no support for restarting auto-negotiation, also there for
>   "free" with PHYLIB
> - The kernel driver reports enabling RX Checksum offload but does not
>   report it through the NETIF_F_* feature bits
> 
> [...]

Here is the summary with links:
  - [net,v2,1/4] net: sb1250-mac: Add ethtool link settings support
    https://git.kernel.org/netdev/net-next/c/b1a2a220435b
  - [net,v2,2/4] net: sb1250-mac: Advertise and honor RX checksum offload
    https://git.kernel.org/netdev/net-next/c/f116890c4050
  - [net,v2,3/4] net: sb1250-mac: Add ethtool autonegotiation restart support
    https://git.kernel.org/netdev/net-next/c/47759d9a9f74
  - [net,v2,4/4] net: sb1250-mac: Fix RX checksum status and header offset setup
    https://git.kernel.org/netdev/net-next/c/e48003f359fb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net v2 4/4] net: sb1250-mac: Fix RX checksum status and header offset setup

[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] In sbdma_rx_process()
  (drivers/net/ethernet/broadcom/sb1250-mac.c, lines 1147-1150), the RX…
- [Low] sbmac_set_iphdr_offset()
  (drivers/net/ethernet/broadcom/sb1250-mac.c, lines 1718-1726) writes…

--

net: sb1250-mac: Fix RX checksum status and header offset setup

The RX path now reads M_DMA_ETHRX_BADTCPCS from descriptor B instead of
descriptor A. The patch also fixes the combined mask/set expression in
sbmac_set_iphdr_offset(). It reprograms the IP header offset on every
channel start and keeps it when unicast and broadcast reception is enabled.

> diff --git a/drivers/net/ethernet/broadcom/sb1250-mac.c b/drivers/net/ethernet/broadcom/sb1250-mac.c
> index 433aa1e244397..7998ab9fc46c4 100644
> --- a/drivers/net/ethernet/broadcom/sb1250-mac.c
> +++ b/drivers/net/ethernet/broadcom/sb1250-mac.c

[ ... ]

> @@ -1145,7 +1146,7 @@ static int sbdma_rx_process(struct sbmac_softc *sc, struct sbmacdma *d,
>  				/* Honor the RX checksum offload feature setting. */
>  				if (dev->features & NETIF_F_RXCSUM) {
>  					if (!((dsc->dscr_a) & M_DMA_ETHRX_BADIP4CS) &&
> -					    !((dsc->dscr_a) & M_DMA_ETHRX_BADTCPCS)) {
> +					    !((dsc->dscr_b) & M_DMA_ETHRX_BADTCPCS)) {
>  						sb->ip_summed = CHECKSUM_UNNECESSARY;
>  						/* don't need to set sb->csum */
>  					} else {

[Severity: Medium]
Is it enough to check these two error bits before marking the skb
CHECKSUM_UNNECESSARY in sbdma_rx_process()?

A clear BADIP4CS or BADTCPCS bit means no failure was reported. It does
not mean the hardware checked the frame. Both bits are specific to IPv4
and TCP:

arch/mips/include/asm/sibyte/sb1250_dma.h:
#define M_DMA_ETHRX_BADIP4CS	    _SB_MAKEMASK1(51)
...
/* Note: This bit is in the DSCR_B options field */
#define M_DMA_ETHRX_BADTCPCS	_SB_MAKEMASK1(0)

The descriptor also holds the hardware's packet classification, and the
driver never reads it:

#define S_DMA_ETHRX_PKTTYPE	    55
#define M_DMA_ETHRX_PKTTYPE	    _SB_MAKEMASK(3, S_DMA_ETHRX_PKTTYPE)
...
#define K_DMA_ETHRX_PKTTYPE_IPV4    0

sbdma_add_rcvbuffer() posts every buffer with dsc->dscr_b = 0, so
BADTCPCS reads as clear unless the engine reports a failure. The MAC also
parses IP at one hard-coded offset, and VLAN detection is never enabled.

Suppose the engine leaves both bits clear for frames it does not
validate. Would IPv6 TCP/UDP/ICMPv6 frames and 802.1Q tagged frames then
be reported as checksum verified, with the stack skipping L4 checksum
validation for them? The same question applies to IPv4 frames that are
not TCP, or that are fragments.

The commit message says:

  Consequently, a frame with a valid IPv4 checksum and a bad TCP checksum
  can be marked CHECKSUM_UNNECESSARY.

The same result still looks reachable for frames the hardware does not
classify as IPv4/TCP.

This patch may also widen the exposure for IPv4. Before it, the
IPHDR_OFFSET field was in practice 0 at runtime. sbmac_channel_start()
cleared sbm_rxfilter, and the old mask expression never set the field.
So IPv4 frames probably failed BADIP4CS and fell back to software
checks.

Now sbmac_channel_start() calls sbmac_set_iphdr_offset() on every start,
so IPv4 frames can pass the header check. Any IPv4 L4 protocol that the
TCP check does not cover would then be marked CHECKSUM_UNNECESSARY.

Would it be safer to set CHECKSUM_UNNECESSARY only when all of these
hold, and leave every other frame as CHECKSUM_NONE?

- G_DMA_ETHRX_PKTTYPE(dsc->dscr_a) == K_DMA_ETHRX_PKTTYPE_IPV4
- the frame is not a fragment
- the L4 protocol is one the hardware verifies

The headers in the tree do not say whether the MAC sets these bits for
frames it does not classify as IPv4. This concern depends on how the
hardware behaves.

[ ... ]

> @@ -1717,7 +1721,8 @@ static void sbmac_set_iphdr_offset(struct sbmac_softc *sc)
>  
>  	/* Hard code the off set to 15 for now */
>  	reg = __raw_readq(sc->sbm_rxfilter);
> -	reg &= ~M_MAC_IPHDR_OFFSET | V_MAC_IPHDR_OFFSET(15);
> +	reg &= ~M_MAC_IPHDR_OFFSET;
> +	reg |= V_MAC_IPHDR_OFFSET(15);
>  	__raw_writeq(reg, sc->sbm_rxfilter);

[Severity: Low]
Is 15 the correct encoding for the IP header offset?

The comment above sbmac_set_iphdr_offset() says:

 *  Set the iphdr offset as 15 assuming ethernet encapsulation

In an untagged Ethernet II frame the IP header starts at byte 14
(ETH_HLEN). Measured from the cache-line-aligned buffer start, after
sbdma_add_rcvbuffer() adds NET_IP_ALIGN, it starts at byte 16.

The field definition in sb1250_mac.h does not give units or a base:

#define S_MAC_IPHDR_OFFSET	_SB_MAKE64(8)
#define M_MAC_IPHDR_OFFSET	_SB_MAKEMASK(8, S_MAC_IPHDR_OFFSET)

This patch is the first to make the value reach the hardware at runtime.
Suppose the field is a plain 0-based byte offset from the start of the
frame. Would the MAC then check the IPv4 header one byte off and set
BADIP4CS on almost every IPv4 frame? RX checksum offload would then
quietly fall back to software every time.

If the hardware uses a different base (for example 1-based) and 15 is
correct, could the comment and the commit message say so?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110230300.2769327-1-f.fainelli%40gmail.com