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