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

From: Tom Lendacky
Date: Wed Nov 20 2024 - 09:23:32 EST


On 11/20/24 02:25, Ingo Molnar wrote:
>
> * Tom Lendacky <thomas.lendacky@xxxxxxx> wrote:
>
>>> /*
>>> * Examine the physical address to determine if it is boot data by checking
>>> * it against the boot params setup_data chain.
>>> */
>>> -static bool memremap_is_setup_data(resource_size_t phys_addr,
>>> - unsigned long size)
>>> +static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
>>
>> Oh, I see why the __ref is needed now, because this calls an __init
>> function based on the early bool.
>>
>> While this nicely consolidates the checking, I'll let the x86
>> maintainers decide whether they like that an __init function is calling
>> a non __init function.
>
> So why would it be a problem? Only non-__init calling __init is a bug,
> because __init functions cease to exist after early bootup. Also,
> calling certain kernel subsystems too early, before they are
> initialized, is a bug as well.

I brought it up because that is what could happen if the wrong boolean
value is supplied to the helper function. The helper function is marked
non-__init but calls a __init function if the boolean value is true, hence
the need for the __ref tagging.

But, I don't anticipate that this helper will be called by anything else
than what is currently calling it and the proper boolean values are set on
those calls.

I just wanted to raise awareness. I'm ok with using __ref, just wanted to
make sure everyone else is, too.

Thanks,
Tom

>
> But calling non-__init functions that have initialized already is like
> totally normal: printk() for example, but also all locking facilities,
> etc.
>
> Am I missing anything here?
>
> Thanks,
>
> Ingo