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

[Baoquan He]

On 11/19/24 at 11:55am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >  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.

Totally agree.

> 
> 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.

OK, will do like this if Tom doesn't oppose it.

During v1, Tom suggested squashing the helper introducing patch and
helper using patch into one patch. I personally prefer splitting code
changes into multiple independent units so that reviewing is focused on
the controversial change, but not the whole big patch including many
controversial points is posted again and again. 

Thanks a lot for careful reviewing and great suggestions.