Re: [PATCH] mlxsw: spectrum_ptp: release parsing depth on MTPPPC write failure

[Petr Machata <petrm@nvidia.com>]

Simon Horman <horms@kernel.org> writes:

> On Sat, Nov 07, 2026 at 12:07:17PM +0000, netdev-bot+sashiko@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential
>> issue(s) to consider.
>> 
>> Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0 (1)
>> 
>> If you believe none of the issues are legitimate, please reply to each
>> issue of Medium severity or higher and explain why the AI is wrong.
>> If any of them are legitimate and you plan to respin, please let patchwork
>> know by including "pw-bot: cr" as a separate line at the end of your reply
>> (one such reply per series is enough).
>> 
>> - [High] The patch fixes the enable-path leak but adds the same leak on
>>   the port-removal path.
>
> I believe the issue flagged here is pre-existing. While it does affect this
> patch, the patch is still an improvement in its own right. I would
> therefore ask for follow-up on the concurrency concern raised here, but I
> do not think it should block progress of this patch.
>
> And while the separate bot email, requesting information on how
> the problem was found, should be answered, overall this looks good to me.
>
> Reviewed-by: Simon Horman <horms@kernel.org>

(Sorry I didn't respond earlier. I saw Krzysztof's emails and assumed
the thread would get bounced. This morning I saw it's still in NEW,
decided to take a closer look, but got sidetracked by some other work.)

I agree. mlxsw_sp_parsing_depth_dec() looks like it can't fail, but
that's not the case, it is fallible, it is just void, because it's
dtor-like.

I've been staring at this a bit and it probably can't stay like this,
mlxsw_sp_parsing_depth_dec() needs to propagate the failure for
reconfigure-like paths. It is IMHO legit to respond to a user request to
stop timestamping with "tried it, didn't work, sorry, we keep on
timestamping". It's only during port split where we have no other option
and if it fails, well that's too bad, stiff upper lip & carry on.

But the code just masks the existing failure with a void return type.
For port removal in particular that means that OK, you manage to disable
timstamping on that port, but if prior to that you failed to dec the
parsing depth, there's a leak anyway.

Contrary to that, devlink reload, which sashiko mentions too, is less of
an issue, because those paths go through mlxsw_sp_fini() and
mlxsw_sp1_init(). You get a warning splat on fini, but then init sets
the refcount to 0 and you start with a clean slate. The splat is not
nice, but there's no leak.

Anyway, I'll be reworking this a bit, but for now:

Reviewed-by: Petr Machata <petrm@nvidia.com>