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@xxxxxxxxx>
>> Cc: Emil Renner Berthing <emil.renner.berthing@xxxxxxxxxxxxx>
>> Cc: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
>> Cc: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>> Cc: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
>> Fixes: edab7204afe5 ("clk: starfive: Add StarFive JH7110 system clock driver")
>> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxx>
>> ---
>> 
>> 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