Re: [PATCH 2/2] Bluetooth: btbcm: automate node cleanup in btbcm_get_board_name()

[Javier Carrasco <javier.carrasco.cruz@gmail.com>]

On 31/10/2024 12:30, Krzysztof Kozlowski wrote:
> On 31/10/2024 12:14, Krzysztof Kozlowski wrote:
>> On 31/10/2024 12:10, Javier Carrasco wrote:
>>> On 31/10/2024 12:08, Krzysztof Kozlowski wrote:
>>>> On 30/10/2024 16:46, Javier Carrasco wrote:
>>>>> Switch to a more robust approach by automating the node release when it
>>>>> goes out of scope, removing the need for explicit calls to
>>>>> of_node_put().
>>>>>
>>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>>>> ---
>>>>>  drivers/bluetooth/btbcm.c | 8 ++------
>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
>>>>> index 400c2663d6b0..a1153ada74d2 100644
>>>>> --- a/drivers/bluetooth/btbcm.c
>>>>> +++ b/drivers/bluetooth/btbcm.c
>>>>> @@ -541,23 +541,19 @@ static const struct bcm_subver_table bcm_usb_subver_table[] = {
>>>>>  static const char *btbcm_get_board_name(struct device *dev)
>>>>>  {
>>>>>  #ifdef CONFIG_OF
>>>>> -	struct device_node *root;
>>>>> +	struct device_node *root __free(device_node) = of_find_node_by_path("/");
>>>>>  	char *board_type;
>>>>>  	const char *tmp;
>>>>>  
>>>>> -	root = of_find_node_by_path("/");
>>>>>  	if (!root)
>>>>>  		return NULL;
>>>>>  
>>>>> -	if (of_property_read_string_index(root, "compatible", 0, &tmp)) {
>>>>> -		of_node_put(root);
>>>>
>>>> You just added this. Don't add code which is immediately removed. It's a
>>>> noop or wrong code.
>>>>
>>>>
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Exactly, I added that code to fix the issue in stable kernels that don't
>>
>> Then send backport for stable.
>>
>>> support the __free() macro, and then I removed it to use a safer
>>> approach from now on.
>>
>> This is not correct approach. We work here on mainline and in mainline
>> this is one logical change: fixing issue. Whether you fix issue with
>> of_node_put or cleanup or by removing of_find_node_by_path() call, it
>> does not matter. All of these are fixing the same, one issue.
>>
>> If you think about stable kernels, then work on backports, not inflate
>> mainline kernel with multiple commits doing the same, creating
>> artificial history.
>>
> 
> And to clarify even more: these stable backports are close to useless,
> because it does not matter for them. No impact, not much benefits,
> nothing improved for users/developers. There is no need to backport
> them, although of course there is no loss by doing so. Therefore entire
> dance affects mainline kernel without any real benefits for stable.
> 
> Your split suggests you don't really know what this dropping reference
> is for.

Such splits were suggested in other threads, and they came exactly for
those reasons: they could not be applied to stable. That was not my
first approach, which was just using __free() to fix the issue. I am not
looking forward to inflating any history, as that's in the end more work
for me.

If a simple patch that adds the cleanup attribute is enough, that's
awesome. I will go for that approach for all cases then, and use your
explanation as a reference if I am asked to split the fix again.

> Best regards,
> Krzysztof
> 

Thanks for your feedback and best regards,
Javier Carrasco