Re: [PATCH v3] dma-buf: cma_heap: Check for device max segment size when attaching

[Andrew Davis <afd@ti.com>]

On 3/6/23 8:48 PM, John Stultz wrote:
> On Mon, Mar 6, 2023 at 8:52 AM Andrew Davis <afd@ti.com> wrote:
>>
>> Although there is usually not such a limitation (and when there is it is
>> often only because the driver forgot to change the super small default),
>> it is still correct here to break scatterlist element into chunks of
>> dma_max_mapping_size().
> 
> Hey Andrew!
>    Thanks for sending this out!
> 
> So *why* is it "correct here to break scatterlist element into chunks
> of  dma_max_mapping_size()." ?
> 

Good question, I'm not 100% sure on the background myself. It seems
since some devices have restrictions on how large a mapping they can
handle in a single run, we should not hand out single scatterlist
elements longer than that.

It is still a contiguous buffer, but some drivers forget to set their
mapping limits and also only check the number of elements == 1 to determine
if a sg is contiguous (which is not correct as there is no rule that
contiguous runs must be merged into a single scatterlist). For those
driver this would be an issue (I've only found one such case in-tree and
sent a fix, https://lore.kernel.org/lkml/20220825162609.14076-1-afd@ti.com/)

>> This might cause some issues for users with misbehaving drivers. If
>> bisecting has landed you on this commit, make sure your drivers both set
>> dma_set_max_seg_size() and are checking for contiguousness correctly.
> 
> Why is this change worth the risk? (If this is really likely to break
> folks, should we maybe provide warnings initially instead? Maybe
> falling back to the old code if we can catch the failure?)
> 
> I don't really object to the change, just want to make sure the commit
> message is more clear on why we should make this change, what the
> benefit will be along with the potential downsides.
> 

I'm not sure it is worth the risk today either, but figured this being a
young enough exporter it could be a good spot to start with for exposing
misbehaving drivers vs some legacy GPU driver exporter. Plus better to
make this change now rather than later in any case.

I don't have any strong reason for this yet though, so I'm find with
just considering this patch an RFC for now.

Thanks,
Andrew

> thanks
> -john