Re: [PATCH net 8/8] net: bcmgenet: mask DMA_TIMEOUT_MASK when reading DMA_RING0_TIMEOUT

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Submission metadata is not upstream-acceptable: the commit has no
  Signed-off-by line (mandatory DCO certification per…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patched
  conversion line makes it visible: bcmgenet_get_coalesce() can report…

--

net: bcmgenet: mask DMA_TIMEOUT_MASK when reading DMA_RING0_TIMEOUT

This patch makes bcmgenet_get_coalesce() mask the DMA_RING0_TIMEOUT
register read with DMA_TIMEOUT_MASK before converting the tick count
into microseconds, so bits outside the 16-bit timeout field no longer
affect the rx_coalesce_usecs value reported to userspace.

Two observations below, one on the commit trailers and one on the
get/set round trip through this conversion.

[Severity: Low]
Looking at the trailers in the commit message:

    Fixes: 4a29645bfe6c ("net: bcmgenet: Implement RX coalescing control knobs")
    Assisted-by: LLM
    Co-authored-by: Cursor <cursoragent@cursor.com>
    Change-Id: I4c5b5019e764dbd0c7e51bd8c7365939a4736e44

There is no Signed-off-by line, which
Documentation/process/submitting-patches.rst requires for the DCO
certification.

Should the Gerrit "Change-Id:" trailer and the
"Co-authored-by: Cursor <cursoragent@cursor.com>" trailer be dropped as
well, given a tool account cannot certify the DCO?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 3a6a2f0759598..b15e64a484b43 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -852,7 +852,8 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
>  	ec->rx_max_coalesced_frames =
>  		bcmgenet_rdma_ring_readl(priv, 0, DMA_MBUF_DONE_THRESH);
>  	ec->rx_coalesce_usecs =
> -		bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT) * 8192 / 1000;
> +		(bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT) &
> +		 DMA_TIMEOUT_MASK) * 8192 / 1000;

[Severity: Medium]
This isn't a bug introduced by this patch, but since the conversion on
this line is being touched: can the value reported here be rejected by
bcmgenet_set_coalesce()?

bcmgenet_set_coalesce() accepts up to (DMA_TIMEOUT_MASK * 8) + 1, i.e.
524281 us:

	if (ec->tx_max_coalesced_frames > DMA_INTR_THRESHOLD_MASK ||
	    ec->tx_max_coalesced_frames == 0 ||
	    ec->rx_max_coalesced_frames > DMA_INTR_THRESHOLD_MASK ||
	    ec->rx_coalesce_usecs > (DMA_TIMEOUT_MASK * 8) + 1)
		return -EINVAL;

bcmgenet_set_rx_coalesce() then rounds that up:

	reg |= DIV_ROUND_UP(usecs * 1000, 8192);

8192 * 63999 = 524279808, which is below 524281000, so 524281 us is
programmed as 64000 ticks.

Reading it back through the patched conversion gives
64000 * 8192 / 1000 = 524288, which is larger than the 524281 the
setter accepts.

Since the ethtool utility does ETHTOOL_GCOALESCE, modifies one field and
then issues ETHTOOL_SCOALESCE with the full structure, after
"ethtool -C eth0 rx-usecs 524281" a later "ethtool -C eth0 rx-frames N"
would return -EINVAL until rx-usecs is lowered.

Would it make sense to derive the range check in
bcmgenet_set_coalesce() from the same tick conversion and rounding used
by bcmgenet_set_rx_coalesce() and this getter, so the round trip is
consistent?

>  
>  	for (i = 0; i <= priv->hw_params->rx_queues; i++) {
>  		ring = &priv->rx_rings[i];

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