On 31/10/2024 12:41, Javier Carrasco wrote:
> 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@xxxxxxxxx>
>>>>>> ---
>>>>>> 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
You mention it third time, but never provided a link. I tried to look
briefly for it but failed. Can you share a lore link?
> 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.
If maintainer asks you to split trivial things like of_node_put() for
simple patches, feel free to Cc me, so I can provide counter arguments.
Best regards,
Krzysztof
