Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 2/2] PM / devfreq: Add governor attribute flag for
 specifc sysfs nodes
To:     Dmitry Osipenko <digetx@gmail.com>, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org
Cc:     lukasz.luba@arm.com, enric.balletbo@collabora.com,
        hl@rock-chips.com, thierry.reding@gmail.com, jonathanh@nvidia.com,
        abel.vesa@nxp.com, k.konieczny@samsung.com,
        b.zolnierkie@samsung.com, chanwoo@kernel.org,
        myungjoo.ham@samsung.com, kyungmin.park@samsung.com
From:   Chanwoo Choi <cw00.choi@samsung.com>
Organization: Samsung Electronics
Message-ID: <5802dfbd-22d2-0efb-cc98-2dbb1d420367@samsung.com>
Date:   Thu, 22 Oct 2020 11:47:02 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101
        Thunderbird/59.0
MIME-Version: 1.0
In-Reply-To: <bee3ef10-ddec-5ad4-9daa-79d9b01651d9@gmail.com>
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20201020030407.21047-1-cw00.choi@samsung.com>
        <CGME20201020025045epcas1p4b26b6f23e2896c2d661b2cfa93505e21@epcas1p4.samsung.com>
        <20201020030407.21047-3-cw00.choi@samsung.com>
        <bee3ef10-ddec-5ad4-9daa-79d9b01651d9@gmail.com>
Precedence: bulk

On 10/22/20 11:30 AM, Dmitry Osipenko wrote:
> Hello Chanwoo,
> 
> 20.10.2020 06:04, Chanwoo Choi пишет:
>> @@ -1354,55 +1365,71 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>  	struct devfreq *df = to_devfreq(dev);
>>  	int ret;
>>  	char str_governor[DEVFREQ_NAME_LEN + 1];
>> -	const struct devfreq_governor *governor, *prev_governor;
>> +	const struct devfreq_governor *new_governor, *prev_governor;
>>  
>>  	if (!df->governor)
>>  		return -EINVAL;
>> +	prev_governor = df->governor;
>>  
>>  	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>  	if (ret != 1)
>>  		return -EINVAL;
>>  
>>  	mutex_lock(&devfreq_list_lock);
>> -	governor = try_then_request_governor(str_governor);
>> -	if (IS_ERR(governor)) {
>> -		ret = PTR_ERR(governor);
>> +	new_governor = try_then_request_governor(str_governor);
>> +	if (IS_ERR(new_governor)) {
>> +		ret = PTR_ERR(new_governor);
>>  		goto out;
>>  	}
>> -	if (df->governor == governor) {
>> +	if (prev_governor == new_governor) {
>>  		ret = 0;
>>  		goto out;
>> -	} else if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)
>> -		|| IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE)) {
>> +	} else if (IS_SUPPORTED_FLAG(prev_governor->flags, IMMUTABLE)
>> +		|| IS_SUPPORTED_FLAG(new_governor->flags, IMMUTABLE)) {
>>  		ret = -EINVAL;
>>  		goto out;
>>  	}
>>  
>> -	ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
>> +	/*
>> +	 * Stop the previous governor and remove the specific sysfs files
>> +	 * which depend on previous governor.
>> +	 */
>> +	ret = prev_governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
>>  	if (ret) {
>>  		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
>> -			 __func__, df->governor->name, ret);
>> +			 __func__, prev_governor->name, ret);
>>  		goto out;
>>  	}
>>  
>> -	prev_governor = df->governor;
>> -	df->governor = governor;
>> -	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>> -	ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>> +	remove_sysfs_files(df, prev_governor);
>> +
>> +	/*
>> +	 * Start the new governor and create the specific sysfs files
>> +	 * which depend on new governor.
>> +	 */
>> +	strncpy(df->governor_name, new_governor->name, DEVFREQ_NAME_LEN);
>> +	ret = new_governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> 
> The "df->governor = new_governor" needs to be set before invocation of
> the event_handler(GOV_START) because governors like a
> performance-governor need to have the df->governor being set before the
> GOV_START callback, otherwise performance governor will use
> devfreq->prev_governor->get_target_freq() in devfreq_update_target(),
> thus setting a wrong (non-max) freq.

You're right. It is my mistake. Thanks for your detailed review.

> 
>>  	if (ret) {
>>  		dev_warn(dev, "%s: Governor %s not started(%d)\n",
>> -			 __func__, df->governor->name, ret);
>> -		df->governor = prev_governor;
>> +			 __func__, new_governor->name, ret);
>>  		strncpy(df->governor_name, prev_governor->name,
>>  			DEVFREQ_NAME_LEN);
>> -		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> 
> Same here.

OK.

> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics