| Message ID | 20240418194302.1466-1-shresthprasad7@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] drivers: video: Simplify device_node cleanup using __free | expand |
^^^ Please fix the subject line to be "backlight: <driver>: ...". I came very close to deleting this patch without reading it ;-) . On Fri, Apr 19, 2024 at 01:13:02AM +0530, Shresth Prasad wrote: > diff --git a/drivers/video/backlight/sky81452-backlight.c b/drivers/video/backlight/sky81452-backlight.c > index eb18c6eb0ff0..3c5d8125080c 100644 > --- a/drivers/video/backlight/sky81452-backlight.c > +++ b/drivers/video/backlight/sky81452-backlight.c > @@ -182,7 +182,7 @@ static const struct attribute_group sky81452_bl_attr_group = { > static struct sky81452_bl_platform_data *sky81452_bl_parse_dt( > struct device *dev) > { > - struct device_node *np = of_node_get(dev->of_node); > + struct device_node *np __free(device_node) = of_node_get(dev->of_node); Do we need to get dev->of_node at all? The device, which we are borrowing, already owns a reference to the node so I don't see any point in this function taking an extra one. So why not simply make this: struct device_node *np = dev->of_node; Daniel.
> Please fix the subject line to be "backlight: <driver>: ...". I came > very close to deleting this patch without reading it ;-) . Really sorry about that, I'll fix it. > Do we need to get dev->of_node at all? The device, which we are > borrowing, already owns a reference to the node so I don't see > any point in this function taking an extra one. > > So why not simply make this: > > struct device_node *np = dev->of_node; Looking at it again, I'm not sure why the call to `of_node_put` is there in the first place. I think removing it will be fine. I'll fix both of these issues and send a patch v2. Regards, Shresth
On Sat, Apr 20, 2024 at 12:22:41AM +0530, Shresth Prasad wrote: > > > Please fix the subject line to be "backlight: <driver>: ...". I came > > very close to deleting this patch without reading it ;-) . > > Really sorry about that, I'll fix it. > > > Do we need to get dev->of_node at all? The device, which we are > > borrowing, already owns a reference to the node so I don't see > > any point in this function taking an extra one. > > > > So why not simply make this: > > > > struct device_node *np = dev->of_node; > > Looking at it again, I'm not sure why the call to `of_node_put` is there in the first place. I think removing it will be fine. > > I'll fix both of these issues and send a patch v2. Just a stupid quesiton: on which platform was this patch tested?
20 Apr 2024 1:13:42 am Dmitry Baryshkov <dmitry.baryshkov@linaro.org>: > On Sat, Apr 20, 2024 at 12:22:41AM +0530, Shresth Prasad wrote: >> >>> Please fix the subject line to be "backlight: <driver>: ...". I came >>> very close to deleting this patch without reading it ;-) . >> >> Really sorry about that, I'll fix it. >> >>> Do we need to get dev->of_node at all? The device, which we are >>> borrowing, already owns a reference to the node so I don't see >>> any point in this function taking an extra one. >>> >>> So why not simply make this: >>> >>> struct device_node *np = dev->of_node; >> >> Looking at it again, I'm not sure why the call to `of_node_put` is there in the first place. I think removing it will be fine. >> >> I'll fix both of these issues and send a patch v2. > > Just a stupid quesiton: on which platform was this patch tested? > > -- > With best wishes > Dmitry I tested the patch on a x86_64 qemu virtual machine Regards, Shresth
diff --git a/drivers/video/backlight/sky81452-backlight.c b/drivers/video/backlight/sky81452-backlight.c index eb18c6eb0ff0..3c5d8125080c 100644 --- a/drivers/video/backlight/sky81452-backlight.c +++ b/drivers/video/backlight/sky81452-backlight.c @@ -182,7 +182,7 @@ static const struct attribute_group sky81452_bl_attr_group = { static struct sky81452_bl_platform_data *sky81452_bl_parse_dt( struct device *dev) { - struct device_node *np = of_node_get(dev->of_node); + struct device_node *np __free(device_node) = of_node_get(dev->of_node); struct sky81452_bl_platform_data *pdata; int num_entry; unsigned int sources[6]; @@ -194,10 +194,8 @@ static struct sky81452_bl_platform_data *sky81452_bl_parse_dt( } pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); - if (!pdata) { - of_node_put(np); + if (!pdata) return ERR_PTR(-ENOMEM); - } of_property_read_string(np, "name", &pdata->name); pdata->ignore_pwm = of_property_read_bool(np, "skyworks,ignore-pwm"); @@ -217,7 +215,6 @@ static struct sky81452_bl_platform_data *sky81452_bl_parse_dt( num_entry); if (ret < 0) { dev_err(dev, "led-sources node is invalid.\n"); - of_node_put(np); return ERR_PTR(-EINVAL); } @@ -237,7 +234,6 @@ static struct sky81452_bl_platform_data *sky81452_bl_parse_dt( if (ret < 0) pdata->boost_current_limit = 2750; - of_node_put(np); return pdata; } #else
Add `__free` function attribute to `np` device_node pointer initialisation and remove of_node_put cleanup for this pointer. The `__free` attribute is used for scope based cleanup instead of manually freeing the resource using `of_node_put`, making cleanup simpler and safer. Suggested-by: Julia Lawall <julia.lawall@inria.fr> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com> --- drivers/video/backlight/sky81452-backlight.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)