Messages in this thread | | | Date | Sat, 15 Apr 2023 07:31:45 +0800 | Subject | Re: [PATCH] clk: starfive: Avoid casting iomem pointers | From | Hal Feng <> |
| |
On Fri, 14 Apr 2023 09:58:47 +0800, Xingyu Wu wrote: > 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 >
Hi, Stephen,
Thanks for your fix to my previous patches, and I have tested this patch on VisionFive 2 board. As Xingyu said above, I think dev_set_drvdata() should also be dropped in clk-starfive-jh7110-sys.c and clk-starfive-jh7110-aon.c.
Best regards, Hal
| |