Re: [PATCH net] net/mlx5: Fix double unregister of HCA_PORTS component

[Gerd Bayer]

On Thu, 2025-12-04 at 19:07 +0200, Moshe Shemesh wrote:
> 
> On 12/4/2025 11:48 AM, Gerd Bayer wrote:
> > 
> > On Wed, 2025-12-03 at 17:14 +0200, Moshe Shemesh wrote:
> > > 
> > > On 12/2/2025 1:12 PM, Gerd Bayer wrote:
> > > > 
> > 
> >    [ ... snip ... ]
> > 
> > > > 
> > > > Fixes: 5a977b5833b7 ("net/mlx5: Lag, move devcom registration to LAG layer")
> > > > Signed-off-by: Gerd Bayer <gbayer@xxxxxxxxxxxxx>
> > > 
> > > Reviewed-by: Moshe Shemesh <moshe@xxxxxxxxxx>> ---
> > > > Hi Shay et al,
> > > > 
> > > 
> > > Hi Gerd,
> > >    I stepped on this bug recently too, without s390 and was about to
> > > submit same fix :) So as you wrote it is unrelated to Lukas' patches and
> > > this fix is correct.
> > 
> > Good to hear. I wonder if you could share how you got to run into this?
> > 
> 
> mlx5_unload_one() can be called from few flows.
> Even that it is always called with devlink lock, serial of 
> mlx5_unload_one() twice caused it. I got it on fw_reset and shutdown. I 
> I will submit also a patch for calling mlx5_drain_fw_reset() on shutdown 
> soon.

I agree, serialization through the devlink lock does not help if
mlx5_unload_one() does not clean up all the references.

> 
> > > 
> > > > 
> > > > I've spotted two additional places where the devcom reference is not
> > > > cleared after calling mlx5_devcom_unregister_component() in
> > > > drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c that I have not
> > > > addressed with a patch, since I'm unclear about how to test these
> > > > paths.
> > > 
> > > As for the other cases, we had the patch 664f76be38a1 ("net/mlx5: Fix
> > > IPsec cleanup over MPV device") and two other cases on shared clock and
> > > SD but I don't see any flow the shared clock or SD can fail,
> > > specifically mlx5_sd_cleanup() checks sd pointer at beginning of the
> > > function and nullify it right after sd_unregister() that free devcom.
> > 
> > I didn't locate any calls to mxl5_devcom_unregister_component() in
> > "shared clock" - is that not yet upstream?
> 
> mlx5_shared_clock_unregister() in 
> drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c

Hah - my fault! I was searching through the indexer's parameterized
cross-references, and w/o CONFIG_PTP_1588_CLOCK that file was excluded.

> 
> > 
> > Regarding SD, I follow that sd_cleanup() is followed immediately after
> > sd_unregister() and does the clean-up. One path remains uncovered
> > though: The error exit at
> > https://elixir.bootlin.com/linux/v6.18/source/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c#L265
> > 
> > Not sure, how likely that is...
> 
> It comes on error flow but after successful 
> mlx5_devcom_register_component() in sd_register(), and that error leads 
> to error flow in mlx5_sd_init(), which calls sd_cleanup() too.
> 
> > 
> > Thanks,
> > Gerd

Thanks for you explanations,
Gerd