Messages in this thread | | | From | Edwin Peer <> | Date | Tue, 26 Oct 2021 13:03:41 -0700 | Subject | Re: [PATCH net-next] netdevsim: Register and unregister devlink traps on probe/remove device |
| |
On Tue, Oct 26, 2021 at 12:22 PM Leon Romanovsky <leon@kernel.org> wrote:
> At least in mlx5 case, reload_enable() was before register_netdev(). > It stayed like this after swapping it with devlink_register().
What am I missing here?
err = mlx5_init_one(dev); if (err) { mlx5_core_err(dev, "mlx5_init_one failed with error code %d\n", err); goto err_init_one; }
err = mlx5_crdump_enable(dev); if (err) dev_err(&pdev->dev, "mlx5_crdump_enable failed with error code %d\n", err);
pci_save_state(pdev); devlink_register(devlink);
Doesn't mlx5_init_one() ultimately result in the netdev being presented to user space, even if it is via aux bus?
> 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.
Right, that's the problem. Once we register the netdev, we're in a race with user space, which may expect to be able to call devlink before we get to devlink_register().
> 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.
Right, our timing is unfortunate and that's on us. It's still not clear to me how to actually fix the devlink reload code without the benefit of something similar to the reload enable API.
> 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.
This isn't about kernel API. This is precisely about existing user space that expects devlink to work immediately after the netdev appears.
> It looks like your failure is in backport code.
Our out-of-tree driver isn't the issue here. I'm talking about the proposed upstream code. The issue is what to do in order to get something workable upstream for devlink reload. We can't move devlink_register() later, that will cause a regression. What do you suggest instead?
Regards, Edwin Peer
| |