Messages in this thread Patch in this message | | | Subject | Re: [PoC] arm: dma-mapping: direct: Apply dma_pfn_offset only when it is valid | From | Peter Ujfalusi <> | Date | Fri, 31 Jan 2020 15:59:54 +0200 |
| |
Hi Christoph,
On 30/01/2020 18.40, Christoph Hellwig wrote: > On Thu, Jan 30, 2020 at 03:04:37PM +0200, Peter Ujfalusi via iommu wrote: >> On 30/01/2020 9.53, Christoph Hellwig wrote: >>> [skipping the DT bits, as I'm everything but an expert on that..] >>> >>> On Mon, Jan 27, 2020 at 04:00:30PM +0200, Peter Ujfalusi wrote: >>>> I agree on the phys_to_dma(). It should fail for addresses which does >>>> not fall into any of the ranges. >>>> It is just a that we in Linux don't have the concept atm for ranges, we >>>> have only _one_ range which applies to every memory address. >>> >>> what does atm here mean? >> >> struct device have only single dma_pfn_offset, one can not have multiple >> ranges defined. If we have then only the first is taken and the physical >> address and dma address is discarded, only the dma_pfn_offset is stored >> and used. >> >>> We have needed multi-range support for quite a while, as common broadcom >>> SOCs do need it. So patches for that are welcome at least from the >>> DMA layer perspective (kinda similar to your pseudo code earlier) >> >> But do they have dma_pfn_offset != 0? > > Well, with that I mean multiple ranges with different offsets. Take > a look at arch/mips/bmips/dma.c:__phys_to_dma() and friends. This > is an existing implementation for mips, but there are arm and arm64 > SOCs using the same logic on the market as well, and we'll want to > support them eventually.
I see. My PoC patch was not too off then ;) So the plan is to have a generic implementation for all of the architecture, right?
>> The dma_pfn_offset is _still_ applied to the mask we are trying to set >> (and validate) via dma-direct. > > And for the general case that is exactly the right thing to do, we > just need to deal with really odd ZONE_DMA placements like yours.
I'm still not convinced, the point of the DMA mask, at least how I see it, to check that the dma address can be handled by the device (DMA, peripheral with built in DMA, etc), it is not against physical address. Doing phys_to_dma() on the mask from the dma_set_mask() is just wrong.
>>> We'll need to find the minimum change to make it work >>> for now without switching ops, even if it isn't the correct one, and >>> then work from there. >> >> Sure, but can we fix the regression by reverting to arm_ops for now only >> if dma_pfn_offset is not 0? It used to work fine in the past at least it >> appeared to work on K2 platforms. > > But that will cause yet another regression in what we have just fixed > with using the generic direct ops, at which points it turns into who > screams louder.
Hehe, I see. I genuinely curious why k2 platform worked just fine with LPAE (it needs it), but guys had issues with LPAE on dra7/am5. The fix for dra7/am5 broke k2. As far as I can see the main (only) difference is that k2 have dma_pfn_offset = 0x780000, while dra7/am5 have it 0 (really direct mapping).
> For now I'm tempted to throw something like this in, which is a bit > of a hack, but actually 100% matches what various architectures have > historically done: > > > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index 6af7ae83c4ad..6ba9ee6e20bd 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -482,6 +482,9 @@ int dma_direct_supported(struct device *dev, u64 mask) > { > u64 min_mask; > > + if (mask >= DMA_BIT_MASK(32)) > + return 1; > +
Right, so skipping phys_to_dma() for the mask and believing that it will work..
It does: audio and dmatest memcpy tests are just fine with this, MMC also probed with ADMA enabled.
As far as I can tell it works as well as falling back to the old arm ops in case of LPAE && dma_pfn_offset != 0
Fwiw: Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Would yo be comfortable to send apply this patch to mainline with Fixes: ad3c7b18c5b3 ("arm: use swiotlb for bounce buffering on LPAE configs")
So it gets picked for stable kernels as well? > if (IS_ENABLED(CONFIG_ZONE_DMA)) > min_mask = DMA_BIT_MASK(zone_dma_bits); > else >
Thank you, - Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
| |