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 <[email protected]>
---
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
MyungJoo/Kyungmin,
Bump. Can we accept this patch please?
-Saravana
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 <[email protected]>
> ---
> 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
> 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 <[email protected]>
> > ---
> > 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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.
-Saravana
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
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 <[email protected]>
---
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
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
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> 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 <[email protected]>
> ---
> 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]);
You are hereby changing the semmantics of the original
available_frequencies node.
When a frequency/voltage pair has been disabled (opp_disable), probably
by opp_disable(), the frequency is no more "available".
However, when the driver author supplied freq_table as well as OPP
in order to see the statistics, the node will behave differently.
Please do not affect the current users as long as it does not give
additional benefit or fix a bug.
Cheers,
MyungJoo.
> + } 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
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 04/27/2014 06:41 PM, MyungJoo Ham 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 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 <[email protected]>
>> ---
>> 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]);
>
> You are hereby changing the semmantics of the original
> available_frequencies node.
>
> When a frequency/voltage pair has been disabled (opp_disable), probably
> by opp_disable(), the frequency is no more "available".
> However, when the driver author supplied freq_table as well as OPP
> in order to see the statistics, the node will behave differently.
>
> Please do not affect the current users as long as it does not give
> additional benefit or fix a bug.
I was actually trying to stick with the semantics as it was documented.
The documentation for this file says it'll show frequencies that are not
allowed by the current min/max settings either. To me, an OPP disable
seems similar to some frequencies "disabled" by min/max settings.
Giving preference to OPP is not a hard change to do, but it seems to go
againsts the documented semantics.
Thoughts?
-Saravana
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation