2024-04-07 07:23:48

by prabhav kumar

Subject: [PATCH next] drivers: soc: qcom: Auto cleanup using __free(device_node)

Use automated cleanup to replace of_node_put() in qcom_smem_resolve_mem().

Signed-off-by: Prabhav Kumar Vaish <[email protected]>
---
drivers/soc/qcom/smem.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 7191fa0c087f..ad1cf8dcc6ec 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -1032,18 +1032,16 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
struct smem_region *region)
{
struct device *dev = smem->dev;
- struct device_node *np;
struct resource r;
int ret;
+ struct device_node *np __free(device_node) = of_parse_phandle(dev->of_node, name, 0);

- np = of_parse_phandle(dev->of_node, name, 0);
if (!np) {
dev_err(dev, "No %s specified\n", name);
return -EINVAL;
}

ret = of_address_to_resource(np, 0, &r);
- of_node_put(np);
if (ret)
return ret;

--
2.34.1

2024-05-19 18:41:22

by prabhav kumar

Subject: Re: [PATCH next] drivers: soc: qcom: Auto cleanup using __free(device_node)

Are there any comments for me regarding the patch ?
Please let me know

On Sun, Apr 7, 2024 at 12:53 PM Prabhav Kumar Vaish
<[email protected]> wrote:
>
> Use automated cleanup to replace of_node_put() in qcom_smem_resolve_mem().
>
> Signed-off-by: Prabhav Kumar Vaish <[email protected]>
> ---
> drivers/soc/qcom/smem.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 7191fa0c087f..ad1cf8dcc6ec 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -1032,18 +1032,16 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> struct smem_region *region)
> {
> struct device *dev = smem->dev;
> - struct device_node *np;
> struct resource r;
> int ret;
> + struct device_node *np __free(device_node) = of_parse_phandle(dev->of_node, name, 0);
>
> - np = of_parse_phandle(dev->of_node, name, 0);
> if (!np) {
> dev_err(dev, "No %s specified\n", name);
> return -EINVAL;
> }
>
> ret = of_address_to_resource(np, 0, &r);
> - of_node_put(np);
> if (ret)
> return ret;
>
> --
> 2.34.1
>

2024-05-21 19:20:32

by Bjorn Andersson

Subject: Re: [PATCH next] drivers: soc: qcom: Auto cleanup using __free(device_node)

On Sun, Apr 07, 2024 at 12:53:30PM +0530, Prabhav Kumar Vaish wrote:
> Use automated cleanup to replace of_node_put() in qcom_smem_resolve_mem().
>

I don't find this easier to read or maintain.

Also, your subject prefix does not match other changes to this driver.

Regards,
Bjorn

> Signed-off-by: Prabhav Kumar Vaish <[email protected]>
> ---
> drivers/soc/qcom/smem.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 7191fa0c087f..ad1cf8dcc6ec 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -1032,18 +1032,16 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> struct smem_region *region)
> {
> struct device *dev = smem->dev;
> - struct device_node *np;
> struct resource r;
> int ret;
> + struct device_node *np __free(device_node) = of_parse_phandle(dev->of_node, name, 0);
>
> - np = of_parse_phandle(dev->of_node, name, 0);
> if (!np) {
> dev_err(dev, "No %s specified\n", name);
> return -EINVAL;
> }
>
> ret = of_address_to_resource(np, 0, &r);
> - of_node_put(np);
> if (ret)
> return ret;
>
> --
> 2.34.1
>

2024-05-21 19:31:47

by prabhav kumar

Subject: Re: [PATCH next] drivers: soc: qcom: Auto cleanup using __free(device_node)

On Wed, May 22, 2024 at 12:50 AM Bjorn Andersson
<[email protected]> wrote:
>
> On Sun, Apr 07, 2024 at 12:53:30PM +0530, Prabhav Kumar Vaish wrote:
> > Use automated cleanup to replace of_node_put() in qcom_smem_resolve_mem().
> >
>
> I don't find this easier to read or maintain.
Should I change it , explaining the change ??
>
> Also, your subject prefix does not match other changes to this driver.
The patch is to add a __free function attribute to np pointer
initialization ensuring
the pointers are freed as soon as they go out of scope, thus removing
the work to
manually free them using of_node_put.
>
> Regards,
> Bjorn
>
> > Signed-off-by: Prabhav Kumar Vaish <[email protected]>
> > ---
> > drivers/soc/qcom/smem.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > index 7191fa0c087f..ad1cf8dcc6ec 100644
> > --- a/drivers/soc/qcom/smem.c
> > +++ b/drivers/soc/qcom/smem.c
> > @@ -1032,18 +1032,16 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> > struct smem_region *region)
> > {
> > struct device *dev = smem->dev;
> > - struct device_node *np;
> > struct resource r;
> > int ret;
> > + struct device_node *np __free(device_node) = of_parse_phandle(dev->of_node, name, 0);
> >
> > - np = of_parse_phandle(dev->of_node, name, 0);
> > if (!np) {
> > dev_err(dev, "No %s specified\n", name);
> > return -EINVAL;
> > }
> >
> > ret = of_address_to_resource(np, 0, &r);
> > - of_node_put(np);
> > if (ret)
> > return ret;
> >
> > --
> > 2.34.1
> >

2024-05-21 19:43:46

by Bjorn Andersson

Subject: Re: [PATCH next] drivers: soc: qcom: Auto cleanup using __free(device_node)

On Wed, May 22, 2024 at 01:01:22AM +0530, prabhav kumar wrote:
> On Wed, May 22, 2024 at 12:50 AM Bjorn Andersson
> <[email protected]> wrote:
> >
> > On Sun, Apr 07, 2024 at 12:53:30PM +0530, Prabhav Kumar Vaish wrote:
> > > Use automated cleanup to replace of_node_put() in qcom_smem_resolve_mem().
> > >
> >
> > I don't find this easier to read or maintain.
> Should I change it , explaining the change ??
> >
> > Also, your subject prefix does not match other changes to this driver.
> The patch is to add a __free function attribute to np pointer
> initialization ensuring
> the pointers are freed as soon as they go out of scope, thus removing
> the work to
> manually free them using of_node_put.

Yes, that is what the __free() attribute does, but I don't find it
easier to read and unless I'm missing something you're not fixing an
actual issue here?

Regards,
Bjorn

> >
> > Regards,
> > Bjorn
> >
> > > Signed-off-by: Prabhav Kumar Vaish <[email protected]>
> > > ---
> > > drivers/soc/qcom/smem.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > > index 7191fa0c087f..ad1cf8dcc6ec 100644
> > > --- a/drivers/soc/qcom/smem.c
> > > +++ b/drivers/soc/qcom/smem.c
> > > @@ -1032,18 +1032,16 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> > > struct smem_region *region)
> > > {
> > > struct device *dev = smem->dev;
> > > - struct device_node *np;
> > > struct resource r;
> > > int ret;
> > > + struct device_node *np __free(device_node) = of_parse_phandle(dev->of_node, name, 0);
> > >
> > > - np = of_parse_phandle(dev->of_node, name, 0);
> > > if (!np) {
> > > dev_err(dev, "No %s specified\n", name);
> > > return -EINVAL;
> > > }
> > >
> > > ret = of_address_to_resource(np, 0, &r);
> > > - of_node_put(np);
> > > if (ret)
> > > return ret;
> > >
> > > --
> > > 2.34.1
> > >

2024-05-22 15:35:55

by prabhav kumar

Subject: Re: [PATCH next] drivers: soc: qcom: Auto cleanup using __free(device_node)

On Wed, May 22, 2024 at 1:13 AM Bjorn Andersson
<[email protected]> wrote:
>
> On Wed, May 22, 2024 at 01:01:22AM +0530, prabhav kumar wrote:
> > On Wed, May 22, 2024 at 12:50 AM Bjorn Andersson
> > <[email protected]> wrote:
> > >
> > > On Sun, Apr 07, 2024 at 12:53:30PM +0530, Prabhav Kumar Vaish wrote:
> > > > Use automated cleanup to replace of_node_put() in qcom_smem_resolve_mem().
> > > >
> > >
> > > I don't find this easier to read or maintain.
> > Should I change it , explaining the change ??
> > >
> > > Also, your subject prefix does not match other changes to this driver.
> > The patch is to add a __free function attribute to np pointer
> > initialization ensuring
> > the pointers are freed as soon as they go out of scope, thus removing
> > the work to
> > manually free them using of_node_put.
>
> Yes, that is what the __free() attribute does, but I don't find it
> easier to read and unless I'm missing something you're not fixing an
> actual issue here?
>
There is not any issue here, the patch sent is the cleanup of unnecessary
of_node_put() function call.

Thanks,
Prabhav

> Regards,
> Bjorn
>
> > >
> > > Regards,
> > > Bjorn
> > >
> > > > Signed-off-by: Prabhav Kumar Vaish <[email protected]>
> > > > ---
> > > > drivers/soc/qcom/smem.c | 4 +---
> > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > > > index 7191fa0c087f..ad1cf8dcc6ec 100644
> > > > --- a/drivers/soc/qcom/smem.c
> > > > +++ b/drivers/soc/qcom/smem.c
> > > > @@ -1032,18 +1032,16 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> > > > struct smem_region *region)
> > > > {
> > > > struct device *dev = smem->dev;
> > > > - struct device_node *np;
> > > > struct resource r;
> > > > int ret;
> > > > + struct device_node *np __free(device_node) = of_parse_phandle(dev->of_node, name, 0);
> > > >
> > > > - np = of_parse_phandle(dev->of_node, name, 0);
> > > > if (!np) {
> > > > dev_err(dev, "No %s specified\n", name);
> > > > return -EINVAL;
> > > > }
> > > >
> > > > ret = of_address_to_resource(np, 0, &r);
> > > > - of_node_put(np);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > --
> > > > 2.34.1
> > > >