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!
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.
> Petr T
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