Re: [PATCH v2 1/2] x86/ioremap: introduce helper to implement xxx_is_setup_data()

[Ingo Molnar <mingo@kernel.org>]

* Baoquan He <bhe@redhat.com> wrote:

> On 11/18/24 at 09:19am, Tom Lendacky wrote:
> > On 11/17/24 19:08, Baoquan He wrote:
> > > Functions memremap_is_setup_data() and early_memremap_is_setup_data()
> > > share completely the same process and handling, except of the
> > > different memremap/unmap invocations.
> > > 
> > > So add helper __memremap_is_setup_data() to extract the common part,
> > > parameter 'early' is used to decide what kind of memremap/unmap
> > > APIs are called. This simplifies codes a lot by removing the duplicated
> > > codes, and also removes the similar code comment above them.
> > > 
> > > And '__ref' is added to __memremap_is_setup_data() to suppress below
> > > section mismatch warning:
> > > 
> > > ARNING: modpost: vmlinux: section mismatch in reference: __memremap_is_setup_data+0x5f (section: .text) ->
> > > early_memunmap (section: .init.text)
> > > 
> > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > ---
> > >  arch/x86/mm/ioremap.c | 108 +++++++++++++++---------------------------
> > >  1 file changed, 38 insertions(+), 70 deletions(-)
> > > 
> > > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> > > index 8d29163568a7..68d78e2b1203 100644
> > > --- a/arch/x86/mm/ioremap.c
> > > +++ b/arch/x86/mm/ioremap.c
> > > @@ -628,12 +628,13 @@ static bool memremap_is_efi_data(resource_size_t phys_addr,
> > >  	return false;
> > >  }
> > >  
> > > +#define SD_SIZE sizeof(struct setup_data)
> > 
> > Nit, I still think you should use "sizeof(*data)" in the code instead of
> > creating a #define.
> 
> Thanks for reviewing, Tom.
> 
> Boris suggested this. Both is fine to me. If there is indeed a tiny
> preference, I would choose SD_SIZE. It's going a bit far, but not too
> far.

Yeah, I'd prefer Boris's SD_SIZE suggestion too: while *normally* we'd 
use the 'sizeof(*data)' pattern, this particular size repeats a number 
of times and not all contexts are obvious - so abstracting it out into 
a trivial define looks like the proper cleanup.

Maybe such material changes should be done in a separate patch though:

   x86/ioremap: Introduce helper to implement xxx_is_setup_data()
   x86/ioremap: Clean up size calculations in xxx_is_setup_data()

... or so, where the first patch is a trivial refactoring that keeps 
the existing patterns - which would make the series easier to review.

Thanks,

	Ingo