I want to convert the axp20x PMIC poweroff handler to use the sys-off
API, so it can be used as a fallback for if the SBI poweroff handler
is unavailable. But the SBI poweroff handler still uses pm_power_off, so
done alone, this would cause the axp20x callback to be called first,
before the SBI poweroff handler has a chance to run.
In order to prevent this change in behavior, the SBI poweroff handler
needs to be converted to the sys-off API first, at a higher priority.
This series performs the conversion, after accounting for the fact that
the SBI poweroff handler is registered quite early during boot.
The first patch is a dependency for both this series and the PSCI
series[1], so I would like to get at least patch 1 merged soon.
[1]: https://lore.kernel.org/lkml/[email protected]/
Samuel Holland (3):
kernel/reboot: Use the static sys-off handler for any priority
riscv: sbi: Share the code for unsupported extension warnings
riscv: sbi: Switch to the sys-off handler API
arch/riscv/include/asm/sbi.h | 1 -
arch/riscv/kernel/sbi.c | 63 +++++++++++++++++++++---------------
kernel/reboot.c | 10 +++---
3 files changed, 41 insertions(+), 33 deletions(-)
--
2.37.4
commit 587b9bfe0668 ("kernel/reboot: Use static handler for
register_platform_power_off()") addded a statically-allocated handler
so register_sys_off_handler() could be called before the slab allocator
is available.
That behavior was limited to the SYS_OFF_PRIO_PLATFORM priority.
However, it is also required for handlers such as PSCI on ARM and SBI on
RISC-V, which should be registered at SYS_OFF_PRIO_FIRMWARE. Currently,
this call stack crashes:
start_kernel()
setup_arch()
psci_dt_init()
psci_0_2_init()
register_sys_off_handler()
kmem_cache_alloc()
Generalize the code to use the statically-allocated handler for the
first registration, regardless of priority. Check .sys_off_cb for
conflicts instead of .cb_data; some callbacks (e.g. firmware drivers)
do not need any per-instance data, so .cb_data could be NULL.
Reviewed-by: Dmitry Osipenko <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---
kernel/reboot.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 3bba88c7ffc6..38c18d4f0a36 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -327,7 +327,7 @@ static int sys_off_notify(struct notifier_block *nb,
return handler->sys_off_cb(&data);
}
-static struct sys_off_handler platform_sys_off_handler;
+static struct sys_off_handler early_sys_off_handler;
static struct sys_off_handler *alloc_sys_off_handler(int priority)
{
@@ -338,10 +338,8 @@ static struct sys_off_handler *alloc_sys_off_handler(int priority)
* Platforms like m68k can't allocate sys_off handler dynamically
* at the early boot time because memory allocator isn't available yet.
*/
- if (priority == SYS_OFF_PRIO_PLATFORM) {
- handler = &platform_sys_off_handler;
- if (handler->cb_data)
- return ERR_PTR(-EBUSY);
+ if (!early_sys_off_handler.sys_off_cb) {
+ handler = &early_sys_off_handler;
} else {
if (system_state > SYSTEM_RUNNING)
flags = GFP_ATOMIC;
@@ -358,7 +356,7 @@ static struct sys_off_handler *alloc_sys_off_handler(int priority)
static void free_sys_off_handler(struct sys_off_handler *handler)
{
- if (handler == &platform_sys_off_handler)
+ if (handler == &early_sys_off_handler)
memset(handler, 0, sizeof(*handler));
else
kfree(handler);
--
2.37.4
Hey Samuel,
On Wed, Dec 28, 2022 at 10:19:12AM -0600, Samuel Holland wrote:
> I want to convert the axp20x PMIC poweroff handler to use the sys-off
> API, so it can be used as a fallback for if the SBI poweroff handler
> is unavailable. But the SBI poweroff handler still uses pm_power_off, so
> done alone, this would cause the axp20x callback to be called first,
> before the SBI poweroff handler has a chance to run.
>
> In order to prevent this change in behavior, the SBI poweroff handler
> needs to be converted to the sys-off API first, at a higher priority.
>
> This series performs the conversion, after accounting for the fact that
> the SBI poweroff handler is registered quite early during boot.
>
> The first patch is a dependency for both this series and the PSCI
> series[1], so I would like to get at least patch 1 merged soon.
>
> [1]: https://lore.kernel.org/lkml/[email protected]/
>
>
> Samuel Holland (3):
> kernel/reboot: Use the static sys-off handler for any priority
> riscv: sbi: Share the code for unsupported extension warnings
> riscv: sbi: Switch to the sys-off handler API
Not what other stuff has reboot support, so I gave it a whirl on
PolarFire SoC & it seemed to work as expected:
Tested-by: Conor Dooley <[email protected]>
Thanks,
Conor.
On Wed, Dec 28, 2022 at 06:30:11PM +0000, Conor Dooley wrote:
> Hey Samuel,
>
> On Wed, Dec 28, 2022 at 10:19:12AM -0600, Samuel Holland wrote:
> > I want to convert the axp20x PMIC poweroff handler to use the sys-off
> > API, so it can be used as a fallback for if the SBI poweroff handler
> > is unavailable. But the SBI poweroff handler still uses pm_power_off, so
> > done alone, this would cause the axp20x callback to be called first,
> > before the SBI poweroff handler has a chance to run.
> >
> > In order to prevent this change in behavior, the SBI poweroff handler
> > needs to be converted to the sys-off API first, at a higher priority.
> >
> > This series performs the conversion, after accounting for the fact that
> > the SBI poweroff handler is registered quite early during boot.
> >
> > The first patch is a dependency for both this series and the PSCI
> > series[1], so I would like to get at least patch 1 merged soon.
> >
> > [1]: https://lore.kernel.org/lkml/[email protected]/
> >
> >
> > Samuel Holland (3):
> > kernel/reboot: Use the static sys-off handler for any priority
> > riscv: sbi: Share the code for unsupported extension warnings
> > riscv: sbi: Switch to the sys-off handler API
>
> Not what other stuff has reboot support, so I gave it a whirl on
I happened to see this today and could barely understand what I wrote.
s/Not what/Not sure what"
> PolarFire SoC & it seemed to work as expected:
> Tested-by: Conor Dooley <[email protected]>
>
> Thanks,
> Conor.
On Wed, 28 Dec 2022 08:19:13 PST (-0800), [email protected] wrote:
> commit 587b9bfe0668 ("kernel/reboot: Use static handler for
> register_platform_power_off()") addded a statically-allocated handler
> so register_sys_off_handler() could be called before the slab allocator
> is available.
>
> That behavior was limited to the SYS_OFF_PRIO_PLATFORM priority.
> However, it is also required for handlers such as PSCI on ARM and SBI on
> RISC-V, which should be registered at SYS_OFF_PRIO_FIRMWARE. Currently,
> this call stack crashes:
>
> start_kernel()
> setup_arch()
> psci_dt_init()
> psci_0_2_init()
> register_sys_off_handler()
> kmem_cache_alloc()
>
> Generalize the code to use the statically-allocated handler for the
> first registration, regardless of priority. Check .sys_off_cb for
> conflicts instead of .cb_data; some callbacks (e.g. firmware drivers)
> do not need any per-instance data, so .cb_data could be NULL.
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> kernel/reboot.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index 3bba88c7ffc6..38c18d4f0a36 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -327,7 +327,7 @@ static int sys_off_notify(struct notifier_block *nb,
> return handler->sys_off_cb(&data);
> }
>
> -static struct sys_off_handler platform_sys_off_handler;
> +static struct sys_off_handler early_sys_off_handler;
>
> static struct sys_off_handler *alloc_sys_off_handler(int priority)
> {
> @@ -338,10 +338,8 @@ static struct sys_off_handler *alloc_sys_off_handler(int priority)
> * Platforms like m68k can't allocate sys_off handler dynamically
> * at the early boot time because memory allocator isn't available yet.
> */
> - if (priority == SYS_OFF_PRIO_PLATFORM) {
> - handler = &platform_sys_off_handler;
> - if (handler->cb_data)
> - return ERR_PTR(-EBUSY);
> + if (!early_sys_off_handler.sys_off_cb) {
> + handler = &early_sys_off_handler;
> } else {
> if (system_state > SYSTEM_RUNNING)
> flags = GFP_ATOMIC;
> @@ -358,7 +356,7 @@ static struct sys_off_handler *alloc_sys_off_handler(int priority)
>
> static void free_sys_off_handler(struct sys_off_handler *handler)
> {
> - if (handler == &platform_sys_off_handler)
> + if (handler == &early_sys_off_handler)
> memset(handler, 0, sizeof(*handler));
> else
> kfree(handler);
Sorry for being slow here, I'd been assuming someone would Ack this but
it looks like maybe there's nobody in the maintainers file for
kernel/reboot.c? I'm fine taking this via the RISC-V tree if that's OK
with people, but the cover letter suggests the patch is necessary for
multiple patch sets.
On 2/14/23 18:17, Palmer Dabbelt wrote:
> On Wed, 28 Dec 2022 08:19:13 PST (-0800), [email protected] wrote:
>> commit 587b9bfe0668 ("kernel/reboot: Use static handler for
>> register_platform_power_off()") addded a statically-allocated handler
>> so register_sys_off_handler() could be called before the slab allocator
>> is available.
>>
>> That behavior was limited to the SYS_OFF_PRIO_PLATFORM priority.
>> However, it is also required for handlers such as PSCI on ARM and SBI on
>> RISC-V, which should be registered at SYS_OFF_PRIO_FIRMWARE. Currently,
>> this call stack crashes:
>>
>> start_kernel()
>> setup_arch()
>> psci_dt_init()
>> psci_0_2_init()
>> register_sys_off_handler()
>> kmem_cache_alloc()
>>
>> Generalize the code to use the statically-allocated handler for the
>> first registration, regardless of priority. Check .sys_off_cb for
>> conflicts instead of .cb_data; some callbacks (e.g. firmware drivers)
>> do not need any per-instance data, so .cb_data could be NULL.
>>
>> Reviewed-by: Dmitry Osipenko <[email protected]>
>> Signed-off-by: Samuel Holland <[email protected]>
>> ---
>>
>> kernel/reboot.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>> index 3bba88c7ffc6..38c18d4f0a36 100644
>> --- a/kernel/reboot.c
>> +++ b/kernel/reboot.c
>> @@ -327,7 +327,7 @@ static int sys_off_notify(struct notifier_block *nb,
>> return handler->sys_off_cb(&data);
>> }
>>
>> -static struct sys_off_handler platform_sys_off_handler;
>> +static struct sys_off_handler early_sys_off_handler;
>>
>> static struct sys_off_handler *alloc_sys_off_handler(int priority)
>> {
>> @@ -338,10 +338,8 @@ static struct sys_off_handler
>> *alloc_sys_off_handler(int priority)
>> * Platforms like m68k can't allocate sys_off handler dynamically
>> * at the early boot time because memory allocator isn't
>> available yet.
>> */
>> - if (priority == SYS_OFF_PRIO_PLATFORM) {
>> - handler = &platform_sys_off_handler;
>> - if (handler->cb_data)
>> - return ERR_PTR(-EBUSY);
>> + if (!early_sys_off_handler.sys_off_cb) {
>> + handler = &early_sys_off_handler;
>> } else {
>> if (system_state > SYSTEM_RUNNING)
>> flags = GFP_ATOMIC;
>> @@ -358,7 +356,7 @@ static struct sys_off_handler
>> *alloc_sys_off_handler(int priority)
>>
>> static void free_sys_off_handler(struct sys_off_handler *handler)
>> {
>> - if (handler == &platform_sys_off_handler)
>> + if (handler == &early_sys_off_handler)
>> memset(handler, 0, sizeof(*handler));
>> else
>> kfree(handler);
>
> Sorry for being slow here, I'd been assuming someone would Ack this but
> it looks like maybe there's nobody in the maintainers file for
> kernel/reboot.c? I'm fine taking this via the RISC-V tree if that's OK
> with people, but the cover letter suggests the patch is necessary for
> multiple patch sets.
See also Dmitry's reply[0] to the PSCI thread. (Maybe I should have sent
both conversions as one series?)
I am happy with the patches going through any tree. The kernel/reboot.c
patch is exactly the same between the two series, so it should not hurt
if it gets merged twice. Though if you take this series through the
RISC-V tree, maybe you want to create a tag for it?
I am not sure exactly what needs to be done here; I am happy to do
anything that would assist getting both series merged for v6.3, to avoid
a regression with axp20x[1].
Regards,
Samuel
[0]:
https://lore.kernel.org/lkml/[email protected]/
[1]:
https://lore.kernel.org/lkml/[email protected]/
On 2/19/23 02:20, Samuel Holland wrote:
> On 2/14/23 18:17, Palmer Dabbelt wrote:
...
>> Sorry for being slow here, I'd been assuming someone would Ack this but
>> it looks like maybe there's nobody in the maintainers file for
>> kernel/reboot.c? I'm fine taking this via the RISC-V tree if that's OK
>> with people, but the cover letter suggests the patch is necessary for
>> multiple patch sets.
>
> See also Dmitry's reply[0] to the PSCI thread. (Maybe I should have sent
> both conversions as one series?)
>
> I am happy with the patches going through any tree. The kernel/reboot.c
> patch is exactly the same between the two series, so it should not hurt
> if it gets merged twice. Though if you take this series through the
> RISC-V tree, maybe you want to create a tag for it?
>
> I am not sure exactly what needs to be done here; I am happy to do
> anything that would assist getting both series merged for v6.3, to avoid
> a regression with axp20x[1].
>
> Regards,
> Samuel
>
> [0]:
> https://lore.kernel.org/lkml/[email protected]/
> [1]:
> https://lore.kernel.org/lkml/[email protected]/
The reboot.c changes should be acked/applied by Rafael.
I noticed that you haven't CC'd the linux-pm ML, maybe that's why it
hasn't got the attention.
--
Best regards,
Dmitry
On Sat, 18 Feb 2023 15:32:06 PST (-0800), [email protected] wrote:
> On 2/19/23 02:20, Samuel Holland wrote:
>> On 2/14/23 18:17, Palmer Dabbelt wrote:
> ...
>>> Sorry for being slow here, I'd been assuming someone would Ack this but
>>> it looks like maybe there's nobody in the maintainers file for
>>> kernel/reboot.c? I'm fine taking this via the RISC-V tree if that's OK
>>> with people, but the cover letter suggests the patch is necessary for
>>> multiple patch sets.
>>
>> See also Dmitry's reply[0] to the PSCI thread. (Maybe I should have sent
>> both conversions as one series?)
>>
>> I am happy with the patches going through any tree. The kernel/reboot.c
>> patch is exactly the same between the two series, so it should not hurt
>> if it gets merged twice. Though if you take this series through the
>> RISC-V tree, maybe you want to create a tag for it?
>>
>> I am not sure exactly what needs to be done here; I am happy to do
>> anything that would assist getting both series merged for v6.3, to avoid
>> a regression with axp20x[1].
>>
>> Regards,
>> Samuel
>>
>> [0]:
>> https://lore.kernel.org/lkml/[email protected]/
>> [1]:
>> https://lore.kernel.org/lkml/[email protected]/
>
> The reboot.c changes should be acked/applied by Rafael.
> I noticed that you haven't CC'd the linux-pm ML, maybe that's why it
> hasn't got the attention.
OK, I'm adding them here. Not sure if we're ment to re-send it to the
list, no rush on my end I'm just scrubbing through some old stuff on
patchwork again.
On Wed, 28 Dec 2022 08:19:12 PST (-0800), [email protected] wrote:
> I want to convert the axp20x PMIC poweroff handler to use the sys-off
> API, so it can be used as a fallback for if the SBI poweroff handler
> is unavailable. But the SBI poweroff handler still uses pm_power_off, so
> done alone, this would cause the axp20x callback to be called first,
> before the SBI poweroff handler has a chance to run.
>
> In order to prevent this change in behavior, the SBI poweroff handler
> needs to be converted to the sys-off API first, at a higher priority.
>
> This series performs the conversion, after accounting for the fact that
> the SBI poweroff handler is registered quite early during boot.
>
> The first patch is a dependency for both this series and the PSCI
> series[1], so I would like to get at least patch 1 merged soon.
>
> [1]: https://lore.kernel.org/lkml/[email protected]/
>
>
> Samuel Holland (3):
> kernel/reboot: Use the static sys-off handler for any priority
> riscv: sbi: Share the code for unsupported extension warnings
> riscv: sbi: Switch to the sys-off handler API
>
> arch/riscv/include/asm/sbi.h | 1 -
> arch/riscv/kernel/sbi.c | 63 +++++++++++++++++++++---------------
> kernel/reboot.c | 10 +++---
> 3 files changed, 41 insertions(+), 33 deletions(-)
Samuel: do you mind rebasing this and resending it with the reboot folks
in the To list? That way we might be able to get an Ack on the generic
bits.
Thanks!
On Fri, 12 May 2023 14:50:17 PDT (-0700), Palmer Dabbelt wrote:
> On Wed, 28 Dec 2022 10:30:11 PST (-0800), Conor Dooley wrote:
>> Hey Samuel,
>>
>> On Wed, Dec 28, 2022 at 10:19:12AM -0600, Samuel Holland wrote:
>>> I want to convert the axp20x PMIC poweroff handler to use the sys-off
>>> API, so it can be used as a fallback for if the SBI poweroff handler
>>> is unavailable. But the SBI poweroff handler still uses pm_power_off, so
>>> done alone, this would cause the axp20x callback to be called first,
>>> before the SBI poweroff handler has a chance to run.
>>>
>>> In order to prevent this change in behavior, the SBI poweroff handler
>>> needs to be converted to the sys-off API first, at a higher priority.
>>>
>>> This series performs the conversion, after accounting for the fact that
>>> the SBI poweroff handler is registered quite early during boot.
>>>
>>> The first patch is a dependency for both this series and the PSCI
>>> series[1], so I would like to get at least patch 1 merged soon.
>>>
>>> [1]: https://lore.kernel.org/lkml/[email protected]/
>>>
>>>
>>> Samuel Holland (3):
>>> kernel/reboot: Use the static sys-off handler for any priority
>>> riscv: sbi: Share the code for unsupported extension warnings
>>> riscv: sbi: Switch to the sys-off handler API
>>
>> Not what other stuff has reboot support, so I gave it a whirl on
>> PolarFire SoC & it seemed to work as expected:
>> Tested-by: Conor Dooley <[email protected]>
>
> Acked-by: Palmer Dabbelt <[email protected]>
>
> in case the reboot folks want to take these. I'm also happy to take the
> reboot change through the RISC-V tree with an Ack. There's some
> discussion about this in the previous patches.
sorry, this is the old version -- I just got lost...
>
>>
>> Thanks,
>> Conor.
On Wed, 28 Dec 2022 10:30:11 PST (-0800), Conor Dooley wrote:
> Hey Samuel,
>
> On Wed, Dec 28, 2022 at 10:19:12AM -0600, Samuel Holland wrote:
>> I want to convert the axp20x PMIC poweroff handler to use the sys-off
>> API, so it can be used as a fallback for if the SBI poweroff handler
>> is unavailable. But the SBI poweroff handler still uses pm_power_off, so
>> done alone, this would cause the axp20x callback to be called first,
>> before the SBI poweroff handler has a chance to run.
>>
>> In order to prevent this change in behavior, the SBI poweroff handler
>> needs to be converted to the sys-off API first, at a higher priority.
>>
>> This series performs the conversion, after accounting for the fact that
>> the SBI poweroff handler is registered quite early during boot.
>>
>> The first patch is a dependency for both this series and the PSCI
>> series[1], so I would like to get at least patch 1 merged soon.
>>
>> [1]: https://lore.kernel.org/lkml/[email protected]/
>>
>>
>> Samuel Holland (3):
>> kernel/reboot: Use the static sys-off handler for any priority
>> riscv: sbi: Share the code for unsupported extension warnings
>> riscv: sbi: Switch to the sys-off handler API
>
> Not what other stuff has reboot support, so I gave it a whirl on
> PolarFire SoC & it seemed to work as expected:
> Tested-by: Conor Dooley <[email protected]>
Acked-by: Palmer Dabbelt <[email protected]>
in case the reboot folks want to take these. I'm also happy to take the
reboot change through the RISC-V tree with an Ack. There's some
discussion about this in the previous patches.
>
> Thanks,
> Conor.