Hello Christoph,
On 22/07/2024 16:58, Christoph Hellwig wrote:
>> +#include "../../../drivers/iommu/dma-iommu.h"
> This is not a public header as you can guess from the file path.
Yes indeed this is not "correct" however I didn't know how to do it properly so I am sending this as "RFC" to get some help to clean this up properly :)
If I understood correctly the remaining part of your review: including this header is not needed anymore since we don't need to call `iommu_setup_dma_ops()` anymore.
So let's remove both the incorrect non-public header include and the call to iommu_setup_dma_ops().
>
>> + switch (dir) {
>> + case DMA_TO_DEVICE:
>> + break;
>> + case DMA_FROM_DEVICE:
>> + break;
>> +
>> + case DMA_BIDIRECTIONAL:
>> + inval_dcache_range(paddr, size);
> Doing this just for bidirectional is weird unless your architecture
> never does any speculative prefetching. Other architectures
> include DMA_FROM_DEVICE here.
Indeed our arch never does any speculative prefetching on data.
>
>> +#ifdef CONFIG_IOMMU_DMA
>> +void arch_teardown_dma_ops(struct device *dev)
>> +{
>> + dev->dma_ops = NULL;
>> +}
>> +#endif /* CONFIG_IOMMU_DMA*/
> This should not be needed right now. And will be completley
> useless once we do the direct calls to dma-iommu which we plan
> to do for Linux 6.12.
Alright, let's remove this.
>
>> +void arch_setup_dma_ops(struct device *dev, bool coherent)
>> +{
>> + dev->dma_coherent = coherent;
>> + if (device_iommu_mapped(dev))
>> + iommu_setup_dma_ops(dev);
>> +}
> And this seems odd, as iommu_setup_dma_ops is called from the iommu
> code and you shouldn't need it here.
Let's remove the `if device_iommu_mapped(dev)) iommu_setup_dma_ops(dev);` then.
>
> I also wonder if we can come up with a way to do the ->dma_coherent
> setup in common code and remove a few of these arch hooks entirely.
Thanks!
Regards,
--
Yann
