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 for Android: free password hash cracker in your pocket

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

Message-id: <54B3AD40.3040308@samsung.com>
Date:	Mon, 12 Jan 2015 20:17:20 +0900
From:	Chanwoo Choi <cw00.choi@...sung.com>
To:	myungjoo.ham@...sung.com
Cc:	"kgene@...nel.org" <kgene@...nel.org>,
	박경민 <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

Dear Myungjoo,

On 01/12/2015 04:15 PM, MyungJoo Ham wrote:
>>   
>>  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.

I think that one devfreq-event device can support multiple devfreq-event types.
but, devfreq-event device might provide only value at one point.

But,
This patch is ambiguous and includes a bug according to your comment
(below switch statement). I'll drop this patch on next patch-set.

This patch will be posted on further patch-set after resolving some issue.

Best Regards,
Chanwoo Choi

> 
> 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?
> 
> 
> []
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{�����x,�ȧ�.��ܨ}���Ơz�&j:+v���.����zZ+��+zf���h���~����i���z�.�w���?����&�)ߢ.fl===
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/