Re: [PATCH v9 07/16] drivers: acpi: implement acpi_dma_configure

[Lorenzo Pieralisi]

On Sat, Dec 03, 2016 at 03:11:09AM +0100, Rafael J. Wysocki wrote:
> On Fri, Dec 2, 2016 at 4:38 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@xxxxxxx> wrote:
> > Rafael, Mark, Suravee,
> >
> > On Mon, Nov 21, 2016 at 10:01:39AM +0000, Lorenzo Pieralisi wrote:
> >> On DT based systems, the of_dma_configure() API implements DMA
> >> configuration for a given device. On ACPI systems an API equivalent to
> >> of_dma_configure() is missing which implies that it is currently not
> >> possible to set-up DMA operations for devices through the ACPI generic
> >> kernel layer.
> >>
> >> This patch fills the gap by introducing acpi_dma_configure/deconfigure()
> >> calls that for now are just wrappers around arch_setup_dma_ops() and
> >> arch_teardown_dma_ops() and also updates ACPI and PCI core code to use
> >> the newly introduced acpi_dma_configure/acpi_dma_deconfigure functions.
> >>
> >> Since acpi_dma_configure() is used to configure DMA operations, the
> >> function initializes the dma/coherent_dma masks to sane default values
> >> if the current masks are uninitialized (also to keep the default values
> >> consistent with DT systems) to make sure the device has a complete
> >> default DMA set-up.
> >
> > I spotted a niggle that unfortunately was hard to spot (and should not
> > be a problem per se but better safe than sorry) and I am not comfortable
> > with it.
> >
> > Following commit d0562674838c ("ACPI / scan: Parse _CCA and setup
> > device coherency") in acpi_bind_one() we check if the acpi_device
> > associated with a device just added supports DMA, first it was
> > done with acpi_check_dma() and then commit 1831eff876bd ("device
> > property: ACPI: Make use of the new DMA Attribute APIs") changed
> > it to acpi_get_dma_attr().
> >
> > The subsequent check (attr != DEV_DMA_NOT_SUPPORTED) is always true
> > on _any_ acpi device we pass to acpi_bind_one() on x86, which was
> > fine because we used it to call arch_setup_dma_ops(), which is a nop
> > on x86. On ARM64 a _CCA method is required to define if a device
> > supports DMA so (attr != DEV_DMA_NOT_SUPPORTED) may well be false.
> >
> > Now, acpi_bind_one() is used to bind an acpi_device to its physical
> > node also for pseudo-devices like cpus and memory nodes. For those
> > objects, on x86, attr will always be != DEV_DMA_NOT_SUPPORTED.
> >
> > So far so good, because on x86 arch_setup_dma_ops() is empty code.
> >
> > With this patch, I use the (attr != DEV_DMA_NOT_SUPPORTED) check
> > to call acpi_dma_configure() which is basically a nop on x86 except
> > that it sets up the dma_mask/coherent_dma_mask to a sane default value
> > (after all we are setting up DMA for the device so it makes sense to
> > initialize the masks there if they were unset since we are configuring
> > DMA for the device in question) for the given device.
> >
> > Problem is, as per the explanation above, we are also setting the
> > default dma masks for pseudo-devices (eg CPUs) that were previously
> > untouched, it should not be a problem per-se but I am not comfortable
> > with that, honestly it does not make much sense.
> >
> > An easy "fix" would be to move the default dma masks initialization out
> > of acpi_dma_configure() (as it was in previous patch versions of this
> > series - I moved it to acpi_dma_configure() just a consolidation point
> > for initializing the masks instead of scattering them in every
> > acpi_dma_configure caller) I can send this as a fix-up patch to Joerg if
> > we think that's the right thing to do (or I can send it to Rafael later
> > when the code is in the merged depending on the timing) just let me
> > know please.
> 
> Why can't arch_setup_dma_ops() set those masks too?

Because the dma masks set-up is done by the caller (see
of_dma_configure()) according to firmware configuration or
platform data knowledge. I wanted to replicate the of_dma_configure()
interface on ACPI for obvious reasons (on ARM systems), I stopped
short of adding ACPI code to mirror of_dma_get_range() equivalent
(through the _DMA object) but I am really really nervous about changing
the code path on x86 because in theory all is fine, in practice even
just setting the masks to sane values can have unexpected consequences,
I just can't know (that's why I wasn't doing it in the first iterations
of this series).

Side note: DT with of_dma_configure() and ACPI with
acpi_create_platform_device() set the default dma mask for all
platform devices already _regardless_ of what they really are, though
arguably acpi_bind_one() touches ways more devices.

I really think that removing the default dma masks settings from
acpi_dma_configure() is the safer thing to do for the time being (or
moving acpi_dma_configure() to acpi_create_platform_device(), where the
DMA masks are set-up by default by core ACPI. Mark, Suravee, what was
the rationale behind calling arch_setup_dma_ops() in acpi_bind_one() ?)

Please let me know, fix-up is trivial however we decide to proceed.

Thank you !
Lorenzo