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

[Edwin Peer <edwin.peer@broadcom.com>]

On Mon, Oct 25, 2021 at 10:56 PM Leon Romanovsky <leon@kernel.org> 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.

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.

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.

> 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 was supposed to update and retest your out-of-tree implementation
> of devlink reload before posting it to the ML. However, if you use
> devlink_*() API correctly, such dependency won't exist.

We dropped the ball there, absolutely, but the out-of-tree
implementation is only relevant to the extent that it is a testing
vehicle for our QA _before_ we post patches upstream. This API change
invalidates all of that testing work, which is regrettable, but not
the end of the world if your change is the right way to go (I don't
think it is).

I'm interested in the form that the _upstream_ implementation of our
devlink reload would need to take. I don't think we can move
devlink_register() later without introducing whole classes of user
space regressions. Thus, fixing our proposed devlink reload code in
the absence of devlink reload enable would entail implementing
precisely the kind of interlock that was previously provided by the
removed API, only internally. It doesn't seem right to remove a useful
shared API in favor of duplicative implementations in drivers to
accomplish the same goal.

> > I imagine other subtle regressions are lying in wait.
>
> Sorry, but we don't have crystal ball and can't guess what else is
> broken in your out-of-tree driver.

In addition to the udev phys port name regressions that we already
know about, it doesn't take a crystal ball to imagine user ifup
scripts that call into devlink. All sorts of things in user space kick
off in response to the presentation of the netdev. User enables
switchdev mode in ifup, that will break. User twiddles a devlink param
in ifup, that'll break too.

Regards,
Edwin Peer