| Message ID | 20231221185152.327231-1-fido_max@inbox.ru (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/1] riscv: set ARCH_DMA_DEFAULT_COHERENT if RISCV_DMA_NONCOHERENT is not set | expand |
+ 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 <fido_max@inbox.ru> > --- > 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 > linux-riscv@lists.infradead.org > 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 <fido_max@inbox.ru> >> --- >> 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 >> linux-riscv@lists.infradead.org >> 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 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, 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. 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?
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.
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.
在 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
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?
在 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 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.
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
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 <fido_max@inbox.ru> --- arch/riscv/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)