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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx/
> 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