| Message ID | 20240527142557.321610-4-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pmdomain/cpuidle-psci: Support s2idle/s2ram on PREEMPT_RT | expand |
Hi Ulf, On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > Using kobject_get_path() means a dynamic memory allocation gets done, which > doesn't work on a PREEMPT_RT based configuration while holding genpd's raw > spinlock. > > To fix the problem, let's convert into using the simpler dev_name(). This > means the information about the path doesn't get presented in debugfs, but > hopefully this shouldn't be an issue. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > Changes in v2: > - New patch. Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain: core: Use dev_name() instead of kobject_get_path() in debugfs") in pmdomain/next. > --- a/drivers/pmdomain/core.c > +++ b/drivers/pmdomain/core.c > @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s, > } > > list_for_each_entry(pm_data, &genpd->dev_list, list_node) { > - kobj_path = kobject_get_path(&pm_data->dev->kobj, > - genpd_is_irq_safe(genpd) ? > - GFP_ATOMIC : GFP_KERNEL); > - if (kobj_path == NULL) > - continue; > - > - seq_printf(s, "\n %-50s ", kobj_path); > + seq_printf(s, "\n %-50s ", dev_name(pm_data->dev)); While some of the old names didn't even fit in 50 characters, the new names need much less space, so perhaps this is a good opportunity to decrease the table width? > rtpm_status_str(s, pm_data->dev); > perf_status_str(s, pm_data->dev); > - kfree(kobj_path); > } > > seq_puts(s, "\n"); Gr{oetje,eeting}s, Geert
On Tue, 20 Aug 2024 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ulf, > > On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > Using kobject_get_path() means a dynamic memory allocation gets done, which > > doesn't work on a PREEMPT_RT based configuration while holding genpd's raw > > spinlock. > > > > To fix the problem, let's convert into using the simpler dev_name(). This > > means the information about the path doesn't get presented in debugfs, but > > hopefully this shouldn't be an issue. > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > Changes in v2: > > - New patch. > > Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain: > core: Use dev_name() instead of kobject_get_path() in debugfs") > in pmdomain/next. > > > --- a/drivers/pmdomain/core.c > > +++ b/drivers/pmdomain/core.c > > @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s, > > } > > > > list_for_each_entry(pm_data, &genpd->dev_list, list_node) { > > - kobj_path = kobject_get_path(&pm_data->dev->kobj, > > - genpd_is_irq_safe(genpd) ? > > - GFP_ATOMIC : GFP_KERNEL); > > - if (kobj_path == NULL) > > - continue; > > - > > - seq_printf(s, "\n %-50s ", kobj_path); > > + seq_printf(s, "\n %-50s ", dev_name(pm_data->dev)); > > While some of the old names didn't even fit in 50 characters, the new > names need much less space, so perhaps this is a good opportunity to > decrease the table width? Sure, it seems reasonable! Do you want to send a patch? > > > rtpm_status_str(s, pm_data->dev); > > perf_status_str(s, pm_data->dev); > > - kfree(kobj_path); > > } > > > > seq_puts(s, "\n"); Kind regards Uffe
Hi Ulf, On Tue, Aug 20, 2024 at 10:58 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Tue, 20 Aug 2024 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > Using kobject_get_path() means a dynamic memory allocation gets done, which > > > doesn't work on a PREEMPT_RT based configuration while holding genpd's raw > > > spinlock. > > > > > > To fix the problem, let's convert into using the simpler dev_name(). This > > > means the information about the path doesn't get presented in debugfs, but > > > hopefully this shouldn't be an issue. > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > > Changes in v2: > > > - New patch. > > > > Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain: > > core: Use dev_name() instead of kobject_get_path() in debugfs") > > in pmdomain/next. > > > > > --- a/drivers/pmdomain/core.c > > > +++ b/drivers/pmdomain/core.c > > > @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s, > > > } > > > > > > list_for_each_entry(pm_data, &genpd->dev_list, list_node) { > > > - kobj_path = kobject_get_path(&pm_data->dev->kobj, > > > - genpd_is_irq_safe(genpd) ? > > > - GFP_ATOMIC : GFP_KERNEL); > > > - if (kobj_path == NULL) > > > - continue; > > > - > > > - seq_printf(s, "\n %-50s ", kobj_path); > > > + seq_printf(s, "\n %-50s ", dev_name(pm_data->dev)); > > > > While some of the old names didn't even fit in 50 characters, the new > > names need much less space, so perhaps this is a good opportunity to > > decrease the table width? > > Sure, it seems reasonable! Do you want to send a patch? I started looking into it. Then I noticed that on some systems (e.g. TI am335x) the device names may have a longer format than the typical <unit-address>.<nodename>. So I wanted to verify on BeagleBone Black, but recent kernels crash during early boot. Apparently that platform was broken between v6.8 and v6.9-rc1. And during bisection, I encountered 3 different failure modes... To be continued... Gr{oetje,eeting}s, Geert
Hi Ulf, On Fri, Aug 30, 2024 at 5:50 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Aug 20, 2024 at 10:58 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Tue, 20 Aug 2024 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > Using kobject_get_path() means a dynamic memory allocation gets done, which > > > > doesn't work on a PREEMPT_RT based configuration while holding genpd's raw > > > > spinlock. > > > > > > > > To fix the problem, let's convert into using the simpler dev_name(). This > > > > means the information about the path doesn't get presented in debugfs, but > > > > hopefully this shouldn't be an issue. > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > --- > > > > Changes in v2: > > > > - New patch. > > > > > > Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain: > > > core: Use dev_name() instead of kobject_get_path() in debugfs") > > > in pmdomain/next. > > > > > > > --- a/drivers/pmdomain/core.c > > > > +++ b/drivers/pmdomain/core.c > > > > @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s, > > > > } > > > > > > > > list_for_each_entry(pm_data, &genpd->dev_list, list_node) { > > > > - kobj_path = kobject_get_path(&pm_data->dev->kobj, > > > > - genpd_is_irq_safe(genpd) ? > > > > - GFP_ATOMIC : GFP_KERNEL); > > > > - if (kobj_path == NULL) > > > > - continue; > > > > - > > > > - seq_printf(s, "\n %-50s ", kobj_path); > > > > + seq_printf(s, "\n %-50s ", dev_name(pm_data->dev)); > > > > > > While some of the old names didn't even fit in 50 characters, the new > > > names need much less space, so perhaps this is a good opportunity to > > > decrease the table width? > > > > Sure, it seems reasonable! Do you want to send a patch? > > I started looking into it. Then I noticed that on some systems > (e.g. TI am335x) the device names may have a longer format than > the typical <unit-address>.<nodename>. So I wanted to verify on > BeagleBone Black, but recent kernels crash during early boot. > Apparently that platform was broken between v6.8 and v6.9-rc1. > And during bisection, I encountered 3 different failure modes... > > To be continued... The longest generic node names documented in the Devicetree Specification are "air-pollution-sensor" and "interrupt-controller" (both counting 20 characters), so a typical device name needs 8 (32-bit unit address) + 1 (dot) + 20 = 29 characters. However, I assume some devices lie outside the 32-bit address space, and thus need more space? With the BeagleBone Black boot issue fixed: "/devices/platform/ocp/5600fe00.target-module" resp. "/devices/platform/ocp/44c00000.interconnect/44c00000.interconnect:segment@200000/44e3e074.target-module" are now shortened to "5600fe00.target-module" resp. "44e3e074.target-module". However, "/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@200000/48000000.interconnect:segment@200000:target-module@0" is shortened to "48000000.interconnect:segment@200000:target-module@0", which is still longer than the old column width... Gr{oetje,eeting}s, Geert
On Mon, 2 Sept 2024 at 16:43, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ulf, > > On Fri, Aug 30, 2024 at 5:50 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Aug 20, 2024 at 10:58 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > On Tue, 20 Aug 2024 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > Using kobject_get_path() means a dynamic memory allocation gets done, which > > > > > doesn't work on a PREEMPT_RT based configuration while holding genpd's raw > > > > > spinlock. > > > > > > > > > > To fix the problem, let's convert into using the simpler dev_name(). This > > > > > means the information about the path doesn't get presented in debugfs, but > > > > > hopefully this shouldn't be an issue. > > > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > --- > > > > > Changes in v2: > > > > > - New patch. > > > > > > > > Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain: > > > > core: Use dev_name() instead of kobject_get_path() in debugfs") > > > > in pmdomain/next. > > > > > > > > > --- a/drivers/pmdomain/core.c > > > > > +++ b/drivers/pmdomain/core.c > > > > > @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s, > > > > > } > > > > > > > > > > list_for_each_entry(pm_data, &genpd->dev_list, list_node) { > > > > > - kobj_path = kobject_get_path(&pm_data->dev->kobj, > > > > > - genpd_is_irq_safe(genpd) ? > > > > > - GFP_ATOMIC : GFP_KERNEL); > > > > > - if (kobj_path == NULL) > > > > > - continue; > > > > > - > > > > > - seq_printf(s, "\n %-50s ", kobj_path); > > > > > + seq_printf(s, "\n %-50s ", dev_name(pm_data->dev)); > > > > > > > > While some of the old names didn't even fit in 50 characters, the new > > > > names need much less space, so perhaps this is a good opportunity to > > > > decrease the table width? > > > > > > Sure, it seems reasonable! Do you want to send a patch? > > > > I started looking into it. Then I noticed that on some systems > > (e.g. TI am335x) the device names may have a longer format than > > the typical <unit-address>.<nodename>. So I wanted to verify on > > BeagleBone Black, but recent kernels crash during early boot. > > Apparently that platform was broken between v6.8 and v6.9-rc1. > > And during bisection, I encountered 3 different failure modes... > > > > To be continued... > > The longest generic node names documented in the Devicetree > Specification are "air-pollution-sensor" and "interrupt-controller" > (both counting 20 characters), so a typical device name needs 8 > (32-bit unit address) + 1 (dot) + 20 = 29 characters. > However, I assume some devices lie outside the 32-bit address space, > and thus need more space? > > With the BeagleBone Black boot issue fixed: > "/devices/platform/ocp/5600fe00.target-module" > resp. "/devices/platform/ocp/44c00000.interconnect/44c00000.interconnect:segment@200000/44e3e074.target-module" > are now shortened to "5600fe00.target-module" resp. "44e3e074.target-module". > However, "/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@200000/48000000.interconnect:segment@200000:target-module@0" > is shortened to "48000000.interconnect:segment@200000:target-module@0", > which is still longer than the old column width... Should we really care about those silly long names? And are those really a problem from genpd debugfs point of view? That said, I don't have a suggestion for a new value of the table width, but I am certainly open to adjusting it to whatever you propose. Kind regards Uffe
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c index 454fccc38df1..90a65bd09d52 100644 --- a/drivers/pmdomain/core.c +++ b/drivers/pmdomain/core.c @@ -3182,7 +3182,6 @@ static int genpd_summary_one(struct seq_file *s, [GENPD_STATE_OFF] = "off" }; struct pm_domain_data *pm_data; - const char *kobj_path; struct gpd_link *link; char state[16]; int ret; @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s, } list_for_each_entry(pm_data, &genpd->dev_list, list_node) { - kobj_path = kobject_get_path(&pm_data->dev->kobj, - genpd_is_irq_safe(genpd) ? - GFP_ATOMIC : GFP_KERNEL); - if (kobj_path == NULL) - continue; - - seq_printf(s, "\n %-50s ", kobj_path); + seq_printf(s, "\n %-50s ", dev_name(pm_data->dev)); rtpm_status_str(s, pm_data->dev); perf_status_str(s, pm_data->dev); - kfree(kobj_path); } seq_puts(s, "\n"); @@ -3393,23 +3385,14 @@ static int devices_show(struct seq_file *s, void *data) { struct generic_pm_domain *genpd = s->private; struct pm_domain_data *pm_data; - const char *kobj_path; int ret = 0; ret = genpd_lock_interruptible(genpd); if (ret) return -ERESTARTSYS; - list_for_each_entry(pm_data, &genpd->dev_list, list_node) { - kobj_path = kobject_get_path(&pm_data->dev->kobj, - genpd_is_irq_safe(genpd) ? - GFP_ATOMIC : GFP_KERNEL); - if (kobj_path == NULL) - continue; - - seq_printf(s, "%s\n", kobj_path); - kfree(kobj_path); - } + list_for_each_entry(pm_data, &genpd->dev_list, list_node) + seq_printf(s, "%s\n", dev_name(pm_data->dev)); genpd_unlock(genpd); return ret;
Using kobject_get_path() means a dynamic memory allocation gets done, which doesn't work on a PREEMPT_RT based configuration while holding genpd's raw spinlock. To fix the problem, let's convert into using the simpler dev_name(). This means the information about the path doesn't get presented in debugfs, but hopefully this shouldn't be an issue. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- Changes in v2: - New patch. --- drivers/pmdomain/core.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)