Re: [PATCH v17 net-next 1/2] octeontx2: use atomic bitops for PF/VF and rep flags

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: octeontx2: lost updates to nic->flags due to non-atomic RMW in otx2_sync_flags_from_rep()
--

commit 5abf537369dcb5cbbccf3809e8b59256537cd8b9
Author: Ratheesh Kannoth <rkannoth@marvell.com>

octeontx2: use atomic bitops for PF/VF and rep flags

This commit changes the flags field in PF/VF and representor structures
from a u64 bitmask to an unsigned long bitmap manipulated via atomic
bitops. It also introduces otx2_sync_flags_from_rep() to copy
representor-owned flags onto the PF context.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> index 5850bc1870a1d..b146ca4be5db1 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
[ ... ]
> @@ -593,6 +607,32 @@ struct otx2_nic {
>  	unsigned long		*af_xdp_zc_qidx;
>  };
>  
> +static inline void otx2_set_flag(struct otx2_nic *nic, unsigned int flag)
> +{
> +	set_bit(flag, &nic->flags);
> +}
> +
> +static inline void otx2_clear_flag(struct otx2_nic *nic, unsigned int flag)
> +{
> +	clear_bit(flag, &nic->flags);
> +}
> +
> +static inline bool otx2_test_flag(struct otx2_nic *nic, unsigned int flag)
> +{
> +	return test_bit(flag, &nic->flags);
> +}
> +
> +static inline void otx2_sync_flags_from_rep(struct otx2_nic *dst,
> +					    unsigned long *src_flags)
> +{
> +	unsigned long src = READ_ONCE(*src_flags);
> +	unsigned long new_flags;
> +
> +	new_flags = (READ_ONCE(dst->flags) & ~OTX2_REP_SYNC_FLAGS_MASK) |
> +		    (src & OTX2_REP_SYNC_FLAGS_MASK);
> +	WRITE_ONCE(dst->flags, new_flags);

[Severity: High]
Does this non-atomic read-modify-write on dst->flags race with concurrent
atomic bit updates?

If another thread modifies a bit in dst->flags (for example, setting
OTX2_FLAG_PORT_UP from an asynchronous mailbox event workqueue) between the
READ_ONCE() and the WRITE_ONCE(), that update would be silently overwritten
and lost.

> +}
> +
>  static inline bool is_otx2_lbkvf(struct pci_dev *pdev)
>  {
>  	return (pdev->device == PCI_DEVID_OCTEONTX2_RVU_AFVF) ||
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> index 0f5d5642d3f7a..7df82c22cc12b 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> @@ -109,14 +109,14 @@ static int rvu_rep_setup_tc_cb(enum tc_setup_type type,
>  	struct rep_dev *rep = cb_priv;
>  	struct otx2_nic *priv = rep->mdev;
>  
> -	if (!(rep->flags & RVU_REP_VF_INITIALIZED))
> +	if (!test_bit(OTX2_FLAG_REP_VF_INITIALIZED, &rep->flags))
>  		return -EINVAL;
>  
> -	if (!(rep->flags & OTX2_FLAG_TC_FLOWER_SUPPORT))
> +	if (!test_bit(OTX2_FLAG_TC_FLOWER_SUPPORT, &rep->flags))
>  		rvu_rep_mcam_flow_init(rep);
>  
>  	priv->netdev = rep->netdev;
> -	priv->flags = rep->flags;
> +	otx2_sync_flags_from_rep(priv, &rep->flags);

[Severity: High]
Is it possible for this call to race with concurrent hardware link events?

Because otx2_sync_flags_from_rep() performs a non-atomic update to 
priv->flags, if an asynchronous hardware event attempts to set or clear a 
bit in priv->flags while this is executing, the hardware state change 
could be discarded, leading to incorrect device state.

>  	priv->pcifunc = rep->pcifunc;
>  	priv->flow_cfg = rep->flow_cfg;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923032217.1732753-1-rkannoth@marvell.com?part=1