Re: [PATCH 23/26] treewide: Rename dma_buf_attach() to dma_buf_sgt_attach()

["Christian König" <christian.koenig@amd.com>]

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 <jgg@nvidia.com>
>>>>>>>
>>>>>>> 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