Re: [PATCH net] net/mlx5: E-switch, Query port function mac only when vport is enabled

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new `if (vport->enabled)` guard in
  mlx5_devlink_port_fn_hw_addr_get()…

--

net/mlx5: E-switch, Query port function mac only when vport is enabled

This commit wraps the mlx5_query_nic_vport_mac_address() call in
mlx5_devlink_port_fn_hw_addr_get() in a vport->enabled check. The goal is
to stop QUERY_NIC_VPORT_CONTEXT from failing in firmware when a port
function is dumped while its vport is not enabled.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index c712848b202db..b83c6c02670f8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -5121,8 +5121,9 @@ int mlx5_devlink_port_fn_hw_addr_get(struct devlink_port *port,
>  
>  	mutex_lock(&esw->state_lock);
>  
> -	mlx5_query_nic_vport_mac_address(esw->dev, vport->vport, true,
> -					 vport->info.mac);
> +	if (vport->enabled)
> +		mlx5_query_nic_vport_mac_address(esw->dev, vport->vport, true,
> +						 vport->info.mac);

[Severity: Medium]
Can this check ever be false here? The commit message says "A port
function can be dumped while its vport is not enabled". The ordering of
devlink port registration against vport enable/disable seems to rule
that out.

mlx5_devlink_port_fn_hw_addr_get() is reachable only through the
.port_fn_hw_addr_get op in mlx5_esw_pf_vf_dl_port_ops and
mlx5_esw_dl_sf_port_ops. Devlink calls that op only on registered ports.

Those ports are registered in mlx5_esw_offloads_devlink_port_register(),
via mlx5_esw_offloads_load_rep(). That happens only after the vport has
been enabled:

drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:mlx5_eswitch_load_vport() {
    ...
	err = mlx5_esw_vport_enable(esw, vport, enabled_events);
	if (err)
		return err;

	err = mlx5_esw_offloads_load_rep(esw, vport);
    ...
}

On teardown, the devlink port is unregistered before the vport is
disabled:

drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:mlx5_eswitch_unload_vport() {
	mlx5_esw_offloads_unload_rep(esw, vport);
	mlx5_esw_vport_disable(esw, vport);
}

mlx5_esw_vport_enable() and mlx5_esw_vport_disable() are the only places
that write vport->enabled. Rep netdev notifications that reach this op
also run inside the same window. So every dump, doit or notify that gets
here should already see vport->enabled == true.

If so, would the quoted QUERY_NIC_VPORT_CONTEXT "bad parameter(0x3),
syndrome (0x29d722)" error still occur after this patch?

One possibility is a vport the driver has enabled but firmware still
rejects for the query, such as an inactive SF. In that case this query
still runs and still fails.

Another candidate is the unconditional query that the same Fixes commit
eea31f21dce1 added to esw_vport_setup(). It runs on every vport enable,
including bulk SF creation:

drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:esw_vport_setup() {
    ...
	mlx5_query_nic_vport_mac_address(esw->dev, vport_num, true,
					 vport->info.mac);
    ...
}

This patch leaves that query unchanged. Was the failing path confirmed
to be mlx5_devlink_port_fn_hw_addr_get() and not esw_vport_setup()?

>  	ether_addr_copy(hw_addr, vport->info.mac);
>  	*hw_addr_len = ETH_ALEN;
>  	mutex_unlock(&esw->state_lock);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110092648.2511196-1-tariqt%40nvidia.com