| Message ID | 20240501-pinctrl-cleanup-v1-0-797ceca46e5c@nxp.com |
|---|---|
| Headers | show |
| Series | pinctrl: Use scope based of_node_put() cleanups | expand |
On Wed, May 01, 2024 at 08:55:59PM +0800, Peng Fan (OSS) wrote: > @@ -879,16 +874,12 @@ static int ti_iodelay_probe(struct platform_device *pdev) > ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl); > if (ret) { > dev_err(dev, "Failed to register pinctrl\n"); > - goto exit_out; > + return ret; > } > > platform_set_drvdata(pdev, iod); > > return pinctrl_enable(iod->pctl); > - > -exit_out: > - of_node_put(np); > - return ret; > } This will call of_node_put() on the success path so it's a behavior change. The original code is buggy, it's supposed to call of_node_put() on the success path here or in ti_iodelay_remove(). If it's supposed to call of_node_put() here, then fine, this is bugfix but if it's supposed to call it in ti_iodelay_remove() then we need to save the pointer somewhere using no_free_ptr(). Probably saving ->np is the safest choice? The original code is already a little bit buggy because it doesn't check for pinctrl_enable() errors and cleanup. diff --git a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c index 040f2c46a868..f40a1476e4ff 100644 --- a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c +++ b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c @@ -156,6 +156,7 @@ struct ti_iodelay_device { const struct ti_iodelay_reg_data *reg_data; struct ti_iodelay_reg_values reg_init_conf_values; + struct device_node *np; }; /** @@ -884,7 +885,12 @@ static int ti_iodelay_probe(struct platform_device *pdev) platform_set_drvdata(pdev, iod); - return pinctrl_enable(iod->pctl); + ret = pinctrl_enable(iod->pctl); + if (ret) + goto exit_out; + + iod->np = no_free_ptr(np); + return 0; exit_out: of_node_put(np); @@ -903,6 +909,7 @@ static void ti_iodelay_remove(struct platform_device *pdev) pinctrl_unregister(iod->pctl); ti_iodelay_pinconf_deinit_dev(iod); + of_node_put(iod->np); /* Expect other allocations to be freed by devm */ }
On Wed, 2024-05-01 at 20:56 +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Use scope based of_node_put() cleanup to simplify code. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > index 5bb8fd0d1e41..61fbfddb5938 100644 > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > @@ -2629,14 +2629,13 @@ static struct regmap *aspeed_g5_acquire_regmap(struct aspeed_pinmux_data *ctx, > return ctx->maps[ip]; > > if (ip == ASPEED_IP_GFX) { > - struct device_node *node; > + struct device_node *node __free(device_node) = NULL; > struct regmap *map; > > node = of_parse_phandle(ctx->dev->of_node, > "aspeed,external-nodes", 0); > if (node) { > map = syscon_node_to_regmap(node); > - of_node_put(node); > if (IS_ERR(map)) > return map; > } else > @@ -2648,7 +2647,7 @@ static struct regmap *aspeed_g5_acquire_regmap(struct aspeed_pinmux_data *ctx, > } > > if (ip == ASPEED_IP_LPC) { > - struct device_node *np; > + struct device_node *np __free(device_node) = NULL; > struct regmap *map; > > np = of_parse_phandle(ctx->dev->of_node, > @@ -2660,7 +2659,6 @@ static struct regmap *aspeed_g5_acquire_regmap(struct aspeed_pinmux_data *ctx, > return ERR_PTR(-ENODEV); > > map = syscon_node_to_regmap(np->parent); > - of_node_put(np); > if (IS_ERR(map)) > return map; I think I agree with Krzysztof's feedback on the Samsung patch[1], and that I prefer the existing approach for the Aspeed driver. My reasoning suggests the existing implementation does the right thing. That said, the code could be adjusted to use early returns and consistent variable names, which might make it easier to reason about. I'll consider a follow-up patch to address that. Regardless, thanks for taking the time to explore the cleanup. Andrew [1]: https://lore.kernel.org/lkml/34193501-5b7b-4ffd-8549-a04c6930d02d@kernel.org/ > } else >
On Thu, May 02, 2024 at 12:28:42AM +0000, Peng Fan wrote: > > Subject: Re: [PATCH 01/21] pinctrl: ti: iodelay: Use scope based of_node_put() > > cleanups > > > > On Wed, May 01, 2024 at 08:55:59PM +0800, Peng Fan (OSS) wrote: > > > @@ -879,16 +874,12 @@ static int ti_iodelay_probe(struct > > platform_device *pdev) > > > ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl); > > > if (ret) { > > > dev_err(dev, "Failed to register pinctrl\n"); > > > - goto exit_out; > > > + return ret; > > > } > > > > > > platform_set_drvdata(pdev, iod); > > > > > > return pinctrl_enable(iod->pctl); > > > - > > > -exit_out: > > > - of_node_put(np); > > > - return ret; > > > } > > > > This will call of_node_put() on the success path so it's a behavior change. The > > original code is buggy, it's supposed to call of_node_put() on the success path > > here or in ti_iodelay_remove(). > > > > If it's supposed to call of_node_put() here, then fine, this is bugfix but if it's > > supposed to call it in ti_iodelay_remove() then we need to save the pointer > > somewhere using no_free_ptr(). Probably saving ->np is the safest choice? > > > > The original code is already a little bit buggy because it doesn't check for > > pinctrl_enable() errors and cleanup. > > It was introduced by > commit 6118714275f0a313ecc296a87ed1af32d9691bed (tag: pinctrl-v4.11-4) > Author: Tony Lindgren <tony@atomide.com> > Date: Thu Mar 30 09:16:39 2017 -0700 > > pinctrl: core: Fix pinctrl_register_and_init() with pinctrl_enable() > > of_node_put is expected in probe, not in remove. > Ah, right. You'll add that for the Fixes tag obviously... regards, dan carpenter
Hi Linus, > Subject: Re: [PATCH 00/21] pinctrl: Use scope based of_node_put() cleanups > > Hi Peng, > > thanks for doing this! I am very much in favor of using scoped management > of resources where it makes it easier to do the right thing. > > I agree with Krzysztof's comment that we should avoid scoping in cases where > there is a clear path grab/use/release so the code is easy to read already as it > is. Let's drop those. Yeah, will drop them in v2. > > I saw there was some patch that was even a fix, perhaps I should pick that > one separately for fixes, but probably it is non-urgent. Not urgent, I will add fix tag in v2. Thanks, Peng. > > I suppose we will just apply v2 after people had some time to look at it! > > Yours, > Linus Walleij
Use scope based of_node_put() to simplify code. It reduces the chance of forgetting of_node_put(), and also simplifies error handling path. I not able to test the changes on all the hardwares, so driver owners, please help review when you have time. This patchset was inspired from Dan's comments on pinctrl-scmi-imx.c, thanks. Signed-off-by: Peng Fan <peng.fan@nxp.com> --- Peng Fan (21): pinctrl: ti: iodelay: Use scope based of_node_put() cleanups pinctrl: tegra: Use scope based of_node_put() cleanups pinctrl: sunplus: Use scope based of_node_put() cleanups pinctrl: stm32: Use scope based of_node_put() cleanups pinctrl: starfive: Use scope based of_node_put() cleanups pinctrl: sprd: Use scope based of_node_put() cleanups pinctrl: spear: Use scope based of_node_put() cleanups pinctrl: samsung: Use scope based of_node_put() cleanups pinctrl: renesas: Use scope based of_node_put() cleanups pinctrl: st: Use scope based of_node_put() cleanups pinctrl: rockchip: Use scope based of_node_put() cleanups pinctrl: k210: Use scope based of_node_put() cleanups pinctrl: equilibrium: Use scope based of_node_put() cleanups pinctrl: at91: Use scope based of_node_put() cleanups pinctrl: s32cc: Use scope based of_node_put() cleanups pinctrl: nomadik: Use scope based of_node_put() cleanups pinctrl: mediatek: Use scope based of_node_put() cleanups pinctrl: freescale: Use scope based of_node_put() cleanups pinctrl: bcm: bcm63xx: Use scope based of_node_put() cleanups pinctrl: aspeed: g5: Use scope based of_node_put() cleanups pinctrl: pinconf-generic: Use scope based of_node_put() cleanups drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 6 ++-- drivers/pinctrl/bcm/pinctrl-bcm63xx.c | 4 +-- drivers/pinctrl/freescale/pinctrl-imx.c | 31 +++++------------ drivers/pinctrl/freescale/pinctrl-imx1-core.c | 19 ++++------ drivers/pinctrl/freescale/pinctrl-mxs.c | 24 +++++-------- drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 4 +-- drivers/pinctrl/mediatek/pinctrl-paris.c | 4 +-- drivers/pinctrl/nomadik/pinctrl-abx500.c | 4 +-- drivers/pinctrl/nomadik/pinctrl-nomadik.c | 4 +-- drivers/pinctrl/nxp/pinctrl-s32cc.c | 31 ++++++----------- drivers/pinctrl/pinconf-generic.c | 7 ++-- drivers/pinctrl/pinctrl-at91-pio4.c | 7 ++-- drivers/pinctrl/pinctrl-at91.c | 17 +++------ drivers/pinctrl/pinctrl-equilibrium.c | 21 +++--------- drivers/pinctrl/pinctrl-k210.c | 7 ++-- drivers/pinctrl/pinctrl-rockchip.c | 14 +++----- drivers/pinctrl/pinctrl-st.c | 40 +++++++--------------- drivers/pinctrl/renesas/pinctrl-rza1.c | 14 +++----- drivers/pinctrl/renesas/pinctrl-rzg2l.c | 10 ++---- drivers/pinctrl/renesas/pinctrl-rzn1.c | 23 ++++--------- drivers/pinctrl/renesas/pinctrl-rzv2m.c | 7 ++-- drivers/pinctrl/renesas/pinctrl.c | 7 ++-- drivers/pinctrl/samsung/pinctrl-exynos-arm.c | 3 +- drivers/pinctrl/samsung/pinctrl-exynos.c | 16 +++------ drivers/pinctrl/samsung/pinctrl-s3c64xx.c | 8 ++--- drivers/pinctrl/samsung/pinctrl-samsung.c | 26 ++++---------- drivers/pinctrl/spear/pinctrl-spear.c | 13 +++---- drivers/pinctrl/sprd/pinctrl-sprd.c | 14 +++----- drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c | 27 ++++++--------- drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c | 18 +++++----- drivers/pinctrl/stm32/pinctrl-stm32.c | 7 ++-- drivers/pinctrl/sunplus/sppctl.c | 4 +-- drivers/pinctrl/tegra/pinctrl-tegra-xusb.c | 7 ++-- drivers/pinctrl/tegra/pinctrl-tegra.c | 10 ++---- drivers/pinctrl/ti/pinctrl-ti-iodelay.c | 37 ++++++++------------ 35 files changed, 154 insertions(+), 341 deletions(-) --- base-commit: bb7a2467e6beef44a80a17d45ebf2931e7631083 change-id: 20240429-pinctrl-cleanup-e4d461c32648 Best regards,