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

[Florian Fainelli <f.fainelli@gmail.com>]

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