Re: Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

Re: Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

[함명주]

> On 04/14/2014 06:36 PM, 함명주 wrote:
> >> MyungJoo/Kyungmin,
> >>
> >> Bump. Can we accept this patch please?
> >>
> >> -Saravana
> > 
> > Nack.
> > 
> > Please note that freq_table is also an optional value, which may
> > be null.
> 
> Ah, I saw that the max_freq would be zero if freq_table was NULL and I
> assumed that it can't be NULL. But I see that the max_freq limit is not
> applied if it's zero. Thanks for catching it.
> 
> > Besides, please be aware that your code is under rcu_read_lock().
> 
> Valid point. I was just trying to keep the diff simple. No one's really
> going to be catting this file often when performance matters.
> 
> > 
> > 
> > Cheers,
> > MyungJoo.
> > 
> > ps. I'll send a related patch (avoid accessing null but not-an-error
> > pointer at other sysfs nodes). Thank you for letting me catch such bugs anyway.
> 
> I can go ahead and do this myself if you don't mind.

No, we don't need it. It was a false alarm.
Reading again, I've found that we've already made other sysfs nodes
check if either freq_table is null or its size is 0.

So, we only need to look at this available_frequencies node now.

I'll add some notes on the ABI doc for available_frequencies soon.

Cheers,
MyungJoo.

> 
> -Saravana
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 
> 
>        
>   
>          
> 

Re: Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

[함명주]

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

> On 04/14/2014 06:36 PM, ÇÔ¸íÁÖ wrote:
> >> MyungJoo/Kyungmin,
> >>
> >> Bump. Can we accept this patch please?
> >>
> >> -Saravana
> > 
> > Nack.
> > 
> > Please note that freq_table is also an optional value, which may
> > be null.
> 
> Ah, I saw that the max_freq would be zero if freq_table was NULL and I
> assumed that it can't be NULL. But I see that the max_freq limit is not
> applied if it's zero. Thanks for catching it.
> 
> > Besides, please be aware that your code is under rcu_read_lock().
> 
> Valid point. I was just trying to keep the diff simple. No one's really
> going to be catting this file often when performance matters.
> 
> > 
> > 
> > Cheers,
> > MyungJoo.
> > 
> > ps. I'll send a related patch (avoid accessing null but not-an-error
> > pointer at other sysfs nodes). Thank you for letting me catch such bugs anyway.
> 
> I can go ahead and do this myself if you don't mind.

No, we don't need it. It was a false alarm.
Reading again, I've found that we've already made other sysfs nodes
check if either freq_table is null or its size is 0.

So, we only need to look at this available_frequencies node now.

I'll add some notes on the ABI doc for available_frequencies soon.

Cheers,
MyungJoo.

> 
> -Saravana
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 
> 
>        
>   
>          
> 
ÿôèº{.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¥

Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 04/14/2014 11:30 PM, 함명주 wrote:
>> On 04/14/2014 06:36 PM, 함명주 wrote:
>>>> MyungJoo/Kyungmin,
>>>>
>>>> Bump. Can we accept this patch please?
>>>>
>>>> -Saravana
>>>
>>> Nack.
>>>
>>> Please note that freq_table is also an optional value, which may
>>> be null.
>>
>> Ah, I saw that the max_freq would be zero if freq_table was NULL and I
>> assumed that it can't be NULL. But I see that the max_freq limit is not
>> applied if it's zero. Thanks for catching it.
>>
>>> Besides, please be aware that your code is under rcu_read_lock().
>>
>> Valid point. I was just trying to keep the diff simple. No one's really
>> going to be catting this file often when performance matters.
>>
>>>
>>>
>>> Cheers,
>>> MyungJoo.
>>>
>>> ps. I'll send a related patch (avoid accessing null but not-an-error
>>> pointer at other sysfs nodes). Thank you for letting me catch such bugs anyway.
>>
>> I can go ahead and do this myself if you don't mind.
> 
> No, we don't need it. It was a false alarm.
> Reading again, I've found that we've already made other sysfs nodes
> check if either freq_table is null or its size is 0.
> 
> So, we only need to look at this available_frequencies node now.
> 
> I'll add some notes on the ABI doc for available_frequencies soon.
> 

Ah, I misunderstood your previous email. I thought you Nack-ed my patch
and decided to send your own patch to replace mine. Ok, I'll fix up mine
and send it out.

-Saravana


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 04/14/2014 11:30 PM, ??? wrote:
>> On 04/14/2014 06:36 PM, ??? wrote:
>>>> MyungJoo/Kyungmin,
>>>>
>>>> Bump. Can we accept this patch please?
>>>>
>>>> -Saravana
>>>
>>> Nack.
>>>
>>> Please note that freq_table is also an optional value, which may
>>> be null.
>>
>> Ah, I saw that the max_freq would be zero if freq_table was NULL and I
>> assumed that it can't be NULL. But I see that the max_freq limit is not
>> applied if it's zero. Thanks for catching it.
>>
>>> Besides, please be aware that your code is under rcu_read_lock().
>>
>> Valid point. I was just trying to keep the diff simple. No one's really
>> going to be catting this file often when performance matters.
>>
>>>
>>>
>>> Cheers,
>>> MyungJoo.
>>>
>>> ps. I'll send a related patch (avoid accessing null but not-an-error
>>> pointer at other sysfs nodes). Thank you for letting me catch such bugs anyway.
>>
>> I can go ahead and do this myself if you don't mind.
> 
> No, we don't need it. It was a false alarm.
> Reading again, I've found that we've already made other sysfs nodes
> check if either freq_table is null or its size is 0.
> 
> So, we only need to look at this available_frequencies node now.
> 
> I'll add some notes on the ABI doc for available_frequencies soon.
> 

Ah, I misunderstood your previous email. I thought you Nack-ed my patch
and decided to send your own patch to replace mine. Ok, I'll fix up mine
and send it out.

-Saravana


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

Some devices use freq_table instead of OPP. For those devices, the
available_frequencies file shows up empty. Fix that by using freq_table to
generate the available_frequencies data when it's available.

OPP find frequency APIs also skips frequencies that have been temporarily
disabled (say, due to thermal, etc). Since available_frequencies is
supposed to show the entire list of available frequencies without taking
temporary limits into consideration, preference is given to freq_table when
available.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
 drivers/devfreq/devfreq.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2042ec3..527cbe2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -912,20 +912,27 @@ static ssize_t available_frequencies_show(struct device *d,
 	struct devfreq *df = to_devfreq(d);
 	struct device *dev = df->dev.parent;
 	struct dev_pm_opp *opp;
+	unsigned int i = 0;
 	ssize_t count = 0;
 	unsigned long freq = 0;
 
-	rcu_read_lock();
-	do {
-		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
-		if (IS_ERR(opp))
-			break;
-
-		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
-				   "%lu ", freq);
-		freq++;
-	} while (1);
-	rcu_read_unlock();
+	if (df->profile->freq_table) {
+		for (i = 0; i < df->profile->max_state; i++)
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					   "%u ", df->profile->freq_table[i]);
+	} else {
+		rcu_read_lock();
+		do {
+			opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+			if (IS_ERR(opp))
+				break;
+
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					   "%lu ", freq);
+			freq++;
+		} while (1);
+		rcu_read_unlock();
+	}
 
 	/* Truncate the trailing space */
 	if (count)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

Some devices use freq_table instead of OPP. For those devices, the
available_frequencies file shows up empty. Fix that by using freq_table to
generate the available_frequencies data when it's available.

OPP find frequency APIs also skips frequencies that have been temporarily
disabled (say, due to thermal, etc). Since available_frequencies is
supposed to show the entire list of available frequencies without taking
temporary limits into consideration, preference is given to freq_table when
available.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
 drivers/devfreq/devfreq.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2042ec3..527cbe2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -912,20 +912,27 @@ static ssize_t available_frequencies_show(struct device *d,
 	struct devfreq *df = to_devfreq(d);
 	struct device *dev = df->dev.parent;
 	struct dev_pm_opp *opp;
+	unsigned int i = 0;
 	ssize_t count = 0;
 	unsigned long freq = 0;
 
-	rcu_read_lock();
-	do {
-		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
-		if (IS_ERR(opp))
-			break;
-
-		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
-				   "%lu ", freq);
-		freq++;
-	} while (1);
-	rcu_read_unlock();
+	if (df->profile->freq_table) {
+		for (i = 0; i < df->profile->max_state; i++)
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					   "%u ", df->profile->freq_table[i]);
+	} else {
+		rcu_read_lock();
+		do {
+			opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+			if (IS_ERR(opp))
+				break;
+
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					   "%lu ", freq);
+			freq++;
+		} while (1);
+		rcu_read_unlock();
+	}
 
 	/* Truncate the trailing space */
 	if (count)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 04/15/2014 11:41 AM, Saravana Kannan wrote:
> Ah, I misunderstood your previous email. I thought you Nack-ed my patch
> and decided to send your own patch to replace mine. Ok, I'll fix up mine
> and send it out.

MyungJoo/Kyungmin,

I sent out an updated patch. Can you please take a look?

Thanks,
Saravana


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 04/15/2014 11:41 AM, Saravana Kannan wrote:
> Ah, I misunderstood your previous email. I thought you Nack-ed my patch
> and decided to send your own patch to replace mine. Ok, I'll fix up mine
> and send it out.

MyungJoo/Kyungmin,

I sent out an updated patch. Can you please take a look?

Thanks,
Saravana


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

[MyungJoo Ham]

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

> On 04/15/2014 11:41 AM, Saravana Kannan wrote:
> > Ah, I misunderstood your previous email. I thought you Nack-ed my patch
> > and decided to send your own patch to replace mine. Ok, I'll fix up mine
> > and send it out.
> 
> MyungJoo/Kyungmin,
> 
> I sent out an updated patch. Can you please take a look?
> 
> Thanks,
> Saravana

I'll be away from most of network system during the weekend.
And I'll be only accessing mobile phones (no laptops or desktops) until next Wednesday.

Please let me look at this a while later. (I'll have other pending tons of messages as well at that time)

Sorry for the extended delay.

Cheers,
MyungJoo.

> 
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 
ÿôèº{.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¥

Re: Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

[MyungJoo Ham]

> On 04/15/2014 11:41 AM, Saravana Kannan wrote:
> > Ah, I misunderstood your previous email. I thought you Nack-ed my patch
> > and decided to send your own patch to replace mine. Ok, I'll fix up mine
> > and send it out.
> 
> MyungJoo/Kyungmin,
> 
> I sent out an updated patch. Can you please take a look?
> 
> Thanks,
> Saravana

I'll be away from most of network system during the weekend.
And I'll be only accessing mobile phones (no laptops or desktops) until next Wednesday.

Please let me look at this a while later. (I'll have other pending tons of messages as well at that time)

Sorry for the extended delay.

Cheers,
MyungJoo.

> 
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 

Re: Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

[함명주]

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

> MyungJoo/Kyungmin,
> 
> Bump. Can we accept this patch please?
> 
> -Saravana

Nack.

Please note that freq_table is also an optional value, which may
be null.

Besides, please be aware that your code is under rcu_read_lock().


Cheers,
MyungJoo.

ps. I'll send a related patch (avoid accessing null but not-an-error
pointer at other sysfs nodes). Thank you for letting me catch such bugs anyway.

> 
> 
> On 04/10/2014 07:54 PM, Saravana Kannan wrote:
> > Some devices use freq_table instead of OPP. For those devices, the
> > available_frequencies file shows up empty. Fix that by using freq_table to
> > generate the available_frequencies data when OPP is not present.
> >
> > Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> > ---
> >   drivers/devfreq/devfreq.c | 15 +++++++++++----
> >   1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 2042ec3..a715d15 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -912,19 +912,26 @@ static ssize_t available_frequencies_show(struct device *d,
> >   	struct devfreq *df = to_devfreq(d);
> >   	struct device *dev = df->dev.parent;
> >   	struct dev_pm_opp *opp;
> > +	unsigned int i = 0, max_state = df->profile->max_state;
> > +	bool use_opp;
> >   	ssize_t count = 0;
> >   	unsigned long freq = 0;
> >
> >   	rcu_read_lock();
> > +	use_opp = dev_pm_opp_get_opp_count(dev) > 0;
> >   	do {
> > -		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> > -		if (IS_ERR(opp))
> > -			break;
> > +		if (use_opp) {
> > +			opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> > +			if (IS_ERR(opp))
> > +				break;
> > +		} else {
> > +			freq = df->profile->freq_table[i++];
> > +		}
> >
> >   		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> >   				   "%lu ", freq);
> >   		freq++;
> > -	} while (1);
> > +	} while (use_opp || (!use_opp && i < max_state));
> >   	rcu_read_unlock();
> >
> >   	/* Truncate the trailing space */
> >
> 
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 
> 
>        
>   
>          
> 
ÿôèº{.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¥

Re: Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

[함명주]

> MyungJoo/Kyungmin,
> 
> Bump. Can we accept this patch please?
> 
> -Saravana

Nack.

Please note that freq_table is also an optional value, which may
be null.

Besides, please be aware that your code is under rcu_read_lock().


Cheers,
MyungJoo.

ps. I'll send a related patch (avoid accessing null but not-an-error
pointer at other sysfs nodes). Thank you for letting me catch such bugs anyway.

> 
> 
> On 04/10/2014 07:54 PM, Saravana Kannan wrote:
> > Some devices use freq_table instead of OPP. For those devices, the
> > available_frequencies file shows up empty. Fix that by using freq_table to
> > generate the available_frequencies data when OPP is not present.
> >
> > Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> > ---
> >   drivers/devfreq/devfreq.c | 15 +++++++++++----
> >   1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 2042ec3..a715d15 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -912,19 +912,26 @@ static ssize_t available_frequencies_show(struct device *d,
> >   	struct devfreq *df = to_devfreq(d);
> >   	struct device *dev = df->dev.parent;
> >   	struct dev_pm_opp *opp;
> > +	unsigned int i = 0, max_state = df->profile->max_state;
> > +	bool use_opp;
> >   	ssize_t count = 0;
> >   	unsigned long freq = 0;
> >
> >   	rcu_read_lock();
> > +	use_opp = dev_pm_opp_get_opp_count(dev) > 0;
> >   	do {
> > -		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> > -		if (IS_ERR(opp))
> > -			break;
> > +		if (use_opp) {
> > +			opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> > +			if (IS_ERR(opp))
> > +				break;
> > +		} else {
> > +			freq = df->profile->freq_table[i++];
> > +		}
> >
> >   		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> >   				   "%lu ", freq);
> >   		freq++;
> > -	} while (1);
> > +	} while (use_opp || (!use_opp && i < max_state));
> >   	rcu_read_unlock();
> >
> >   	/* Truncate the trailing space */
> >
> 
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 
> 
>        
>   
>          
>