[PATCH] clk: starfive: Avoid casting iomem pointers

[PATCH] clk: starfive: Avoid casting iomem pointers

[Stephen Boyd]

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);
+	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

Re: [PATCH] clk: starfive: Avoid casting iomem pointers

[Conor Dooley]

[-- 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 --]

Re: [PATCH] clk: starfive: Avoid casting iomem pointers

[Stephen Boyd]

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.

Re: [PATCH] clk: starfive: Avoid casting iomem pointers

[Conor Dooley]

[-- 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 --]

Re: [PATCH] clk: starfive: Avoid casting iomem pointers

[Stephen Boyd]

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

Re: [PATCH] clk: starfive: Avoid casting iomem pointers

[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

Re: [PATCH] clk: starfive: Avoid casting iomem pointers

[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

Re: [PATCH] clk: starfive: Avoid casting iomem pointers

[Stephen Boyd]

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.