Re: [PATCH net-next 15/15] net: enetc: generate MR interrupt mask based on the number of enabled VFs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `PSIIER_MR_MASK` macro passes an invalid range to `GENMASK_U32` when `num_vfs` is 0, violating the API contract.
- [Low] The patch leaves behind `ENETC_PSIMSGRR_MR_MASK` as dead code.
--

commit f4d94f53cb9779f541f15301b0d99d09fa09287a
Author: Wei Fang <wei.fang@nxp.com>

net: enetc: generate MR interrupt mask based on the number of enabled VFs

This commit replaces the fixed ENETC_PSIIER_MR_MASK constant with a
dynamically calculated macro to support ENETC v4 hardware, which can
have varying numbers of VFs. It uses GENMASK_U32 to generate the mask
based on the pf->num_vfs value.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> index 94f53762cea82..77dd7913d1999 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> @@ -94,7 +94,8 @@ static inline u32 enetc_vsi_set_msize(u32 size)
>  #define ENETC_SICAPR1	0x904
>  
>  #define ENETC_PSIIER	0xa00
> -#define ENETC_PSIIER_MR_MASK	GENMASK(2, 1)
> +#define  PSIIER_MR_MASK(num_vf)	GENMASK_U32((num_vf), 1)

The commit message mentions this should correctly handle devices with 0 VFs.
If num_vf is 0, this expands to GENMASK_U32(0, 1). Does this violate the
GENMASK requirement that the high bit must be greater than or equal to the
low bit?

Could this macro be updated to explicitly handle the zero case, perhaps by
using a conditional, to avoid triggering warnings from static analyzers?

> +
>  #define ENETC_PSIIDR	0xa08
>  #define ENETC_SITXIDR	0xa18
>  #define ENETC_SIRXIDR	0xa28

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> index 73e32c9b65a89..e9963ea154b04 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> @@ -162,15 +162,16 @@ static void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int vf_id,
>  static void enetc_msg_task(struct work_struct *work)
>  {
>  	struct enetc_pf *pf = container_of(work, struct enetc_pf, msg_task);
> +	u32 mr_mask = PSIIER_MR_MASK(pf->num_vfs);
>  	struct enetc_hw *hw = &pf->si->hw;
> -	unsigned long mr_mask;
> +	u32 mr_status;
>  	int i;
>  
>  	for (;;) {
> -		mr_mask = enetc_rd(hw, ENETC_PSIMSGRR) & ENETC_PSIMSGRR_MR_MASK;
> -		if (!mr_mask) {

Since ENETC_PSIMSGRR_MR_MASK is no longer used here, is it now dead code?

Should the definition of ENETC_PSIMSGRR_MR_MASK in enetc_hw.h be removed in
this patch to avoid leaving behind unused macros?

> +		mr_status = enetc_rd(hw, ENETC_PSIMSGRR) & mr_mask;
> +		if (!mr_status) {
>  			/* re-arm MR interrupts, w1c the IDR reg */
> -			enetc_wr(hw, ENETC_PSIIDR, ENETC_PSIIER_MR_MASK);
> +			enetc_wr(hw, ENETC_PSIIDR, mr_mask);
>  			enetc_msg_set_mr_int(pf, true);
>  			return;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511080805.2052495-1-wei.fang@nxp.com?part=15