Re: [PATCH net-next v2 3/5] devlink: Allow set specific ops callbacks dynamically

[Jakub Kicinski]

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?

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

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

> > >  	struct xarray snapshot_ids;
> > >  	struct devlink_dev_stats stats;
> > >  	struct device *dev;  

> > > +EXPORT_SYMBOL_GPL(devlink_set_ops);  
> > 
> > I still don't like this. IMO using feature bits to dynamically mask-off
> > capabilities has much better properties. We already have static caps
> > in devlink_ops (first 3 members), we should build on top of that.   
> 
> These capabilities are for specific operation, like flash or reload.
> They control how these flows will work, they don't control if this flow
> is valid or not.
> 
> You are too focused on reload caps, but mutliport mlx5 device doesn't
> support eswitch too. I just didn't remove the eswitch callbacks to
> stay focused on more important work - making devlink better. :)
> 
> Even if we decide to use new flag in devlink_ops, we will still need to
> add this devlink_set_ops() patch, because the value of that new flag
> will be known very late in initialization phase, after FW capabilities
> are known and I will need to overwrite RO memory.

Yes, you can change the caps at run time, that's perfectly reasonable.
You'll also be able to define more fine grained caps going forward as
needed.

> Jakub,
> 
> Can we please continue with the current approach? It doesn't expose any
> user visible API and everything here will be easy rewrite differently
> if such needs arise.
> 
> We have so much ahead, like removing devlink_lock, rewriting devlink->lock,
> fixing devlink reload of IB part, e.t.c

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.