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

[Lorenzo Pieralisi]

On Mon, Dec 05, 2016 at 03:22:02PM +0530, Sricharan wrote:
> Hi Lorenzo,
> 
> >
> >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.
> >
> 
> So is it better that either removing the masks from acpi_dma_configure
> (or) creating the wrapper as Rafael mentioned, than moving
> acpi_dma_configure itself , because with something like iommu probe
> deferral that is tried, acpi_dma_configure is getting invoked from a
> device's really_probe, a different path again ?

Yes, I thought about that too. Given what I said above (ie I would like
to extend the mask set-up with _DMA object - that is generic ACPI but
can affect legacy x86 - and if that does not work through IORT specific
bindings), as per Rafael suggestion I added an iort_set_dma_mask wrapper
that is a NOP on x86, leaving acpi_dma_configure() unchanged for ARM64.

Patch coming, thanks everyone.

Lorenzo