Re: [PATCH v2] net: mscc: ocelot: add missing lock protection in ocelot_port_xmit()

[Ziyi Guo]

On Tue, Feb 3, 2026 at 5:41 AM Paolo Abeni <pabeni@xxxxxxxxxx> 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.