linux-kernel - Re: [PATCH net-next] net/mlx5: Support devlink port state for host PF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a78c486d-a92d-43de-9c8c-3bb10b82ba37@nvidia.com>
Date: Thu, 5 Feb 2026 19:25:28 +0200
From: Moshe Shemesh <moshe@...dia.com>
To: Paolo Abeni <pabeni@...hat.com>, Tariq Toukan <tariqt@...dia.com>, "Eric
 Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Andrew Lunn
	<andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>
CC: Saeed Mahameed <saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>,
	Mark Bloch <mbloch@...dia.com>, <netdev@...r.kernel.org>,
	<linux-rdma@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Gal Pressman
	<gal@...dia.com>, Or Har-Toov <ohartoov@...dia.com>, Jiri Pirko
	<jiri@...dia.com>, Parav Pandit <parav@...dia.com>
Subject: Re: [PATCH net-next] net/mlx5: Support devlink port state for host PF



On 2/5/2026 5:57 PM, Paolo Abeni wrote:
> 
> On 2/3/26 11:24 AM, Tariq Toukan wrote:
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> index 4b7a1ce7f406..5fbfabe28bdb 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> @@ -1304,24 +1304,52 @@ static int mlx5_eswitch_load_ec_vf_vports(struct mlx5_eswitch *esw, u16 num_ec_v
>>        return err;
>>   }
>>
>> -static int host_pf_enable_hca(struct mlx5_core_dev *dev)
>> +int mlx5_esw_host_pf_enable_hca(struct mlx5_core_dev *dev)
>>   {
>> -     if (!mlx5_core_is_ecpf(dev))
>> +     struct mlx5_eswitch *esw = dev->priv.eswitch;
>> +     struct mlx5_vport *vport;
>> +     int err;
>> +
>> +     if (!mlx5_core_is_ecpf(dev) || !mlx5_esw_allowed(esw))
>>                return 0;
> 
> I was able to miss the AI feedback here:
> 
> ---
> The old host_pf_enable_hca() only checked mlx5_core_is_ecpf(dev) before
> calling mlx5_cmd_host_pf_enable_hca(). The new function adds a check for
> mlx5_esw_allowed(esw), which returns false when esw is NULL or when the
> device is not an eswitch manager.
> 
> When called from mlx5_host_pf_init() in ecpf.c on an ECPF device that is
> not an eswitch manager (the path when mlx5_ecpf_esw_admins_host_pf()
> returns false), this new condition will cause the function to return 0
> without enabling the HCA.

The additional check I added here is actually redundant either if we get 
from old code path or from the new caller, both will call it as eswitch 
manager only.
So there is no concern on the old code behavior change, but I can follow 
up with a patch to remove the redundant check, though not critical.

> 
> Is this behavior change intentional? The old code would enable the HCA
> in this configuration, but the new code skips it.
> 
> The same concern applies to mlx5_esw_host_pf_disable_hca() below.
> ---
> 
> and indeed it looks relevant. I think you have to follow-up or send a
> revert, whatever it's easier/faster.
> 
>> @@ -1347,7 +1375,7 @@ mlx5_eswitch_enable_pf_vf_vports(struct mlx5_eswitch *esw,
>>
>>        if (mlx5_esw_host_functions_enabled(esw->dev)) {
>>                /* Enable external host PF HCA */
>> -             ret = host_pf_enable_hca(esw->dev);
>> +             ret = mlx5_esw_host_pf_enable_hca(esw->dev);
> 
> Just FTR, more AI feedback here:
> 
> ---
> The old host_pf_disable_hca() was a void function. The new
> mlx5_esw_host_pf_disable_hca() returns int and can fail, but the return
> value is not checked here in the error path.
> 
> If mlx5_esw_host_pf_disable_hca() fails, it returns without setting
> vport->pf_activated = false. This leaves pf_activated set to true even
> though the HCA state may be inconsistent.
> 
> Later, mlx5_devlink_pf_port_fn_state_get() reads vport->pf_activated to
> report state to userspace, which could then report incorrect state.
> 
> Should the return value be checked, or should the pf_activated flag be
> updated unconditionally to reflect the intended state?

When disable function is part of the unload/teardown/error flows we 
don't check result. The function is not void anymore only for the new 
use case that it is called from devlink and return value is checked there.
Thanks, Moshe.

> 
> The same pattern appears in mlx5_eswitch_disable_pf_vf_vports().
> ---
>