Re: [PATCH net-next] netdevsim: Register and unregister devlink traps on probe/remove device

[Leon Romanovsky]

On Tue, Oct 26, 2021 at 10:34:39AM -0700, Edwin Peer wrote:
> On Mon, Oct 25, 2021 at 10:56 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
> 
> > > Could we also revert 82465bec3e97 ("devlink: Delete reload
> > > enable/disable interface")?
> >
> > Absolutely not.
> 
> Although the following patch doesn't affect bnxt_en directly, I
> believe the change that will ultimately cause regressions are the
> patches of the form:
> 
> 64ea2d0e7263 ("net/mlx5: Accept devlink user input after driver
> initialization complete")
> 
> Removing the reload enable interface is merely the reason you're
> moving devlink_register() later here, but it's the swapping of the
> relative order of devlinqk_register() and register_netdev() which is
> the problem.

At least in mlx5 case, reload_enable() was before register_netdev().
It stayed like this after swapping it with devlink_register().

> 
> Our proposed devlink reload depends on the netdev being registered.
> This was previously gated by reload enable, but that is a secondary
> issue. The real question is whether you now require devlink_register()
> to go last in general? If so, that's a problem because we'll race with
> user space. User visible regressions will definitely follow.

No, it is not requirement, but my suggestion. You need to be aware that
after call to devlink_register(), the device will be fully open for devlink
netlink access. So it is strongly advised to put devlink_register to be the
last command in PCI initialization sequence.

It is exactly like it was before, but instead of one _reload_ interface
which had extra enable/disable logic, we are protecting all other set/get
commands. Before the change, you was able to send any devlink netlink commands
and crash unprotected driver.

> 
> The bnxt_en driver was only saved from such regressions because you
> did not carry out the same change there as you've done here.
> Otherwise, you would have broken bnxt_en as a consequence. I'm
> obviously not as familiar with mlx5, but I think you may have already
> broken it. I imagine the only reason customers haven't complained
> about this change yet is that few, if any, are running the net-next
> code.

This is not how mlx5 is implemented at all. We use auxiliary bus to
separate PCI core driver and eth netdev driver.

And yes, we have customers who rely on upstream code and test it
methodically.

> 
> > In a nutshell, latest devlink_register() implementation is better
> > implementation of previously existed "reload enable/disable" boolean.
> >
> > You don't need to reorder whole devlink logic, just put a call to
> > devlink_register() in the place where you wanted to put your
> > devlink_reload_enable().
> 
> We can't though, because of the two patches I pointed out previously.
> Moving devlink_register() to the existing devlink_reload_enable()
> location puts it after register_netdev(). That will cause a regression
> with udev and phys port name. We already have the failing test case
> and customer bug report for this. That is why devlink_register() was
> moved earlier in bnxt_en. We can't now do the opposite and move it
> later.

You obviously need to fix your code. Upstream version of bnxt driver
doesn't have reload_* support, so all this regression blaming it not
relevant here.

In upstream code, devlink_register() doesn't accept ops like it was
before and position of that call does only one thing - opens devlink
netlink access. All kernel devlink APIs continue to be accessible even
before devlink_register.

It looks like your failure is in backport code.

Thanks