Message-ID: <8f8f131d-437c-6bf8-9e44-1c3a6c01e68f@ti.com>
Date:   Fri, 10 Mar 2023 10:54:57 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v3] dma-buf: cma_heap: Check for device max segment size
 when attaching
Content-Language: en-US
To:     John Stultz <jstultz@google.com>
CC:     Sumit Semwal <sumit.semwal@linaro.org>,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        Liam Mark <lmark@codeaurora.org>,
        Brian Starkey <Brian.Starkey@arm.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        <dri-devel@lists.freedesktop.org>,
        <linaro-mm-sig@lists.linaro.org>, <linux-kernel@vger.kernel.org>
References: <20230306165143.1671-1-afd@ti.com>
 <CANDhNCrzi+9BszTj+9v9t6cHOcxv=4C1yQZnZZ-_7FWoxwAd5A@mail.gmail.com>
From:   Andrew Davis <afd@ti.com>
In-Reply-To: <CANDhNCrzi+9BszTj+9v9t6cHOcxv=4C1yQZnZZ-_7FWoxwAd5A@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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