Re: [PATCH v7 1/6] riscv: mm: dma-noncoherent: Switch using function pointers for cache management

[Geert Uytterhoeven]

Hi Prabhakar,

Thanks for your patch!

On Thu, Mar 30, 2023 at 10:42 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Currently, selecting which CMOs to use on a given platform is done using
> and ALTERNATIVE_X() macro. This was manageable when there were just two

the ALTERNATIVE_X()

> CMO implementations, but now that there are more and more platforms coming
> needing custom CMOs, the use of the ALTERNATIVE_X() macro is unmanageable.
>
> To avoid such issues this patch switches to use of function pointers

"the use" or "using"

> instead of ALTERNATIVE_X() macro for cache management (the only drawback

the ALTERNATIVE_X()

> being performance over the previous approach).
>
> void (*clean_range)(unsigned long addr, unsigned long size);
> void (*inv_range)(unsigned long addr, unsigned long size);
> void (*flush_range)(unsigned long addr, unsigned long size);
>
> The above function pointers are provided to be overridden for platforms
> needing CMO.
>
> Convert ZICBOM and T-HEAD CMO to use function pointers.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

> --- a/arch/riscv/errata/thead/errata.c
> +++ b/arch/riscv/errata/thead/errata.c

> +#ifdef CONFIG_ERRATA_THEAD_CMO

> +static void thead_register_cmo_ops(void)
> +{
> +       riscv_noncoherent_register_cache_ops(&thead_cmo_ops);
> +}
> +#else
> +static void thead_register_cmo_ops(void) {}
> +#endif

> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c

> @@ -75,3 +83,12 @@ void riscv_noncoherent_supported(void)
>              "Non-coherent DMA support enabled without a block size\n");
>         noncoherent_supported = true;
>  }
> +
> +void riscv_noncoherent_register_cache_ops(const struct riscv_cache_ops *ops)
> +{
> +       if (!ops)
> +               return;

This is never true.
I guess originally you wanted to call riscv_noncoherent_register_cache_ops()
unconditionally from common code, instead of the various *register_cmo_ops()?
But that would have required something like

#ifdef CONFIG_ERRATA_THEAD_CMO
#define THEAD_CMO_OPS_PTR   (&thead_cmo_ops)
#else
#define THEAD_CMO_OPS_PTR   NULL
#endif

Or can we come up with some macro like pm_ptr(), but that also takes
care of the "&", so we can do "#define thead_cmo_ops NULL"?

> +
> +       noncoherent_cache_ops = *ops;
> +}
> +EXPORT_SYMBOL_GPL(riscv_noncoherent_register_cache_ops);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds