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
> > 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
> >>>  wrote:
> >>> > Rafael, Mark, Suravee,
> >>> >
> >>> > On Mon, Nov 21, 2016 at 10:01:39AM +, 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 

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
> > 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
> >>>  wrote:
> >>> > Rafael, Mark, Suravee,
> >>> >
> >>> > On Mon, Nov 21, 2016 at 10:01:39AM +, 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 

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

[Sricharan]

Hi Lorenzo,

>
>On Sat, Dec 3, 2016 at 11:39 AM, Lorenzo Pieralisi
> 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
>>>  wrote:
>>> > Rafael, Mark, Suravee,
>>> >
>>> > On Mon, Nov 21, 2016 at 10:01:39AM +, 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 

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

[Sricharan]

Hi Lorenzo,

>
>On Sat, Dec 3, 2016 at 11:39 AM, Lorenzo Pieralisi
> 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
>>>  wrote:
>>> > Rafael, Mark, Suravee,
>>> >
>>> > On Mon, Nov 21, 2016 at 10:01:39AM +, 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(), 

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
 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
>>  wrote:
>> > Rafael, Mark, Suravee,
>> >
>> > On Mon, Nov 21, 2016 at 10:01:39AM +, 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 

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
 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
>>  wrote:
>> > Rafael, Mark, Suravee,
>> >
>> > On Mon, Nov 21, 2016 at 10:01:39AM +, 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 

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
>  wrote:
> > Rafael, Mark, Suravee,
> >
> > On Mon, Nov 21, 2016 at 10:01:39AM +, 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

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
>  wrote:
> > Rafael, Mark, Suravee,
> >
> > On Mon, Nov 21, 2016 at 10:01:39AM +, 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

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

[Rafael J. Wysocki]

On Fri, Dec 2, 2016 at 4:38 PM, Lorenzo Pieralisi
 wrote:
> Rafael, Mark, Suravee,
>
> On Mon, Nov 21, 2016 at 10:01:39AM +, 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?

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Fri, Dec 2, 2016 at 4:38 PM, Lorenzo Pieralisi
 wrote:
> Rafael, Mark, Suravee,
>
> On Mon, Nov 21, 2016 at 10:01:39AM +, 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?

Thanks,
Rafael

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

[Lorenzo Pieralisi]

Rafael, Mark, Suravee,

On Mon, Nov 21, 2016 at 10:01:39AM +, 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.

Thanks !
Lorenzo

> The DMA range size passed to arch_setup_dma_ops() is sized according
> to the device coherent_dma_mask (starting at address 0x0), mirroring the
> DT probing path behaviour when a dma-ranges property is not provided
> for the device being probed; this changes the current arch_setup_dma_ops()
> call parameters in the ACPI probing case, but since arch_setup_dma_ops()
> is a NOP on all architectures but ARM/ARM64 this patch does not change
> the current kernel behaviour on them.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Acked-by: Bjorn Helgaas  [pci]
> Acked-by: Rafael J. Wysocki 
> Reviewed-by: Tomasz Nowicki 
> Tested-by: Hanjun Guo 
> Tested-by: Tomasz Nowicki 
> Cc: Bjorn Helgaas 
> Cc: Robin Murphy 
> Cc: Tomasz Nowicki 
> Cc: Joerg Roedel 
> Cc: "Rafael J. Wysocki" 
> ---
>  drivers/acpi/glue.c |  4 ++--
>  drivers/acpi/scan.c | 40 
>  drivers/pci/probe.c |  3 +--
>  include/acpi/acpi_bus.h |  2 ++
>  include/linux/acpi.h|  5 +
>  5 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 5ea5dc2..f8d6564 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -227,8 +227,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
>  
>   attr = acpi_get_dma_attr(acpi_dev);
>   if (attr != DEV_DMA_NOT_SUPPORTED)
> - arch_setup_dma_ops(dev, 0, 0, NULL,
> -attr 

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

[Lorenzo Pieralisi]

Rafael, Mark, Suravee,

On Mon, Nov 21, 2016 at 10:01:39AM +, 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.

Thanks !
Lorenzo

> The DMA range size passed to arch_setup_dma_ops() is sized according
> to the device coherent_dma_mask (starting at address 0x0), mirroring the
> DT probing path behaviour when a dma-ranges property is not provided
> for the device being probed; this changes the current arch_setup_dma_ops()
> call parameters in the ACPI probing case, but since arch_setup_dma_ops()
> is a NOP on all architectures but ARM/ARM64 this patch does not change
> the current kernel behaviour on them.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Acked-by: Bjorn Helgaas  [pci]
> Acked-by: Rafael J. Wysocki 
> Reviewed-by: Tomasz Nowicki 
> Tested-by: Hanjun Guo 
> Tested-by: Tomasz Nowicki 
> Cc: Bjorn Helgaas 
> Cc: Robin Murphy 
> Cc: Tomasz Nowicki 
> Cc: Joerg Roedel 
> Cc: "Rafael J. Wysocki" 
> ---
>  drivers/acpi/glue.c |  4 ++--
>  drivers/acpi/scan.c | 40 
>  drivers/pci/probe.c |  3 +--
>  include/acpi/acpi_bus.h |  2 ++
>  include/linux/acpi.h|  5 +
>  5 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 5ea5dc2..f8d6564 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -227,8 +227,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
>  
>   attr = acpi_get_dma_attr(acpi_dev);
>   if (attr != DEV_DMA_NOT_SUPPORTED)
> - arch_setup_dma_ops(dev, 0, 0, NULL,
> -attr == DEV_DMA_COHERENT);
> + acpi_dma_configure(dev, attr);
>  
>   acpi_physnode_link_name(physical_node_name, node_id);
>   retval = sysfs_create_link(_dev->dev.kobj, >kobj,
> @@ -251,6 +250,7 @@ int 

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

[Lorenzo Pieralisi]

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.

The DMA range size passed to arch_setup_dma_ops() is sized according
to the device coherent_dma_mask (starting at address 0x0), mirroring the
DT probing path behaviour when a dma-ranges property is not provided
for the device being probed; this changes the current arch_setup_dma_ops()
call parameters in the ACPI probing case, but since arch_setup_dma_ops()
is a NOP on all architectures but ARM/ARM64 this patch does not change
the current kernel behaviour on them.

Signed-off-by: Lorenzo Pieralisi 
Acked-by: Bjorn Helgaas  [pci]
Acked-by: Rafael J. Wysocki 
Reviewed-by: Tomasz Nowicki 
Tested-by: Hanjun Guo 
Tested-by: Tomasz Nowicki 
Cc: Bjorn Helgaas 
Cc: Robin Murphy 
Cc: Tomasz Nowicki 
Cc: Joerg Roedel 
Cc: "Rafael J. Wysocki" 
---
 drivers/acpi/glue.c |  4 ++--
 drivers/acpi/scan.c | 40 
 drivers/pci/probe.c |  3 +--
 include/acpi/acpi_bus.h |  2 ++
 include/linux/acpi.h|  5 +
 5 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 5ea5dc2..f8d6564 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -227,8 +227,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
*acpi_dev)
 
attr = acpi_get_dma_attr(acpi_dev);
if (attr != DEV_DMA_NOT_SUPPORTED)
-   arch_setup_dma_ops(dev, 0, 0, NULL,
-  attr == DEV_DMA_COHERENT);
+   acpi_dma_configure(dev, attr);
 
acpi_physnode_link_name(physical_node_name, node_id);
retval = sysfs_create_link(_dev->dev.kobj, >kobj,
@@ -251,6 +250,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
*acpi_dev)
return 0;
 
  err:
+   acpi_dma_deconfigure(dev);
ACPI_COMPANION_SET(dev, NULL);
put_device(dev);
put_device(_dev->dev);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 3d1856f..45b5710 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1370,6 +1370,46 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device 
*adev)
return DEV_DMA_NON_COHERENT;
 }
 
+/**
+ * acpi_dma_configure - Set-up DMA configuration for the device.
+ * @dev: The pointer to the device
+ * @attr: device dma attributes
+ */
+void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
+{
+   /*
+* Set default coherent_dma_mask to 32 bit.  Drivers are expected to
+* setup the correct supported mask.
+*/
+   if (!dev->coherent_dma_mask)
+   dev->coherent_dma_mask = DMA_BIT_MASK(32);
+
+   /*
+* Set it to coherent_dma_mask by default if the architecture
+* code has not set it.
+*/
+   if (!dev->dma_mask)
+   dev->dma_mask = >coherent_dma_mask;
+
+   /*
+* Assume dma valid range starts at 0 and covers the whole
+* coherent_dma_mask.
+*/
+   arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, NULL,
+  attr == DEV_DMA_COHERENT);
+}
+EXPORT_SYMBOL_GPL(acpi_dma_configure);
+
+/**
+ * acpi_dma_deconfigure - Tear-down DMA configuration for the device.
+ * @dev: The pointer to the device
+ */
+void acpi_dma_deconfigure(struct device *dev)
+{
+   arch_teardown_dma_ops(dev);
+}
+EXPORT_SYMBOL_GPL(acpi_dma_deconfigure);
+
 static void acpi_init_coherency(struct acpi_device *adev)
 {
unsigned long long cca = 0;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ab00267..c29e07a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1738,8 +1738,7 @@ static void pci_dma_configure(struct pci_dev *dev)
if (attr == DEV_DMA_NOT_SUPPORTED)
dev_warn(>dev, "DMA not supported.\n");
else
-   arch_setup_dma_ops(>dev, 0, 0, NULL,
-  attr == 

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

[Lorenzo Pieralisi]

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.

The DMA range size passed to arch_setup_dma_ops() is sized according
to the device coherent_dma_mask (starting at address 0x0), mirroring the
DT probing path behaviour when a dma-ranges property is not provided
for the device being probed; this changes the current arch_setup_dma_ops()
call parameters in the ACPI probing case, but since arch_setup_dma_ops()
is a NOP on all architectures but ARM/ARM64 this patch does not change
the current kernel behaviour on them.

Signed-off-by: Lorenzo Pieralisi 
Acked-by: Bjorn Helgaas  [pci]
Acked-by: Rafael J. Wysocki 
Reviewed-by: Tomasz Nowicki 
Tested-by: Hanjun Guo 
Tested-by: Tomasz Nowicki 
Cc: Bjorn Helgaas 
Cc: Robin Murphy 
Cc: Tomasz Nowicki 
Cc: Joerg Roedel 
Cc: "Rafael J. Wysocki" 
---
 drivers/acpi/glue.c |  4 ++--
 drivers/acpi/scan.c | 40 
 drivers/pci/probe.c |  3 +--
 include/acpi/acpi_bus.h |  2 ++
 include/linux/acpi.h|  5 +
 5 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 5ea5dc2..f8d6564 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -227,8 +227,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
*acpi_dev)
 
attr = acpi_get_dma_attr(acpi_dev);
if (attr != DEV_DMA_NOT_SUPPORTED)
-   arch_setup_dma_ops(dev, 0, 0, NULL,
-  attr == DEV_DMA_COHERENT);
+   acpi_dma_configure(dev, attr);
 
acpi_physnode_link_name(physical_node_name, node_id);
retval = sysfs_create_link(_dev->dev.kobj, >kobj,
@@ -251,6 +250,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
*acpi_dev)
return 0;
 
  err:
+   acpi_dma_deconfigure(dev);
ACPI_COMPANION_SET(dev, NULL);
put_device(dev);
put_device(_dev->dev);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 3d1856f..45b5710 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1370,6 +1370,46 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device 
*adev)
return DEV_DMA_NON_COHERENT;
 }
 
+/**
+ * acpi_dma_configure - Set-up DMA configuration for the device.
+ * @dev: The pointer to the device
+ * @attr: device dma attributes
+ */
+void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
+{
+   /*
+* Set default coherent_dma_mask to 32 bit.  Drivers are expected to
+* setup the correct supported mask.
+*/
+   if (!dev->coherent_dma_mask)
+   dev->coherent_dma_mask = DMA_BIT_MASK(32);
+
+   /*
+* Set it to coherent_dma_mask by default if the architecture
+* code has not set it.
+*/
+   if (!dev->dma_mask)
+   dev->dma_mask = >coherent_dma_mask;
+
+   /*
+* Assume dma valid range starts at 0 and covers the whole
+* coherent_dma_mask.
+*/
+   arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, NULL,
+  attr == DEV_DMA_COHERENT);
+}
+EXPORT_SYMBOL_GPL(acpi_dma_configure);
+
+/**
+ * acpi_dma_deconfigure - Tear-down DMA configuration for the device.
+ * @dev: The pointer to the device
+ */
+void acpi_dma_deconfigure(struct device *dev)
+{
+   arch_teardown_dma_ops(dev);
+}
+EXPORT_SYMBOL_GPL(acpi_dma_deconfigure);
+
 static void acpi_init_coherency(struct acpi_device *adev)
 {
unsigned long long cca = 0;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ab00267..c29e07a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1738,8 +1738,7 @@ static void pci_dma_configure(struct pci_dev *dev)
if (attr == DEV_DMA_NOT_SUPPORTED)
dev_warn(>dev, "DMA not supported.\n");
else
-   arch_setup_dma_ops(>dev, 0, 0, NULL,
-  attr == DEV_DMA_COHERENT);
+   acpi_dma_configure(>dev, attr);
}
 
pci_put_host_bridge_device(bridge);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index c1a524d..4242c31 100644
---