Re: [PATCH net 1/2] net/mlx5e: psp: Fix invalid access on PSP dev registration fail

[Cosmin Ratiu <cratiu@nvidia.com>]

On Tue, 2026-04-21 at 08:09 -0700, Jakub Kicinski wrote:
> On Tue, 21 Apr 2026 14:33:51 +0000 Cosmin Ratiu wrote:
> > > > priv->psp and steering at the time of mlx5e_psp_register() is
> > > > inert
> > > > without the PSP device. Cleaning it on psp_dev_create() failure
> > > > would
> > > > be weird, it's cleaned up anyway on netdev teardown. The fact
> > > > that
> > > > only
> > > > memory allocations can fail inside psp_dev_create() is
> > > > irrelevant
> > > > here.
> > > > psp_dev_create() failing shouldn't bring down the whole
> > > > netdevice,
> > > > so
> > > > logging a message and continuing is ok (which is what is also
> > > > done
> > > > for
> > > > macsec and ktls).  
> > > 
> > > This is a misguided cargo cult. Or something motivated by OOT
> > > compatibility. Alex D sometimes tries to do the same thing with
> > > Meta
> > > drivers. I don't get it. Of course we want the device to be
> > > operational
> > > if some *device* init fails. The compatibility matrix with all
> > > device
> > > generations and fw versions could justify that. But continuing
> > > init
> > > when a single-page kmalloc failed is pure silliness.  
> > 
> > I am not sure about the wider context, but from the POV of the
> > driver,
> > it's calling $thing from the kernel which can fail and it needs to
> > do
> > something about it, either fail the entire netdev bringup or accept
> > that $thing won't be functional and continue without it. The driver
> > shouldn't need to know what $thing does inside and how it can fail,
> > which can change over time. Today it's a kmalloc(), tomorrow it may
> > be
> > something else.
> 
> Like what?

The inner workings of $thing aren't and shouldn't be relevant, no?
Maybe tomorrow the kernel will lazy-init some TCP shenanigans for the
first PSP device being initialized or whatever, or maybe some other
moving parts inside can fail. It's an abstraction, why make it
unnecessarily leaky for the purpose of writing driver code?

> 
> > It doesn't and shouldn't matter for the local decision
> > to continue or not without $thing working.
> > 
> > Isn't this reasonable?
> 
> No, the normal thing to do is to propagate errors.
> If you want to diverge from that _you_ should have a reason,
> a better reason than a vague "kernel can fail".
> I'd prefer for the driver to fail in an obvious way.
> Which will be immediately spotted by the operator, not 2 weeks
> later when 10% of the fleet is upgraded already.
> The only exception I'd make is to keep devlink registered in
> case the fix is to flash a different FW.

In this case, PSP not working would be spotted on the next PSP dev-get
op which produces zilch instead of working devices.

But I understand what you want. You'd like the netdevice to either be
fully initialized with all supported+configured protocols or fail the
open operation. No intermediate/partial states. This is a non-trivial
refactor for mlx5, because mlx5_nic_enable() returns nothing.
Refactoring seems possible though, its only caller is
mlx5e_attach_netdev(), which returns errors. It's certainly not
something that should be done for a net fix though.

I have a series pending for net-next where the PSP configuration is
hooked to mlx5e_psp_set_config(). I will look into implementing what
you propose there and propagate errors.

Meanwhile, do you want to take these fixes (1 and 2) or maybe just 2
for net or not?

Cosmin.