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