Re: [PATCH v14 net-next] octeontx2-pf: add mqprio bandwidth offload for NIX TX schedulers

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-09-09 at 11:55:43, sashiko-bot@kernel.org (sashiko-bot@kernel.org) wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Data Race / Invalid Cast: Mixing atomic bit operations with non-atomic bitwise operations on `u64 pf->flags` causes data races and memory corruption.
I agree that non-atomic accesses to pf->flags present an existing race condition across the driver.

In this patch (targeted for net-next), the flag accesses relevant to this change have been updated. To address the broader existing issue properly,
I will send a separate bug fix patch targeted for net to convert all remaining pf->flags accesses to atomic bitops (set_bit/clear_bit/test_bit).

As part of this commit (to net-next), all data races w.r.t to the flags (reported by sashiko) are fixed. I agree, there is an exisiting issue
of data races as flag is not set atomically elsewhere. we will convert all access to flags to set_bit/clear_bit as patch to net (as a fix patch).

> - [Medium] State Desynchronization: Failing to clear hardware shapers in `otx2_mqprio_down()` skips software state cleanup, permanently bricking mqprio state.

If otx2_mqprio_down() fails, it is typically due to an underlying mailbox communication failure with the AF, which we log to alert the user.
Because the hardware communication channel is compromised at that point, attempting additional mailbox calls to recover hardware state is likely to fail as well.

> --
>
> commit e74bdebdda4c26c75ad9a6fe1f2ba38bec570025
> Author: Ratheesh Kannoth <rkannoth@marvell.com>
> Subject: octeontx2-pf: add mqprio bandwidth offload for NIX TX schedulers
>
> This commit adds mqprio bandwidth offload for channel-mode mqprio with
> TC_MQPRIO_SHAPER_BW_RATE. It programs per-queue MDQ CIR/PIR through the NIX
> TX scheduler mailbox for each non-QoS transmit queue. The patch also manages
> the tearing down and rebuilding of the TX scheduler hierarchy when adding or
> replacing the qdisc.
>
> Link: https://lore.kernel.org/netdev/20260904031553.3196916-1-rkannoth@marvell.com/
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > @@ -907,11 +907,13 @@ static int otx2_mbox_up_handler_rep_event_up_notify(struct otx2_nic *pf,
> >
> >  	if (info->event == RVU_EVENT_PORT_STATE) {
> >  		if (info->evt_data.port_state) {
> > -			pf->flags |= OTX2_FLAG_PORT_UP;
> > +			set_bit(__builtin_ctzll(OTX2_FLAG_PORT_UP),
> > +				(unsigned long *)&pf->flags);
>
> [Severity: High]
> Could mixing atomic bit operations here with non-atomic bitwise operations
> elsewhere cause data races and memory corruption?
>
> For example, otx2_open() updates pf->flags using non-atomic read-modify-write
> operations:
>
>     pf->flags &= ~OTX2_FLAG_INTF_DOWN;
>     pf->flags &= ~OTX2_FLAG_PORT_UP;
>
> If an asynchronous link event triggers the mbox handler to update
> OTX2_FLAG_PORT_UP atomically while otx2_open() is non-atomically
> modifying pf->flags, the atomic updates could be overwritten and lost.
>
> Additionally, casting a u64 pointer to an unsigned long pointer for
> atomic operations can corrupt memory on 32-bit big-endian architectures.
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> [ ... ]
> > +int otx2_mqprio_down(struct otx2_nic *pfvf)
> > +{
> > +	int err = 0;
> > +
> > +	if (!pfvf->mqprio.rate_limit)
> > +		return 0;
> > +
> > +	if (netif_running(pfvf->netdev) &&
> > +	    otx2_mqprio_mdq_allocated(pfvf))
> > +		err = otx2_nix_tm_clear_queue_shaper(pfvf);
> > +
> > +	if (err) {
> > +		netdev_err(pfvf->netdev,
> > +			   "mqprio: failed to clear hardware shapers: %d\n",
> > +			   err);
> > +		return err;
>
> [Severity: Medium]
> Does returning early here permanently desynchronize the software state if
> clearing the hardware shapers fails?
>
> When removing the mqprio qdisc via a tc qdisc del command, the kernel
> calls ndo_setup_tc with hw=0 and ignores the return value. If
> otx2_nix_tm_clear_queue_shaper() fails (for instance, due to a mailbox
> timeout), returning early skips the call to otx2_mqprio_clear_sw().
>
> Since the kernel unconditionally destroys the qdisc software object, the
> driver will continue to incorrectly enforce rates during ndo_open and
> reject features like XDP, PFC, and HTB because it falsely believes
> mqprio is still active.
>
> > +	}
> > +
> > +	otx2_mqprio_clear_sw(pfvf);
> > +
> > +	return 0;
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260908062437.251739-1-rkannoth@marvell.com?part=1
>