On Tue, 2022-09-27 at 06:40 +0000, Christophe Leroy wrote:
> > + /* Flush on the EA that may be executed in case of a non-
> > coherent icache */
> > + icbi(prog_addr);
>
> prog_addr is a misleading name ? Is that the address at which you
> program it ? Is that the address the programs runs at ?
>
> exec_addr was a lot more explicit as it clearly defines the address
> at
> which the code is executed.
I'm not sure what it could be confused for other than "the address the
program uses" (be it uses for executing, or uses as data). I just
called it that because it's not necessarily executed, so 'exec_addr' is
misleading (to the extent it matters in the first place...).
> > + if (IS_ENABLED(CONFIG_PPC64) && L1_CACHE_BYTES < 64)
> > + icbi(prog_addr + size - 1);
>
> This doesn't exist today.
>
> I'd rather have:
>
> BUILD_BUG_ON(IS_ENABLED(CONFIG_PPC64) && L1_CACHE_BYTES <
> 64);
Sure, I can adjust the style.
> > +static int __always_inline __do_patch_memory(void *dest, unsigned
> > long src, size_t size)
> > {
> > int err;
> > u32 *patch_addr;
> > - unsigned long text_poke_addr;
> > pte_t *pte;
> > - unsigned long pfn = get_patch_pfn(addr);
> > -
> > - text_poke_addr = (unsigned
> > long)__this_cpu_read(text_poke_area)->addr & PAGE_MASK;
> > - patch_addr = (u32 *)(text_poke_addr +
> > offset_in_page(addr));
> > + unsigned long text_poke_addr = (unsigned
> > long)__this_cpu_read(text_poke_area)->addr & PAGE_MASK;
> > + unsigned long pfn = get_patch_pfn(dest);
> >
> > + patch_addr = (u32 *)(text_poke_addr +
> > offset_in_page(dest));
>
> Can we avoid this churn ?
> Ok, you want to change 'addr' to 'dest', can we leave everything else
> as
> is ?
'addr' was only renamed because the v1 used a pointer to the data, so
'addr' was ambiguous. I'll restore it to 'addr' for v3.
I'll also restore the formatting.
