Re: [PATCH 01/13] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.

From: Lukas Wunner <lukas@wunner.de>
To: Jonathan Cameron <jic23@kernel.org>
Cc: linux-iio@vger.kernel.org,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	"Len Brown" <lenb@kernel.org>,
	linux-acpi@vger.kernel.org,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Daniel Scally" <djrscally@gmail.com>,
	"Heikki Krogerus" <heikki.krogerus@linux.intel.com>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Cosmin Tanislav" <cosmin.tanislav@analog.com>,
	"Mihail Chindris" <mihail.chindris@analog.com>,
	"Rasmus Villemoes" <linux@rasmusvillemoes.dk>,
	"Tomislav Denis" <tomislav.denis@avl.com>,
	"Marek Vasut" <marex@denx.de>,
	"Olivier Moysan" <olivier.moysan@foss.st.com>,
	"Fabrice Gasnier" <fabrice.gasnier@foss.st.com>,
	"Lad Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>,
	"Marijn Suijten" <marijn.suijten@somainline.org>,
	"Marius Cristea" <marius.cristea@microchip.com>,
	"Ibrahim Tilki" <Ibrahim.Tilki@analog.com>,
	"Jonathan Cameron" <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH 01/13] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Date: Sun, 21 Jan 2024 19:06:03 +0100	[thread overview]
Message-ID: <20240121180603.GA13937@wunner.de> (raw)
In-Reply-To: <20240114172009.179893-2-jic23@kernel.org>

On Sun, Jan 14, 2024 at 05:19:57PM +0000, Jonathan Cameron wrote:
> This allows the following
> 
> struct fwnode_handle *child __free(fwnode_handle) = NULL;
> 
> device_for_each_child_node(dev, child) {
> 	if (false)
> 		return -EINVAL;
> }
> 
> without the fwnode_handle_put() call which tends to complicate early
> exits from such loops and lead to resource leak bugs.
> 
> Can also be used where the fwnode_handle was obtained from a call
> such as fwnode_find_reference() as it will safely do nothing if
> IS_ERR() is true.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v1: Thanks to Andy for reviewing the RFC.
>     Add check for if (!IS_ERR_OR_NULL(_T)) to allow the compiler to optimize
>     cases where it knows the passed in parameter is NULL or an error pointer.

Heads-up:  Using IS_ERR_OR_NULL() in DEFINE_FREE() macros bloats
the code with additional IS_ERR() checks and NULL pointer checks.

See the detailed explanation in this patch which adds a DEFINE_FREE()
macro for x509_free_certificate():

https://lore.kernel.org/all/70ecd3904a70d2b92f8f1e04365a2b9ce66fac25.1705857475.git.lukas@wunner.de/

I'm wondering if a solution might be to stop returning IS_ERR()
from "constructors" such as x509_cert_parse() and instead assign
the created "object" (x509_certificate) to a call-by-reference
pointer and return an integer.  If the returned integer is not 0,
inhibit "destruction" of the "object" with no_free_ptr().

Thoughts?

> +DEFINE_FREE(fwnode_handle, struct fwnode_handle *,
> +	    if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))

If you do not align the "if" to the opening parenthesis,
checkpatch --strict complains:
"CHECK: Alignment should match open parenthesis"

If you do align to the opening parenthesis, it complains:
"WARNING: Statements should start on a tabstop"

I chose the latter for x509_free_certificate() for aesthetic reasons.

Either way, checkpatch still emits:

ERROR: trailing statements should be on next line"
#183: FILE: crypto/asymmetric_keys/x509_parser.h:49:
+	    if (!IS_ERR_OR_NULL(_T)) x509_free_certificate(_T))

Can't make it happy with these new-fangled DEFINE_FREE macros it seems. :(

Thanks,

Lukas