Re: [PATCH net-next 2/8] net/mlx5: Use v1 response layout for query_esw_functions

[Simon Horman]

On Fri, May 15, 2026 at 02:59:09PM +0300, Moshe Shemesh wrote:
> 
> 
> On 5/14/2026 10:18 PM, Simon Horman wrote:
> > From: 'Simon Horman' <horms@xxxxxxxxxx>
> > 
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://sashiko.dev
> > ---
> > net/mlx5: Use v1 response layout for query_esw_functions
> > 
> > This patch teaches mlx5_esw_query_functions() to use the v1 response
> > layout (MLX5_QUERY_ESW_FUNC_OP_MOD_LAYOUT_V1) when the firmware
> > advertises query_host_net_function_v1, allocates the output buffer
> > based on query_host_net_function_num_max, and adds a parser for the
> > new network_function_params layout in mlx5_esw_get_host_pf_info().
> > 
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > index 861e79ddb489..8b62dde7eb70 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > 
> > [ ... ]
> > 
> > > @@ -1100,12 +1133,55 @@ mlx5_esw_host_pf_from_host_params(const void *entry)
> > >        };
> > >   }
> > > 
> > > -struct mlx5_esw_pf_info mlx5_esw_get_host_pf_info(const u32 *out)
> > > +static struct mlx5_esw_pf_info
> > > +mlx5_esw_host_pf_from_net_func_params(const u8 *entry, int num_entries)
> > > +{
> > > +     int i;
> > > +
> > > +     for (i = 0; i < num_entries; i++) {
> > > +             int pf_type, state;
> > > +
> > > +             pf_type = MLX5_GET(network_function_params, entry, pci_pf_type);
> > > +             if (pf_type != MLX5_PCI_PF_TYPE_EXTERNAL_HOST_PF) {
> > > +                     entry += MLX5_UN_SZ_BYTES(net_function_params);
> > > +                     continue;
> > > +             }
> > > +
> > > +             state = MLX5_GET(network_function_params, entry, vhca_state);
> > > +
> > > +             return (struct mlx5_esw_pf_info) {
> > > +                     .pf_disabled = state != MLX5_VHCA_STATE_IN_USE,
> > 
> > [Medium]
> > The commit message says "this change does not introduce new
> > functionality," but the legacy parser reads an explicit firmware
> > host_pf_disabled bit while the v1 parser derives pf_disabled from a
> > multi-valued vhca_state enum:
> > 
> >    .pf_disabled = state != MLX5_VHCA_STATE_IN_USE,
> > 
> > Is this mapping intended to be exactly equivalent to the legacy
> > host_pf_disabled bit across all five vhca_state values
> > (INVALID/ALLOCATED/ACTIVE/IN_USE/TEARDOWN_REQUEST)?
> > 
> > The same vhca_state value is interpreted differently elsewhere in
> > mlx5 for SFs. mlx5_sf_to_devlink_opstate() in
> > drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c reports:
> > 
> >      case MLX5_VHCA_STATE_IN_USE:
> >      case MLX5_VHCA_STATE_TEARDOWN_REQUEST:
> >              return DEVLINK_PORT_FN_OPSTATE_ATTACHED;
> >      case MLX5_VHCA_STATE_INVALID:
> >      case MLX5_VHCA_STATE_ALLOCATED:
> >      case MLX5_VHCA_STATE_ACTIVE:
> >      default:
> >              return DEVLINK_PORT_FN_OPSTATE_DETACHED;
> > 
> > so SFs in TEARDOWN_REQUEST are reported as ATTACHED, while a host
> > PF in TEARDOWN_REQUEST would now end up as pf_disabled = true and
> > therefore DETACHED via mlx5_devlink_pf_port_fn_state_get():
> 
> PFs do not use the state TEARDOWN_REQUEST.
> Once the ECPF sets the PF disbale_hca, it is down and can't be used till
> ECPF does PF enable_hca.
> So state != MLX5_VHCA_STATE_IN_USE is the equivalent to the legacy
> host_pf_disabled.

This was the main thing I was concerned about.
Thanks for clarifying.

> 
> > 
> > > +                     .num_of_vfs = MLX5_GET(network_function_params,
> > > +                                            entry, pci_num_vfs),
> > > +                     .total_vfs = MLX5_GET(network_function_params,
> > > +                                           entry, pci_total_vfs),
> > > +                     .host_number = MLX5_GET(network_function_params,
> > > +                                             entry, host_number),
> > > +             };
> > > +     }
> > 
> > [Medium]
> > pf_disabled also feeds esw_vfs_changed_event_handler(), which skips
> > VF reload when it is true:
> > 
> > > @@ -3716,7 +3716,7 @@ static void esw_vfs_changed_event_handler(struct mlx5_eswitch *esw)
> > [ ... ]
> > > -     host_pf_info = mlx5_esw_get_host_pf_info(out);
> > > +     host_pf_info = mlx5_esw_get_host_pf_info(esw->dev, out);
> > >        new_num_vfs = host_pf_info.num_of_vfs;
> > > 
> > >        if (new_num_vfs == esw->esw_funcs.num_vfs || host_pf_info.pf_disabled)
> > 
> > Could the commit message spell out the firmware/PRM mapping between
> > the legacy host_pf_disabled bit and the vhca_state enum so it is
> > clear that ACTIVE, ALLOCATED and TEARDOWN_REQUEST really should be
> > treated as disabled here?
> 
> Yes, I can add explanation on that in commit message.
> 
> > 
> > Would it also be worth aligning with mlx5_sf_to_devlink_opstate()
> > (at least for TEARDOWN_REQUEST) so the devlink opstate reported via
> > mlx5_devlink_pf_port_fn_state_get() stays consistent between SFs and
> > host PFs on v1-capable firmware?
> 
> Same answer as first comment above.
> 
>