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

On Mon, Oct 25, 2021 at 04:19:07PM -0700, Edwin Peer wrote:

On Sun, Oct 24, 2021 at 3:35 PM Ido Schimmel [off-list ref] wrote:

quoted

On Sun, Oct 24, 2021 at 11:42:11AM +0300, Leon Romanovsky wrote:

quoted

From: Leon Romanovsky <leonro@nvidia.com>

Align netdevsim to be like all other physical devices that register and
unregister devlink traps during their probe and removal respectively.

No, this is incorrect. Out of the three drivers that support both reload
and traps, both netdevsim and mlxsw unregister the traps during reload.
Here is another report from syzkaller about mlxsw [1].

Please revert both 22849b5ea595 ("devlink: Remove not-executed trap
policer notifications") and 8bbeed485823 ("devlink: Remove not-executed
trap group notifications").

Could we also revert 82465bec3e97 ("devlink: Delete reload
enable/disable interface")? 

Absolutely not.

This interface is needed because bnxt_en cannot reorder devlink last.
If Leon had fully carried out the re-ordering in our driver he would
have introduced a udev phys_port_name regression because of:

cda2cab0771 ("bnxt_en: Move devlink_register before registering netdev")

and:

ab178b058c4 ("bnxt: remove ndo_get_phys_port_name implementation")

devlink_register() doesn't do anything except performing as a barrier.

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().

I think this went unnoticed for bnxt_en, because Michael had not yet
posted our devlink reload patches, which presently rely on the reload
enable/disable API. Absent horrible kludges in reload down/up which
currently depends on the netdev, there doesn't appear to be a clean
way to resolve the circular dependency without the interlocks this API
provides.

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.

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.

Thanks

Regards,
Edwin Peer