Re: [PATCH] dma-mapping: Relax warnings for per-device areas

From: Robin Murphy
Date: Fri Jul 06 2018 - 12:46:09 EST


On 06/07/18 15:19, Fredrik Noring wrote:
Hi Robin,

On Fri, Jul 06, 2018 at 12:57:11PM +0100, Robin Murphy wrote:
On 05/07/18 20:36, Christoph Hellwig wrote:
- BUG_ON(!ops);
- WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
-
if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
return cpu_addr;
+ BUG_ON(!ops);
+ WARN_ON_ONCE(dev && !dev->coherent_dma_mask);

I think doing dma on a device without ops is completely broken no matter
what you think of it, so I very much disagree with that part of the change.

Also while I don't think not having a dma mask is a good idea even for
a driver purely using dma coherent pools. If the pools really are on
the device itself I can see why it might not matter, but for the case
commonly used on some ARM SOCs where we just reserve memory for certain
devices from a system pool it very much does matter.

There really is no good excuse to not set a coherent mask in the drivers.

Right, I was rather on the fence about this - on the one hand it is
objectively wrong per the API for drivers to call dma_alloc_coherent()
without a prior successful dma_set_coherent_mask() call, but then I thought
that in the case when they're *only* using it as a proxy for
dma_alloc_from_dev_coherent() and explicitly don't want regular allocations
from kernel memory to ever happen, then maybe it might be somewhat
reasonable. But indeed I hadn't really given enough thought to the
reserved-memory carveout case, where we definitely don't want to let a
legitimate warning be hidden on a developer's machine but hit by users with
different system configurations.

Fredrik, are you happy to fix up your driver to initialise a suitable mask
at probe time?

The driver currently only uses dma_declare_coherent_memory and
dma_release_declared_memory. It allocates 256 KiB of DMA memory from the IOP
(a MIPS R3000 I/O processor that is separate from the main MIPS R5900) with

iop_dma_addr = iop_alloc(size);

and then declares it with

dma_declare_coherent_memory(dev,
iop_bus_to_phys(iop_dma_addr),
iop_dma_addr, size, flags);

where iop_bus_to_phys is:

#define IOP_MEMORY_BASE_ADDRESS 0x1c000000
phys_addr_t iop_bus_to_phys(iop_addr_t baddr)
{
return (u32)baddr + IOP_MEMORY_BASE_ADDRESS;
}

Does dma_set_coherent_mask want a device object representing the IOP? Such
a thing is currently not implemented, but can certainly be done.

Nope, just the same OHCI device as the dma_declare_coherent_memory() call.

As you noted, the kernel cannot and must not allocate any kind of normal
memory for this device. Typical DMA addresses 0-0x200000 are mapped to
0x1c000000-0x1c200000 and is memory managed exclusively by the IOP. What
would be a suitable mask for that?

For the sake of accuracy, I guess maybe DMA_BIT_MASK(20) since that's what the OHCI's effective addressing capability is, even if it does happen to be to remote IOP RAM. Alternatively, there is perhaps some degree of argument for deliberately picking a nonzero but useless value like 1, although it looks like the MIPS allocator (at least the dma-default one) never actually checks whether the page it gets is within range of the device's coherent mask, which it probably should do.

drivers/usb/host/ohci-sm501.c and drivers/usb/host/ohci-tmio.c are similar,
so I suppose they need to be fixed as well?

Possibly, or maybe they get away with it by virtue of bus code setting some default mask. Since those look to be pretty old bits of hardware that I know nothing about, I'd be hesitant to start adding stuff. If there is anyone still using such things and running mainline on them, I guess we'll hear about it in due course...

Robin.