Received-SPF: pass (google.com: domain of linux-kernel+bounces-142029-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 12 Apr 2024 08:21:30 +0200 (CEST)
From: Julia Lawall <julia.lawall@inria.fr>
To: Greg KH <gregkh@linuxfoundation.org>, Rob Herring <robh@kernel.org>, 
    Saravana Kannan <saravanak@google.com>, devicetree@vger.kernel.org
cc: Roman Storozhenko <romeusmeister@gmail.com>, jirislaby@kernel.org, 
    skhan@linuxfoundation.org, javier.carrasco.cruz@gmail.com, 
    linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Subject: Re: [PATCH] sysrq: Auto release device node using __free attribute
In-Reply-To: <2024041211-statistic-reformist-bf70@gregkh>
Message-ID: <alpine.DEB.2.22.394.2404120807400.3229@hadrien>
References: <20240411180256.61001-1-romeusmeister@gmail.com> <2024041111-tummy-boil-a6aa@gregkh> <b6b33aa-faef-6919-7125-c2db11e784ee@inria.fr> <2024041211-statistic-reformist-bf70@gregkh>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

[Adding the OF maintainers and device tree mailing list]

> > Maybe it would be nice to get rid of of_node_puts in the general case?
>
> That's a call for the of maintainer to make, and then if so, to do it
> across the whole tree, right?

Sure.

Rob and Saravana, what do you think about the following:

* Is it ok to use __free(device_tree) directly in a declaration, or is
there some macro that should be used instead?

* If is ok to use __free(device_tree) even in simple cases where a
variable is just declared at the start of the scope and freed at the end,
and there is no internediate error handling code?

Please see for example:

https://lore.kernel.org/lkml/alpine.DEB.2.22.394.2401291455430.8649@hadrien/

But I don't think we can do the whole thing at once, in one patch.  There
are a lot of things that need to be checked, and it don't break anything
to do them one at a time.

>
> > Even though this one is not very annoying, there are some other functions
> > where there are many of_node_puts, and convoluted error handling code to
> > incorporate them on both the success and failure path.  So maybe it would
> > be better to avoid the situation of having them sometimes and not having
> > them other times?  But this is an opinion, and if the general consensus is
> > to only get rid of the cases that currently add complexity, then that is
> > possible too.
>
> Let's keep things simple until it has to be complex please.

I meant that the current code is complex and error prone, and using __free
eliminates code that is ugly and has led to memory leaks in the past (and
a lot of effort to find and fix them in the past).  Even if some instances
don't have that property, they may become more complex in the future, if
some error condition is detected.

julia