Re: [PATCH net-next v2 10/11] net: ethernet: qualcomm: ppe: Add IPQ5424 service code configuration

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch adds PPE_SC_BYPASS_EGRESS_SMAC_MC_DROP (24),
  PPE_SC_BYPASS_EGRESS_L2_FLOODING (25) and…

--

net: ethernet: qualcomm: ppe: Add IPQ5424 service code configuration

This patch makes the stride of the L2 service code table depend on the
SoC: 0x20 on IPQ5424 and 0x10 on IPQ9574. On IPQ5424 it also writes a
second word per entry with the extended egress bypass bitmap. Three new
egress bypass bits (24-26) are added for source MAC multicast drop, L2
flooding and tunnel exception counter control.

> diff --git a/drivers/net/ethernet/qualcomm/ppe/ppe_config.c b/drivers/net/ethernet/qualcomm/ppe/ppe_config.c
> index 017d2e8342511..3dc1bcc8949eb 100644
> --- a/drivers/net/ethernet/qualcomm/ppe/ppe_config.c
> +++ b/drivers/net/ethernet/qualcomm/ppe/ppe_config.c

[ ... ]

> @@ -1490,9 +1490,16 @@ int ppe_sc_config_set(struct ppe_device *ppe_dev, int sc, struct ppe_sc_cfg cfg)
>  			  test_bit(PPE_SC_BYPASS_COUNTER_RX, cfg.bitmaps.counter));
>  	val |= FIELD_PREP(PPE_IN_L2_SERVICE_TBL_TX_CNT_EN,
>  			  test_bit(PPE_SC_BYPASS_COUNTER_TX, cfg.bitmaps.counter));
> -	reg = PPE_IN_L2_SERVICE_TBL_ADDR + PPE_IN_L2_SERVICE_TBL_INC * sc;
> +	reg = PPE_IN_L2_SERVICE_TBL_ADDR + PPE_IN_L2_SERVICE_TBL_INC(ppe_dev) * sc;
>
> -	ret = regmap_write(ppe_dev->regmap, reg, val);
> +	in_l2_val[0] = val;
> +
> +	bitmap_value = bitmap_read(cfg.bitmaps.egress, PPE_SC_BYPASS_EGRESS_SMAC_MC_DROP,
> +				   PPE_SC_BYPASS_EGRESS_SIZE - PPE_SC_BYPASS_EGRESS_SMAC_MC_DROP);
> +	in_l2_val[1] = FIELD_PREP(PPE_IN_L2_SERVICE_TBL_W1_BYPASS_BITMAP_EXT, bitmap_value);

[Severity: Low]
Is the CPU service code meant to bypass all three new checks on IPQ5424?

The new enum values raise PPE_SC_BYPASS_EGRESS_SIZE from 24 to 27.
ppe_servcode_init() is unchanged, and it still fills the whole egress
bitmap and clears only one bit:

drivers/net/ethernet/qualcomm/ppe/ppe_config.c:ppe_servcode_init() {
    ...
	bitmap_fill(sc_cfg.bitmaps.egress, PPE_SC_BYPASS_EGRESS_SIZE);
	clear_bit(PPE_SC_BYPASS_EGRESS_ACL_POST_ROUTING_CHECK, sc_cfg.bitmaps.egress);

	return ppe_sc_config_set(ppe_dev, PPE_EDMA_SC_BYPASS_ID, sc_cfg);
}

Because of this, bitmap_read() here returns 0x7, and that value goes
into word 1 of the entry for PPE_EDMA_SC_BYPASS_ID on IPQ5424. Service
code 1 then bypasses source MAC multicast drop, L2 flooding and tunnel
exception counter control.

This matches the existing policy of bypassing every egress feature
except the ACL post-routing check, and IPQ9574 is unaffected. However,
neither the commit message nor a code comment says that the default CPU
service code now enables these bypasses.

Could the commit message, or a comment in ppe_servcode_init(), say
whether this is intended?

> +
> +	ret = regmap_bulk_write(ppe_dev->regmap, reg, in_l2_val,
> +				PPE_IN_L2_SERVICE_TBL_WORDS(ppe_dev));
>  	if (ret)
>  		return ret;
>

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-qcom_ipq5424_ppe-v2-0-8c7b057e2c43%40oss.qualcomm.com