| Message ID | 20230221124613.2859-1-jiaxun.yang@flygoat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | MIPS DMA coherence fixes | expand |
Can you explain the motivation here? Also why riscv patches are at the end of a mips fіxes series?
> 2023年2月21日 17:54，Christoph Hellwig <hch@lst.de> 写道： > > Can you explain the motivation here? Also why riscv patches are at > the end of a mips fіxes series? Ah sorry for any confusion. So the main purpose of this patch is to fix MIPS’s broken per-device coherency. To be more precise, we want to be able to control the default coherency for all devices probed from devicetree in early boot code. To achieve that I decided to reuse dma_default_coherent to set default coherency for devicetree. And all later patches are severing for this purpose. Thanks - Jiaxun
On 2023-02-21 18:15, Jiaxun Yang wrote: > > >> 2023年2月21日 17:54，Christoph Hellwig <hch@lst.de> 写道： >> >> Can you explain the motivation here? Also why riscv patches are at >> the end of a mips fіxes series? > > Ah sorry for any confusion. > > So the main purpose of this patch is to fix MIPS’s broken per-device coherency. > To be more precise, we want to be able to control the default coherency for all devices probed from > devicetree in early boot code. Including the patch which actually does that would be helpful. As it is, patches 4-7 here just appear to be moving an option around for no practical effect. Robin. > To achieve that I decided to reuse dma_default_coherent to set default coherency for devicetree. > And all later patches are severing for this purpose. > > Thanks > - Jiaxun
> 2023年2月21日 19:46，Robin Murphy <robin.murphy@arm.com> 写道： > > On 2023-02-21 18:15, Jiaxun Yang wrote: >>> 2023年2月21日 17:54，Christoph Hellwig <hch@lst.de> 写道： >>> >>> Can you explain the motivation here? Also why riscv patches are at >>> the end of a mips fіxes series? >> Ah sorry for any confusion. >> So the main purpose of this patch is to fix MIPS’s broken per-device coherency. >> To be more precise, we want to be able to control the default coherency for all devices probed from >> devicetree in early boot code. > > Including the patch which actually does that would be helpful. As it is, patches 4-7 here just appear to be moving an option around for no practical effect. Well the affect is default coherency of devicetree probed devices are now following dma_default_coherent instead of a static Kconfig option. For MIPS platform, dma_default_coherent will be determined by boot code. Thanks - Jiaxun
On 2023-02-21 19:55, Jiaxun Yang wrote: > > >> 2023年2月21日 19:46，Robin Murphy <robin.murphy@arm.com> 写道： >> >> On 2023-02-21 18:15, Jiaxun Yang wrote: >>>> 2023年2月21日 17:54，Christoph Hellwig <hch@lst.de> 写道： >>>> >>>> Can you explain the motivation here? Also why riscv patches are at >>>> the end of a mips fіxes series? >>> Ah sorry for any confusion. >>> So the main purpose of this patch is to fix MIPS’s broken per-device coherency. >>> To be more precise, we want to be able to control the default coherency for all devices probed from >>> devicetree in early boot code. >> >> Including the patch which actually does that would be helpful. As it is, patches 4-7 here just appear to be moving an option around for no practical effect. > > Well the affect is default coherency of devicetree probed devices are now following dma_default_coherent > instead of a static Kconfig option. For MIPS platform, dma_default_coherent will be determined by boot code. "Will be" is the issue I'm getting at. We can't review some future promise of a patch, we can only review actual patches. And it's hard to meaningfully review preparatory patches for some change without the full context of that change. Thanks, Robin.
> 2023年2月22日 12:55，Robin Murphy <robin.murphy@arm.com> 写道： > > On 2023-02-21 19:55, Jiaxun Yang wrote: >>> 2023年2月21日 19:46，Robin Murphy <robin.murphy@arm.com> 写道： >>> >>> On 2023-02-21 18:15, Jiaxun Yang wrote: >>>>> 2023年2月21日 17:54，Christoph Hellwig <hch@lst.de> 写道： >>>>> >>>>> Can you explain the motivation here? Also why riscv patches are at >>>>> the end of a mips fіxes series? >>>> Ah sorry for any confusion. >>>> So the main purpose of this patch is to fix MIPS’s broken per-device coherency. >>>> To be more precise, we want to be able to control the default coherency for all devices probed from >>>> devicetree in early boot code. >>> >>> Including the patch which actually does that would be helpful. As it is, patches 4-7 here just appear to be moving an option around for no practical effect. >> Well the affect is default coherency of devicetree probed devices are now following dma_default_coherent >> instead of a static Kconfig option. For MIPS platform, dma_default_coherent will be determined by boot code. > > "Will be" is the issue I'm getting at. We can't review some future promise of a patch, we can only review actual patches. And it's hard to meaningfully review preparatory patches for some change without the full context of that change. Actually this already present in current MIPS platform code. arch/mips/mti-malta is setting dma_default_coherent on boot, and it’s devicetree does not explicitly specify coherency. Thanks - Jiaxun > > Thanks, > Robin.
On 2023-02-22 13:04, Jiaxun Yang wrote: > > >> 2023年2月22日 12:55，Robin Murphy <robin.murphy@arm.com> 写道： >> >> On 2023-02-21 19:55, Jiaxun Yang wrote: >>>> 2023年2月21日 19:46，Robin Murphy <robin.murphy@arm.com> 写道： >>>> >>>> On 2023-02-21 18:15, Jiaxun Yang wrote: >>>>>> 2023年2月21日 17:54，Christoph Hellwig <hch@lst.de> 写道： >>>>>> >>>>>> Can you explain the motivation here? Also why riscv patches are at >>>>>> the end of a mips fіxes series? >>>>> Ah sorry for any confusion. >>>>> So the main purpose of this patch is to fix MIPS’s broken per-device coherency. >>>>> To be more precise, we want to be able to control the default coherency for all devices probed from >>>>> devicetree in early boot code. >>>> >>>> Including the patch which actually does that would be helpful. As it is, patches 4-7 here just appear to be moving an option around for no practical effect. >>> Well the affect is default coherency of devicetree probed devices are now following dma_default_coherent >>> instead of a static Kconfig option. For MIPS platform, dma_default_coherent will be determined by boot code. >> >> "Will be" is the issue I'm getting at. We can't review some future promise of a patch, we can only review actual patches. And it's hard to meaningfully review preparatory patches for some change without the full context of that change. > > Actually this already present in current MIPS platform code. > > arch/mips/mti-malta is setting dma_default_coherent on boot, and it’s devicetree does not explicitly specify coherency. OK, this really needs to be explained much more clearly. I read this series as 3 actual fix patches, then 3 patches adding a new option to replace an existing one on the grounds that it "can be useful" for unspecified purposes, then a final cleanup patch removing the old option that has now been superseded. Going back and looking closely I see there is actually a brief mention in the cleanup patch that it also happens to fix some issue, but even then it doesn't clearly explain what the issue really is or how and why the fix works and is appropriate. Ideally, functional fixes and cleanup should be in distinct patches whenever that is reasonable. Sometimes the best fix is inherently a cleanup, but in such cases the patch should always be presented as the fix being its primary purpose. Please also use the cover letter to give reviewers an overview of the whole series if it's not merely a set of loosely-related patches that just happened to be convenient so send all together. I think I do at least now understand the underlying problem well enough to have a think about whether this is the best way to address it. Thanks, Robin.
Jiaxun Yang (7): MIPS: Remove DMA_PERDEV_COHERENT MIPS: Always select ARCH_HAS_SYNC_DMA_FOR_CPU for noncoherent platforms MIPS: c-r4k: Always install dma flush functions dma-mapping: Always provide dma_default_coherent dma-mapping: Provide CONFIG_ARCH_DMA_DEFAULT_COHERENT riscv: Select ARCH_DMA_DEFAULT_COHERENT of: address: Use dma_default_coherent to determine default coherency arch/mips/Kconfig | 16 ++-------------- arch/mips/mm/c-r4k.c | 12 +++--------- arch/powerpc/Kconfig | 1 - arch/riscv/Kconfig | 2 +- drivers/of/Kconfig | 4 ---- drivers/of/address.c | 2 +- include/linux/dma-map-ops.h | 1 + kernel/dma/Kconfig | 3 +++ kernel/dma/mapping.c | 6 +++++- 9 files changed, 16 insertions(+), 31 deletions(-)