* Re: [PATCH] clk: starfive: Avoid casting iomem pointers
2023-04-13 20:55 [PATCH] clk: starfive: Avoid casting iomem pointers Stephen Boyd
@ 2023-04-13 21:26 ` Conor Dooley
2023-04-13 22:01 ` Stephen Boyd
2023-04-13 22:48 ` Stephen Boyd
2023-04-14 1:58 ` Xingyu Wu
2 siblings, 1 reply; 8+ messages in thread
From: Conor Dooley @ 2023-04-13 21:26 UTC (permalink / raw)
To: Stephen Boyd
Cc: Michael Turquette, linux-kernel, linux-clk, patches,
Tommaso Merciai, Emil Renner Berthing, Hal Feng, Conor Dooley,
Xingyu Wu
[-- Attachment #1: Type: text/plain, Size: 6437 bytes --]
On Thu, Apr 13, 2023 at 01:55:28PM -0700, 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.
Cool, thanks for doing it!
> 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);
Huh, I think you didn't explicitly mention this one, but it's actually
part of the UAF fix AFAICT?
When I did the aux device stuff for the clk-mpfs driver, I copied from
peci as there were almost no examples of aux dev stuff in-tree.
It looks like subsequently to me starting development, this fix landed:
1c11289b34ab ("peci: cpu: Fix use-after-free in adev_release()")
It similarly moves the uninit() to the release callback...
I think I need the below (whitespace damaged):
diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c
index 4f0a19db7ed7..cc5d7dee59f0 100644
--- a/drivers/clk/microchip/clk-mpfs.c
+++ b/drivers/clk/microchip/clk-mpfs.c
@@ -374,14 +374,13 @@ static void mpfs_reset_unregister_adev(void *_adev)
struct auxiliary_device *adev = _adev;
auxiliary_device_delete(adev);
+ auxiliary_device_uninit(adev);
}
static void mpfs_reset_adev_release(struct device *dev)
{
struct auxiliary_device *adev = to_auxiliary_dev(dev);
- auxiliary_device_uninit(adev);
-
kfree(adev);
}
Anyways, for this patch:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
> }
>
> 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);
> + 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
> --
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] clk: starfive: Avoid casting iomem pointers
2023-04-13 21:26 ` Conor Dooley
@ 2023-04-13 22:01 ` Stephen Boyd
2023-04-13 22:22 ` Conor Dooley
0 siblings, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2023-04-13 22:01 UTC (permalink / raw)
To: Conor Dooley
Cc: Michael Turquette, linux-kernel, linux-clk, patches,
Tommaso Merciai, Emil Renner Berthing, Hal Feng, Conor Dooley,
Xingyu Wu
Quoting Conor Dooley (2023-04-13 14:26:56)
> > @@ -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);
>
> Huh, I think you didn't explicitly mention this one, but it's actually
> part of the UAF fix AFAICT?
>
> When I did the aux device stuff for the clk-mpfs driver, I copied from
> peci as there were almost no examples of aux dev stuff in-tree.
> It looks like subsequently to me starting development, this fix landed:
> 1c11289b34ab ("peci: cpu: Fix use-after-free in adev_release()")
>
> It similarly moves the uninit() to the release callback...
>
> I think I need the below (whitespace damaged):
Yeah that looks better. Care to send a proper patch for it?
> diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c
> index 4f0a19db7ed7..cc5d7dee59f0 100644
> --- a/drivers/clk/microchip/clk-mpfs.c
> +++ b/drivers/clk/microchip/clk-mpfs.c
> @@ -374,14 +374,13 @@ static void mpfs_reset_unregister_adev(void *_adev)
> struct auxiliary_device *adev = _adev;
>
> auxiliary_device_delete(adev);
> + auxiliary_device_uninit(adev);
> }
>
> static void mpfs_reset_adev_release(struct device *dev)
> {
> struct auxiliary_device *adev = to_auxiliary_dev(dev);
>
> - auxiliary_device_uninit(adev);
> -
> kfree(adev);
> }
>
> Anyways, for this patch:
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] clk: starfive: Avoid casting iomem pointers
2023-04-13 22:01 ` Stephen Boyd
@ 2023-04-13 22:22 ` Conor Dooley
0 siblings, 0 replies; 8+ messages in thread
From: Conor Dooley @ 2023-04-13 22:22 UTC (permalink / raw)
To: Stephen Boyd
Cc: Michael Turquette, linux-kernel, linux-clk, patches,
Tommaso Merciai, Emil Renner Berthing, Hal Feng, Conor Dooley,
Xingyu Wu
[-- Attachment #1: Type: text/plain, Size: 203 bytes --]
On Thu, Apr 13, 2023 at 03:01:02PM -0700, Stephen Boyd wrote:
> Yeah that looks better. Care to send a proper patch for it?
Yup, no problem: 20230413-critter-synopsis-dac070a86cb4@spud
Cheers,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clk: starfive: Avoid casting iomem pointers
2023-04-13 20:55 [PATCH] clk: starfive: Avoid casting iomem pointers Stephen Boyd
2023-04-13 21:26 ` Conor Dooley
@ 2023-04-13 22:48 ` Stephen Boyd
2023-04-14 1:58 ` Xingyu Wu
2 siblings, 0 replies; 8+ messages in thread
From: Stephen Boyd @ 2023-04-13 22:48 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, patches, Tommaso Merciai,
Emil Renner Berthing, Hal Feng, Conor Dooley, Xingyu Wu
Quoting Stephen Boyd (2023-04-13 13:55:28)
> 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>
> ---
Applied to clk-next
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] clk: starfive: Avoid casting iomem pointers
2023-04-13 20:55 [PATCH] clk: starfive: Avoid casting iomem pointers Stephen Boyd
2023-04-13 21:26 ` Conor Dooley
2023-04-13 22:48 ` Stephen Boyd
@ 2023-04-14 1:58 ` Xingyu Wu
2023-04-14 23:31 ` Hal Feng
2 siblings, 1 reply; 8+ messages in thread
From: Xingyu Wu @ 2023-04-14 1:58 UTC (permalink / raw)
To: Stephen Boyd, Michael Turquette
Cc: linux-kernel, linux-clk, patches, Tommaso Merciai,
Emil Renner Berthing, Hal Feng, Conor Dooley
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] clk: starfive: Avoid casting iomem pointers
2023-04-14 1:58 ` Xingyu Wu
@ 2023-04-14 23:31 ` Hal Feng
2023-04-18 0:30 ` Stephen Boyd
0 siblings, 1 reply; 8+ messages in thread
From: Hal Feng @ 2023-04-14 23:31 UTC (permalink / raw)
To: Xingyu Wu, Stephen Boyd, Michael Turquette
Cc: linux-kernel, linux-clk, patches, Tommaso Merciai,
Emil Renner Berthing, Conor Dooley
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] clk: starfive: Avoid casting iomem pointers
2023-04-14 23:31 ` Hal Feng
@ 2023-04-18 0:30 ` Stephen Boyd
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Boyd @ 2023-04-18 0:30 UTC (permalink / raw)
To: Hal Feng, Michael Turquette, Xingyu Wu
Cc: linux-kernel, linux-clk, patches, Tommaso Merciai,
Emil Renner Berthing, Conor Dooley
Quoting Hal Feng (2023-04-14 16:31:45)
>
> 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.
>
Sure. I see you sent the patch. Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread