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: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.
>

I fixed an issue as one logical change, and tagged it for stable kernels
so it can be automatically applied. Then a second logical change
switched to the new approach, removing the old solution. If that
happened with a few weeks in between, it would be ok, right? And no one
would have to choose the fixes to backport for a given stable kernel.

I have also had cases where the maintainer preferred my approach instead
of fixing an old bug with a new facility, and the suggestion was
splitting into two patches.

But in the end I want to fix the issue in mainline kernel, so I will
squash the patches and leave the backporting for the ones who might be
interested in it, removing the stable tag.


> If you think about stable kernels, then work on backports, not inflate
> mainline kernel with multiple commits doing the same, creating
> artificial history.
> 
> Best regards,
> Krzysztof
> 

Thanks for your feedback and best regards,
Javier Carrasco