Re: arch/superh: Suspicious coherent DMA allocations for CEU video buffers

Hi Jacopo,

On Mon, 3 Jul 2023 00:32:54 +0200
Jacopo Mondi <[email protected]> wrote:

> Hi Petr,
> 
> On Thu, Jun 29, 2023 at 03:11:24PM +0200, Petr Tesařík wrote:
> > Hi Jacopo,
> >
> > I've just noticed a few calls to dma_declare_coherent_memory() which
> > look suspicious to me, all authored by you:
> >
> > - arch/sh/boards/mach-ap325rxa/setup.c
> > - arch/sh/boards/mach-ecovec24/setup.c
> > - arch/sh/boards/mach-kfr2r09/setup.c
> > - arch/sh/boards/mach-migor/setup.c
> > - arch/sh/boards/mach-se/7724/setup.c
> >
> > In these files, the last argument to dma_declare_coherent_memory()
> > looks like the last address to be used, but the expected value should
> > be the size of the reserved region, e.g.:
> >
> > 	dma_declare_coherent_memory(&ap325rxa_ceu_device.dev,
> > 			ceu_dma_membase, ceu_dma_membase,
> > 			ceu_dma_membase + CEU_BUFFER_MEMORY_SIZE - 1);
> >
> > Do you (or anyone else on Cc) agree that this is a braino, and all these
> > boards should actually use CEU_BUFFER_MEMORY_SIZE as the size of their
> > DMA pools rather than membase + CEU_BUFFER_MEMORY_SIZE - 1?
> >  
> 
> Thanks for spotting this.. I tried to track down where this comes from
> digging out the CEU and the platform file from 4.16, but it seems this
> simply is a brain fart from my side.
> 
> I presume this is very much dead code, as the commit message of
> 39fb993038e1 ("media: arch: sh: ap325rxa: Use new renesas-ceu camera
> driver") says: "Compile tested only." and nobody has ever reported
> bugs.
> 
> Feel free to send patches and cc me, as long as this code is around
> it's nice to have it correct at least.

Thank you for confirmation. I'll do that.

> Thank you!
>    j
> 
> PS: if you ask me, as nobody clearly run this code in the last 5
> years, I would simply drop all these platform files. But that's
> maintainers' call.

Since the beginning of the coherent region is declared correctly, it is
possible that it just happens to work, merely unreliably, frustrating
users with occasional freezes/crashes which may not be easily linked
with the CEU camera.

OTOH it is much more likely that this code is simply dead.

Petr T