| Message ID | 20220427224924.592546-13-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | The panic notifiers refactor | expand |
On 4/28/22 00:49, Guilherme G. Piccoli wrote: > The panic notifiers' callbacks execute in an atomic context, with > interrupts/preemption disabled, and all CPUs not running the panic > function are off, so it's very dangerous to wait on a regular > spinlock, there's a risk of deadlock. > > This patch refactors the panic notifier of parisc/power driver > to make use of spin_trylock - for that, we've added a second > version of the soft-power function. Also, some comments were > reorganized and trailing white spaces, useless header inclusion > and blank lines were removed. > > Cc: Helge Deller <deller@gmx.de> > Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> You may add: Acked-by: Helge Deller <deller@gmx.de> # parisc Helge > --- > arch/parisc/include/asm/pdc.h | 1 + > arch/parisc/kernel/firmware.c | 27 +++++++++++++++++++++++---- > drivers/parisc/power.c | 17 ++++++++++------- > 3 files changed, 34 insertions(+), 11 deletions(-) > > diff --git a/arch/parisc/include/asm/pdc.h b/arch/parisc/include/asm/pdc.h > index b643092d4b98..7a106008e258 100644 > --- a/arch/parisc/include/asm/pdc.h > +++ b/arch/parisc/include/asm/pdc.h > @@ -83,6 +83,7 @@ int pdc_do_firm_test_reset(unsigned long ftc_bitmap); > int pdc_do_reset(void); > int pdc_soft_power_info(unsigned long *power_reg); > int pdc_soft_power_button(int sw_control); > +int pdc_soft_power_button_panic(int sw_control); > void pdc_io_reset(void); > void pdc_io_reset_devices(void); > int pdc_iodc_getc(void); > diff --git a/arch/parisc/kernel/firmware.c b/arch/parisc/kernel/firmware.c > index 6a7e315bcc2e..0e2f70b592f4 100644 > --- a/arch/parisc/kernel/firmware.c > +++ b/arch/parisc/kernel/firmware.c > @@ -1232,15 +1232,18 @@ int __init pdc_soft_power_info(unsigned long *power_reg) > } > > /* > - * pdc_soft_power_button - Control the soft power button behaviour > - * @sw_control: 0 for hardware control, 1 for software control > + * pdc_soft_power_button{_panic} - Control the soft power button behaviour > + * @sw_control: 0 for hardware control, 1 for software control > * > * > * This PDC function places the soft power button under software or > * hardware control. > - * Under software control the OS may control to when to allow to shut > - * down the system. Under hardware control pressing the power button > + * Under software control the OS may control to when to allow to shut > + * down the system. Under hardware control pressing the power button > * powers off the system immediately. > + * > + * The _panic version relies in spin_trylock to prevent deadlock > + * on panic path. > */ > int pdc_soft_power_button(int sw_control) > { > @@ -1254,6 +1257,22 @@ int pdc_soft_power_button(int sw_control) > return retval; > } > > +int pdc_soft_power_button_panic(int sw_control) > +{ > + int retval; > + unsigned long flags; > + > + if (!spin_trylock_irqsave(&pdc_lock, flags)) { > + pr_emerg("Couldn't enable soft power button\n"); > + return -EBUSY; /* ignored by the panic notifier */ > + } > + > + retval = mem_pdc_call(PDC_SOFT_POWER, PDC_SOFT_POWER_ENABLE, __pa(pdc_result), sw_control); > + spin_unlock_irqrestore(&pdc_lock, flags); > + > + return retval; > +} > + > /* > * pdc_io_reset - Hack to avoid overlapping range registers of Bridges devices. > * Primarily a problem on T600 (which parisc-linux doesn't support) but > diff --git a/drivers/parisc/power.c b/drivers/parisc/power.c > index 456776bd8ee6..8512884de2cf 100644 > --- a/drivers/parisc/power.c > +++ b/drivers/parisc/power.c > @@ -37,7 +37,6 @@ > #include <linux/module.h> > #include <linux/init.h> > #include <linux/kernel.h> > -#include <linux/notifier.h> > #include <linux/panic_notifier.h> > #include <linux/reboot.h> > #include <linux/sched/signal.h> > @@ -175,16 +174,21 @@ static void powerfail_interrupt(int code, void *x) > > > > -/* parisc_panic_event() is called by the panic handler. > - * As soon as a panic occurs, our tasklets above will not be > - * executed any longer. This function then re-enables the > - * soft-power switch and allows the user to switch off the system > +/* > + * parisc_panic_event() is called by the panic handler. > + * > + * As soon as a panic occurs, our tasklets above will not > + * be executed any longer. This function then re-enables > + * the soft-power switch and allows the user to switch off > + * the system. We rely in pdc_soft_power_button_panic() > + * since this version spin_trylocks (instead of regular > + * spinlock), preventing deadlocks on panic path. > */ > static int parisc_panic_event(struct notifier_block *this, > unsigned long event, void *ptr) > { > /* re-enable the soft-power switch */ > - pdc_soft_power_button(0); > + pdc_soft_power_button_panic(0); > return NOTIFY_DONE; > } > > @@ -193,7 +197,6 @@ static struct notifier_block parisc_panic_block = { > .priority = INT_MAX, > }; > > - > static int __init power_init(void) > { > unsigned long ret;
On 28/04/2022 13:55, Helge Deller wrote: > [...] > You may add: > Acked-by: Helge Deller <deller@gmx.de> # parisc > > Helge Thanks Helge, added! Cheers, Guilherme > > >> --- >> arch/parisc/include/asm/pdc.h | 1 + >> arch/parisc/kernel/firmware.c | 27 +++++++++++++++++++++++---- >> drivers/parisc/power.c | 17 ++++++++++------- >> 3 files changed, 34 insertions(+), 11 deletions(-) >> >> diff --git a/arch/parisc/include/asm/pdc.h b/arch/parisc/include/asm/pdc.h >> index b643092d4b98..7a106008e258 100644 >> --- a/arch/parisc/include/asm/pdc.h >> +++ b/arch/parisc/include/asm/pdc.h >> @@ -83,6 +83,7 @@ int pdc_do_firm_test_reset(unsigned long ftc_bitmap); >> int pdc_do_reset(void); >> int pdc_soft_power_info(unsigned long *power_reg); >> int pdc_soft_power_button(int sw_control); >> +int pdc_soft_power_button_panic(int sw_control); >> void pdc_io_reset(void); >> void pdc_io_reset_devices(void); >> int pdc_iodc_getc(void); >> diff --git a/arch/parisc/kernel/firmware.c b/arch/parisc/kernel/firmware.c >> index 6a7e315bcc2e..0e2f70b592f4 100644 >> --- a/arch/parisc/kernel/firmware.c >> +++ b/arch/parisc/kernel/firmware.c >> @@ -1232,15 +1232,18 @@ int __init pdc_soft_power_info(unsigned long *power_reg) >> } >> >> /* >> - * pdc_soft_power_button - Control the soft power button behaviour >> - * @sw_control: 0 for hardware control, 1 for software control >> + * pdc_soft_power_button{_panic} - Control the soft power button behaviour >> + * @sw_control: 0 for hardware control, 1 for software control >> * >> * >> * This PDC function places the soft power button under software or >> * hardware control. >> - * Under software control the OS may control to when to allow to shut >> - * down the system. Under hardware control pressing the power button >> + * Under software control the OS may control to when to allow to shut >> + * down the system. Under hardware control pressing the power button >> * powers off the system immediately. >> + * >> + * The _panic version relies in spin_trylock to prevent deadlock >> + * on panic path. >> */ >> int pdc_soft_power_button(int sw_control) >> { >> @@ -1254,6 +1257,22 @@ int pdc_soft_power_button(int sw_control) >> return retval; >> } >> >> +int pdc_soft_power_button_panic(int sw_control) >> +{ >> + int retval; >> + unsigned long flags; >> + >> + if (!spin_trylock_irqsave(&pdc_lock, flags)) { >> + pr_emerg("Couldn't enable soft power button\n"); >> + return -EBUSY; /* ignored by the panic notifier */ >> + } >> + >> + retval = mem_pdc_call(PDC_SOFT_POWER, PDC_SOFT_POWER_ENABLE, __pa(pdc_result), sw_control); >> + spin_unlock_irqrestore(&pdc_lock, flags); >> + >> + return retval; >> +} >> + >> /* >> * pdc_io_reset - Hack to avoid overlapping range registers of Bridges devices. >> * Primarily a problem on T600 (which parisc-linux doesn't support) but >> diff --git a/drivers/parisc/power.c b/drivers/parisc/power.c >> index 456776bd8ee6..8512884de2cf 100644 >> --- a/drivers/parisc/power.c >> +++ b/drivers/parisc/power.c >> @@ -37,7 +37,6 @@ >> #include <linux/module.h> >> #include <linux/init.h> >> #include <linux/kernel.h> >> -#include <linux/notifier.h> >> #include <linux/panic_notifier.h> >> #include <linux/reboot.h> >> #include <linux/sched/signal.h> >> @@ -175,16 +174,21 @@ static void powerfail_interrupt(int code, void *x) >> >> >> >> -/* parisc_panic_event() is called by the panic handler. >> - * As soon as a panic occurs, our tasklets above will not be >> - * executed any longer. This function then re-enables the >> - * soft-power switch and allows the user to switch off the system >> +/* >> + * parisc_panic_event() is called by the panic handler. >> + * >> + * As soon as a panic occurs, our tasklets above will not >> + * be executed any longer. This function then re-enables >> + * the soft-power switch and allows the user to switch off >> + * the system. We rely in pdc_soft_power_button_panic() >> + * since this version spin_trylocks (instead of regular >> + * spinlock), preventing deadlocks on panic path. >> */ >> static int parisc_panic_event(struct notifier_block *this, >> unsigned long event, void *ptr) >> { >> /* re-enable the soft-power switch */ >> - pdc_soft_power_button(0); >> + pdc_soft_power_button_panic(0); >> return NOTIFY_DONE; >> } >> >> @@ -193,7 +197,6 @@ static struct notifier_block parisc_panic_block = { >> .priority = INT_MAX, >> }; >> >> - >> static int __init power_init(void) >> { >> unsigned long ret; >
On 28/04/2022 13:55, Helge Deller wrote: > [...] > You may add: > Acked-by: Helge Deller <deller@gmx.de> # parisc > > Helge Hi Helge, do you think would be possible to still pick this one for v5.19 or do you prefer to hold for the next release? I'm working on V2, so if it's merged for 5.19 I won't send it again. Thanks, Guilherme
Hello Guilherme, On 5/23/22 22:40, Guilherme G. Piccoli wrote: > On 28/04/2022 13:55, Helge Deller wrote: >> [...] >> You may add: >> Acked-by: Helge Deller <deller@gmx.de> # parisc > > Hi Helge, do you think would be possible to still pick this one for > v5.19 or do you prefer to hold for the next release? Actually, I'd prefer if you push this patch together with the whole series upstream at once. The patch itself makes not much sense without your series... > I'm working on V2, so if it's merged for 5.19 I won't send it again. Helge
On 23/05/2022 18:31, Helge Deller wrote: > Hello Guilherme, > > On 5/23/22 22:40, Guilherme G. Piccoli wrote: >> On 28/04/2022 13:55, Helge Deller wrote: >>> [...] >>> You may add: >>> Acked-by: Helge Deller <deller@gmx.de> # parisc >> >> Hi Helge, do you think would be possible to still pick this one for >> v5.19 or do you prefer to hold for the next release? > > Actually, I'd prefer if you push this patch together with the whole > series upstream at once. The patch itself makes not much sense without > your series... > >> I'm working on V2, so if it's merged for 5.19 I won't send it again. > > Helge Sure Helge, I guess I can do that - will resubmit for V2. But notice the patch is self-contained, as it fixes a current issue in the code - the risk for a lockup due to spinlock taking on atomic context. It doesn't require the panic refactor to be merged in order to achieve its goal... I agree that such issue is rare to trigger though, so definitely no hurry is needed =) Cheers, Guilherme
diff --git a/arch/parisc/include/asm/pdc.h b/arch/parisc/include/asm/pdc.h index b643092d4b98..7a106008e258 100644 --- a/arch/parisc/include/asm/pdc.h +++ b/arch/parisc/include/asm/pdc.h @@ -83,6 +83,7 @@ int pdc_do_firm_test_reset(unsigned long ftc_bitmap); int pdc_do_reset(void); int pdc_soft_power_info(unsigned long *power_reg); int pdc_soft_power_button(int sw_control); +int pdc_soft_power_button_panic(int sw_control); void pdc_io_reset(void); void pdc_io_reset_devices(void); int pdc_iodc_getc(void); diff --git a/arch/parisc/kernel/firmware.c b/arch/parisc/kernel/firmware.c index 6a7e315bcc2e..0e2f70b592f4 100644 --- a/arch/parisc/kernel/firmware.c +++ b/arch/parisc/kernel/firmware.c @@ -1232,15 +1232,18 @@ int __init pdc_soft_power_info(unsigned long *power_reg) } /* - * pdc_soft_power_button - Control the soft power button behaviour - * @sw_control: 0 for hardware control, 1 for software control + * pdc_soft_power_button{_panic} - Control the soft power button behaviour + * @sw_control: 0 for hardware control, 1 for software control * * * This PDC function places the soft power button under software or * hardware control. - * Under software control the OS may control to when to allow to shut - * down the system. Under hardware control pressing the power button + * Under software control the OS may control to when to allow to shut + * down the system. Under hardware control pressing the power button * powers off the system immediately. + * + * The _panic version relies in spin_trylock to prevent deadlock + * on panic path. */ int pdc_soft_power_button(int sw_control) { @@ -1254,6 +1257,22 @@ int pdc_soft_power_button(int sw_control) return retval; } +int pdc_soft_power_button_panic(int sw_control) +{ + int retval; + unsigned long flags; + + if (!spin_trylock_irqsave(&pdc_lock, flags)) { + pr_emerg("Couldn't enable soft power button\n"); + return -EBUSY; /* ignored by the panic notifier */ + } + + retval = mem_pdc_call(PDC_SOFT_POWER, PDC_SOFT_POWER_ENABLE, __pa(pdc_result), sw_control); + spin_unlock_irqrestore(&pdc_lock, flags); + + return retval; +} + /* * pdc_io_reset - Hack to avoid overlapping range registers of Bridges devices. * Primarily a problem on T600 (which parisc-linux doesn't support) but diff --git a/drivers/parisc/power.c b/drivers/parisc/power.c index 456776bd8ee6..8512884de2cf 100644 --- a/drivers/parisc/power.c +++ b/drivers/parisc/power.c @@ -37,7 +37,6 @@ #include <linux/module.h> #include <linux/init.h> #include <linux/kernel.h> -#include <linux/notifier.h> #include <linux/panic_notifier.h> #include <linux/reboot.h> #include <linux/sched/signal.h> @@ -175,16 +174,21 @@ static void powerfail_interrupt(int code, void *x) -/* parisc_panic_event() is called by the panic handler. - * As soon as a panic occurs, our tasklets above will not be - * executed any longer. This function then re-enables the - * soft-power switch and allows the user to switch off the system +/* + * parisc_panic_event() is called by the panic handler. + * + * As soon as a panic occurs, our tasklets above will not + * be executed any longer. This function then re-enables + * the soft-power switch and allows the user to switch off + * the system. We rely in pdc_soft_power_button_panic() + * since this version spin_trylocks (instead of regular + * spinlock), preventing deadlocks on panic path. */ static int parisc_panic_event(struct notifier_block *this, unsigned long event, void *ptr) { /* re-enable the soft-power switch */ - pdc_soft_power_button(0); + pdc_soft_power_button_panic(0); return NOTIFY_DONE; } @@ -193,7 +197,6 @@ static struct notifier_block parisc_panic_block = { .priority = INT_MAX, }; - static int __init power_init(void) { unsigned long ret;
The panic notifiers' callbacks execute in an atomic context, with interrupts/preemption disabled, and all CPUs not running the panic function are off, so it's very dangerous to wait on a regular spinlock, there's a risk of deadlock. This patch refactors the panic notifier of parisc/power driver to make use of spin_trylock - for that, we've added a second version of the soft-power function. Also, some comments were reorganized and trailing white spaces, useless header inclusion and blank lines were removed. Cc: Helge Deller <deller@gmx.de> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- arch/parisc/include/asm/pdc.h | 1 + arch/parisc/kernel/firmware.c | 27 +++++++++++++++++++++++---- drivers/parisc/power.c | 17 ++++++++++------- 3 files changed, 34 insertions(+), 11 deletions(-)