On Fri, May 03, 2019 at 02:05:47PM +0200, Christian König wrote: > Am 30.04.19 um 19:31 schrieb Russell King - ARM Linux admin: > > On Tue, Apr 30, 2019 at 01:10:02PM +0200, Christian König wrote: > > > Add a structure for the parameters of dma_buf_attach, this makes it much easier > > > to add new parameters later on. > > I don't understand this reasoning. What are the "new parameters" that > > are being proposed, and why do we need to put them into memory to pass > > them across this interface? > > > > If the intention is to make it easier to change the interface, passing > > parameters in this manner mean that it's easy for the interface to > > change and drivers not to notice the changes, since the compiler will > > not warn (unless some member of the structure that the driver is using > > gets removed, in which case it will error.) > > > > Additions to the structure will go unnoticed by drivers - what if the > > caller is expecting some different kind of behaviour, and the driver > > ignores that new addition? > > Well, exactly that's the intention here: That the drivers using this > interface should be able to ignore the new additions for now as long as they > are not going to use them. > > The background is that we have multiple interface changes in the pipeline, > and each step requires new optional parameters. > > > This doesn't seem to me like a good idea. > > Well, the obvious alternatives are: > > a) Change all drivers to explicitly provide NULL/0 for the new parameters. > > b) Use a wrapper, so that the function signature of dma_buf_attach stays the > same. > > Key point here is that I have an invalidation callback change, a P2P patch > set and some locking changes which all require adding new parameters or > flags. And at each step I would then start to change all drivers, adding > some more NULL pointers or flags with 0 default value. > > I'm actually perfectly fine going down any route, but this just seemed to me > simplest and with the least risk of breaking anything. Opinions? I think given all our discussions and plans the argument object makes tons of sense. Much easier to document well than a long list of parameters. Maybe we should make it const, so it could work like an ops/func table and we could store it as a pointer in the dma_buf_attachment? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
