On Tue, Oct 05, 2021 at 05:39:40PM -0700, Jakub Kicinski wrote: > On Tue, 5 Oct 2021 22:15:40 +0300 Leon Romanovsky wrote: > > On Tue, Oct 05, 2021 at 11:32:13AM -0700, Jakub Kicinski wrote: > > > On Tue, 5 Oct 2021 10:32:45 +0300 Leon Romanovsky wrote: > > > > It is impossible, devlink_register() is part of .probe() flow and if it > > > > wasn't called -> probe didn't success -> net_device doesn't exist. > > > > > > Are you talking about reality or the bright future brought by auxbus? > > > > I looked on all the drivers which called to devlink_alloc() which is > > starting point before devlink_register(). All of them used it in the > > probe. My annotation patch checks that too. > > > > https://lore.kernel.org/linux-rdma/f65772d429d2c259bbc18cf5b1bbe61e39eb7081.1633284302.git.leonro@xxxxxxxxxx/T/#u > > > > So IMHO, it is reality. > > You say that yet below you admit flashing is broken :/ I said more than once, lifetime of devlink is broken. It is placed in wrong layer, pretend to implement some of driver core functionality without proper protections and have wrong locks. At least, I didn't break flash update, there is no change in logic of flash after any of my changes. Exactly like before, user was able to trigger flash update through this devlink_compat_flash_update() call without any protection. Let's chose random kernel version (v5.11) https://elixir.bootlin.com/linux/v5.11/source/net/core/devlink.c#L10245 as you can see, it doesn't hold ANY driver core locks, so it can be called in any time during driver .probe() or .remove(). Drivers that have implemented ops.flash_update() have no idea about that. > > > > > We are not having net_device without "connected" device beneath, aren't we? > > > > > > > > At least drivers that I checked are not prepared at all to handle call > > > > to devlink->ops.flash_update() if they didn't probe successfully. > > > > > > Last time I checked you moved the devlink_register() at the end of > > > probe which for all no-auxbus drivers means after register_netdev(). > > > > I need to add a check of if(devlink_register) inside devlink_compat_flash_update(). > > ... and the workarounds start to pile up. It is not a workaround, but attempt to fix this mess. I separated devlink netlink callers from the kernel flow and just need to continue to fix these external to devlink callers. > > > > I don't like it. If you're feeling strongly please gather support of > > > other developers. Right now it's my preference against yours. I don't > > > even see you making arguments that your approach is better, just that > > > mine is not perfect and requires some similar changes. > > > > I have an idea of how to keep static ops and allow devlink_set_ops() > > like functionality. > > > > What about if I group ops by some sort of commonalities? > > > > In my case, it will be devlink_reload_ops, which will include reload > > relevant callbacks and provide devlink_set_reload_ops() wrapper to set > > them? > > > > It will ensure that all pointers are const without need to have feature > > bits. > > I don't understand why you keep pushing the op reassignment. It is not reassignment, but ability to assign proper callbacks from the beginning. The idea is to make sure that lifetime of devlink is managed by proper ops callbacks, based on them we can control everything inside devlink by ensuring right locks, order e.t.c. I want to get rid from random *_enabled flags that always forgotten and adds complexity that don't give any advantage only issues. I'm also changing devlink to allow parallel execution and for that I need to have reliable devlink instance with minimal number of locks. Thanks
