Received-SPF: pass (google.com: domain of linux-kernel+bounces-140422-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 11 Apr 2024 14:17:52 +0200
From: Greg KH <gregkh@linuxfoundation.org>
To: R SUNDAR <prosunofficial@gmail.com>
Cc: heikki.krogerus@linux.intel.com, dmitry.baryshkov@linaro.org,
	neil.armstrong@linaro.org, christophe.jaillet@wanadoo.fr,
	u.kleine-koenig@pengutronix.de, skhan@linuxfoundation.org,
	javier.carrasco.cruz@gmail.com, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, Julia Lawall <julia.lawall@inria.fr>
Subject: Re: [PATCH] usb: typec: mux: replace of_node_put() with __free
 [linux-next]
Message-ID: <2024041103-doornail-professor-7c1e@gregkh>
References: <20240410165222.5192-1-prosunofficial@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240410165222.5192-1-prosunofficial@gmail.com>

On Wed, Apr 10, 2024 at 10:22:22PM +0530, R SUNDAR wrote:
> use the new cleanup magic to replace of_node_put() with
> __free(device_node) marking to auto release and to simplify the error
> paths.
> 
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: R SUNDAR <prosunofficial@gmail.com>

All CAPS for your name?  Is that how it is written on documents (for
some countries this is normal, but usually not, so I have to ask.)

> ---
>  drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c
> index b17826713753..72ec9ef3ac58 100644
> --- a/drivers/usb/typec/mux/nb7vpq904m.c
> +++ b/drivers/usb/typec/mux/nb7vpq904m.c
> @@ -315,29 +315,27 @@ static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = {
>  
>  static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7)
>  {
> -	struct device_node *ep;
>  	u32 data_lanes[4];
>  	int ret, i, j;
> -
> -	ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
> +	struct device_node *ep __free(device_node) =

This use of __free() for device_node would be the first one in the tree
(outside of a macro in a .h file.)  Is this really what the OF
maintainer wants to have happen here for this type of pattern?

And if so, why this random USB driver being the first one?  Have you
tested this?


> +		of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
>  
>  	if (ep) {

Nit, this function should be rewritten to not work like this, the
"common" path should not be indented, but only the exception (i.e. bail
if ep is not allocated properly.)

thanks,

greg k-h