[PATCH net-next 0/3]: net: sb1250-mac: ethtool link updates

[PATCH net-next 0/3]: 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

Florian Fainelli (3):
  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

 drivers/net/ethernet/broadcom/sb1250-mac.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

-- 
2.53.0

[PATCH net-next 1/3] 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.

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

Re: [PATCH net-next 1/3] net: sb1250-mac: Add ethtool link settings support

[Maciej W. Rozycki]

On Thu, 5 Nov 2026, Florian Fainelli wrote:

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

(I should probably list myself as a maintainer for all the SB1250 platform 
stuff since I've done most of the updates in the last two decades or so.)

  Maciej

Re: [PATCH net-next 1/3] net: sb1250-mac: Add ethtool link settings support

[Daniel Machon]

> 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.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Daniel Machon <daniel.machon@microchip.com>

[PATCH net-next 2/3] 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.

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

Re: [PATCH net-next 2/3] net: sb1250-mac: Advertise and honor RX checksum offload

[Maciej W. Rozycki]

On Thu, 5 Nov 2026, Florian Fainelli wrote:

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

Acked-by: Maciej W. Rozycki <macro@orcam.me.uk>

  Maciej

Re: [PATCH net-next 2/3] net: sb1250-mac: Advertise and honor RX checksum offload

[Daniel Machon]

> 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.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Daniel Machon <daniel.machon@microchip.com>

Re: [PATCH net-next 2/3] net: sb1250-mac: Advertise and honor RX checksum offload

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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).

- [High] sbdma_rx_process()
  (drivers/net/ethernet/broadcom/sb1250-mac.c:1146-1149) reads the…
- [Medium] The RX checksum engine's IP-header-offset setup, which the
  probe comment calls necessary ("This is needed for PASS2 for Rx H/W…

--

net: sb1250-mac: Advertise and honor RX checksum offload

This patch adds NETIF_F_RXCSUM to hw_features. It also enables the
offload by default on chip revisions where sbmac_set_iphdr_offset()
reports hardware checksum support. The receive path now tests the active
feature bit, not sc->rx_hw_checksum, before it marks packets
CHECKSUM_UNNECESSARY.

> diff --git a/drivers/net/ethernet/broadcom/sb1250-mac.c b/drivers/net/ethernet/broadcom/sb1250-mac.c
> index 1843b52e96f51..546415d3134c6 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;

[Severity: High]
In sbdma_rx_process(), is M_DMA_ETHRX_BADTCPCS tested against the right
descriptor word? The header defines it as a dscr_b bit:

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

In dscr_a, bit 0 is the low bit of the RX buffer's physical address,
which sbdma_add_rcvbuffer() programs:

	dsc->dscr_a = virt_to_phys(sb_new->data) |
		V_DMA_DSCRA_A_SIZE(NUMCACHEBLKS(pktsize + NET_IP_ALIGN)) |
		M_DMA_DSCRA_INTERRUPT;

sb_new->data is set up with sbdma_align_skb(sb_new, SMP_CACHE_BYTES,
NET_IP_ALIGN). The address is therefore cache-line aligned plus 2, so
that bit is always 0.

If so, the hardware's TCP checksum result is never read. Any frame with
a valid IPv4 header checksum gets CHECKSUM_UNNECESSARY. That includes
TCP segments the hardware flagged as having a bad TCP checksum, and the
stack then skips software verification of the payload.

This decoding seems to go back to the original driver code. Still, the
commit message says "The driver already trusts hardware RX checksum
status". This patch rewrites the same conditional and makes it a
default-on offload that ethtool reports.

Should this test dsc->dscr_b & M_DMA_ETHRX_BADTCPCS instead? The same
loop already reads dscr_b through G_DMA_DSCRB_PKT_SIZE(), so the
hardware does write that word back.

The later patch in the series ("Add ethtool autonegotiation restart
support") doesn't change this check either.

> @@ -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;
> +	}

[Severity: Medium]
Is the IP header offset that the checksum engine relies on actually
programmed while the interface is running? The comment above calls it
necessary, but sbmac_set_iphdr_offset() does:

	/* 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);
	__raw_writeq(reg, sc->sbm_rxfilter);

Because of operator precedence, this only masks the register and never
writes 15 into the field. S_MAC_IPHDR_OFFSET is 8 and the field is 8
bits wide, so the field ends up as the old value's low four bits.

There is a second problem. The path sbmac_open() ->
sbmac_set_channel_state() -> sbmac_channel_start() writes the whole of
sbm_rxfilter:

	__raw_writeq(0, s->sbm_rxfilter);
	...
	__raw_writeq(M_MAC_UCAST_EN | M_MAC_BCAST_EN, s->sbm_rxfilter);

That resets IPHDR_OFFSET to 0. sbmac_mii_poll() also calls
sbmac_channel_start() on link changes, and nothing reprograms the field
afterwards.

So while traffic is flowing, the checksum engine uses an IP header
offset the driver never intended. NETIF_F_RXCSUM is now advertised and
turned on by default based only on the revision check, and
sbdma_rx_process() trusts the engine's status bits.

Could the field be set with something like:

	reg &= ~M_MAC_IPHDR_OFFSET;
	reg |= V_MAC_IPHDR_OFFSET(15);

and kept across the rxfilter writes in sbmac_channel_start()? This code
predates the patch, and the later patches in the series don't touch it.

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

Re: [PATCH net-next 2/3] net: sb1250-mac: Advertise and honor RX checksum offload

[Florian Fainelli]

On 11/9/26 02:37, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · 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).
> 
> - [High] sbdma_rx_process()
>    (drivers/net/ethernet/broadcom/sb1250-mac.c:1146-1149) reads the…
> - [Medium] The RX checksum engine's IP-header-offset setup, which the
>    probe comment calls necessary ("This is needed for PASS2 for Rx H/W…
> 
> --
> 
> net: sb1250-mac: Advertise and honor RX checksum offload
> 
> This patch adds NETIF_F_RXCSUM to hw_features. It also enables the
> offload by default on chip revisions where sbmac_set_iphdr_offset()
> reports hardware checksum support. The receive path now tests the active
> feature bit, not sc->rx_hw_checksum, before it marks packets
> CHECKSUM_UNNECESSARY.
> 
>> diff --git a/drivers/net/ethernet/broadcom/sb1250-mac.c b/drivers/net/ethernet/broadcom/sb1250-mac.c
>> index 1843b52e96f51..546415d3134c6 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;
> 
> [Severity: High]
> In sbdma_rx_process(), is M_DMA_ETHRX_BADTCPCS tested against the right
> descriptor word? The header defines it as a dscr_b bit:
> 
> arch/mips/include/asm/sibyte/sb1250_dma.h:
> /* Note: This bit is in the DSCR_B options field */
> #define M_DMA_ETHRX_BADTCPCS	_SB_MAKEMASK1(0)
> 
> In dscr_a, bit 0 is the low bit of the RX buffer's physical address,
> which sbdma_add_rcvbuffer() programs:
> 
> 	dsc->dscr_a = virt_to_phys(sb_new->data) |
> 		V_DMA_DSCRA_A_SIZE(NUMCACHEBLKS(pktsize + NET_IP_ALIGN)) |
> 		M_DMA_DSCRA_INTERRUPT;
> 
> sb_new->data is set up with sbdma_align_skb(sb_new, SMP_CACHE_BYTES,
> NET_IP_ALIGN). The address is therefore cache-line aligned plus 2, so
> that bit is always 0.
> 
> If so, the hardware's TCP checksum result is never read. Any frame with
> a valid IPv4 header checksum gets CHECKSUM_UNNECESSARY. That includes
> TCP segments the hardware flagged as having a bad TCP checksum, and the
> stack then skips software verification of the payload.
> 
> This decoding seems to go back to the original driver code. Still, the
> commit message says "The driver already trusts hardware RX checksum
> status". This patch rewrites the same conditional and makes it a
> default-on offload that ethtool reports.
> 
> Should this test dsc->dscr_b & M_DMA_ETHRX_BADTCPCS instead? The same
> loop already reads dscr_b through G_DMA_DSCRB_PKT_SIZE(), so the
> hardware does write that word back.
> 
> The later patch in the series ("Add ethtool autonegotiation restart
> support") doesn't change this check either.
> 
>> @@ -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;
>> +	}
> 
> [Severity: Medium]
> Is the IP header offset that the checksum engine relies on actually
> programmed while the interface is running? The comment above calls it
> necessary, but sbmac_set_iphdr_offset() does:
> 
> 	/* 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);
> 	__raw_writeq(reg, sc->sbm_rxfilter);
> 
> Because of operator precedence, this only masks the register and never
> writes 15 into the field. S_MAC_IPHDR_OFFSET is 8 and the field is 8
> bits wide, so the field ends up as the old value's low four bits.
> 
> There is a second problem. The path sbmac_open() ->
> sbmac_set_channel_state() -> sbmac_channel_start() writes the whole of
> sbm_rxfilter:
> 
> 	__raw_writeq(0, s->sbm_rxfilter);
> 	...
> 	__raw_writeq(M_MAC_UCAST_EN | M_MAC_BCAST_EN, s->sbm_rxfilter);
> 
> That resets IPHDR_OFFSET to 0. sbmac_mii_poll() also calls
> sbmac_channel_start() on link changes, and nothing reprograms the field
> afterwards.
> 
> So while traffic is flowing, the checksum engine uses an IP header
> offset the driver never intended. NETIF_F_RXCSUM is now advertised and
> turned on by default based only on the revision check, and
> sbdma_rx_process() trusts the engine's status bits.
> 
> Could the field be set with something like:
> 
> 	reg &= ~M_MAC_IPHDR_OFFSET;
> 	reg |= V_MAC_IPHDR_OFFSET(15);
> 
> and kept across the rxfilter writes in sbmac_channel_start()? This code
> predates the patch, and the later patches in the series don't touch it.
> 

Both issues are valid and predate this series. I’ll send separate fixes 
for the descriptor word and IP header offset programming.
-- 
Florian

Re: [PATCH net-next 2/3] net: sb1250-mac: Advertise and honor RX checksum offload

[Jakub Kicinski]

On Mon, 9 Nov 2026 09:52:30 -0800 Florian Fainelli wrote:
> Both issues are valid and predate this series. I’ll send separate fixes 
> for the descriptor word and IP header offset programming.

The High one really looks like something that should be fixed _first_
-- 
pw-bot: defer

[PATCH net-next 3/3] 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.

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

Re: [PATCH net-next 3/3] net: sb1250-mac: Add ethtool autonegotiation restart support

[Maciej W. Rozycki]

On Thu, 5 Nov 2026, Florian Fainelli wrote:

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

  Maciej

Re: [PATCH net-next 3/3] net: sb1250-mac: Add ethtool autonegotiation restart support

[Daniel Machon]

> 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.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Daniel Machon <daniel.machon@microchip.com>

Re: [PATCH net-next 0/3]: net: sb1250-mac: ethtool link updates

[Maciej W. Rozycki]

On Thu, 5 Nov 2026, Florian Fainelli wrote:

> This patch series addresses a few minor issues:

 Thank you for doing this work.

 I ran minimal testing by booting my SWARM system with the changes applied 
which runs NFS-rooted over one of the sb1250-mac interfaces, so the driver 
gets exercised, and I have acked the individual patches separately now.

  Maciej