drivers/net/ethernet/mscc/ocelot_net.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
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 <n7l8m4@u.northwestern.edu>
---
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);
kfree_skb(skb);
return NETDEV_TX_OK;
}
@@ -582,6 +589,7 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
ocelot_fdma_inject_frame(ocelot, port, rew_op, skb, dev);
} else {
ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
+ ocelot_unlock_inj_grp(ocelot, 0);
consume_skb(skb);
}
--
2.34.1On 1/31/26 5:49 AM, Ziyi Guo wrote:
> 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 <n7l8m4@u.northwestern.edu>
> ---
> 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
On Tue, Feb 3, 2026 at 5:41 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> 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...
Hi Paolo,
Thank you very much for your review and comments!
How about we use a new separate helper function like this for previous
'Check timestamp' block:
```
static bool ocelot_xmit_timestamp(struct ocelot *ocelot, int port,
struct sk_buff *skb, u32 *rew_op)
{
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)) {
kfree_skb(skb);
return false;
}
if (clone)
OCELOT_SKB_CB(skb)->clone = clone;
*rew_op = ocelot_ptp_rew_op(skb);
}
return true;
}
```
So for the function ocelot_port_xmit()
it will be:
```
static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct
net_device *dev)
{
struct ocelot_port_private *priv = netdev_priv(dev);
struct ocelot_port *ocelot_port = &priv->port;
struct ocelot *ocelot = ocelot_port->ocelot;
int port = priv->port.index;
u32 rew_op = 0;
/* FDMA path: uses its own locking, handle separately */
if (static_branch_unlikely(&ocelot_fdma_enabled)) {
if (!ocelot_xmit_timestamp(ocelot, port, skb, &rew_op))
return NETDEV_TX_OK;
ocelot_fdma_inject_frame(ocelot, port, rew_op, skb, dev);
return NETDEV_TX_OK;
}
/* Register injection path: needs inj_lock held throughout */
ocelot_lock_inj_grp(ocelot, 0);
if (!ocelot_can_inject(ocelot, 0)) {
ocelot_unlock_inj_grp(ocelot, 0);
return NETDEV_TX_BUSY;
}
if (!ocelot_xmit_timestamp(ocelot, port, skb, &rew_op)) {
ocelot_unlock_inj_grp(ocelot, 0);
return NETDEV_TX_OK;
}
ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
ocelot_unlock_inj_grp(ocelot, 0);
consume_skb(skb);
return NETDEV_TX_OK;
}
```
Feel free to let me know your thoughts!
I can send a v3 version patch once we're aligned.
On Wed, Feb 04, 2026 at 11:49:49PM -0600, Ziyi Guo wrote:
> On Tue, Feb 3, 2026 at 5:41 AM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > 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...
>
>
> Hi Paolo,
>
> Thank you very much for your review and comments!
>
> How about we use a new separate helper function like this for previous
> 'Check timestamp' block:
>
> ```
> static bool ocelot_xmit_timestamp(struct ocelot *ocelot, int port,
> struct sk_buff *skb, u32 *rew_op)
> {
> 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)) {
> kfree_skb(skb);
> return false;
> }
>
> if (clone)
> OCELOT_SKB_CB(skb)->clone = clone;
>
> *rew_op = ocelot_ptp_rew_op(skb);
> }
>
> return true;
> }
> ```
>
> So for the function ocelot_port_xmit()
>
> it will be:
>
> ```
> static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct
> net_device *dev)
> {
> struct ocelot_port_private *priv = netdev_priv(dev);
> struct ocelot_port *ocelot_port = &priv->port;
> struct ocelot *ocelot = ocelot_port->ocelot;
> int port = priv->port.index;
> u32 rew_op = 0;
>
> /* FDMA path: uses its own locking, handle separately */
> if (static_branch_unlikely(&ocelot_fdma_enabled)) {
> if (!ocelot_xmit_timestamp(ocelot, port, skb, &rew_op))
> return NETDEV_TX_OK;
>
> ocelot_fdma_inject_frame(ocelot, port, rew_op, skb, dev);
> return NETDEV_TX_OK;
> }
>
> /* Register injection path: needs inj_lock held throughout */
> ocelot_lock_inj_grp(ocelot, 0);
>
> if (!ocelot_can_inject(ocelot, 0)) {
> ocelot_unlock_inj_grp(ocelot, 0);
> return NETDEV_TX_BUSY;
> }
>
> if (!ocelot_xmit_timestamp(ocelot, port, skb, &rew_op)) {
> ocelot_unlock_inj_grp(ocelot, 0);
> return NETDEV_TX_OK;
> }
>
> ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
>
> ocelot_unlock_inj_grp(ocelot, 0);
>
> consume_skb(skb);
>
> return NETDEV_TX_OK;
> }
> ```
>
> Feel free to let me know your thoughts!
>
> I can send a v3 version patch once we're aligned.
The idea is not bad, but I would move one step further.
Refactor the rew_op handling into an ocelot_xmit_timestamp() function as
a first preparatory patch. The logic will need to be called from two
places and it's good not to duplicate it.
Then create two separate ocelot_port_xmit_fdma() and ocelot_port_xmit_inj(),
as a second preparatory patch.
static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
{
if (static_branch_unlikely(&ocelot_fdma_enabled))
return ocelot_port_xmit_fdma(skb, dev);
return ocelot_port_xmit_inj(skb, dev);
}
Now, as the third patch, add the required locking in ocelot_port_xmit_inj().
It's best for the FDMA vs register injection code paths to be as
separate as possible.
On Sun, Feb 8, 2026 at 7:35 AM Vladimir Oltean
<vladimir.oltean@nxp.com> wrote:
>
> The idea is not bad, but I would move one step further.
>
> Refactor the rew_op handling into an ocelot_xmit_timestamp() function as
> a first preparatory patch. The logic will need to be called from two
> places and it's good not to duplicate it.
>
> Then create two separate ocelot_port_xmit_fdma() and ocelot_port_xmit_inj(),
> as a second preparatory patch.
>
> static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> if (static_branch_unlikely(&ocelot_fdma_enabled))
> return ocelot_port_xmit_fdma(skb, dev);
>
> return ocelot_port_xmit_inj(skb, dev);
> }
>
> Now, as the third patch, add the required locking in ocelot_port_xmit_inj().
>
> It's best for the FDMA vs register injection code paths to be as
> separate as possible.
Thanks Vladimir and folks for your time. I'll split it into three
patches as series, and send a v3 version.
Best,
Ziyi
© 2016 - 2026 Red Hat, Inc.