linux-kernel - Re: [PATCH v4 2/2] PM / devfreq: Add governor attribute flag for specifc sysfs nodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bee3ef10-ddec-5ad4-9daa-79d9b01651d9@gmail.com>
Date:   Thu, 22 Oct 2020 05:30:19 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Chanwoo Choi <cw00.choi@...sung.com>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Cc:     lukasz.luba@....com, enric.balletbo@...labora.com,
        hl@...k-chips.com, thierry.reding@...il.com, jonathanh@...dia.com,
        abel.vesa@....com, k.konieczny@...sung.com,
        b.zolnierkie@...sung.com, chanwoo@...nel.org,
        myungjoo.ham@...sung.com, kyungmin.park@...sung.com
Subject: Re: [PATCH v4 2/2] PM / devfreq: Add governor attribute flag for
 specifc sysfs nodes

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.

>  	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.