Re: [PATCH V8 1/8] PM / devfreq: Add cpu based scaling support to passive_governor

From: Chanwoo Choi
Date: Mon May 31 2021 - 03:55:16 EST

Next message: Johan Hovold: "Re: [PATCH 3/3] media: rtl28xxu: fix zero-length control request"
Previous message: syzbot: "[syzbot] kernel BUG in assertfail"
In reply to: Chanwoo Choi: "Re: [PATCH V8 1/8] PM / devfreq: Add cpu based scaling support to passive_governor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 5/31/21 4:42 PM, Hsin-Yi Wang wrote:
>
>
> On Mon, May 31, 2021 at 3:37 PM Chanwoo Choi <cw00.choi@xxxxxxxxxxx <mailto:cw00.choi@xxxxxxxxxxx>> wrote:
>
> Hi,
>
> On 5/31/21 12:22 PM, andrew-sh.cheng wrote:
> > On Wed, 2021-05-26 at 12:08 +0900, Chanwoo Choi wrote:
> >> Hi,
> >> On 5/26/21 11:22 AM, andrew-sh.cheng wrote:
> >>> On Thu, 2021-04-08 at 11:47 +0900, Chanwoo Choi wrote:
> >>>> On 4/1/21 9:16 AM, Chanwoo Choi wrote:
> >>>>> On 3/31/21 10:03 PM, andrew-sh.cheng wrote:
> >>>>>> On Wed, 2021-03-31 at 17:35 +0900, Chanwoo Choi wrote:
> >>>>>>> On 3/31/21 5:27 PM, Chanwoo Choi wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> On 3/31/21 5:03 PM, andrew-sh.cheng wrote:
> >>>>>>>>> On Thu, 2021-03-25 at 17:14 +0900, Chanwoo Choi wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> You are missing to add these patches to linux-pm mailing list.
> >>>>>>>>>> Need to send them to linu-pm ML.
> >>>>>>>>>>
> >>>>>>>>>> Also, before received this series, I tried to clean-up these patches
> >>>>>>>>>> on testing branch[1]. So that I add my comment with my clean-up case.
> >>>>>>>>>> [1] https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing-passive-gov__;!!CTRNKA9wMg0ARbw!zIrzeDp9vPnm1_SDzVPuzqdHn3zWie9DnfBXaA-j9-CSrVc6aR9_rJQQiw81_CgAPh9XRRs$
> >>>>>>>>>>
> >>>>>>>>>> And 'Saravana Kannan <skannan@xxxxxxxxxxxxxx <mailto:skannan@xxxxxxxxxxxxxx>>' is wrong email address.
> >>>>>>>>>> Please update the email or drop this email.
> >>>>>>>>>
> >>>>>>>>> Hi Chanwoo,
> >>>>>>>>>
> >>>>>>>>> Thank you for the advices.
> >>>>>>>>> I will resend patch v9 (add to linux-pm ML), remove this patch, and note
> >>>>>>>>> that my patch set base on
> >>>>>>>>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing-passive-gov__;!!CTRNKA9wMg0ARbw!yUlsuxrL5PcbF7o6A9DlCfvoA6w8V8VXKjYIybYyiJg3D0HM-lI2xRuxLUV6b3UJ8WFhg_g$
> >>>>>>>>
> >>>>>>>> I has not yet test this patch[1] on devfreq-testing-passive-gov branch.
> >>>>>>>> So that if possible, I'd like you to test your patches with this patch[1]
> >>>>>>>> and then if there is no problem, could you send the next patches with patch[1]?
> >>>>>>>>
> >>>>>>>> [1]https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing-passive-gov&id=39c80d11a8f42dd63ecea1e0df595a0ceb83b454__;!!CTRNKA9wMg0ARbw!yUlsuxrL5PcbF7o6A9DlCfvoA6w8V8VXKjYIybYyiJg3D0HM-lI2xRuxLUV6b3UJR2cQqZs$
> >>>>>>>
> >>>>>>>
> >>>>>>> Sorry for the confusion. I make the devfreq-testing-passive-gov[1]
> >>>>>>> branch based on latest devfreq-next branch.
> >>>>>>> [1] https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing-passive-gov__;!!CTRNKA9wMg0ARbw!yUlsuxrL5PcbF7o6A9DlCfvoA6w8V8VXKjYIybYyiJg3D0HM-lI2xRuxLUV6b3UJ8WFhg_g$
> >>>>>>>
> >>>>>>> First of all, if possible, I want to test them[1] with your patches in this series.
> >>>>>>> And then if there are no any problem, please let me know. After confirmed from you,
> >>>>>>> I'll send the patches of devfreq-testing-passive-gov[1] branch.
> >>>>>>> How about that?
> >>>>>>>
> >>>>>> Hi Chanwoo~
> >>>>>>
> >>>>>> We will use this on Google Chrome project.
> >>>>>> Google Hsin-Yi has test your patch + my patch set v8 [2~8]
> >>>>>>
> >>>>>> make sure cci devfreqs runs with cpufreq.
> >>>>>> suspend resume
> >>>>>> speedometer2 benchmark
> >>>>>> It is okay.
> >>>>>>
> >>>>>> Please send the patches of devfreq-testing-passive-gov[1] branch.
> >>>>>>
> >>>>>> I will send patch v9 base on yours latter.
> >>>>>
> >>>>> Thanks for your test. I'll send the patches today.
> >>>>
> >>>> I'm sorry for delay because when I tested the patches
> >>>> for devfreq parent type on Odroid-xu3, there are some problem
> >>>> related to lazy linking of OPP. So I'm trying to analyze them.
> >>>> Unfortunately, we need to postpone these patches to next linux
> >>>> version.
> >>>>
> >>> Hi Chanwoo Choi~
> >>>
> >>> It is said that you are busy on another task recently.
> >>> May I know your plan on this patch?
> >>> Thank you.
> >>
> >> Sorry for late work. I have a question.
> >> When I tested exynos-bus.c with adding the 'required-opp' property
> >> on odroid-xu3 board. I got some fail about
> >>
> >> When calling _set_required_opps(), always _set_required_opp() returns
> >> -EBUSY error because of following lazy linking case[1].
> >>
> >> [1] https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.13-rc3/source/drivers/opp/core.c*L896__;Iw!!CTRNKA9wMg0ARbw!3eNxwDZRy-Ev5BHGxT-BxCz4qrNy0NZohQuBGW36krkwOkl_WX8yBmxlqSk9hxp_kxspMJI$
> >>
> >> /* required-opps not fully initialized yet */
> >> if (lazy_linking_pending(opp_table))
> >> return -EBUSY;
> >>
> >>
> >> For calling dev_pm_opp_of_add_table(), lazy_link_required_opp_table() function
> >> will be called. But, there is constraint[2]. If is_genpd of opp_table is false,
> >> driver/opp/of.c cannot resolve the lazy linking issue.
> >>
> >> [2] https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.13-rc3/source/drivers/opp/of.c*L386__;Iw!!CTRNKA9wMg0ARbw!3eNxwDZRy-Ev5BHGxT-BxCz4qrNy0NZohQuBGW36krkwOkl_WX8yBmxlqSk9hxp_QFUVY9E$
> >>
> >> /* Link required OPPs for all OPPs of the newly added OPP table */
> >> static void lazy_link_required_opp_table(struct opp_table *new_table)
> >> {
> >> struct opp_table *opp_table, *temp, **required_opp_tables;
> >> struct device_node *required_np, *opp_np, *required_table_np;
> >> struct dev_pm_opp *opp;
> >> int i, ret;
> >>
> >> /*
> >> * We only support genpd's OPPs in the "required-opps" for now,
> >> * as we don't know much about other cases.
> >> */
> >> if (!new_table->is_genpd)
> >> return;
> >>
> >> Even if this case, there are no problem on your test case?
> >>
> >
> > Hi Chanwoo~
> > Sorry for late reply.
> > Yes, we meet similar issue.
> > Google member Hsin-Yi had helped deal with this issue on Chrome project.
> >
> > Patch segment:
> > @ /drivers/opp/of.c
> >
> > /* Link required OPPs for all OPPs of the newly added OPP table */
> > static void lazy_link_required_opp_table(struct opp_table *new_table)
> > {
> > struct opp_table *opp_table, *temp, **required_opp_tables;
> > struct device_node *required_np, *opp_np, *required_table_np;
> > struct dev_pm_opp *opp;
> > int i, ret;
> >
> > + /*
> > + * We only support genpd's OPPs in the "required-opps" for now,
> > + * as we don't know much about other cases.
> > + */
> > + if (!new_table->is_genpd)
> > + return;
> >
> >
> > Hsin-Yi replied this issue in the discussion list in the original lazy
> > link thread:
> > https://patchwork.kernel.org/project/linux-pm/patch/20190717222340.137578-4-saravanak@xxxxxxxxxx/#23932203
> >
> > Loop Hsin-YI here.
> > You can discuss with her if needing more detail.
> >
> > Thank you both.
> >
>
> Thanks. First of all, we need to resolve and discuss this issue.
>
>
> Hi Chanwoo,
>
> We think removing the genpd check is sufficient for our use case since we only use the lazy link for opp table translation.

Hi Hsin-Yi,

IMHO, I think 'is_genpd' checking should be removed for devices except for genpd
like as following:

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index c582a9ca397b..b54d3a985515 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
lazy = true;
continue;
}
-
- /*
- * We only support genpd's OPPs in the "required-opps" for now,
- * as we don't know how much about other cases. Error out if the
- * required OPP doesn't belong to a genpd.
- */
- if (!required_opp_tables[i]->is_genpd) {
- dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
- required_np);
- goto free_required_tables;
- }
}

/* Let's do the linking later on */
@@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
struct dev_pm_opp *opp;
int i, ret;

- /*
- * We only support genpd's OPPs in the "required-opps" for now,
- * as we don't know much about other cases.
- */
- if (!new_table->is_genpd)
- return;
-
mutex_lock(&opp_table_lock);

list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
@@ -874,7 +856,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
return ERR_PTR(-ENOMEM);

ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
- if (ret < 0 && !opp_table->is_genpd) {
+ if (ret < 0) {
dev_err(dev, "%s: opp key field not found\n", __func__);
goto free_opp;
}

--
Best Regards,
Chanwoo Choi
Samsung Electronics

Next message: Johan Hovold: "Re: [PATCH 3/3] media: rtl28xxu: fix zero-length control request"
Previous message: syzbot: "[syzbot] kernel BUG in assertfail"
In reply to: Chanwoo Choi: "Re: [PATCH V8 1/8] PM / devfreq: Add cpu based scaling support to passive_governor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]