# Re: \[PATCH v2\] net: mscc: ocelot: add missing lock protection in ocelot\_port\_xmit()

- From: Vladimir Oltean \<vladimir.oltean@nxp.com\>
- Date: 2026-02-08 13:35:32 
- List: netdev
- Page: https://ratatoskr.run/netdev/2026/02/3406680
- Thread: https://ratatoskr.run/netdev/2026/01/3395606/t.md

````text
On Wed, Feb 04, 2026 at 11:49:49PM -0600, Ziyi Guo wrote:
> On Tue, Feb 3, 2026 at 5:41 AM Paolo Abeni [ref] 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.
````