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

On Wed, Feb 04, 2026 at 11:49:49PM -0600, Ziyi Guo wrote:

On Tue, Feb 3, 2026 at 5:41 AM Paolo Abeni [off-list ref] wrote:

quoted

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.