[PATCH V7 0/2] bugfix based on csky linux-next

[PATCH V7 0/2] bugfix based on csky linux-next

[Guo Ren]

Because I've sent to Stephen Roth with the linux-4.19-rc3 git-tree for
linux-next, I shouldn't rebase any more.

 - Fixup smp.c irq mapping problem found by Marc Zyngier
 - Fixup dma_sync_for_cpu/device in dma-mapping.c

Guo Ren (2):
  csky/dma: bugfix dma_sync_for_cpu/device
  csky: remove irq_mapping from smp.c.

 arch/csky/include/asm/smp.h |  4 ++++
 arch/csky/kernel/smp.c      | 18 +++++++++++++-----
 arch/csky/mm/dma-mapping.c  |  5 +++--
 3 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH V7 1/2] csky/dma: bugfix dma_sync_for_cpu/device

[Guo Ren]

ref: https://lkml.org/lkml/2018/5/18/1068

        map             for_cpu         for_device      unmap
TO_DEV  writeback       none            writeback       none
TO_CPU  invalidate      invalidate*     invalidate      invalidate*
BIDIR   writeback       invalidate      writeback       invalidate

Signed-off-by: Guo Ren <ren_guo@c-sky.com>
---
 arch/csky/mm/dma-mapping.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/csky/mm/dma-mapping.c b/arch/csky/mm/dma-mapping.c
index 16c2087..30a2041 100644
--- a/arch/csky/mm/dma-mapping.c
+++ b/arch/csky/mm/dma-mapping.c
@@ -217,7 +217,8 @@ void arch_sync_dma_for_device(struct device *dev, phys_addr_t paddr,
 		break;
 	case DMA_FROM_DEVICE:
 	case DMA_BIDIRECTIONAL:
-		BUG();
+		dma_wbinv_range(vaddr + offset, vaddr + offset + size);
+		break;
 	default:
 		BUG();
 	}
@@ -240,7 +241,7 @@ void arch_sync_dma_for_cpu(struct device *dev, phys_addr_t paddr,
 
 	switch (dir) {
 	case DMA_TO_DEVICE:
-		BUG();
+		break;
 	case DMA_FROM_DEVICE:
 	case DMA_BIDIRECTIONAL:
 		dma_wbinv_range(vaddr + offset, vaddr + offset + size);
-- 
2.7.4

[PATCH V7 2/2] csky: remove irq_mapping from smp.c.

[Guo Ren]

 - remove irq_mapping from smp.c to irq-driver
 - Add set_ipi_irq_mapping api to irq-driver
 - update asm/smp.h

Signed-off-by: Guo Ren <ren_guo@c-sky.com>
---
 arch/csky/include/asm/smp.h |  4 ++++
 arch/csky/kernel/smp.c      | 18 +++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/arch/csky/include/asm/smp.h b/arch/csky/include/asm/smp.h
index 9a53abf..fed3a5a 100644
--- a/arch/csky/include/asm/smp.h
+++ b/arch/csky/include/asm/smp.h
@@ -7,6 +7,8 @@
 
 #ifdef CONFIG_SMP
 
+#define IPI_IRQ	15
+
 void __init setup_smp(void);
 
 void __init setup_smp_ipi(void);
@@ -19,6 +21,8 @@ void arch_send_call_function_single_ipi(int cpu);
 
 void __init set_send_ipi(void (*func)(const unsigned long *, unsigned long));
 
+void __init set_ipi_irq_mapping(int (*func)(void));
+
 #define raw_smp_processor_id()	(current_thread_info()->cpu)
 
 #endif /* CONFIG_SMP */
diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
index 522c73f..f8343f6 100644
--- a/arch/csky/kernel/smp.c
+++ b/arch/csky/kernel/smp.c
@@ -20,8 +20,6 @@
 #include <asm/mmu_context.h>
 #include <asm/pgalloc.h>
 
-#define IPI_IRQ	15
-
 static struct {
 	unsigned long bits ____cacheline_aligned;
 } ipi_data[NR_CPUS] __cacheline_aligned;
@@ -121,13 +119,23 @@ void __init enable_smp_ipi(void)
 	enable_percpu_irq(IPI_IRQ, 0);
 }
 
+static int (*arch_ipi_irq_mapping)(void) = NULL;
+
+void __init set_ipi_irq_mapping(int (*func)(void))
+{
+	if (arch_ipi_irq_mapping)
+		return;
+
+	arch_ipi_irq_mapping = func;
+}
+
 void __init setup_smp_ipi(void)
 {
-	int rc;
+	int rc, irq;
 
-	irq_create_mapping(NULL, IPI_IRQ);
+	irq = arch_ipi_irq_mapping();
 
-	rc = request_percpu_irq(IPI_IRQ, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
+	rc = request_percpu_irq(irq, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
 	if (rc)
 		panic("%s IRQ request failed\n", __func__);
 
-- 
2.7.4

Re: [PATCH V7 2/2] csky: remove irq_mapping from smp.c.

[Arnd Bergmann]

On Tue, Sep 18, 2018 at 3:48 PM Guo Ren <ren_guo@c-sky.com> wrote:
>
>  - remove irq_mapping from smp.c to irq-driver
>  - Add set_ipi_irq_mapping api to irq-driver
>  - update asm/smp.h
>
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>

The patch seems ok to me, but please try to improve the changelog comments.
Generally speaking, use full English sentences instead of an enumerated list,
and explain what the change is for rather than what you are doing. The text
you have here can easily be derived from looking at the code changes,
but I still don't understand what caused you to change it, and why this
is better than the previous version.

       Arnd

Re: [PATCH V7 1/2] csky/dma: bugfix dma_sync_for_cpu/device

[Arnd Bergmann]

On Tue, Sep 18, 2018 at 3:48 PM Guo Ren <ren_guo@c-sky.com> wrote:
>
> ref: https://lkml.org/lkml/2018/5/18/1068
>
>         map             for_cpu         for_device      unmap
> TO_DEV  writeback       none            writeback       none
> TO_CPU  invalidate      invalidate*     invalidate      invalidate*
> BIDIR   writeback       invalidate      writeback       invalidate
>
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>

Same comment as for the other patch: Explain why the original
version is wrong first. When giving a reference to some other
discussion, use the "Link" tag above your Signed-off-by line.
To point to a discussion on lkml, lore.kernel.org is the
recommended archive, so it would become

Link: https://lore.kernel.org/lkml/20180518215548.GH17671@n2100.armlinux.org.uk/

       Arnd

Re: [PATCH V7 2/2] csky: remove irq_mapping from smp.c.

[Guo Ren]

On Mon, Sep 24, 2018 at 10:23:14PM +0200, Arnd Bergmann wrote:
> On Tue, Sep 18, 2018 at 3:48 PM Guo Ren <ren_guo@c-sky.com> wrote:
> >
> >  - remove irq_mapping from smp.c to irq-driver
> >  - Add set_ipi_irq_mapping api to irq-driver
> >  - update asm/smp.h
> >
> > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> 
> The patch seems ok to me, but please try to improve the changelog comments.
> Generally speaking, use full English sentences instead of an enumerated list,
> and explain what the change is for rather than what you are doing. The text
Ok, I'll improve it.
> you have here can easily be derived from looking at the code changes,
> but I still don't understand what caused you to change it, and why this
> is better than the previous version.
The changelog comment will change to:

There are some feedbacks from irqchip, and we need to adjust "smp.c & smp.h"
to match the csky_mptimer modification.
 - Move IPI_IRQ define into drivers/irqchip/csky_mpintc.c, because it's a
   interrupt controller specific.
 - Bugfix request_irq with IPI_IRQ, we must use irq_mapping return value not
   directly use IPI_IRQ. The modification also involves csky_mpintc.

Best Regards
 Guo Ren

Re: [PATCH V7 1/2] csky/dma: bugfix dma_sync_for_cpu/device

[Guo Ren]

On Mon, Sep 24, 2018 at 10:38:04PM +0200, Arnd Bergmann wrote:
> On Tue, Sep 18, 2018 at 3:48 PM Guo Ren <ren_guo@c-sky.com> wrote:
> >
> > ref: https://lkml.org/lkml/2018/5/18/1068
> >
> >         map             for_cpu         for_device      unmap
> > TO_DEV  writeback       none            writeback       none
> > TO_CPU  invalidate      invalidate*     invalidate      invalidate*
> > BIDIR   writeback       invalidate      writeback       invalidate
> >
> > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> 
> Same comment as for the other patch: Explain why the original
> version is wrong first. When giving a reference to some other
> discussion, use the "Link" tag above your Signed-off-by line.
> To point to a discussion on lkml, lore.kernel.org is the
> recommended archive, so it would become
> 
> Link: https://lore.kernel.org/lkml/20180518215548.GH17671@n2100.armlinux.org.uk/
Ok, I'll improve the comment:

Fixup dma_mapping error in linux-4.19-rc3, and we must implement all
DMA_TO_DEVICE/FROM_DEVICE/BIDIRECTIONAL for both sync_dma_for_device/cpu.
The implementation of arch should follow the following rules:

         map             for_cpu         for_device      unmap
 TO_DEV  writeback       none            writeback       none
 TO_CPU  invalidate      invalidate*     invalidate      invalidate*
 BIDIR   writeback       invalidate      writeback       invalidate

 Link: https://lore.kernel.org/lkml/20180518215548.GH17671@n2100.armlinux.org.uk/
 Signed-off-by: Guo Ren <ren_guo@c-sky.com>

Best Regards
 Guo Ren

Re: [PATCH V7 1/2] csky/dma: bugfix dma_sync_for_cpu/device

[Arnd Bergmann]

On Tue, Sep 25, 2018 at 11:46 AM Guo Ren <ren_guo@c-sky.com> wrote:
>
> On Mon, Sep 24, 2018 at 10:38:04PM +0200, Arnd Bergmann wrote:
> > On Tue, Sep 18, 2018 at 3:48 PM Guo Ren <ren_guo@c-sky.com> wrote:
> > >
> > > ref: https://lkml.org/lkml/2018/5/18/1068
> > >
> > >         map             for_cpu         for_device      unmap
> > > TO_DEV  writeback       none            writeback       none
> > > TO_CPU  invalidate      invalidate*     invalidate      invalidate*
> > > BIDIR   writeback       invalidate      writeback       invalidate
> > >
> > > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> >
> > Same comment as for the other patch: Explain why the original
> > version is wrong first. When giving a reference to some other
> > discussion, use the "Link" tag above your Signed-off-by line.
> > To point to a discussion on lkml, lore.kernel.org is the
> > recommended archive, so it would become
> >
> > Link: https://lore.kernel.org/lkml/20180518215548.GH17671@n2100.armlinux.org.uk/
> Ok, I'll improve the comment:
>
> Fixup dma_mapping error in linux-4.19-rc3, and we must implement all
> DMA_TO_DEVICE/FROM_DEVICE/BIDIRECTIONAL for both sync_dma_for_device/cpu.
> The implementation of arch should follow the following rules:
>
> ...

That seems ok, but it's better to start with a description of the problem
rather than the 'Fixup dma_mapping error in linux-4.19-rc3' part.
I would write this like:

| The arch_sync_dma_for_cpu()/arch_sync_dma_for_device()
| implementation is broken for some combinations that end up
| in a BUG() instead of performing the necessary flushes.
|
| The implementation of arch should follow the following rules:
| ...

The imperative 'Fix up dma_mapping error ...' is what belongs
into the subject line, and is ideally revisited at the end of the
changelog comment if you want to add more detail about what
you do.

       Arnd

Re: [PATCH V7 1/2] csky/dma: bugfix dma_sync_for_cpu/device

[Guo Ren]

On Tue, Sep 25, 2018 at 11:59:48AM +0200, Arnd Bergmann wrote:
> On Tue, Sep 25, 2018 at 11:46 AM Guo Ren <ren_guo@c-sky.com> wrote:
> >
> > On Mon, Sep 24, 2018 at 10:38:04PM +0200, Arnd Bergmann wrote:
> > > On Tue, Sep 18, 2018 at 3:48 PM Guo Ren <ren_guo@c-sky.com> wrote:
> > > >
> > > > ref: https://lkml.org/lkml/2018/5/18/1068
> > > >
> > > >         map             for_cpu         for_device      unmap
> > > > TO_DEV  writeback       none            writeback       none
> > > > TO_CPU  invalidate      invalidate*     invalidate      invalidate*
> > > > BIDIR   writeback       invalidate      writeback       invalidate
> > > >
> > > > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> > >
> > > Same comment as for the other patch: Explain why the original
> > > version is wrong first. When giving a reference to some other
> > > discussion, use the "Link" tag above your Signed-off-by line.
> > > To point to a discussion on lkml, lore.kernel.org is the
> > > recommended archive, so it would become
> > >
> > > Link: https://lore.kernel.org/lkml/20180518215548.GH17671@n2100.armlinux.org.uk/
> > Ok, I'll improve the comment:
> >
> > Fixup dma_mapping error in linux-4.19-rc3, and we must implement all
> > DMA_TO_DEVICE/FROM_DEVICE/BIDIRECTIONAL for both sync_dma_for_device/cpu.
> > The implementation of arch should follow the following rules:
> >
> > ...
> 
> That seems ok, but it's better to start with a description of the problem
> rather than the 'Fixup dma_mapping error in linux-4.19-rc3' part.
> I would write this like:
> 
> | The arch_sync_dma_for_cpu()/arch_sync_dma_for_device()
> | implementation is broken for some combinations that end up
> | in a BUG() instead of performing the necessary flushes.
> |
> | The implementation of arch should follow the following rules:
> | ...
> 
> The imperative 'Fix up dma_mapping error ...' is what belongs
> into the subject line, and is ideally revisited at the end of the
> changelog comment if you want to add more detail about what
> you do.
Nice tips, Thx. I'll continue to improve my comments.

Best Regards
 Guo Ren