[PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next]

[PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next]

[R SUNDAR]

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>
---
 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) =
+		of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
 
 	if (ep) {
 		ret = of_property_count_u32_elems(ep, "data-lanes");
 		if (ret == -EINVAL)
 			/* Property isn't here, consider default mapping */
-			goto out_done;
+			return 0;
 		if (ret < 0)
-			goto out_error;
+			return ret;
 
 		if (ret != DATA_LANES_COUNT) {
 			dev_err(&nb7->client->dev, "expected 4 data lanes\n");
-			ret = -EINVAL;
-			goto out_error;
+			return -EINVAL;
 		}
 
 		ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);
 		if (ret)
-			goto out_error;
+			return ret;
 
 		for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
 			for (j = 0; j < DATA_LANES_COUNT; j++) {
@@ -358,18 +356,11 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7)
 			break;
 		default:
 			dev_err(&nb7->client->dev, "invalid data lanes mapping\n");
-			ret = -EINVAL;
-			goto out_error;
+			return -EINVAL;
 		}
 	}
 
-out_done:
-	ret = 0;
-
-out_error:
-	of_node_put(ep);
-
-	return ret;
+	return 0;
 }
 
 static int nb7vpq904m_probe(struct i2c_client *client)
-- 
2.34.1

Re: [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next]

[Dmitry Baryshkov]

On Wed, 10 Apr 2024 at 19:52, R SUNDAR <prosunofficial@gmail.com> 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>
> ---
>  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) =
> +               of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);

Please stick this to the top of the function. Otherwise LGTM.

>
>         if (ep) {
>                 ret = of_property_count_u32_elems(ep, "data-lanes");
>                 if (ret == -EINVAL)
>                         /* Property isn't here, consider default mapping */
> -                       goto out_done;
> +                       return 0;
>                 if (ret < 0)
> -                       goto out_error;
> +                       return ret;
>
>                 if (ret != DATA_LANES_COUNT) {
>                         dev_err(&nb7->client->dev, "expected 4 data lanes\n");
> -                       ret = -EINVAL;
> -                       goto out_error;
> +                       return -EINVAL;
>                 }
>
>                 ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);
>                 if (ret)
> -                       goto out_error;
> +                       return ret;
>
>                 for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
>                         for (j = 0; j < DATA_LANES_COUNT; j++) {
> @@ -358,18 +356,11 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7)
>                         break;
>                 default:
>                         dev_err(&nb7->client->dev, "invalid data lanes mapping\n");
> -                       ret = -EINVAL;
> -                       goto out_error;
> +                       return -EINVAL;
>                 }
>         }
>
> -out_done:
> -       ret = 0;
> -
> -out_error:
> -       of_node_put(ep);
> -
> -       return ret;
> +       return 0;
>  }
>
>  static int nb7vpq904m_probe(struct i2c_client *client)
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next]

[Greg KH]

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

Re: [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next]

[sundar.R]

Hi greg k-h,

Thanks for comments provided.

On 11/04/24 17:47, Greg KH wrote:
> 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.)

In documents it was like that.I also thought to change to small letters 
in git user name.
will change to small letter.

>> -
>> -	ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
>> +	struct device_node *ep __free(device_node) =
> 
> And if so, why this random USB driver being the first one?  Have you
> tested this?

I haven't tested it.I picked this usb driver to get cleanup module 
changes with __free.
I believe since of_node_put() is scope based here,we can use cleanup to 
remove the goto label uses in case of error.
> >
>> +		of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
>>   
>>   	if (ep) {
> 
>  the "common" path should not be indented, but only the exception (i.e. bail
> if ep is not allocated properly.)
I Tried in same line initialization, it exceeds column limit.
so indented in next line.


> 
> thanks,
> 
> greg k-h

Regards,
Sundar