Re: [PATCH net 8/8] net: bcmgenet: mask DMA_TIMEOUT_MASK when reading DMA_RING0_TIMEOUT
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-22 00:58:56
Also in:
lkml
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 [off-list ref]
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 [off-list ref]" trailer be dropped as
well, given a tool account cannot certify the DCO?
quoted hunk ↗ jump to hunk
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