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

[Shay Drori <shayd@nvidia.com>]

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...