Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b41d8cf4-70aa-3b64-5254-46d92a188f63@starfivetech.com>
Date:   Fri, 14 Apr 2023 09:58:47 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH] clk: starfive: Avoid casting iomem pointers
Content-Language: en-US
To:     Stephen Boyd <sboyd@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>
CC:     <linux-kernel@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <patches@lists.linux.dev>,
        Tommaso Merciai <tomm.merciai@gmail.com>,
        "Emil Renner Berthing" <emil.renner.berthing@canonical.com>,
        Hal Feng <hal.feng@starfivetech.com>,
        Conor Dooley <conor.dooley@microchip.com>
References: <20230413205528.4044216-1-sboyd@kernel.org>
From:   Xingyu Wu <xingyu.wu@starfivetech.com>
In-Reply-To: <20230413205528.4044216-1-sboyd@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

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