On 1/31/26 5:49 AM, Ziyi Guo wrote:
quoted hunk ↗ jump to hunk
ocelot_port_xmit() calls ocelot_can_inject() and ocelot_port_inject_frame()
without holding ocelot->inj_lock. However, both functions have
lockdep_assert_held(&ocelot->inj_lock) indicating that callers must hold
this lock.
The correct caller felix_port_deferred_xmit() properly acquires the lock
using ocelot_lock_inj_grp() before calling these functions.
Add ocelot_lock_inj_grp()/ocelot_unlock_inj_grp() around the injection
code path in ocelot_port_xmit() to fix the missing lock protection.
Fixes: c5e12ac3beb0 ("net: mscc: ocelot: serialize access to the injection/extraction groups")
Signed-off-by: Ziyi Guo <redacted>
---
v2:
- Add missing Fixes tag
drivers/net/ethernet/mscc/ocelot_net.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 469784d3a1a6..da8579abea1e 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -559,15 +559,22 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
int port = priv->port.index;
u32 rew_op = 0;
- if (!static_branch_unlikely(&ocelot_fdma_enabled) &&
- !ocelot_can_inject(ocelot, 0))
- return NETDEV_TX_BUSY;
+ if (!static_branch_unlikely(&ocelot_fdma_enabled)) {
+ ocelot_lock_inj_grp(ocelot, 0);
+
+ if (!ocelot_can_inject(ocelot, 0)) {
+ ocelot_unlock_inj_grp(ocelot, 0);
+ return NETDEV_TX_BUSY;
+ }
+ }
/* Check if timestamping is needed */
if (ocelot->ptp && (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
struct sk_buff *clone = NULL;
if (ocelot_port_txtstamp_request(ocelot, port, skb, &clone)) {
+ if (!static_branch_unlikely(&ocelot_fdma_enabled))
+ ocelot_unlock_inj_grp(ocelot, 0);
I'm under the impression this static_branch_unlikely() usage is racy,
i.e. on CONFIG_PREEMPT kernel execution could enter this branch but not
the paired lock.
What about moving the 'Check timestamp' block in a separate helper and
use a single static_branch_unlikely() branch? something alike the
following, completely untested and unfinished:
if (!static_branch_unlikely(&ocelot_fdma_enabled)) {
int ret = NETDEV_TX_OK;
ocelot_lock_inj_grp(ocelot, 0);
if (!ocelot_can_inject(ocelot, 0)) {
ret = NETDEV_TX_BUSY;
goto unlock;
}
if (!ocelot_timestamp_check())
goto unlock;
ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
consume_skb(skb);
unlock:
ocelot_unlock_inj_grp(ocelot, 0);
return ret;
}
if (!ocelot_timestamp_check())
return NETDEV_TX_OK;
ocelot_fdma_inject_frame(ocelot, port, rew_op, skb, dev);
return NETDEV_TX_OK;
Well, after scratching the above, I noted it would probably better to
invert the two branches...
/P