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

On Tue, Oct 26, 2021 at 10:34:39AM -0700, Edwin Peer wrote:

On Mon, Oct 25, 2021 at 10:56 PM Leon Romanovsky [off-list ref] wrote:

quoted

quoted

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.

quoted

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