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

[Rafael J. Wysocki]

On Sat, Dec 3, 2016 at 11:39 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> wrote:
> 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() ?)

Alternatively, you can add one more arch wrapper that will be a no-op
on x86 and that will set up the default masks and call
arch_setup_dma_ops() on ARM.  Then, you can invoke that from
acpi_dma_configure().

Or make the definition of acpi_dma_configure() itself depend on the
architecture.

Thanks,
Rafael