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

[sashiko-bot@kernel.org]

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

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