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: > > On Mon, Oct 04, 2021 at 04:44:13PM -0700, Jakub Kicinski wrote: > > > On Sun, 3 Oct 2021 21:12:04 +0300 Leon Romanovsky wrote: > > > > From: Leon Romanovsky <leonro@xxxxxxxxxx> > > > > > > > > Introduce new devlink call to set specific ops callback during > > > > device initialization phase after devlink_alloc() is already > > > > called. > > > > > > > > This allows us to set specific ops based on device property which > > > > is not known at the beginning of driver initialization. > > > > > > > > For the sake of simplicity, this API lacks any type of locking and > > > > needs to be called before devlink_register() to make sure that no > > > > parallel access to the ops is possible at this stage. > > > > > > The fact that it's not registered does not mean that the callbacks > > > won't be invoked. Look at uses of devlink_compat_flash_update(). > > > > 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. > > > 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(). > > > > > diff --git a/net/core/devlink.c b/net/core/devlink.c > > > > index 4e484afeadea..25c2aa2b35cd 100644 > > > > --- a/net/core/devlink.c > > > > +++ b/net/core/devlink.c > > > > @@ -53,7 +53,7 @@ struct devlink { > > > > struct list_head trap_list; > > > > struct list_head trap_group_list; > > > > struct list_head trap_policer_list; > > > > - const struct devlink_ops *ops; > > > > + struct devlink_ops ops; > > > > > > Security people like ops to live in read-only memory. You're making > > > them r/w for every devlink instance now. > > > > Yes, but we are explicitly copy every function pointer, which is safe. > > The goal is for ops to live in pages which are mapped read-only, > so that heap overflows can overwrite the pointers. <...> > 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. Thanks
