[RFC PATCH 1/2] dma-mapping: Clean up dma_set_*mask() hooks

On 08/07/18 16:07, Christoph Hellwig wrote:

On Fri, Jul 06, 2018 at 03:20:34PM +0100, Robin Murphy wrote:

quoted

quoted

What are you trying to do?  I really don't want to see more users of
the hooks as they are are a horribly bad idea.

I really need to fix the ongoing problem we have where, due to funky
integrations, devices suffer some downstream addressing limit (described by
DT dma-ranges or ACPI IORT/_DMA) which we carefully set up in
dma_configure(), but then just gets lost when the driver probes and
innocently calls dma_set_mask() with something wider. I think it's
effectively the generalised case of the VIA 32-bit quirk, if I understand
that one correctly.

I'd much rather fix this in generic code.  How funky are your limitations?
In fact when I did the 32-bit quirk (which will also be used by a Xiling
PCIe root port usable on a lot of architectures) I did initially consider
adding a bus_dma_mask or similar to struct device, but opted for the
most simple implementation for now.  I'd be happy to chanfe this.

Especially these days where busses and IP blocks are generally not tied
to a specific cpu instruction set I really believe that having any
more architecture code than absolutely required is a bad idea.

Oh, for sure, the generic fix would be the longer-term goal, this was 
just an expedient compromise because I want to get *something* landed 
for 4.19. Since in practice this is predominantly affecting arm64, doing 
the arch-specific fix to appease affected customers then working to 
generalise it afterwards seemed to carry the lowest risk.

That said, I think I can see a relatively safe and clean alternative 
approach based on converting dma_32bit_limit to a mask, so I'll spin 
some patches around that idea ASAP to continue the discussion.

quoted

The approach that seemed to me to be safest is largely based on the one
proposed in a thread from ages ago[1]; namely to make dma_configure()
better at distinguishing firmware-specified masks from bus defaults,
capture the firmware mask in dev->archdata during arch_setup_dma_ops(),
then use the custom set_mask routines to ensure any subsequent updates
never exceed that. It doesn't seem possible to make this work robustly
without storing *some* additional per-device data, and for that archdata is
a lesser evil than struct device itself. Plus even though it's not actually
an arch-specific issue it feels like there's such a risk of breaking other
platforms that I'm reticent to even try handling it entirely in generic
code.

My plan for a few merge windows from now is that dma_mask and
coherent_mask are 100% in device control and dma_set_mask will never
fail.  It will be up to the dma ops to make sure things are addressible.

It's entirely possible to plug an old PCI soundcard via a bridge adapter 
into a modern board where the card's 24-bit DMA mask reaches nothing but 
the SoC's boot flash, and no IOMMU is available (e.g. some of the 
smaller NXP Layercape stuff); I still think there should be an error in 
such rare cases when DMA is utterly impossible, but otherwise I agree it 
would be much nicer for drivers to just provide their preferred mask and 
let the ops massage it as necessary.

Robin.