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

[MyungJoo Ham <myungjoo.ham@samsung.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 3186 bytes --]

>   
>  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@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.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?


[]
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥