linux-kernel - Re: [PATCHv7 02/10] devfreq: event: Add the list of supported devfreq-event type

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-next>] [day] [month] [year] [list]

Date:	Mon, 12 Jan 2015 07:15:55 +0000 (GMT)
From:	MyungJoo Ham <myungjoo.ham@...sung.com>
To:	최찬우 <cw00.choi@...sung.com>,
	"kgene@...nel.org" <kgene@...nel.org>
Cc:	박경민 <kyungmin.park@...sung.com>,
	"rafael.j.wysocki@...el.com" <rafael.j.wysocki@...el.com>,
	"mark.rutland@....com" <mark.rutland@....com>,
	ABHILASH KESAVAN <a.kesavan@...sung.com>,
	"tomasz.figa@...il.com" <tomasz.figa@...il.com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	대인기 <inki.dae@...sung.com>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-samsung-soc@...r.kernel.org" 
	<linux-samsung-soc@...r.kernel.org>
Subject: Re: [PATCHv7 02/10] devfreq: event: Add the list of supported
 devfreq-event type

>   
>  This patch adds the list of supported devfreq-event type as following.
> Each devfreq-event device driver would support the various devfreq-event type
> for devfreq governor at the same time.
> - DEVFREQ_EVENT_TYPE_RAW_DATA
> - DEVFREQ_EVENT_TYPE_UTILIZATION
> - DEVFREQ_EVENT_TYPE_BANDWIDTH
> - DEVFREQ_EVENT_TYPE_LATENCY

Did you try to say:

A devfreq-event device may support multiple devfreq-event types
simultaneously.

If so, your switch expressions are going to screw up.


> 
> Cc: MyungJoo Ham <myungjoo.ham@...sung.com>
> Cc: Kyungmin Park <kyungmin.park@...sung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
> ---
>  drivers/devfreq/devfreq-event.c | 58 ++++++++++++++++++++++++++++++++++++-----
>  include/linux/devfreq-event.h   | 25 +++++++++++++++---
>  2 files changed, 73 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
> index 81448ba..64c1764 100644
> --- a/drivers/devfreq/devfreq-event.c
> +++ b/drivers/devfreq/devfreq-event.c
>  
[]
> -	mutex_lock(&edev->lock);
> -	ret = edev->desc->ops->get_event(edev, edata);
> -	mutex_unlock(&edev->lock);
> +	switch (type) {

Bitwise value with switch? (what if type = RAW_DATA | BANDWIDTH, meaning
this is raw data of the bandwitdh.)

> +	case DEVFREQ_EVENT_TYPE_RAW_DATA:
> +	case DEVFREQ_EVENT_TYPE_BANDWIDTH:
> +	case DEVFREQ_EVENT_TYPE_LATENCY:
> +		if ((edata->event > EVENT_TYPE_RAW_DATA_MAX) ||
> +			(edata->total_event > EVENT_TYPE_RAW_DATA_MAX)) {

Is it possible for unsigned long edata->event/total_event to be
  > EVENT_TYPE_RAW_DATA_MAX = ULONG_MAX?

What was your intention?

If you were trying to detect overflow, you need to rethink about it.
If not, (overflow is harmless or not going to happen) you don't need to
check it.


> +			edata->event = edata->total_event = 0;
> +			ret = -EINVAL;
> +		}
> +		break;
> +	case DEVFREQ_EVENT_TYPE_UTILIZATION:
> +		edata->total_event = EVENT_TYPE_UTILIZATION_MAX;
>  
> -	if ((edata->total_event <= 0)
> -		|| (edata->event > edata->total_event)) {
> +		if (edata->event > EVENT_TYPE_UTILIZATION_MAX) {
> +			edata->event = edata->total_event = 0;
> +			ret = -EINVAL;
> +		}
> +		break;
> +	default:
>  		edata->event = edata->total_event = 0;
>  		ret = -EINVAL;
> +		break;
>  	}
>  
> +	mutex_unlock(&edev->lock);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(devfreq_event_get_event);
> diff --git a/include/linux/devfreq-event.h b/include/linux/devfreq-event.h
> index b7363f5..13a5703 100644
> --- a/include/linux/devfreq-event.h
> +++ b/include/linux/devfreq-event.h
> @@ -36,6 +36,14 @@ struct devfreq_event_dev {
>  	const struct devfreq_event_desc *desc;
>  };
>  
> +/* The supported type by devfreq-event device */
> +enum devfreq_event_type {
> +	DEVFREQ_EVENT_TYPE_RAW_DATA	= BIT(0),
> +	DEVFREQ_EVENT_TYPE_UTILIZATION	= BIT(1),
> +	DEVFREQ_EVENT_TYPE_BANDWIDTH	= BIT(2),
> +	DEVFREQ_EVENT_TYPE_LATENCY	= BIT(3),
> +};
> +

(Being curious) Is it possible to have multiple types
simultaneously?


[]