On 12/11/2024 9:44 PM, Robin Murphy wrote:
> On 2024-12-11 2:31 am, Baochen Qiang wrote:
>>
>>
>> On 12/11/2024 7:06 AM, Tim Harvey wrote:
>>> On Mon, Dec 9, 2024 at 8:11 PM Christoph Hellwig <hch@xxxxxx> wrote:
>>>>
>>>> On Mon, Dec 09, 2024 at 11:15:02AM -0800, Tim Harvey wrote:
>>>>> After a lot of back and forth and investigation this is due to the
>>>>> IMX8M SoC's not having an IOMMU thus swiotlb is being used and ath11k
>>>>> is requesting some buffers that are too large for swiotlb to provide.
>>>>> There is a specific patch which added the HAL_WBM2SW_RELEASE buffers
>>>>> to cacheable memory that could be reverted to fix this but the concern
>>>>> was that it would impact performance moving those buffers to
>>>>> non-cacheable memory (there are three ~1MiB buffers being allocated):
>>>>> commit d0e2523bfa9cb ("ath11k: allocate HAL_WBM2SW_RELEASE ring from
>>>>> cacheable memory").
>>>>
>>>> The combination of "buffers" and "swiotlb" sounds like Robin was right
>>>> below.
>>>>
>>>>> The chain of events as best I can tell are:
>>>>>
>>>>> commit 6452f0a3d565 ("ath11k: allocate dst ring descriptors from
>>>>> cacheable memory")
>>>>> - Nov 12 2021 (made it into Linux 5.17)
>>>>> - changes allocation of reo_dst rings to cacheable memory to allow
>>>>> cached descriptor access to optimize CPU usage
>>>>> - this is flawed because it uses virt_to_phys() to allocate cacheable
>>>>> memory which does not work on systems with an IOMMU enabled or using
>>>>> software IOMMU (swiotlb); this causes a kernel crash on client
>>>>> association
>>>>
>>>> And this is where it started to take a wrong turn, that everyhing
>>>> later basically made worse. If you have long living and potentially
>>>> large DMA allocations, you need to use dma_alloc_* interfaces.
>>>>
>>>> 5.17 already had dma_alloc_pages for quite a while which was and still is
>>>> the proper interface to use. For much older kernel you'd be stuck
>>>> with dma_alloc_noncoherent or dma_alloc_attrs with the right flag,
>>>> but even that would have been much better.
>>>
>>> Christoph,
>>>
>>> I'm not clear what you are suggesting be done here. Are you suggesting
>>> that ath11k has been using the wrong mechanism by calling
>>> dma_map_single for cached DMA buffers? I'm not all that familiar with
>>> ath11k so I can't tell what buffers are considered long living.
>>
>> those buffers are allocated when driver load and freed when driver unload, so IMO they are
>> long living.
>
> The point is that if this driver wants a notion of "cached DMA buffers", then it should
> allocate such buffers the proper way, not try to reinvent it badly. That means using
> dma_alloc_pages(), or modern dma_alloc_noncoherent() which is essentially the same thing
> but with the dma_map_page() call automatically done for you as well.
yeah, you are right, Robin. didn't know there are convenient interfaces like these already.
Tim, can you work out a patch then?
>
> Thanks,
> Robin.
