"name" is allocated and freed in intel_backlight_device_register().
The initial allocation just duplicates "intel_backlight".

Later, if a device with this name has already been registered, another
dynamically generated one is allocated using kasprintf().

So at the end of the function, when "name" is freed, it can point either to
the initial static literal "intel_backlight" or to the kasprintf()'ed one.

So kfree_const() is used.

However, when built as a module, kstrdup_const() and kfree_const() don't
work as one would expect and are just plain kstrdup() and kfree().


Slightly change the logic and introduce a new variable to hold the
address returned by kasprintf() should it be used.

This saves a memory allocation/free and avoids these _const functions,
which names can be confusing when used with code built as module.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.

For the records, this patch is a clean-up effort related to discussions at:
  - https://lore.kernel.org/all/ZvHurCYlCoi1ZTCX@skv.local/
  - https://lore.kernel.org/all/20240924050937.697118-1-senozhatsky@chromium.org/
---
 drivers/gpu/drm/i915/display/intel_backlight.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index 9e05745d797d..bf7686aa044f 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -914,9 +914,9 @@ int intel_backlight_device_register(struct intel_connector *connector)
 {
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
+	const char *name, *new_name = NULL;
 	struct backlight_properties props;
 	struct backlight_device *bd;
-	const char *name;
 	int ret = 0;
 
 	if (WARN_ON(panel->backlight.device))
@@ -949,10 +949,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
 	else
 		props.power = BACKLIGHT_POWER_OFF;
 
-	name = kstrdup_const("intel_backlight", GFP_KERNEL);
-	if (!name)
-		return -ENOMEM;
-
+	name = "intel_backlight";
 	bd = backlight_device_get_by_name(name);
 	if (bd) {
 		put_device(&bd->dev);
@@ -963,11 +960,11 @@ 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);
-		name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
-				 i915->drm.primary->index, connector->base.name);
-		if (!name)
+		new_name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
+				     i915->drm.primary->index, connector->base.name);
+		if (!new_name)
 			return -ENOMEM;
+		name = new_name;
 	}
 	bd = backlight_device_register(name, connector->base.kdev, connector,
 				       &intel_backlight_device_ops, &props);
@@ -987,7 +984,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
 		    connector->base.base.id, connector->base.name, name);
 
 out:
-	kfree_const(name);
+	kfree(new_name);
 
 	return ret;
 }
-- 
2.46.1

On Sat, 28 Sep 2024, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> "name" is allocated and freed in intel_backlight_device_register().
> The initial allocation just duplicates "intel_backlight".
>
> Later, if a device with this name has already been registered, another
> dynamically generated one is allocated using kasprintf().
>
> So at the end of the function, when "name" is freed, it can point either to
> the initial static literal "intel_backlight" or to the kasprintf()'ed one.
>
> So kfree_const() is used.
>
> However, when built as a module, kstrdup_const() and kfree_const() don't
> work as one would expect and are just plain kstrdup() and kfree().
>
>
> Slightly change the logic and introduce a new variable to hold the
> address returned by kasprintf() should it be used.
>
> This saves a memory allocation/free and avoids these _const functions,
> which names can be confusing when used with code built as module.

Okay, I'd rather revert your earlier commit 379b63e7e682
("drm/i915/display: Save a few bytes of memory in
intel_backlight_device_register()") than add this.

The code simplicity is much more important than saving a few bytes.

BR,
Jani.



>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only.
>
> For the records, this patch is a clean-up effort related to discussions at:
>   - https://lore.kernel.org/all/ZvHurCYlCoi1ZTCX@skv.local/
>   - https://lore.kernel.org/all/20240924050937.697118-1-senozhatsky@chromium.org/
> ---
>  drivers/gpu/drm/i915/display/intel_backlight.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 9e05745d797d..bf7686aa044f 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -914,9 +914,9 @@ int intel_backlight_device_register(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
> +	const char *name, *new_name = NULL;
>  	struct backlight_properties props;
>  	struct backlight_device *bd;
> -	const char *name;
>  	int ret = 0;
>  
>  	if (WARN_ON(panel->backlight.device))
> @@ -949,10 +949,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>  	else
>  		props.power = BACKLIGHT_POWER_OFF;
>  
> -	name = kstrdup_const("intel_backlight", GFP_KERNEL);
> -	if (!name)
> -		return -ENOMEM;
> -
> +	name = "intel_backlight";
>  	bd = backlight_device_get_by_name(name);
>  	if (bd) {
>  		put_device(&bd->dev);
> @@ -963,11 +960,11 @@ 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);
> -		name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
> -				 i915->drm.primary->index, connector->base.name);
> -		if (!name)
> +		new_name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
> +				     i915->drm.primary->index, connector->base.name);
> +		if (!new_name)
>  			return -ENOMEM;
> +		name = new_name;
>  	}
>  	bd = backlight_device_register(name, connector->base.kdev, connector,
>  				       &intel_backlight_device_ops, &props);
> @@ -987,7 +984,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>  		    connector->base.base.id, connector->base.name, name);
>  
>  out:
> -	kfree_const(name);
> +	kfree(new_name);
>  
>  	return ret;
>  }

-- 
Jani Nikula, Intel

Le 30/09/2024 à 09:48, Jani Nikula a écrit :
> On Sat, 28 Sep 2024, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>> "name" is allocated and freed in intel_backlight_device_register().
>> The initial allocation just duplicates "intel_backlight".
>>
>> Later, if a device with this name has already been registered, another
>> dynamically generated one is allocated using kasprintf().
>>
>> So at the end of the function, when "name" is freed, it can point either to
>> the initial static literal "intel_backlight" or to the kasprintf()'ed one.
>>
>> So kfree_const() is used.
>>
>> However, when built as a module, kstrdup_const() and kfree_const() don't
>> work as one would expect and are just plain kstrdup() and kfree().
>>
>>
>> Slightly change the logic and introduce a new variable to hold the
>> address returned by kasprintf() should it be used.
>>
>> This saves a memory allocation/free and avoids these _const functions,
>> which names can be confusing when used with code built as module.
> 
> Okay, I'd rather revert your earlier commit 379b63e7e682
> ("drm/i915/display: Save a few bytes of memory in
> intel_backlight_device_register()") than add this.

Hi,

that works for me. Thanks and sorry for the noise.

CJ

> 
> The code simplicity is much more important than saving a few bytes.
> 
> BR,
> Jani.
> 
> 
> 
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Compile tested only.
>>
>> For the records, this patch is a clean-up effort related to discussions at:
>>    - https://lore.kernel.org/all/ZvHurCYlCoi1ZTCX@skv.local/
>>    - https://lore.kernel.org/all/20240924050937.697118-1-senozhatsky@chromium.org/
>> ---
>>   drivers/gpu/drm/i915/display/intel_backlight.c | 17 +++++++----------
>>   1 file changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
>> index 9e05745d797d..bf7686aa044f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
>> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
>> @@ -914,9 +914,9 @@ int intel_backlight_device_register(struct intel_connector *connector)
>>   {
>>   	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>>   	struct intel_panel *panel = &connector->panel;
>> +	const char *name, *new_name = NULL;
>>   	struct backlight_properties props;
>>   	struct backlight_device *bd;
>> -	const char *name;
>>   	int ret = 0;
>>   
>>   	if (WARN_ON(panel->backlight.device))
>> @@ -949,10 +949,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>>   	else
>>   		props.power = BACKLIGHT_POWER_OFF;
>>   
>> -	name = kstrdup_const("intel_backlight", GFP_KERNEL);
>> -	if (!name)
>> -		return -ENOMEM;
>> -
>> +	name = "intel_backlight";
>>   	bd = backlight_device_get_by_name(name);
>>   	if (bd) {
>>   		put_device(&bd->dev);
>> @@ -963,11 +960,11 @@ 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);
>> -		name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
>> -				 i915->drm.primary->index, connector->base.name);
>> -		if (!name)
>> +		new_name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
>> +				     i915->drm.primary->index, connector->base.name);
>> +		if (!new_name)
>>   			return -ENOMEM;
>> +		name = new_name;
>>   	}
>>   	bd = backlight_device_register(name, connector->base.kdev, connector,
>>   				       &intel_backlight_device_ops, &props);
>> @@ -987,7 +984,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>>   		    connector->base.base.id, connector->base.name, name);
>>   
>>   out:
>> -	kfree_const(name);
>> +	kfree(new_name);
>>   
>>   	return ret;
>>   }
> 

On Tue, 01 Oct 2024, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> Le 30/09/2024 à 09:48, Jani Nikula a écrit :
>> On Sat, 28 Sep 2024, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>>> "name" is allocated and freed in intel_backlight_device_register().
>>> The initial allocation just duplicates "intel_backlight".
>>>
>>> Later, if a device with this name has already been registered, another
>>> dynamically generated one is allocated using kasprintf().
>>>
>>> So at the end of the function, when "name" is freed, it can point either to
>>> the initial static literal "intel_backlight" or to the kasprintf()'ed one.
>>>
>>> So kfree_const() is used.
>>>
>>> However, when built as a module, kstrdup_const() and kfree_const() don't
>>> work as one would expect and are just plain kstrdup() and kfree().
>>>
>>>
>>> Slightly change the logic and introduce a new variable to hold the
>>> address returned by kasprintf() should it be used.
>>>
>>> This saves a memory allocation/free and avoids these _const functions,
>>> which names can be confusing when used with code built as module.
>> 
>> Okay, I'd rather revert your earlier commit 379b63e7e682
>> ("drm/i915/display: Save a few bytes of memory in
>> intel_backlight_device_register()") than add this.
>
> Hi,
>
> that works for me. Thanks and sorry for the noise.

Will you send the revert?

BR,
Jani.


-- 
Jani Nikula, Intel

Le 02/10/2024 à 13:51, Jani Nikula a écrit :
> On Tue, 01 Oct 2024, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>> Le 30/09/2024 à 09:48, Jani Nikula a écrit :
>>> On Sat, 28 Sep 2024, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>>>> "name" is allocated and freed in intel_backlight_device_register().
>>>> The initial allocation just duplicates "intel_backlight".
>>>>
>>>> Later, if a device with this name has already been registered, another
>>>> dynamically generated one is allocated using kasprintf().
>>>>
>>>> So at the end of the function, when "name" is freed, it can point either to
>>>> the initial static literal "intel_backlight" or to the kasprintf()'ed one.
>>>>
>>>> So kfree_const() is used.
>>>>
>>>> However, when built as a module, kstrdup_const() and kfree_const() don't
>>>> work as one would expect and are just plain kstrdup() and kfree().
>>>>
>>>>
>>>> Slightly change the logic and introduce a new variable to hold the
>>>> address returned by kasprintf() should it be used.
>>>>
>>>> This saves a memory allocation/free and avoids these _const functions,
>>>> which names can be confusing when used with code built as module.
>>>
>>> Okay, I'd rather revert your earlier commit 379b63e7e682
>>> ("drm/i915/display: Save a few bytes of memory in
>>> intel_backlight_device_register()") than add this.
>>
>> Hi,
>>
>> that works for me. Thanks and sorry for the noise.
> 
> Will you send the revert?
> 
> BR,
> Jani.
> 
> 

Will do.

CJ