Re: [PATCH net V4 3/4] net/mlx5e: SD, Fix missing cleanup on probe error

Shay Drori
2026-04-30T12:53:10Z

On 30/04/2026 4:42, Jakub Kicinski wrote:
> External email: Use caution opening links or attachments
>
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net/mlx5e: SD, Fix missing cleanup on probe error
>
> This commit adds a missing mlx5_sd_cleanup() call to the probe error path.
> The commit message notes a similar gap exists on the resume path leaving
> sd == NULL on failure, but defers the fix to a follow-up series.
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index 5a46870c4b74..e21affd0ffc4 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> [ ... ]
>> @@ -6912,9 +6912,16 @@ static int mlx5e_probe(struct auxiliary_device *adev,
>> return err;
>>
>> actual_adev = mlx5_sd_get_adev(mdev, adev, edev->idx);
>> - if (actual_adev)
>> - return _mlx5e_probe(actual_adev);
>> + if (actual_adev) {
>> + err = _mlx5e_probe(actual_adev);
>> + if (err)
>> + goto sd_cleanup;
>> + }
>> return 0;
>> +
>> +sd_cleanup:
>> + mlx5_sd_cleanup(mdev);
>> + return err;
>> }
>>
>> static void _mlx5e_remove(struct auxiliary_device *adev)
>
> Will leaving the resume gap unfixed as mentioned in the commit message cause
> a NULL pointer dereference when a secondary device is later removed?
>
> If a secondary device fails resume, sd becomes NULL. Later during removal,
> mlx5_sd_get_adev() will check !sd and return the secondary adev.
>
> This causes _mlx5e_remove() to be executed on the secondary adev:
>
> _mlx5e_remove()
> {
> struct mlx5_adev *edev = container_of(adev, struct mlx5_adev, adev);
> struct mlx5e_dev *mlx5e_dev = auxiliary_get_drvdata(adev);
> struct net_device *netdev = mlx5e_dev->netdev;
> ...
> }
>
> Since _mlx5e_probe() is never called on secondary devices,
> auxiliary_get_drvdata(adev) returns NULL. Does this mean accessing
> mlx5e_dev->netdev will crash?
>
> Would it be safer to add a NULL check for mlx5e_dev in _mlx5e_remove()
> until the proper PM lifecycle fixes are implemented?

IMO, a proper fix is needed here. PM lifecycle is implemented, the bug
is in case of error flow...