On 10/27/26 15:44, Jason Gunthorpe wrote: > On Tue, Oct 27, 2026 at 03:28:44PM +0100, Christian König wrote: >> On 10/27/26 15:08, Jason Gunthorpe wrote: >>> On Tue, Oct 27, 2026 at 02:17:16PM +0100, Christian König wrote: >>>> On 10/27/26 13:38, Jason Gunthorpe wrote: >>>>> On Tue, Oct 27, 2026 at 01:07:36PM +0100, Christian König wrote: >>>>>> On 10/26/26 06:36, Alistair Popple wrote: >>>>>>> From: Jason Gunthorpe <[email protected]> >>>>>>> >>>>>>> This attach function always creates a SGT mapping type importer. >>>>>> >>>>>> Yeah that is a clear NO-GO. The attachment should be the same for >>>>>> the new interface as well. >>>>> >>>>> Not sure I understand this comment... >>>>> >>>>> This was intended as a helper so that existing drivers can run through >>>>> the sgt based mapping type flow without having to open code it. It is >>>>> hardwired to only do sgt like this. If you want to use more than sgt >>>>> as your mapping type don't use this function >>>>> >>>>> The point was to create a clear distinction in the API names when >>>>> things were in a SGT-only flow vs using the newer negotiable flow. >>>> >>>> Well that is my point, the dma_buf_attach() or rather >>>> dynamic_attach() function should still be part of the new flow. >>> >>> It is, this is just a helper wrapper effectively to run the flow >>> exactly as it is right now wired to only do sgt. >>> >>> Unless you want to see that open coded in ever driver using the more >>> general functions, I'm not sure I see the alternative? >> >> I still don't see what you mean. > > The function is this: > > struct dma_buf_attachment *dma_buf_sgt_attach(struct dma_buf *dmabuf, > struct device *dev) > { > struct dma_buf_mapping_match sgt_match[] = { > DMA_BUF_IMAPPING_SGT(dev, DMA_SGT_NO_P2P), > }; > > return dma_buf_mapping_attach(dmabuf, sgt_match, ARRAY_SIZE(sgt_match), > NULL, NULL);
Yeah. exactly that is a clear NO-GO from my side. The existing dma_buf_dynamic_attach() function should already have everything you need to negotiate the mapping type. Don't add any new dma_buf_mapping_attach() function since attachments are generally not mapping related. Regards, Christian. > } > EXPORT_SYMBOL_NS_GPL(dma_buf_sgt_attach, "DMA_BUF"); > > Which is just a wrapper around the more general attach function, > forcing it to do SGT. > > I don't see why we should label a function that can ony do SGT anything > other than under the SGT names. > > If you want to get rid of it then the above has to get dropped into > every caller, or something like... > >> If a driver can only deal with sgt then it won't implement the >> dma_buf_attach_ops->supported_mapping_types (or whatever we name >> that). >> >> So if either dma_buf_attach_ops is NULL or >> dma_buf_attach_ops->supported_mapping_types is NULL then we only >> match to the sgt export. > > This is basically changing how dma_buf_mapping_attach() works to hard > wire in SGT behaviors. I think this is something you'd regret down the > road as it starts muddling up different concerns.. > > dma_buf_sgt_attach() is very readable for what the driver is doing, > and grepable. Implicitly doing things when other things are NULL is > more indirect. > >> What I want to avoid is to get just another dma_buf_attach() variant >> since we already added a parameter structure here. > > It is a pretty standard kernel style to do things like this rather > than expose super APIs with complex signatures to every caller. If > there are many places with the same operation it does tend to help > alot to mark them with a helper function like this. > > Jason
