Re: [PATCH net 08/10] i40e: fix memcmp of pointer in i40e_hw_set_dcb_config()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
i40e: fix memcmp of pointer in i40e_hw_set_dcb_config()

The patch changes the "need reconfiguration" check in
i40e_hw_set_dcb_config() so that memcmp() compares the two
struct i40e_dcbx_config objects instead of the two pointer variables on
the stack. With the old code the compare could never succeed, so the
early return was never taken.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index a04683004a567..0ac1be6289f6b 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -6907,7 +6907,7 @@ static int i40e_hw_set_dcb_config(struct i40e_pf *pf,
>  	int ret;
>  
>  	/* Check if need reconfiguration */
> -	if (!memcmp(&new_cfg, &old_cfg, sizeof(new_cfg))) {
> +	if (!memcmp(new_cfg, old_cfg, sizeof(*new_cfg))) {
>  		dev_dbg(&pf->pdev->dev, "No Change in DCB Config required.\n");
>  		return 0;
>  	}

[Severity: High]
Does this compare now always report "no change" when the function is
reached from the tail of i40e_hw_dcb_config()?

i40e_hw_set_dcb_config() takes its old side straight from the PF:

	struct i40e_dcbx_config *old_cfg = &pf->hw.local_dcbx_config;

but i40e_hw_dcb_config() has already published the new configuration into
that same object before making the call:

i40e_main.c:i40e_hw_dcb_config() {
	...
	old_cfg = &hw->local_dcbx_config;
	...
	/* Update the port DCBx configuration */
	*old_cfg = *new_cfg;

	/* Changes in configuration update VEB/VSI */
	i40e_dcb_reconfigure(pf);
out:
	...
		/* registers are set, lets apply */
		if (test_bit(I40E_HW_CAP_USE_SET_LLDP_MIB, pf->hw.caps))
			ret = i40e_hw_set_dcb_config(pf, new_cfg);
}

So new_cfg and pf->hw.local_dcbx_config are byte-identical at that point,
memcmp() returns 0, and the callee logs "No Change in DCB Config required."
and returns 0.

Does that make the "registers are set, lets apply" step dead code for this
caller? Everything after the early return is skipped, including the etsrec
normalization and the only code that pushes the configuration to firmware:

	*old_cfg = *new_cfg;
	old_cfg->etsrec = old_cfg->etscfg;
	ret = i40e_set_dcb_config(&pf->hw);

i40e_dcb.c:i40e_set_dcb_config() is the sole path that builds an LLDPDU via
i40e_dcb_config_to_lldp() from hw->local_dcbx_config and issues
i40e_aq_set_lldp_mib() for the local MIB. With the early return taken, the
DCB registers, packet buffer and PFC state plus the cached
local_dcbx_config carry the new values while the firmware's local DCBX MIB
still holds the previous one.

Which callers does this affect? The DCBNL entry points in i40e_dcb_nl.c
(i40e_dcbnl_ieee_setets(), i40e_dcbnl_ieee_setpfc(), i40e_dcbnl_ieee_setapp(),
i40e_dcbnl_ieee_delapp() and the CEE i40e_dcbnl_cee_set_all()) all copy
*old_cfg into pf->tmp_cfg and then call i40e_hw_dcb_config(pf, &pf->tmp_cfg),
so on parts with I40E_HW_CAP_USE_SET_LLDP_MIB the AQC is no longer sent for
those changes.

Is the resulting behaviour also inverted? On the error paths of
i40e_hw_dcb_config() (the goto out after i40e_aq_config_switch_comp_ets() or
i40e_aq_dcb_updated() fails) the "*old_cfg = *new_cfg" assignment is skipped,
the two structs still differ, and the full quiesce / Set LLDP MIB /
reconfigure sequence does run. That would mean the AQC is now issued only
when the earlier register programming failed.

The i40e_dcb_sw_default_config() call site looks unaffected, since its
freshly memset pf->tmp_cfg differs from a previously cached
local_dcbx_config (etsrec in particular), so the compare still fails there.

Would moving the comparison ahead of the caller's "*old_cfg = *new_cfg", or
not pre-publishing the config in the caller, be a safer shape for a fix that
is tagged for stable?

The commit message says:

    As a result, the memcmp never compares the actual configuration data,
    meaning the "no change needed" early return never fires. Every call to
    this function performs a full DCB reconfiguration (quiescing all VSIs,
    reprogramming via "Set LLDP MIB" AQC, and reconfiguring VEB/VSIs) even
    when the configuration has not changed.

Given the i40e_hw_dcb_config() call site above, is it accurate to describe
the only effect as a redundant reconfiguration? For that caller the old
comparison was load-bearing: comparing &pf->tmp_cfg against
&pf->hw.local_dcbx_config could never match, which is what kept the
Set Local LLDP MIB programming running. Could the changelog cover what
happens at that call site after the fix?