| Message ID | 20200506174238.15385-21-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On 5/6/2020 7:42 PM, Sergey.Semin@baikalelectronics.ru wrote: > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > Recent commit e61a41256edf ("cpufreq: dev_pm_qos_update_request() can > return 1 on success") fixed a problem when active policies traverse > was falsely stopped due to invalidly treating the non-zero return value > from freq_qos_update_request() method as an error. Yes, that function > can return positive values if the requested update actually took place. > The current problem is that the returned value is then passed to the > return cell of the cpufreq_boost_set_sw() (set_boost callback) method. > This value is then also analyzed for being non-zero, which is also > treated as having an error. As a result during the boost activation > we'll get an error returned while having the QOS frequency update > successfully performed. Fix this by returning a negative value from the > cpufreq_boost_set_sw() if actual error was encountered and zero > otherwise treating any positive values as the successful operations > completion. > > Fixes: 18c49926c4bf ("cpufreq: Add QoS requests for userspace constraints") > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Paul Burton <paulburton@kernel.org> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: linux-mips@vger.kernel.org > Cc: devicetree@vger.kernel.org > Cc: stable@vger.kernel.org > --- > drivers/cpufreq/cpufreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 045f9fe157ce..5870cdca88cf 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state) > break; > } > > - return ret; > + return ret < 0 ? ret : 0; > } > > int cpufreq_boost_trigger_state(int state) IMO it is better to update the caller of this function to handle the positive value possibly returned by it correctly. Thanks!
Hello Rafael, On Fri, May 15, 2020 at 05:58:47PM +0200, Rafael J. Wysocki wrote: > On 5/6/2020 7:42 PM, Sergey.Semin@baikalelectronics.ru wrote: > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > Recent commit e61a41256edf ("cpufreq: dev_pm_qos_update_request() can > > return 1 on success") fixed a problem when active policies traverse > > was falsely stopped due to invalidly treating the non-zero return value > > from freq_qos_update_request() method as an error. Yes, that function > > can return positive values if the requested update actually took place. > > The current problem is that the returned value is then passed to the > > return cell of the cpufreq_boost_set_sw() (set_boost callback) method. > > This value is then also analyzed for being non-zero, which is also > > treated as having an error. As a result during the boost activation > > we'll get an error returned while having the QOS frequency update > > successfully performed. Fix this by returning a negative value from the > > cpufreq_boost_set_sw() if actual error was encountered and zero > > otherwise treating any positive values as the successful operations > > completion. > > > > Fixes: 18c49926c4bf ("cpufreq: Add QoS requests for userspace constraints") > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Cc: Paul Burton <paulburton@kernel.org> > > Cc: Ralf Baechle <ralf@linux-mips.org> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: linux-mips@vger.kernel.org > > Cc: devicetree@vger.kernel.org > > Cc: stable@vger.kernel.org > > --- > > drivers/cpufreq/cpufreq.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 045f9fe157ce..5870cdca88cf 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state) > > break; > > } > > - return ret; > > + return ret < 0 ? ret : 0; > > } > > int cpufreq_boost_trigger_state(int state) > > IMO it is better to update the caller of this function to handle the > positive value possibly returned by it correctly. Could you elaborate why? Viresh seems to be ok with this solution. As I see it the caller doesn't expect the positive value returned by the original freq_qos_update_request(). It just doesn't need to know whether the effective policy has been updated or not, it only needs to make sure the operations has been successful. Moreover the positive value is related only to the !last! active policy, which doesn't give the caller a full picture of the policy change anyway. So taking all of these into account I'd leave the fix as is. Regards, -Sergey > > Thanks! > >
On 16-05-20, 15:52, Serge Semin wrote: > On Fri, May 15, 2020 at 05:58:47PM +0200, Rafael J. Wysocki wrote: > > > @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state) > > > break; > > > } > > > - return ret; > > > + return ret < 0 ? ret : 0; > > > } > > > int cpufreq_boost_trigger_state(int state) > > > > IMO it is better to update the caller of this function to handle the > > positive value possibly returned by it correctly. > > Could you elaborate why? Viresh seems to be ok with this solution. And it is absolutely fine for Rafael to not agree with it :) > As I see it the caller doesn't expect the positive value returned by the > original freq_qos_update_request(). It just doesn't need to know whether the > effective policy has been updated or not, it only needs to make sure the > operations has been successful. Moreover the positive value is related only > to the !last! active policy, which doesn't give the caller a full picture > of the policy change anyway. So taking all of these into account I'd leave the > fix as is. Rafael: This function is called via a function pointer, which can call this or a platform dependent routine (like in acpi-cpufreq.c), and it would be reasonable IMO for the return of that callback to only look for 0 or negative values, as is generally done in the kernel. And so this solution looked okay to me as the positive return is coming from the implementation detail here.
On 5/18/2020 9:41 AM, Viresh Kumar wrote: > On 16-05-20, 15:52, Serge Semin wrote: >> On Fri, May 15, 2020 at 05:58:47PM +0200, Rafael J. Wysocki wrote: >>>> @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state) >>>> break; >>>> } >>>> - return ret; >>>> + return ret < 0 ? ret : 0; >>>> } >>>> int cpufreq_boost_trigger_state(int state) >>> IMO it is better to update the caller of this function to handle the >>> positive value possibly returned by it correctly. >> Could you elaborate why? Viresh seems to be ok with this solution. > And it is absolutely fine for Rafael to not agree with it :) > >> As I see it the caller doesn't expect the positive value returned by the >> original freq_qos_update_request(). It just doesn't need to know whether the >> effective policy has been updated or not, it only needs to make sure the >> operations has been successful. Moreover the positive value is related only >> to the !last! active policy, which doesn't give the caller a full picture >> of the policy change anyway. So taking all of these into account I'd leave the >> fix as is. > Rafael: This function is called via a function pointer, which can call > this or a platform dependent routine (like in acpi-cpufreq.c), and it > would be reasonable IMO for the return of that callback to only look > for 0 or negative values, as is generally done in the kernel. But it only has one caller that can easily check ret < 0 instead of just ret, so the extra branch can be saved. That said if you really only want it to return 0 on success, you may as well add a ret = 0; statement (with a comment explaining why it is needed) after the last break in the loop. Cheers!
On 18-05-20, 11:53, Rafael J. Wysocki wrote: > That said if you really only want it to return 0 on success, you may as well > add a ret = 0; statement (with a comment explaining why it is needed) after > the last break in the loop. That can be done as well, but will be a bit less efficient as the loop will execute once for each policy, and so the statement will run multiple times. Though it isn't going to add any significant latency in the code.
On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: > On 18-05-20, 11:53, Rafael J. Wysocki wrote: > > That said if you really only want it to return 0 on success, you may as well > > add a ret = 0; statement (with a comment explaining why it is needed) after > > the last break in the loop. > > That can be done as well, but will be a bit less efficient as the loop > will execute once for each policy, and so the statement will run > multiple times. Though it isn't going to add any significant latency > in the code. Right. However, the logic in this entire function looks somewhat less than straightforward to me, because it looks like it should return an error on the first policy without a frequency table (having a frequency table depends on the driver and that is the same for all policies, so it is pointless to iterate any further in that case). Also, the error should not be -EINVAL, because that means "invalid argument" which would be the state value. So I would do something like this: --- drivers/cpufreq/cpufreq.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) Index: linux-pm/drivers/cpufreq/cpufreq.c =================================================================== --- linux-pm.orig/drivers/cpufreq/cpufreq.c +++ linux-pm/drivers/cpufreq/cpufreq.c @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) static int cpufreq_boost_set_sw(int state) { struct cpufreq_policy *policy; - int ret = -EINVAL; for_each_active_policy(policy) { + int ret; + if (!policy->freq_table) - continue; + return -ENXIO; ret = cpufreq_frequency_table_cpuinfo(policy, policy->freq_table); if (ret) { pr_err("%s: Policy frequency update failed\n", __func__); - break; + return ret; } ret = freq_qos_update_request(policy->max_freq_req, policy->max); if (ret < 0) - break; + return ret; } - return ret; + return 0; } int cpufreq_boost_trigger_state(int state)
On 18-05-20, 12:22, Rafael J. Wysocki wrote: > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: > > On 18-05-20, 11:53, Rafael J. Wysocki wrote: > > > That said if you really only want it to return 0 on success, you may as well > > > add a ret = 0; statement (with a comment explaining why it is needed) after > > > the last break in the loop. > > > > That can be done as well, but will be a bit less efficient as the loop > > will execute once for each policy, and so the statement will run > > multiple times. Though it isn't going to add any significant latency > > in the code. > > Right. > > However, the logic in this entire function looks somewhat less than > straightforward to me, because it looks like it should return an > error on the first policy without a frequency table (having a frequency > table depends on the driver and that is the same for all policies, so it > is pointless to iterate any further in that case). > > Also, the error should not be -EINVAL, because that means "invalid > argument" which would be the state value. > > So I would do something like this: > > --- > drivers/cpufreq/cpufreq.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/cpufreq/cpufreq.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > +++ linux-pm/drivers/cpufreq/cpufreq.c > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) > static int cpufreq_boost_set_sw(int state) > { > struct cpufreq_policy *policy; > - int ret = -EINVAL; > > for_each_active_policy(policy) { > + int ret; > + > if (!policy->freq_table) > - continue; > + return -ENXIO; > > ret = cpufreq_frequency_table_cpuinfo(policy, > policy->freq_table); > if (ret) { > pr_err("%s: Policy frequency update failed\n", > __func__); > - break; > + return ret; > } > > ret = freq_qos_update_request(policy->max_freq_req, policy->max); > if (ret < 0) > - break; > + return ret; > } > > - return ret; > + return 0; > } > > int cpufreq_boost_trigger_state(int state) Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote: > On 18-05-20, 12:22, Rafael J. Wysocki wrote: > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote: > > > > That said if you really only want it to return 0 on success, you may as well > > > > add a ret = 0; statement (with a comment explaining why it is needed) after > > > > the last break in the loop. > > > > > > That can be done as well, but will be a bit less efficient as the loop > > > will execute once for each policy, and so the statement will run > > > multiple times. Though it isn't going to add any significant latency > > > in the code. > > > > Right. > > > > However, the logic in this entire function looks somewhat less than > > straightforward to me, because it looks like it should return an > > error on the first policy without a frequency table (having a frequency > > table depends on the driver and that is the same for all policies, so it > > is pointless to iterate any further in that case). > > > > Also, the error should not be -EINVAL, because that means "invalid > > argument" which would be the state value. > > > > So I would do something like this: > > > > --- > > drivers/cpufreq/cpufreq.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > =================================================================== > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) > > static int cpufreq_boost_set_sw(int state) > > { > > struct cpufreq_policy *policy; > > - int ret = -EINVAL; > > > > for_each_active_policy(policy) { > > + int ret; > > + > > if (!policy->freq_table) > > - continue; > > + return -ENXIO; > > > > ret = cpufreq_frequency_table_cpuinfo(policy, > > policy->freq_table); > > if (ret) { > > pr_err("%s: Policy frequency update failed\n", > > __func__); > > - break; > > + return ret; > > } > > > > ret = freq_qos_update_request(policy->max_freq_req, policy->max); > > if (ret < 0) > > - break; > > + return ret; > > } > > > > - return ret; > > + return 0; > > } > > > > int cpufreq_boost_trigger_state(int state) > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> Ok. Thanks for the comments. Shall I resend the patch with update Rafael suggests or you'll merge the Rafael's fix in yourself? -Sergey > > -- > viresh
On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote: > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote: > > On 18-05-20, 12:22, Rafael J. Wysocki wrote: > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote: > > > > > That said if you really only want it to return 0 on success, you may as well > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after > > > > > the last break in the loop. > > > > > > > > That can be done as well, but will be a bit less efficient as the loop > > > > will execute once for each policy, and so the statement will run > > > > multiple times. Though it isn't going to add any significant latency > > > > in the code. > > > > > > Right. > > > > > > However, the logic in this entire function looks somewhat less than > > > straightforward to me, because it looks like it should return an > > > error on the first policy without a frequency table (having a frequency > > > table depends on the driver and that is the same for all policies, so it > > > is pointless to iterate any further in that case). > > > > > > Also, the error should not be -EINVAL, because that means "invalid > > > argument" which would be the state value. > > > > > > So I would do something like this: > > > > > > --- > > > drivers/cpufreq/cpufreq.c | 11 ++++++----- > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > > =================================================================== > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) > > > static int cpufreq_boost_set_sw(int state) > > > { > > > struct cpufreq_policy *policy; > > > - int ret = -EINVAL; > > > > > > for_each_active_policy(policy) { > > > + int ret; > > > + > > > if (!policy->freq_table) > > > - continue; > > > + return -ENXIO; > > > > > > ret = cpufreq_frequency_table_cpuinfo(policy, > > > policy->freq_table); > > > if (ret) { > > > pr_err("%s: Policy frequency update failed\n", > > > __func__); > > > - break; > > > + return ret; > > > } > > > > > > ret = freq_qos_update_request(policy->max_freq_req, policy->max); > > > if (ret < 0) > > > - break; > > > + return ret; > > > } > > > > > > - return ret; > > > + return 0; > > > } > > > > > > int cpufreq_boost_trigger_state(int state) > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > Ok. Thanks for the comments. Shall I resend the patch with update Rafael > suggests or you'll merge the Rafael's fix in yourself? I'll apply the fix directly, thanks!
On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote: > On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote: > > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote: > > > On 18-05-20, 12:22, Rafael J. Wysocki wrote: > > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: > > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote: > > > > > > That said if you really only want it to return 0 on success, you may as well > > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after > > > > > > the last break in the loop. > > > > > > > > > > That can be done as well, but will be a bit less efficient as the loop > > > > > will execute once for each policy, and so the statement will run > > > > > multiple times. Though it isn't going to add any significant latency > > > > > in the code. > > > > > > > > Right. > > > > > > > > However, the logic in this entire function looks somewhat less than > > > > straightforward to me, because it looks like it should return an > > > > error on the first policy without a frequency table (having a frequency > > > > table depends on the driver and that is the same for all policies, so it > > > > is pointless to iterate any further in that case). > > > > > > > > Also, the error should not be -EINVAL, because that means "invalid > > > > argument" which would be the state value. > > > > > > > > So I would do something like this: > > > > > > > > --- > > > > drivers/cpufreq/cpufreq.c | 11 ++++++----- > > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > > > =================================================================== > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) > > > > static int cpufreq_boost_set_sw(int state) > > > > { > > > > struct cpufreq_policy *policy; > > > > - int ret = -EINVAL; > > > > > > > > for_each_active_policy(policy) { > > > > + int ret; > > > > + > > > > if (!policy->freq_table) > > > > - continue; > > > > + return -ENXIO; > > > > > > > > ret = cpufreq_frequency_table_cpuinfo(policy, > > > > policy->freq_table); > > > > if (ret) { > > > > pr_err("%s: Policy frequency update failed\n", > > > > __func__); > > > > - break; > > > > + return ret; > > > > } > > > > > > > > ret = freq_qos_update_request(policy->max_freq_req, policy->max); > > > > if (ret < 0) > > > > - break; > > > > + return ret; > > > > } > > > > > > > > - return ret; > > > > + return 0; > > > > } > > > > > > > > int cpufreq_boost_trigger_state(int state) > > > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > Ok. Thanks for the comments. Shall I resend the patch with update Rafael > > suggests or you'll merge the Rafael's fix in yourself? > > I'll apply the fix directly, thanks! Great. Is it going to be available in the repo: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/ ? I'll need it to back port into my local kernel tree. Thanks. -Sergey > > >
On Mon, May 18, 2020 at 12:46 PM Serge Semin <Sergey.Semin@baikalelectronics.ru> wrote: > > On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote: > > On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote: > > > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote: > > > > On 18-05-20, 12:22, Rafael J. Wysocki wrote: > > > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: > > > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote: > > > > > > > That said if you really only want it to return 0 on success, you may as well > > > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after > > > > > > > the last break in the loop. > > > > > > > > > > > > That can be done as well, but will be a bit less efficient as the loop > > > > > > will execute once for each policy, and so the statement will run > > > > > > multiple times. Though it isn't going to add any significant latency > > > > > > in the code. > > > > > > > > > > Right. > > > > > > > > > > However, the logic in this entire function looks somewhat less than > > > > > straightforward to me, because it looks like it should return an > > > > > error on the first policy without a frequency table (having a frequency > > > > > table depends on the driver and that is the same for all policies, so it > > > > > is pointless to iterate any further in that case). > > > > > > > > > > Also, the error should not be -EINVAL, because that means "invalid > > > > > argument" which would be the state value. > > > > > > > > > > So I would do something like this: > > > > > > > > > > --- > > > > > drivers/cpufreq/cpufreq.c | 11 ++++++----- > > > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > > > > =================================================================== > > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) > > > > > static int cpufreq_boost_set_sw(int state) > > > > > { > > > > > struct cpufreq_policy *policy; > > > > > - int ret = -EINVAL; > > > > > > > > > > for_each_active_policy(policy) { > > > > > + int ret; > > > > > + > > > > > if (!policy->freq_table) > > > > > - continue; > > > > > + return -ENXIO; > > > > > > > > > > ret = cpufreq_frequency_table_cpuinfo(policy, > > > > > policy->freq_table); > > > > > if (ret) { > > > > > pr_err("%s: Policy frequency update failed\n", > > > > > __func__); > > > > > - break; > > > > > + return ret; > > > > > } > > > > > > > > > > ret = freq_qos_update_request(policy->max_freq_req, policy->max); > > > > > if (ret < 0) > > > > > - break; > > > > > + return ret; > > > > > } > > > > > > > > > > - return ret; > > > > > + return 0; > > > > > } > > > > > > > > > > int cpufreq_boost_trigger_state(int state) > > > > > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > > Ok. Thanks for the comments. Shall I resend the patch with update Rafael > > > suggests or you'll merge the Rafael's fix in yourself? > > > > I'll apply the fix directly, thanks! > > Great. Is it going to be available in the repo: > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/ > ? Yes, it is. Please see the bleeding-edge branch in there, thanks!
On Mon, May 18, 2020 at 12:51:15PM +0200, Rafael J. Wysocki wrote: > On Mon, May 18, 2020 at 12:46 PM Serge Semin > <Sergey.Semin@baikalelectronics.ru> wrote: > > > > On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote: > > > On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote: > > > > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote: > > > > > On 18-05-20, 12:22, Rafael J. Wysocki wrote: > > > > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: > > > > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote: > > > > > > > > That said if you really only want it to return 0 on success, you may as well > > > > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after > > > > > > > > the last break in the loop. > > > > > > > > > > > > > > That can be done as well, but will be a bit less efficient as the loop > > > > > > > will execute once for each policy, and so the statement will run > > > > > > > multiple times. Though it isn't going to add any significant latency > > > > > > > in the code. > > > > > > > > > > > > Right. > > > > > > > > > > > > However, the logic in this entire function looks somewhat less than > > > > > > straightforward to me, because it looks like it should return an > > > > > > error on the first policy without a frequency table (having a frequency > > > > > > table depends on the driver and that is the same for all policies, so it > > > > > > is pointless to iterate any further in that case). > > > > > > > > > > > > Also, the error should not be -EINVAL, because that means "invalid > > > > > > argument" which would be the state value. > > > > > > > > > > > > So I would do something like this: > > > > > > > > > > > > --- > > > > > > drivers/cpufreq/cpufreq.c | 11 ++++++----- > > > > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > > > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > > > > > =================================================================== > > > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > > > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) > > > > > > static int cpufreq_boost_set_sw(int state) > > > > > > { > > > > > > struct cpufreq_policy *policy; > > > > > > - int ret = -EINVAL; > > > > > > > > > > > > for_each_active_policy(policy) { > > > > > > + int ret; > > > > > > + > > > > > > if (!policy->freq_table) > > > > > > - continue; > > > > > > + return -ENXIO; > > > > > > > > > > > > ret = cpufreq_frequency_table_cpuinfo(policy, > > > > > > policy->freq_table); > > > > > > if (ret) { > > > > > > pr_err("%s: Policy frequency update failed\n", > > > > > > __func__); > > > > > > - break; > > > > > > + return ret; > > > > > > } > > > > > > > > > > > > ret = freq_qos_update_request(policy->max_freq_req, policy->max); > > > > > > if (ret < 0) > > > > > > - break; > > > > > > + return ret; > > > > > > } > > > > > > > > > > > > - return ret; > > > > > > + return 0; > > > > > > } > > > > > > > > > > > > int cpufreq_boost_trigger_state(int state) > > > > > > > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > > > > Ok. Thanks for the comments. Shall I resend the patch with update Rafael > > > > suggests or you'll merge the Rafael's fix in yourself? > > > > > > I'll apply the fix directly, thanks! > > > > Great. Is it going to be available in the repo: > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/ > > ? > > Yes, it is. Please see the bleeding-edge branch in there, thanks! No credits with at least Reported-by tag? That's sad.( -Sergey
On Mon, May 18, 2020 at 12:56 PM Serge Semin <Sergey.Semin@baikalelectronics.ru> wrote: > > On Mon, May 18, 2020 at 12:51:15PM +0200, Rafael J. Wysocki wrote: > > On Mon, May 18, 2020 at 12:46 PM Serge Semin > > <Sergey.Semin@baikalelectronics.ru> wrote: > > > > > > On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote: > > > > On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote: > > > > > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote: > > > > > > On 18-05-20, 12:22, Rafael J. Wysocki wrote: > > > > > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: > > > > > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote: > > > > > > > > > That said if you really only want it to return 0 on success, you may as well > > > > > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after > > > > > > > > > the last break in the loop. > > > > > > > > > > > > > > > > That can be done as well, but will be a bit less efficient as the loop > > > > > > > > will execute once for each policy, and so the statement will run > > > > > > > > multiple times. Though it isn't going to add any significant latency > > > > > > > > in the code. > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > However, the logic in this entire function looks somewhat less than > > > > > > > straightforward to me, because it looks like it should return an > > > > > > > error on the first policy without a frequency table (having a frequency > > > > > > > table depends on the driver and that is the same for all policies, so it > > > > > > > is pointless to iterate any further in that case). > > > > > > > > > > > > > > Also, the error should not be -EINVAL, because that means "invalid > > > > > > > argument" which would be the state value. > > > > > > > > > > > > > > So I would do something like this: > > > > > > > > > > > > > > --- > > > > > > > drivers/cpufreq/cpufreq.c | 11 ++++++----- > > > > > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > > > > > > =================================================================== > > > > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > > > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > > > > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) > > > > > > > static int cpufreq_boost_set_sw(int state) > > > > > > > { > > > > > > > struct cpufreq_policy *policy; > > > > > > > - int ret = -EINVAL; > > > > > > > > > > > > > > for_each_active_policy(policy) { > > > > > > > + int ret; > > > > > > > + > > > > > > > if (!policy->freq_table) > > > > > > > - continue; > > > > > > > + return -ENXIO; > > > > > > > > > > > > > > ret = cpufreq_frequency_table_cpuinfo(policy, > > > > > > > policy->freq_table); > > > > > > > if (ret) { > > > > > > > pr_err("%s: Policy frequency update failed\n", > > > > > > > __func__); > > > > > > > - break; > > > > > > > + return ret; > > > > > > > } > > > > > > > > > > > > > > ret = freq_qos_update_request(policy->max_freq_req, policy->max); > > > > > > > if (ret < 0) > > > > > > > - break; > > > > > > > + return ret; > > > > > > > } > > > > > > > > > > > > > > - return ret; > > > > > > > + return 0; > > > > > > > } > > > > > > > > > > > > > > int cpufreq_boost_trigger_state(int state) > > > > > > > > > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > > > > > > Ok. Thanks for the comments. Shall I resend the patch with update Rafael > > > > > suggests or you'll merge the Rafael's fix in yourself? > > > > > > > > I'll apply the fix directly, thanks! > > > > > > Great. Is it going to be available in the repo: > > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/ > > > ? > > > > Yes, it is. Please see the bleeding-edge branch in there, thanks! > > No credits with at least Reported-by tag? That's sad.( OK, done now, but you are not the only reported of it, so I've added the other reporter too. Thanks!
Hi Rafael, On 2020/5/18 19:05, Rafael J. Wysocki wrote: > On Mon, May 18, 2020 at 12:56 PM Serge Semin > <Sergey.Semin@baikalelectronics.ru> wrote: >> >> On Mon, May 18, 2020 at 12:51:15PM +0200, Rafael J. Wysocki wrote: >>> On Mon, May 18, 2020 at 12:46 PM Serge Semin >>> <Sergey.Semin@baikalelectronics.ru> wrote: >>>> >>>> On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote: >>>>> On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote: >>>>>> On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote: >>>>>>> On 18-05-20, 12:22, Rafael J. Wysocki wrote: >>>>>>>> On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote: >>>>>>>>> On 18-05-20, 11:53, Rafael J. Wysocki wrote: >>>>>>>>>> That said if you really only want it to return 0 on success, you may as well >>>>>>>>>> add a ret = 0; statement (with a comment explaining why it is needed) after >>>>>>>>>> the last break in the loop. >>>>>>>>> >>>>>>>>> That can be done as well, but will be a bit less efficient as the loop >>>>>>>>> will execute once for each policy, and so the statement will run >>>>>>>>> multiple times. Though it isn't going to add any significant latency >>>>>>>>> in the code. >>>>>>>> >>>>>>>> Right. >>>>>>>> >>>>>>>> However, the logic in this entire function looks somewhat less than >>>>>>>> straightforward to me, because it looks like it should return an >>>>>>>> error on the first policy without a frequency table (having a frequency >>>>>>>> table depends on the driver and that is the same for all policies, so it >>>>>>>> is pointless to iterate any further in that case). >>>>>>>> >>>>>>>> Also, the error should not be -EINVAL, because that means "invalid >>>>>>>> argument" which would be the state value. >>>>>>>> >>>>>>>> So I would do something like this: >>>>>>>> >>>>>>>> --- >>>>>>>> drivers/cpufreq/cpufreq.c | 11 ++++++----- >>>>>>>> 1 file changed, 6 insertions(+), 5 deletions(-) >>>>>>>> >>>>>>>> Index: linux-pm/drivers/cpufreq/cpufreq.c >>>>>>>> =================================================================== >>>>>>>> --- linux-pm.orig/drivers/cpufreq/cpufreq.c >>>>>>>> +++ linux-pm/drivers/cpufreq/cpufreq.c >>>>>>>> @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits) >>>>>>>> static int cpufreq_boost_set_sw(int state) >>>>>>>> { >>>>>>>> struct cpufreq_policy *policy; >>>>>>>> - int ret = -EINVAL; >>>>>>>> >>>>>>>> for_each_active_policy(policy) { >>>>>>>> + int ret; >>>>>>>> + >>>>>>>> if (!policy->freq_table) >>>>>>>> - continue; >>>>>>>> + return -ENXIO; >>>>>>>> >>>>>>>> ret = cpufreq_frequency_table_cpuinfo(policy, >>>>>>>> policy->freq_table); >>>>>>>> if (ret) { >>>>>>>> pr_err("%s: Policy frequency update failed\n", >>>>>>>> __func__); >>>>>>>> - break; >>>>>>>> + return ret; >>>>>>>> } >>>>>>>> >>>>>>>> ret = freq_qos_update_request(policy->max_freq_req, policy->max); >>>>>>>> if (ret < 0) >>>>>>>> - break; >>>>>>>> + return ret; >>>>>>>> } >>>>>>>> >>>>>>>> - return ret; >>>>>>>> + return 0; >>>>>>>> } >>>>>>>> >>>>>>>> int cpufreq_boost_trigger_state(int state) >>>>>>> >>>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org> >>>>>> >>>>>> Ok. Thanks for the comments. Shall I resend the patch with update Rafael >>>>>> suggests or you'll merge the Rafael's fix in yourself? >>>>> >>>>> I'll apply the fix directly, thanks! >>>> >>>> Great. Is it going to be available in the repo: >>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/ >>>> ? >>> >>> Yes, it is. Please see the bleeding-edge branch in there, thanks! Thanks for CCing me. I will write my next version based on this branch. Thanks, Xiongfeng >> >> No credits with at least Reported-by tag? That's sad.( > > OK, done now, but you are not the only reported of it, so I've added > the other reporter too. > > Thanks! > > . >
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 045f9fe157ce..5870cdca88cf 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state) break; } - return ret; + return ret < 0 ? ret : 0; } int cpufreq_boost_trigger_state(int state)