Not all the RISCV are DMA coherent by default. Moreover we have
RISCV_DMA_NONCOHERENT option.
So set ARCH_DMA_DEFAULT_COHERENT only when RISCV_DMA_NONCOHERENT is not set
Fixes: c00a60d6f4a1 ("of: address: always use dma_default_coherent for default coherency")
Signed-off-by: Maxim Kochetkov <[email protected]>
---
arch/riscv/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d6824bec2c00..111c5d92d503 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -14,7 +14,7 @@ config RISCV
def_bool y
select ACPI_GENERIC_GSI if ACPI
select ACPI_REDUCED_HARDWARE_ONLY if ACPI
- select ARCH_DMA_DEFAULT_COHERENT
+ select ARCH_DMA_DEFAULT_COHERENT if !RISCV_DMA_NONCOHERENT
select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
select ARCH_ENABLE_THP_MIGRATION if TRANSPARENT_HUGEPAGE
--
2.40.1
+ Christoph
I don't think this patch is correct. Regardless of whether we support
cache management operations, DMA is assumed to be coherent unless
peripherals etc are specified to otherwise in DT (or however ACPI deals
with that kind of thing).
What problem are you trying to solve here?
On Thu, Dec 21, 2023 at 09:51:52PM +0300, Maxim Kochetkov wrote:
> Not all the RISCV are DMA coherent by default.
What is a "RISCV"? I believe this sentence should be "not all RISC-V
systems are DMA coherent." but that is provided for by the
"dma-noncoherent" property, set for peripherals (or buses) that are not
DMA coherent.
> Moreover we have
> RISCV_DMA_NONCOHERENT option.
> So set ARCH_DMA_DEFAULT_COHERENT only when RISCV_DMA_NONCOHERENT is not set
>
> Fixes: c00a60d6f4a1 ("of: address: always use dma_default_coherent for default coherency")
> Signed-off-by: Maxim Kochetkov <[email protected]>
> ---
> arch/riscv/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d6824bec2c00..111c5d92d503 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -14,7 +14,7 @@ config RISCV
> def_bool y
> select ACPI_GENERIC_GSI if ACPI
> select ACPI_REDUCED_HARDWARE_ONLY if ACPI
> - select ARCH_DMA_DEFAULT_COHERENT
> + select ARCH_DMA_DEFAULT_COHERENT if !RISCV_DMA_NONCOHERENT
I think this is actually buggy, for things like distro kernels
RISCV_DMA_COHERENT will always be set, but those kernels are expected
to be used on systems that are cache coherent also.
Thanks,
Conor.
> select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
> select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
> select ARCH_ENABLE_THP_MIGRATION if TRANSPARENT_HUGEPAGE
> --
> 2.40.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
在 2023/12/21 20:29, Conor Dooley 写道:
> + Christoph
>
> I don't think this patch is correct. Regardless of whether we support
> cache management operations, DMA is assumed to be coherent unless
> peripherals etc are specified to otherwise in DT (or however ACPI deals
> with that kind of thing).
>
> What problem are you trying to solve here?
>
> On Thu, Dec 21, 2023 at 09:51:52PM +0300, Maxim Kochetkov wrote:
>> Not all the RISCV are DMA coherent by default.
Sorry for chime in here.
IMO if your platform is not coherent by default, just insert
"dma-noncoherent"
at devicetree root node.
Thanks
- Jiaxun
> What is a "RISCV"? I believe this sentence should be "not all RISC-V
> systems are DMA coherent." but that is provided for by the
> "dma-noncoherent" property, set for peripherals (or buses) that are not
> DMA coherent.
>
>> Moreover we have
>> RISCV_DMA_NONCOHERENT option.
>> So set ARCH_DMA_DEFAULT_COHERENT only when RISCV_DMA_NONCOHERENT is not set
>>
>> Fixes: c00a60d6f4a1 ("of: address: always use dma_default_coherent for default coherency")
>> Signed-off-by: Maxim Kochetkov <[email protected]>
>> ---
>> arch/riscv/Kconfig | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index d6824bec2c00..111c5d92d503 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -14,7 +14,7 @@ config RISCV
>> def_bool y
>> select ACPI_GENERIC_GSI if ACPI
>> select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>> - select ARCH_DMA_DEFAULT_COHERENT
>> + select ARCH_DMA_DEFAULT_COHERENT if !RISCV_DMA_NONCOHERENT
> I think this is actually buggy, for things like distro kernels
> RISCV_DMA_COHERENT will always be set, but those kernels are expected
> to be used on systems that are cache coherent also.
>
> Thanks,
> Conor.
>
>> select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
>> select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
>> select ARCH_ENABLE_THP_MIGRATION if TRANSPARENT_HUGEPAGE
>> --
>> 2.40.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Thu, Dec 21, 2023 at 10:27:33PM +0000, Jiaxun Yang wrote:
>
>
> 在 2023/12/21 20:29, Conor Dooley 写道:
>> + Christoph
>>
>> I don't think this patch is correct. Regardless of whether we support
>> cache management operations, DMA is assumed to be coherent unless
>> peripherals etc are specified to otherwise in DT (or however ACPI deals
>> with that kind of thing).
>>
>> What problem are you trying to solve here?
>>
>> On Thu, Dec 21, 2023 at 09:51:52PM +0300, Maxim Kochetkov wrote:
>>> Not all the RISCV are DMA coherent by default.
>
> Sorry for chime in here.
> IMO if your platform is not coherent by default, just insert
> "dma-noncoherent"
> at devicetree root node.
Exactly. ARCH_DMA_DEFAULT_COHERENTis a setting that just says for
a given architecture assumes coherent unless otherwise specified,
which has historically been the case for mips. Not setting it means
non-coherent unless specified, which has historially been the case
for arm.
RISC-V starte out without support for non-coherent DMA, and high ups
in RISCV still told me in 2019 that RISC-V doesn't need cache
management instructions because no new hardware would ever not be
dma coherent. Yeah, right..
Anyay, Linux for RISC-V has historically been coherent only and then
coherent default, so this option is wrong, and you need to mark
you platform as non-coherent by inserting dma-noncoherent somewhere.
On Fri, Dec 22, 2023 at 05:39:34PM +0300, Maxim Kochetkov wrote:
>
>
> On 22.12.2023 07:14, Christoph Hellwig wrote:
> > On Thu, Dec 21, 2023 at 10:27:33PM +0000, Jiaxun Yang wrote:
> > >
> > >
> > > 在 2023/12/21 20:29, Conor Dooley 写道:
> > > > + Christoph
> > > >
> > > > I don't think this patch is correct. Regardless of whether we support
> > > > cache management operations, DMA is assumed to be coherent unless
> > > > peripherals etc are specified to otherwise in DT (or however ACPI deals
> > > > with that kind of thing).
> > > >
> > > > What problem are you trying to solve here?
> > > >
> > > > On Thu, Dec 21, 2023 at 09:51:52PM +0300, Maxim Kochetkov wrote:
> > > > > Not all the RISCV are DMA coherent by default.
> > >
> > > Sorry for chime in here.
> > > IMO if your platform is not coherent by default, just insert
> > > "dma-noncoherent"
> > > at devicetree root node.
> >
> > Exactly. ARCH_DMA_DEFAULT_COHERENTis a setting that just says for
> > a given architecture assumes coherent unless otherwise specified,
> > which has historically been the case for mips. Not setting it means
> > non-coherent unless specified, which has historially been the case
> > for arm.
> >
> > RISC-V starte out without support for non-coherent DMA, and high ups
> > in RISCV still told me in 2019 that RISC-V doesn't need cache
> > management instructions because no new hardware would ever not be
> > dma coherent. Yeah, right..
> >
> > Anyay, Linux for RISC-V has historically been coherent only and then
> > coherent default, so this option is wrong, and you need to mark
> > you platform as non-coherent by inserting dma-noncoherent somewhere.
> >
> Conor, Christoph, Jiaxun, thanks for quick feedback!
>
> The problem is very simple:
> For non mips platforms dma_default_coherent is used at of_dma_is_coherent()
> and device_initialize().
> of_dma_is_coherent() affects only DT devices. And we can override it with
> "dma-coherent"/"dma-noncoherent". ACPI devices can specify by
> "attr == DEV_DMA_COHERENT". But all other devices (platform_device, usb,
I would have expected that usb devices "inherit" the value from the usb
controller whose bus they are on. Similarly, platform devices are on a
bus that should be marked as non-coherent if that is the case.
Christoph certainly knows better how things operate here however.
> etc..) do not have this feature. These devices will use value from
> device_initialize(). And we have no possibility to change
> dma_default_coherent value by disabling ARCH_DMA_DEFAULT_COHERENT.
> Moreover, changing dma_default_coherent from false to true may cause
> regression for other devices.
How can there be a regression when dma has been coherent by default for
the RISC-V kernel from day 1?
> That is why I suggest possibility to disable
> ARCH_DMA_DEFAULT_COHERENT by RISCV_DMA_NONCOHERENT option.
> It will works like for PPC:
> .....
> config PPC
> bool
> default y
> #
> # Please keep this list sorted alphabetically.
> #
> select ARCH_32BIT_OFF_T if PPC32
> select ARCH_DISABLE_KASAN_INLINE if PPC_RADIX_MMU
> select ARCH_DMA_DEFAULT_COHERENT if !NOT_COHERENT_CACHE
> .....
> Doesn't the option RISCV_DMA_NONCOHERENT say the DMA should be non-coherent
> by default?
No. That option only controls whether or not support for cache
management operations is built into the kernel. It is expected that this
option can be enabled for multiplatform kernels, like those used by
distributions, that will run on systems that are entirely coherent as
well as those that are not.
It does not work the same was as this PPC option appears to.
Cheers,
Conor.
On Fri, Dec 22, 2023 at 02:54:19PM +0000, Conor Dooley wrote:
> > of_dma_is_coherent() affects only DT devices. And we can override it with
> > "dma-coherent"/"dma-noncoherent". ACPI devices can specify by
> > "attr == DEV_DMA_COHERENT". But all other devices (platform_device, usb,
>
> I would have expected that usb devices "inherit" the value from the usb
> controller whose bus they are on. Similarly, platform devices are on a
> bus that should be marked as non-coherent if that is the case.
> Christoph certainly knows better how things operate here however.
usb is not a DMAable devices, you need to use the USB layer helpers
that call the DMA API on the host controller's device. platform_device
must have a device tree and the dma-noncoherent attribute somewhere in
the hierarchy.
在 2023/12/22 15:38, Maxim Kochetkov 写道:
>
>
> On 22.12.2023 17:54, Conor Dooley wrote:
>
>>> etc..) do not have this feature. These devices will use value from
>>> device_initialize(). And we have no possibility to change
>>> dma_default_coherent value by disabling ARCH_DMA_DEFAULT_COHERENT.
>>> Moreover, changing dma_default_coherent from false to true may cause
>>> regression for other devices.
>>
>> How can there be a regression when dma has been coherent by default for
>> the RISC-V kernel from day 1?
>
> Before ARCH_DMA_DEFAULT_COHERENT patch dma_default_coherent was used
> unassigned as "false" in device_initialize():
> ..........
> #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> dev->dma_coherent = dma_default_coherent;
> #endif
> ..........
> And now it becomes "true". It may change behavior of other non-DT
> drivers.
I don't see any problem here, default is default.
Actually leaving those device with dev->dma_coherent = false is risky,
because
we can't guarantee underlying cache flush functions are here.
If a non-dt device do need to override it, it should be done in
arch_setup_dma_ops.
Thanks
- Jiaxun
在 2023/12/22 15:53, Maxim Kochetkov 写道:
>
>
> On 22.12.2023 18:45, Jiaxun Yang wrote:
>>
>>
>> 在 2023/12/22 15:38, Maxim Kochetkov 写道:
>>>
>>>
>>> On 22.12.2023 17:54, Conor Dooley wrote:
>>>
>>>>> etc..) do not have this feature. These devices will use value from
>>>>> device_initialize(). And we have no possibility to change
>>>>> dma_default_coherent value by disabling ARCH_DMA_DEFAULT_COHERENT.
>>>>> Moreover, changing dma_default_coherent from false to true may cause
>>>>> regression for other devices.
>>>>
>>>> How can there be a regression when dma has been coherent by default for
>>>> the RISC-V kernel from day 1?
>>>
>>> Before ARCH_DMA_DEFAULT_COHERENT patch dma_default_coherent was used
>>> unassigned as "false" in device_initialize():
>>> ..........
>>> #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>>> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>>> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>>> dev->dma_coherent = dma_default_coherent;
>>> #endif
>>> ..........
>>> And now it becomes "true". It may change behavior of other non-DT
>>> drivers.
>> I don't see any problem here, default is default.
>> Actually leaving those device with dev->dma_coherent = false is
>> risky, because
>> we can't guarantee underlying cache flush functions are here.
>>
>> If a non-dt device do need to override it, it should be done in
>> arch_setup_dma_ops.
>
> But arch_setup_dma_ops() is called only from of_dma_configure_id() and
> acpi_dma_configure_id(). So it works only for DT and ACPI devices. What
> about platform_device?
Ah I see, that's the problem, in MIPS's use case all DMA capable devices
are following platform's default coherency. For RISC-V we assume all
device are enabled by ACPI or DT.
Perhaps you can override it in driver, but that will make drivers
platform dependent.
I'll leave this question to Christoph.
Thanks
- Jiaxun
On 22.12.2023 17:54, Conor Dooley wrote:
>> etc..) do not have this feature. These devices will use value from
>> device_initialize(). And we have no possibility to change
>> dma_default_coherent value by disabling ARCH_DMA_DEFAULT_COHERENT.
>> Moreover, changing dma_default_coherent from false to true may cause
>> regression for other devices.
>
> How can there be a regression when dma has been coherent by default for
> the RISC-V kernel from day 1?
Before ARCH_DMA_DEFAULT_COHERENT patch dma_default_coherent was used
unassigned as "false" in device_initialize():
..........
#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
dev->dma_coherent = dma_default_coherent;
#endif
..........
And now it becomes "true". It may change behavior of other non-DT drivers.
On 22.12.2023 18:45, Jiaxun Yang wrote:
>
>
> 在 2023/12/22 15:38, Maxim Kochetkov 写道:
>>
>>
>> On 22.12.2023 17:54, Conor Dooley wrote:
>>
>>>> etc..) do not have this feature. These devices will use value from
>>>> device_initialize(). And we have no possibility to change
>>>> dma_default_coherent value by disabling ARCH_DMA_DEFAULT_COHERENT.
>>>> Moreover, changing dma_default_coherent from false to true may cause
>>>> regression for other devices.
>>>
>>> How can there be a regression when dma has been coherent by default for
>>> the RISC-V kernel from day 1?
>>
>> Before ARCH_DMA_DEFAULT_COHERENT patch dma_default_coherent was used
>> unassigned as "false" in device_initialize():
>> ..........
>> #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>> dev->dma_coherent = dma_default_coherent;
>> #endif
>> ..........
>> And now it becomes "true". It may change behavior of other non-DT
>> drivers.
> I don't see any problem here, default is default.
> Actually leaving those device with dev->dma_coherent = false is risky,
> because
> we can't guarantee underlying cache flush functions are here.
>
> If a non-dt device do need to override it, it should be done in
> arch_setup_dma_ops.
But arch_setup_dma_ops() is called only from of_dma_configure_id() and
acpi_dma_configure_id(). So it works only for DT and ACPI devices. What
about platform_device?
On Fri, Dec 22, 2023 at 04:01:43PM +0000, Jiaxun Yang wrote:
>>
>> But arch_setup_dma_ops() is called only from of_dma_configure_id() and
>> acpi_dma_configure_id(). So it works only for DT and ACPI devices. What
>> about platform_device?
>
> Ah I see, that's the problem, in MIPS's use case all DMA capable devices
> are following platform's default coherency. For RISC-V we assume all device
> are enabled by ACPI or DT.
>
> Perhaps you can override it in driver, but that will make drivers platform
> dependent.
>
> I'll leave this question to Christoph.
I've already said it. You must not have DMA capable devices that aren't
declared in ACPI or OF, just like on any modern Linux platform.
What devices are you concerned about anyway Maxim?
On 23.12.2023 07:59, Christoph Hellwig wrote:
> On Fri, Dec 22, 2023 at 04:01:43PM +0000, Jiaxun Yang wrote:
>>>
>>> But arch_setup_dma_ops() is called only from of_dma_configure_id() and
>>> acpi_dma_configure_id(). So it works only for DT and ACPI devices. What
>>> about platform_device?
>>
>> Ah I see, that's the problem, in MIPS's use case all DMA capable devices
>> are following platform's default coherency. For RISC-V we assume all device
>> are enabled by ACPI or DT.
>>
>> Perhaps you can override it in driver, but that will make drivers platform
>> dependent.
>>
>> I'll leave this question to Christoph.
>
> I've already said it. You must not have DMA capable devices that aren't
> declared in ACPI or OF, just like on any modern Linux platform.
Ok. I've got the point. Thank you for clarification.
>
> What devices are you concerned about anyway Maxim?
I have 3rd party external out of tree camera driver. csi, isp, dewarp
components are OF, but common media device is created as
platform_device. I will convert is to OF.