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: <00ed0b46-54db-330e-8ff8-146ff9a09491@gmail.com>
Date:   Thu, 13 Oct 2022 04:00:50 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v2] devfreq: governor: Add a private governor_data for
 governors in devfreq
Content-Language: en-US
To:     Kant Fan <kant@allwinnertech.com>, myungjoo.ham@samsung.com,
        kyungmin.park@samsung.com, cw00.choi@samsung.com
Cc:     linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20221010072202.78731-1-kant@allwinnertech.com>
From:   Chanwoo Choi <cwchoi00@gmail.com>
In-Reply-To: <20221010072202.78731-1-kant@allwinnertech.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

I'm sorry for late reply. It looks good to me.
Instead, this patch should contain the 'Fixes' information
with the following commit because the changed code was merged
on the patch[1]. Also, need to send it to stable mailing list.

[1] ce26c5bb9569d8b826f01b8620fc16d8da6821e9
PM / devfreq: Add basic governors

Lastly, I think that need to change the patch title as following:
- PM / devfreq: governor: Add a private governor_data for governor


On 22. 10. 10. 16:22, Kant Fan wrote:
> The member void *data in the structure devfreq can be overwrite
> by governor_userspace. For example:
> 1. The device driver assigned the devfreq governor to simple_ondemand
> by the function devfreq_add_device() and init the devfreq member
> void *data to a pointer of a static structure devfreq_simple_ondemand_data
> by the function devfreq_add_device().
> 2. The user changed the devfreq governor to userspace by the command
> "echo userspace > /sys/class/devfreq/.../governor".
> 3. The governor userspace alloced a dynamic memory for the struct
> userspace_data and assigend the member void *data of devfreq to
> this memory by the function userspace_init().
> 4. The user changed the devfreq governor back to simple_ondemand
> by the command "echo simple_ondemand > /sys/class/devfreq/.../governor".
> 5. The governor userspace exited and assigned the member void *data
> in the structure devfreq to NULL by the function userspace_exit().
> 6. The governor simple_ondemand fetched the static information of
> devfreq_simple_ondemand_data in the function
> devfreq_simple_ondemand_func() but the member void *data of devfreq was
> assigned to NULL by the function userspace_exit().
> 7. The information of upthreshold and downdifferential is lost
> and the governor simple_ondemand can't work correctly.
> 
> The member void *data in the structure devfreq is designed for
> a static pointer used in a governor and inited by the function
> devfreq_add_device(). This patch add an element named governor_data
> in the devfreq structure which can be used by a governor(E.g userspace)
> who want to assign a private data to do some private things.
> 
> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> 
> Signed-off-by: Kant Fan <kant@allwinnertech.com>
> ---
>  drivers/devfreq/governor_userspace.c | 12 ++++++------
>  include/linux/devfreq.h              |  7 ++++---
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
> index ab9db7adb3ad..d69672ccacc4 100644
> --- a/drivers/devfreq/governor_userspace.c
> +++ b/drivers/devfreq/governor_userspace.c
> @@ -21,7 +21,7 @@ struct userspace_data {
>  
>  static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq)
>  {
> -	struct userspace_data *data = df->data;
> +	struct userspace_data *data = df->governor_data;
>  
>  	if (data->valid)
>  		*freq = data->user_frequency;
> @@ -40,7 +40,7 @@ static ssize_t set_freq_store(struct device *dev, struct device_attribute *attr,
>  	int err = 0;
>  
>  	mutex_lock(&devfreq->lock);
> -	data = devfreq->data;
> +	data = devfreq->governor_data;
>  
>  	sscanf(buf, "%lu", &wanted);
>  	data->user_frequency = wanted;
> @@ -60,7 +60,7 @@ static ssize_t set_freq_show(struct device *dev,
>  	int err = 0;
>  
>  	mutex_lock(&devfreq->lock);
> -	data = devfreq->data;
> +	data = devfreq->governor_data;
>  
>  	if (data->valid)
>  		err = sprintf(buf, "%lu\n", data->user_frequency);
> @@ -91,7 +91,7 @@ static int userspace_init(struct devfreq *devfreq)
>  		goto out;
>  	}
>  	data->valid = false;
> -	devfreq->data = data;
> +	devfreq->governor_data = data;
>  
>  	err = sysfs_create_group(&devfreq->dev.kobj, &dev_attr_group);
>  out:
> @@ -107,8 +107,8 @@ static void userspace_exit(struct devfreq *devfreq)
>  	if (devfreq->dev.kobj.sd)
>  		sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
>  
> -	kfree(devfreq->data);
> -	devfreq->data = NULL;
> +	kfree(devfreq->governor_data);
> +	devfreq->governor_data = NULL;
>  }
>  
>  static int devfreq_userspace_handler(struct devfreq *devfreq,
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 34aab4dd336c..d265af3fb0a4 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -152,8 +152,8 @@ struct devfreq_stats {
>   * @max_state:		count of entry present in the frequency table.
>   * @previous_freq:	previously configured frequency value.
>   * @last_status:	devfreq user device info, performance statistics
> - * @data:	Private data of the governor. The devfreq framework does not
> - *		touch this.
> + * @data:	devfreq core pass to governors, governor should not change it.
> + * @governor_data:	private data for governors, devfreq core doesn't touch it.
>   * @user_min_freq_req:	PM QoS minimum frequency request from user (via sysfs)
>   * @user_max_freq_req:	PM QoS maximum frequency request from user (via sysfs)
>   * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
> @@ -193,7 +193,8 @@ struct devfreq {
>  	unsigned long previous_freq;
>  	struct devfreq_dev_status last_status;
>  
> -	void *data; /* private data for governors */
> +	void *data;
> +	void *governor_data;
>  
>  	struct dev_pm_qos_request user_min_freq_req;
>  	struct dev_pm_qos_request user_max_freq_req;

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi