Re: [PATCH] drm/i915/display: Remove kstrdup_const() and kfree_const() usage

[Christophe JAILLET]

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.


[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;
> >   }