Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <c881de5f-5a1e-19ff-0ae6-f68032c79f03@arm.com>
Date:   Fri, 25 Mar 2022 09:01:55 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2] thermal: devfreq_cooling: use local ops instead of
 global ops
Content-Language: en-US
To:     Kant Fan <kant@allwinnertech.com>
Cc:     amitk@kernel.org, rui.zhang@intel.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        allwinner-opensource-support@allwinnertech.com,
        stable@vger.kernel.org, rafael@kernel.org,
        daniel.lezcano@linaro.org, ionela.voinescu@arm.com
References: <20220325073030.91919-1-kant@allwinnertech.com>
From:   Lukasz Luba <lukasz.luba@arm.com>
In-Reply-To: <20220325073030.91919-1-kant@allwinnertech.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Kant,

On 3/25/22 07:30, Kant Fan wrote:
> Fix access illegal address problem in following condition:
> There are muti devfreq cooling devices in system, some of them has
> em model but other does not, energy model ops such as state2power will
> append to global devfreq_cooling_ops when the cooling device with
> em model register. It makes the cooling device without em model
> also use devfreq_cooling_ops after appending when register later by
> of_devfreq_cooling_register_power() or of_devfreq_cooling_register().
> 
> IPA governor regards the cooling devices without em model as a power actor
> because they also have energy model ops, and will access illegal address
> at dfc->em_pd when execute cdev->ops->get_requested_power,
> cdev->ops->state2power or cdev->ops->power2state.
> 
> Fixes: 615510fe13bd2 ("thermal: devfreq_cooling: remove old power model and use EM")
> Cc: stable@vger.kernel.org # 5.13+
> Signed-off-by: Kant Fan <kant@allwinnertech.com>
> ---
>   drivers/thermal/devfreq_cooling.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> index 4310cb342a9f..d38a80adec73 100644
> --- a/drivers/thermal/devfreq_cooling.c
> +++ b/drivers/thermal/devfreq_cooling.c
> @@ -358,21 +358,28 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
>   	struct thermal_cooling_device *cdev;
>   	struct device *dev = df->dev.parent;
>   	struct devfreq_cooling_device *dfc;
> +	struct thermal_cooling_device_ops *ops;
>   	char *name;
>   	int err, num_opps;
>   
> -	dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
> -	if (!dfc)
> +	ops = kmemdup(&devfreq_cooling_ops, sizeof(*ops), GFP_KERNEL);
> +	if (!ops)
>   		return ERR_PTR(-ENOMEM);
>   
> +	dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
> +	if (!dfc) {
> +		err = -ENOMEM;
> +		goto free_ops;
> +	}
> +
>   	dfc->devfreq = df;
>   
>   	dfc->em_pd = em_pd_get(dev);
>   	if (dfc->em_pd) {
> -		devfreq_cooling_ops.get_requested_power =
> +		ops->get_requested_power =
>   			devfreq_cooling_get_requested_power;
> -		devfreq_cooling_ops.state2power = devfreq_cooling_state2power;
> -		devfreq_cooling_ops.power2state = devfreq_cooling_power2state;
> +		ops->state2power = devfreq_cooling_state2power;
> +		ops->power2state = devfreq_cooling_power2state;
>   
>   		dfc->power_ops = dfc_power;
>   
> @@ -407,8 +414,7 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
>   	if (!name)
>   		goto remove_qos_req;
>   
> -	cdev = thermal_of_cooling_device_register(np, name, dfc,
> -						  &devfreq_cooling_ops);
> +	cdev = thermal_of_cooling_device_register(np, name, dfc, ops);
>   	kfree(name);
>   
>   	if (IS_ERR(cdev)) {
> @@ -429,6 +435,8 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
>   	kfree(dfc->freq_table);
>   free_dfc:
>   	kfree(dfc);
> +free_ops:
> +	kfree(ops);
>   
>   	return ERR_PTR(err);
>   }
> @@ -510,11 +518,13 @@ EXPORT_SYMBOL_GPL(devfreq_cooling_em_register);
>   void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
>   {
>   	struct devfreq_cooling_device *dfc;
> +	const struct thermal_cooling_device_ops *ops;
>   	struct device *dev;
>   
>   	if (IS_ERR_OR_NULL(cdev))
>   		return;
>   
> +	ops = cdev->ops;
>   	dfc = cdev->devdata;
>   	dev = dfc->devfreq->dev.parent;
>   
> @@ -525,5 +535,6 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
>   
>   	kfree(dfc->freq_table);
>   	kfree(dfc);
> +	kfree(ops);
>   }
>   EXPORT_SYMBOL_GPL(devfreq_cooling_unregister);


Thank you for updating it, LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Regards,
Lukasz