Messages in this thread Patch in this message | | | Date | Fri, 14 Apr 2023 09:58:47 +0800 | Subject | Re: [PATCH] clk: starfive: Avoid casting iomem pointers | From | Xingyu Wu <> |
| |
On 2023/4/14 4:55, Stephen Boyd wrote: > Let's use a wrapper struct for the auxiliary_device made in > jh7110_reset_controller_register() so that we can stop casting iomem > pointers. The casts trip up tools like sparse, and make for some awkward > casts that are largely unnecessary. While we're here, change the > allocation from devm and actually free the auxiliary_device memory in > the release function. This avoids any use after free problems where the > parent device driver is unbound from the device but the > auxiliuary_device is still in use accessing devm freed memory. > > Cc: Tommaso Merciai <tomm.merciai@gmail.com> > Cc: Emil Renner Berthing <emil.renner.berthing@canonical.com> > Cc: Hal Feng <hal.feng@starfivetech.com> > Cc: Conor Dooley <conor.dooley@microchip.com> > Cc: Xingyu Wu <xingyu.wu@starfivetech.com> > Fixes: edab7204afe5 ("clk: starfive: Add StarFive JH7110 system clock driver") > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > > I can take this via clk tree. > > drivers/clk/starfive/clk-starfive-jh7110-sys.c | 15 ++++++++++++--- > drivers/reset/starfive/reset-starfive-jh7110.c | 9 ++++++--- > include/soc/starfive/reset-starfive-jh71x0.h | 17 +++++++++++++++++ > 3 files changed, 35 insertions(+), 6 deletions(-) > create mode 100644 include/soc/starfive/reset-starfive-jh71x0.h > > diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c > index 5ec210644e1d..851b93d0f371 100644 > --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c > +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c > @@ -11,6 +11,9 @@ > #include <linux/init.h> > #include <linux/io.h> > #include <linux/platform_device.h> > +#include <linux/slab.h> > + > +#include <soc/starfive/reset-starfive-jh71x0.h> > > #include <dt-bindings/clock/starfive,jh7110-crg.h> > > @@ -335,26 +338,32 @@ static void jh7110_reset_unregister_adev(void *_adev) > struct auxiliary_device *adev = _adev; > > auxiliary_device_delete(adev); > + auxiliary_device_uninit(adev); > } > > static void jh7110_reset_adev_release(struct device *dev) > { > struct auxiliary_device *adev = to_auxiliary_dev(dev); > + struct jh71x0_reset_adev *rdev = to_jh71x0_reset_adev(adev); > > - auxiliary_device_uninit(adev); > + kfree(rdev); > } > > int jh7110_reset_controller_register(struct jh71x0_clk_priv *priv, > const char *adev_name, > u32 adev_id) > { > + struct jh71x0_reset_adev *rdev; > struct auxiliary_device *adev; > int ret; > > - adev = devm_kzalloc(priv->dev, sizeof(*adev), GFP_KERNEL); > - if (!adev) > + rdev = kzalloc(sizeof(*rdev), GFP_KERNEL); > + if (!rdev) > return -ENOMEM; > > + rdev->base = priv->base; > + > + adev = &rdev->adev; > adev->name = adev_name; > adev->dev.parent = priv->dev; > adev->dev.release = jh7110_reset_adev_release; > diff --git a/drivers/reset/starfive/reset-starfive-jh7110.c b/drivers/reset/starfive/reset-starfive-jh7110.c > index c1b3a490d951..2d26ae95c8cc 100644 > --- a/drivers/reset/starfive/reset-starfive-jh7110.c > +++ b/drivers/reset/starfive/reset-starfive-jh7110.c > @@ -7,6 +7,8 @@ > > #include <linux/auxiliary_bus.h> > > +#include <soc/starfive/reset-starfive-jh71x0.h> > + > #include "reset-starfive-jh71x0.h" > > #include <dt-bindings/reset/starfive,jh7110-crg.h> > @@ -33,14 +35,15 @@ static int jh7110_reset_probe(struct auxiliary_device *adev, > const struct auxiliary_device_id *id) > { > struct jh7110_reset_info *info = (struct jh7110_reset_info *)(id->driver_data); > - void __iomem **base = (void __iomem **)dev_get_drvdata(adev->dev.parent);
Thank you for doing that. BTW, if drop the dev_get_drvdata(), the dev_set_drvdata() should also be dropped.
diff --git a/drivers/clk/starfive/clk-starfive-jh7110-aon.c b/drivers/clk/starfive/clk-starfive-jh7110-aon.c index a2799fe8a234..62954eb7b50a 100644 --- a/drivers/clk/starfive/clk-starfive-jh7110-aon.c +++ b/drivers/clk/starfive/clk-starfive-jh7110-aon.c @@ -83,8 +83,6 @@ static int jh7110_aoncrg_probe(struct platform_device *pdev) if (IS_ERR(priv->base)) return PTR_ERR(priv->base);
- dev_set_drvdata(priv->dev, (void *)(&priv->base)); - for (idx = 0; idx < JH7110_AONCLK_END; idx++) { u32 max = jh7110_aonclk_data[idx].max; struct clk_parent_data parents[4] = {}; diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c index 5ec210644e1d..0cda33fd47f8 100644 --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c @@ -393,8 +393,6 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev) if (IS_ERR(priv->base)) return PTR_ERR(priv->base);
- dev_set_drvdata(priv->dev, (void *)(&priv->base)); - /* * These PLL clocks are not actually fixed factor clocks and can be * controlled by the syscon registers of JH7110. They will be dropped
> + struct jh71x0_reset_adev *rdev = to_jh71x0_reset_adev(adev); > + void __iomem *base = rdev->base; > > if (!info || !base) > return -ENODEV; > > return reset_starfive_jh71x0_register(&adev->dev, adev->dev.parent->of_node, > - *base + info->assert_offset, > - *base + info->status_offset, > + base + info->assert_offset, > + base + info->status_offset, > NULL, > info->nr_resets, > NULL); > diff --git a/include/soc/starfive/reset-starfive-jh71x0.h b/include/soc/starfive/reset-starfive-jh71x0.h > new file mode 100644 > index 000000000000..47b486ececc5 > --- /dev/null > +++ b/include/soc/starfive/reset-starfive-jh71x0.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __SOC_STARFIVE_RESET_JH71X0_H > +#define __SOC_STARFIVE_RESET_JH71X0_H > + > +#include <linux/auxiliary_bus.h> > +#include <linux/compiler_types.h> > +#include <linux/container_of.h> > + > +struct jh71x0_reset_adev { > + void __iomem *base; > + struct auxiliary_device adev; > +}; > + > +#define to_jh71x0_reset_adev(_adev) \ > + container_of((_adev), struct jh71x0_reset_adev, adev) > + > +#endif > > base-commit: 601e5d464d535d655917c2cfb29c394d367fb676
Best regards, Xingyu Wu
| |