On Fri, 04 Oct 2024, Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
> Le 04/10/2024 à 11:35, Jani Nikula a écrit :
>> On Thu, 03 Oct 2024, Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
>>> kstrdup_const() and kfree_const() can be confusing in code built as a
>>> module. In such a case, it does not do what one could expect from the name
>>> of the functions.
>>>
>>> The code is not wrong by itself, but in such a case, it is equivalent to
>>> kstrdup() and kfree().
>>>
>>> So, keep thinks simple and straightforward.
>>>
>>> This reverts commit 379b63e7e682 ("drm/i915/display: Save a few bytes of
>>> memory in intel_backlight_device_register()")
>>
>> Sorry, I guess I'm confused here. Or I just didn't read the commit
>> message to [1] properly. Or both.
>>
>> So the whole point of [1] was that the _const versions can be confusing
>> if i915 is builtin? But not wrong?
>
> I'll try to explain the whole story and (try to) be clearer.
Thanks for the thorough explanations, pushed to drm-intel-next.
BR,
Jani.
>
>
> [2] the intent of this initial patch was a micro-optimization which was
> expected to save a few bytes of memory. The naming of the function
> looked promising. However kstrdup_const() only saves the allocation
> within the rodata section of the kernel [5,6]. The mechanism does not
> work for code built as module.
>
> This patch *is not* broken by itself, it is just pointless most of the
> time. So keeping it as-is is just fine, from my point of view.
>
> If built as a module, kstrdup_const() is just a plain kstrdup() and
> kfree_const() is just kfree().
>
>
>
> [3] was a variation that tried to avoid the allocation in all cases,
> should it be built as a module or not.
> Being a micro-optimization of a slow path, your argument of keeping
> things simple is just fine for me.
>
>
>
> [4] just revert [2].
> [2] was not broken, so [4] does not fix anything. It just makes things
> simpler and as before.
>
>
> So the whole point of [1,3] was that the _const versions can be
> confusing if i915 is *NOT* builtin.
> But it *is* not wrong, just likely useless in such a case.
>
> So, from my point of view, keeping [2] as is, or applying [3] or [4] on
> top of it does not change things much, and each solution is correct.
>
>
>
> The idea behind removing some usage of _const() function in modules is
> related to the patch proposal [7] and more precisely the response of
> Christoph Hellwig [8]. The patch [7] will not be applied because it
> breaks things.
> So, should this API be removed one day, or at least removed for modules,
> the more preparation work is already done (up to now: 4,9,10] the better
> it is.
>
> CJ
>
>
>
> [2]: 379b63e7e682 ("drm/i915/display: Save a few bytes of memory in
> intel_backlight_device_register()")
>
> [3]:
> https://lore.kernel.org/all/3b3d3af8739e3016f3f80df0aa85b3c06230a385.1727533674.git.christophe.jaillet@xxxxxxxxxx/
>
> [4]:
> https://lore.kernel.org/all/f82be2ee3ac7d18dd9982b5368a88a5bf2aeb777.1727977199.git.christophe.jaillet@xxxxxxxxxx/
>
> [5]: https://elixir.bootlin.com/linux/v6.12-rc1/source/mm/util.c#L84
> [6]:
> https://elixir.bootlin.com/linux/v6.12-rc1/source/include/asm-generic/sections.h#L177
>
> [7]:
> https://lore.kernel.org/all/20240924050937.697118-1-senozhatsky@xxxxxxxxxxxx/
> [8]: https://lore.kernel.org/all/ZvJfhDrv-eArtU8Y@xxxxxxxxxxxxx/
>
> [9]:
> https://lore.kernel.org/all/63ac20f64234b7c9ea87a7fa9baf41e8255852f7.1727374631.git.christophe.jaillet@xxxxxxxxxx/
> [10]:
> https://lore.kernel.org/all/06630f9ec3e153d0e7773b8d97a17e7c53e0d606.1727375615.git.christophe.jaillet@xxxxxxxxxx/
>
>>
>> BR,
>> Jani.
>>
>>
>> [1] https://lore.kernel.org/r/3b3d3af8739e3016f3f80df0aa85b3c06230a385.1727533674.git.christophe.jaillet@xxxxxxxxxx
>>
>>
>>
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_backlight.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
>>> index 9e05745d797d..3f81a726cc7d 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
>>> @@ -949,7 +949,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>>> else
>>> props.power = BACKLIGHT_POWER_OFF;
>>>
>>> - name = kstrdup_const("intel_backlight", GFP_KERNEL);
>>> + name = kstrdup("intel_backlight", GFP_KERNEL);
>>> if (!name)
>>> return -ENOMEM;
>>>
>>> @@ -963,7 +963,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>>> * compatibility. Use unique names for subsequent backlight devices as a
>>> * fallback when the default name already exists.
>>> */
>>> - kfree_const(name);
>>> + kfree(name);
>>> name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
>>> i915->drm.primary->index, connector->base.name);
>>> if (!name)
>>> @@ -987,7 +987,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>>> connector->base.base.id, connector->base.name, name);
>>>
>>> out:
>>> - kfree_const(name);
>>> + kfree(name);
>>>
>>> return ret;
>>> }
>>
>
--
Jani Nikula, Intel
