Re: [PATCH v2 1/2] PM / devfreq: Clean up the devfreq instance name in sysfs attr

[Chanwoo Choi <cw00.choi@samsung.com>]

On 7/13/20 5:31 PM, Chanwoo Choi wrote:
> The sysfs attr interface used eithere 'df' or 'devfreq' for devfreq instance
> name. In order to keep the consistency and to improve the readabilty,
> unify the instance name as 'df'. Add add the missing conditional statement
> to prevent the fault.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 94 +++++++++++++++++++++++++--------------
>  1 file changed, 60 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 5320c3b37f35..286957f760f1 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1280,18 +1280,20 @@ EXPORT_SYMBOL(devfreq_remove_governor);
>  static ssize_t name_show(struct device *dev,
>  			struct device_attribute *attr, char *buf)
>  {
> -	struct devfreq *devfreq = to_devfreq(dev);
> -	return sprintf(buf, "%s\n", dev_name(devfreq->dev.parent));
> +	struct devfreq *df = to_devfreq(dev);
> +	return sprintf(buf, "%s\n", dev_name(df->dev.parent));
>  }
>  static DEVICE_ATTR_RO(name);
>  
>  static ssize_t governor_show(struct device *dev,
>  			     struct device_attribute *attr, char *buf)
>  {
> -	if (!to_devfreq(dev)->governor)
> +	struct devfreq *df = to_devfreq(dev);
> +
> +	if (!df->governor)
>  		return -EINVAL;
>  
> -	return sprintf(buf, "%s\n", to_devfreq(dev)->governor->name);
> +	return sprintf(buf, "%s\n", df->governor->name);
>  }
>  
>  static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
> @@ -1302,6 +1304,9 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	char str_governor[DEVFREQ_NAME_LEN + 1];
>  	const struct devfreq_governor *governor, *prev_governor;
>  
> +	if (!df->governor)
> +		return -EINVAL;
> +
>  	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>  	if (ret != 1)
>  		return -EINVAL;
> @@ -1315,20 +1320,18 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	if (df->governor == governor) {
>  		ret = 0;
>  		goto out;
> -	} else if ((df->governor && df->governor->immutable) ||
> -					governor->immutable) {
> +	} else if (df->governor->immutable || governor->immutable) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  
> -	if (df->governor) {
> -		ret = df->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);
> -			goto out;
> -		}
> +	ret = df->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);
> +		goto out;
>  	}
> +
>  	prev_governor = df->governor;
>  	df->governor = governor;
>  	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
> @@ -1363,13 +1366,16 @@ static ssize_t available_governors_show(struct device *d,
>  	struct devfreq *df = to_devfreq(d);
>  	ssize_t count = 0;
>  
> +	if (!df->governor)
> +		return -EINVAL;
> +
>  	mutex_lock(&devfreq_list_lock);
>  
>  	/*
>  	 * The devfreq with immutable governor (e.g., passive) shows
>  	 * only own governor.
>  	 */
> -	if (df->governor && df->governor->immutable) {
> +	if (df->governor->immutable) {
>  		count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
>  				  "%s ", df->governor_name);
>  	/*
> @@ -1403,27 +1409,37 @@ static ssize_t cur_freq_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
>  	unsigned long freq;
> -	struct devfreq *devfreq = to_devfreq(dev);
> +	struct devfreq *df = to_devfreq(dev);
>  
> -	if (devfreq->profile->get_cur_freq &&
> -		!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
> +	if (!df->profile)
> +		return -EINVAL;
> +
> +	if (df->profile->get_cur_freq &&
> +		!df->profile->get_cur_freq(df->dev.parent, &freq))
>  		return sprintf(buf, "%lu\n", freq);
>  
> -	return sprintf(buf, "%lu\n", devfreq->previous_freq);
> +	return sprintf(buf, "%lu\n", df->previous_freq);
>  }
>  static DEVICE_ATTR_RO(cur_freq);
>  
>  static ssize_t target_freq_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> -	return sprintf(buf, "%lu\n", to_devfreq(dev)->previous_freq);
> +	struct devfreq *df = to_devfreq(dev);
> +
> +	return sprintf(buf, "%lu\n", df->previous_freq);
>  }
>  static DEVICE_ATTR_RO(target_freq);
>  
>  static ssize_t polling_interval_show(struct device *dev,
>  				     struct device_attribute *attr, char *buf)
>  {
> -	return sprintf(buf, "%d\n", to_devfreq(dev)->profile->polling_ms);
> +	struct devfreq *df = to_devfreq(dev);
> +
> +	if (!df->profile)
> +		return -EINVAL;
> +
> +	return sprintf(buf, "%d\n", df->profile->polling_ms);
>  }
>  
>  static ssize_t polling_interval_store(struct device *dev,
> @@ -1551,6 +1567,9 @@ static ssize_t available_frequencies_show(struct device *d,
>  	ssize_t count = 0;
>  	int i;
>  
> +	if (!df->profile)
> +		return -EINVAL;
> +
>  	mutex_lock(&df->lock);
>  
>  	for (i = 0; i < df->profile->max_state; i++)
> @@ -1571,49 +1590,53 @@ static DEVICE_ATTR_RO(available_frequencies);
>  static ssize_t trans_stat_show(struct device *dev,
>  			       struct device_attribute *attr, char *buf)
>  {
> -	struct devfreq *devfreq = to_devfreq(dev);
> +	struct devfreq *df = to_devfreq(dev);
>  	ssize_t len;
>  	int i, j;
> -	unsigned int max_state = devfreq->profile->max_state;
> +	unsigned int max_state;
> +
> +	if (!df->profile)
> +		return -EINVAL;
> +	max_state = df->profile->max_state;
>  
>  	if (max_state == 0)
>  		return sprintf(buf, "Not Supported.\n");
>  
> -	mutex_lock(&devfreq->lock);
> -	if (!devfreq->stop_polling &&
> -			devfreq_update_status(devfreq, devfreq->previous_freq)) {
> -		mutex_unlock(&devfreq->lock);
> +	mutex_lock(&df->lock);
> +	if (!df->stop_polling &&
> +			devfreq_update_status(df, df->previous_freq)) {
> +		mutex_unlock(&df->lock);
>  		return 0;
>  	}
> -	mutex_unlock(&devfreq->lock);
> +	mutex_unlock(&df->lock);
>  
>  	len = sprintf(buf, "     From  :   To\n");
>  	len += sprintf(buf + len, "           :");
>  	for (i = 0; i < max_state; i++)
>  		len += sprintf(buf + len, "%10lu",
> -				devfreq->profile->freq_table[i]);
> +				df->profile->freq_table[i]);
>  
>  	len += sprintf(buf + len, "   time(ms)\n");
>  
>  	for (i = 0; i < max_state; i++) {
> -		if (devfreq->profile->freq_table[i]
> -					== devfreq->previous_freq) {
> +		if (df->profile->freq_table[i]
> +					== df->previous_freq) {
>  			len += sprintf(buf + len, "*");
>  		} else {
>  			len += sprintf(buf + len, " ");
>  		}
>  		len += sprintf(buf + len, "%10lu:",
> -				devfreq->profile->freq_table[i]);
> +				df->profile->freq_table[i]);
>  		for (j = 0; j < max_state; j++)
>  			len += sprintf(buf + len, "%10u",
> -				devfreq->stats.trans_table[(i * max_state) + j]);
> +				df->stats.trans_table[(i * max_state) + j]);
>  
>  		len += sprintf(buf + len, "%10llu\n", (u64)
> -			jiffies64_to_msecs(devfreq->stats.time_in_state[i]));
> +			jiffies64_to_msecs(df->stats.time_in_state[i]));
>  	}
>  
>  	len += sprintf(buf + len, "Total transition : %u\n",
> -					devfreq->stats.total_trans);
> +					df->stats.total_trans);
>  	return len;
>  }
>  
> @@ -1624,6 +1647,9 @@ static ssize_t trans_stat_store(struct device *dev,
>  	struct devfreq *df = to_devfreq(dev);
>  	int err, value;
>  
> +	if (!df->profile)
> +		return -EINVAL;
> +
>  	if (df->profile->max_state == 0)
>  		return count;
>  
> 

Applied it. It is just clean-up patch for patch2.
patch2 needs more discussion. So, only apply patch1.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics