Re: [PATCH v11 3/4] arm64: topology: Support SMT control on ACPI based system
On 2025/3/4 23:07, Pierre Gondois wrote:
>
>
> On 3/4/25 11:02, Sudeep Holla wrote:
>> On Tue, Mar 04, 2025 at 09:25:02AM +0100, Pierre Gondois wrote:
>>>
>>>
>>> On 3/3/25 15:40, Yicong Yang wrote:
On 2025/3/3 19:16, Sudeep Holla wrote:
> On Mon, Mar 03, 2025 at 10:56:12AM +0100, Pierre Gondois wrote:
>> On 2/28/25 20:06, Sudeep Holla wrote:
>
> Ditto as previous patch, can get rid if it is default 1.
>
On non-SMT platforms, not calling cpu_smt_set_num_threads() leaves
cpu_smt_num_threads uninitialized to UINT_MAX:
smt/active:0
smt/control:-1
If cpu_smt_set_num_threads() is called:
active:0
control:notsupported
So it might be slightly better to still initialize max_smt_thread_num.
>>>
>>> Sure, what I meant is to have max_smt_thread_num set to 1 by default is
>>> that is what needed anyways and the above code does that now.
>>>
>>> Why not start with initialised to 1 instead ?
>>> Of course some current logic needs to change around testing it for zero.
>>>
>>
>> I think there would still be a way to check against the default value.
>> If we have:
>> unsigned int max_smt_thread_num = 1;
>>
>> then on a platform with 2 threads, the detection condition would trigger:
>> xa_for_each(&hetero_cpu, hetero_id, entry) {
>> if (entry->thread_num != max_smt_thread_num && max_smt_thread_num)
>> < (entry->thread_num=2) and (max_smt_thread_num=1)
>> pr_warn_once("Heterogeneous SMT topology is partly
>> supported by SMT control\n");
>>
>> so we would need an additional variable:
>> bool is_initialized = false;
>
> Sure, we could do that or skip the check if max_smt_thread_num == 1 ?
>
> I mean
> if (entry->thread_num != max_smt_thread_num && max_smt_thread_num !=
> 1)
>
>>>
>>> I think it will be problematic if we parse:
>>> - first a CPU with 1 thread
>>> - then a CPU with 2 threads
>>>
>>> in that case we should detect the 'Heterogeneous SMT topology',
>>> but we cannot because we don't know whether max_smt_thread_num=1
>>> because 1 is the default value or we found a CPU with one thread.
>>
>> Right, but as per Dietmar's and my previous response, it may be a valid
>> case. See latest response from Dietmar which is explicitly requesting
>> support for this. It may need some special handling if we decide to support
>> that.
>
> Ah ok, right indeed.
> For heterogeneous SMT platforms, the 'smt/control' file is able to accept
> on/off/forceoff strings. But providing the max #count of threads as an
> integer would
> be wrong if the CPU doesn't have this #count of threads.
>
> Initially the idea was to just warn that support might be needed for
> heterogeneous
> SMT platforms, and let whoever would have such platform solve this case, but
> just
> disabling the integer interface in this case would solve the issue
> generically.
>
ok so let's regard the asymmetric platform as a valid case as suggested (also
mentioned
by Dietmar on another thread) and remove the check here. Will update and test.
Thanks.
Re: [PATCH] crypto: powerpc: Mark ghashp8-ppc.o as an OBJECT_FILES_NON_STANDARD
On 3/5/25 4:32 AM, Christophe Leroy wrote:
The following build warning has been reported:
arch/powerpc/crypto/ghashp8-ppc.o: warning: objtool: .text+0x22c:
unannotated intra-function call
This happens due to commit bb7f054f4de2 ("objtool/powerpc: Add support
for decoding all types of uncond branches")
Disassembly of arch/powerpc/crypto/ghashp8-ppc.o shows:
arch/powerpc/crypto/ghashp8-ppc.o: file format elf64-powerpcle
Disassembly of section .text:
0140 :
140:f8 ff 00 3c lis r0,-8
...
20c:20 00 80 4e blr
210:00 00 00 00 .long 0x0
214:00 0c 14 00 .long 0x140c00
218:00 00 04 00 .long 0x4
21c:00 00 00 00 .long 0x0
220:47 48 41 53 rlwimi. r1,r26,9,1,3
224:48 20 66 6f xoris r6,r27,8264
228:72 20 50 6f xoris r16,r26,8306
22c:77 65 72 49 bla 1726574 <==
...
It corresponds to the following code in ghashp8-ppc.o :
_GLOBAL(gcm_ghash_p8)
lis0,0xfff8
...
blr
.long0
.byte0,12,0x14,0,0,0,4,0
.long0
.sizegcm_ghash_p8,.-gcm_ghash_p8
.byte
71,72,65,83,72,32,102,111,114,32,80,111,119,101,114,73,83,65,32,50,46,48,55,44,32,67,82,89,80,84,79,71,65,77,83,32,98,121,32,60,97,112,112,114,111,64,111,112,101,110,115,115,108,46,111,114,103,62,0
.align2
.align2
In fact this is raw data that is after the function end and that is
not text so shouldn't be disassembled as text. But ghashp8-ppc.S is
generated by a perl script and should have been marked as
OBJECT_FILES_NON_STANDARD.
Now that 'bla' is understood as a call instruction, that raw data
is mis-interpreted as an infra-function call.
Mark ghashp8-ppc.o as a OBJECT_FILES_NON_STANDARD to avoid this
warning.
Reported-by: Venkat Rao Bagalkote
Closes:
https://lore.kernel.org/all/8c4c3fc2-2bd7-4148-af68-2f504d611...@linux.ibm.com
Cc: Danny Tsen
Fixes: 109303336a0c ("crypto: vmx - Move to arch/powerpc/crypto")
Signed-off-by: Christophe Leroy
---
arch/powerpc/crypto/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/crypto/Makefile b/arch/powerpc/crypto/Makefile
index 9b38f4a7bc15..2f00b22b0823 100644
--- a/arch/powerpc/crypto/Makefile
+++ b/arch/powerpc/crypto/Makefile
@@ -51,3 +51,4 @@ $(obj)/aesp8-ppc.S $(obj)/ghashp8-ppc.S: $(obj)/%.S:
$(src)/%.pl FORCE
OBJECT_FILES_NON_STANDARD_aesp10-ppc.o := y
OBJECT_FILES_NON_STANDARD_ghashp10-ppc.o := y
OBJECT_FILES_NON_STANDARD_aesp8-ppc.o := y
+OBJECT_FILES_NON_STANDARD_ghashp8-ppc.o := y
Objtool is not invoked on this file anymore, and the warning is no
longer seen with this patch.
Patch looks good to me.
Reviewed-by: Sathvika Vasireddy
Thanks,
Sathvika
Re: [PATCH v4 0/2] Add stop_on_panic support for watchdog
Hello George, On 05.03.25 13:15, George Cherian wrote: >> On 05.03.25 12:28, George Cherian wrote: that can't be disabled and would protect against system lock up: Consider a memory-corruption bug (perhaps externally via DMA), which partially overwrites both main and kdump kernel. With a disabled watchdog, the system may not be able to recover on its own. >>> >>> Yes, that is the reason why the kernel command-line is optional and by >>> default it is set to zero. >>> So that in cases if you have a corrupted kdump kernel then watchdog kicks >>> in. >> >> The existing option isn't enough for the kdump kernel use case. >> If we (i.e. you) are going to do something about it, wouldn't it be >> better to have a solution that's applicable to a wider number of >> watchdog devices? > > I need a slight clarification here. > 1. reset_on_panic takes the number of seconds to be reloaded to watchdog HW, > so that it initiates a > watchdog reset after the specified timeout, if kdump kernel fails to boot or > hung while booting. Yes. > 2. in case reset_on_panic = 0 then it behaves like stop_on_panic=1. > Is this what you meant? Alternatively, reset_on_panic = 0 could also mean stopping the watchdog as you do now. I haven't thought though yet what would make the most sense. > I would let Guenter comment on this approach. +1. >> If you are serious with the watchdog use, you'll want to use the watchdog to >> monitor kernel startup as well. If the bootloader can set a watchdog timeout >> just before starting the kernel and it doesn't expire before the kernel >> watchdog >> driver takes over, why can't we do the same just before starting the >> dumpkernel? > > Yes, in an ideal world with ideal HW. But there are HW with issues which > cannot have large > enough Watchdog time. Such HW would boot from FW to kernel without watchdog > enabled. > And stop_on_panic does the similar for kdump kernel too. Yes, but there is likely more kinds of watchdog devices that can not be disabled, so it makes sense to have a solution that is more broadly applicable from the get-go. Cheers, Ahmad > > -George >> >> Thanks, >> Ahmad >> >> >>> >>> Thanks, >>> Ahmad >>> >>> Changelog: >>> v1 -> v2 >>> - Remove the per driver flag setting option >>> - Take the parameter via kernel command-line parameter to watchdog_core. >>> >>> v2 -> v3 >>> - Remove the helper function watchdog_stop_on_panic() from watchdog.h. >>> - There are no users for this. >>> >>> v3 -> v4 >>> - Since the panic notifier is in atomic context, watchdog functions >>> which sleep can't be called. >>> - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop >>> function sleeps. >>> - Simplify the stop_on_panic kernel command line parsing. >>> - Enable the panic notiffier only if the watchdog stop function doesn't >>> sleep >>> >>> George Cherian (2): >>> watchdog: Add a new flag WDIOF_STOP_MAYSLEEP >>> drivers: watchdog: Add support for panic notifier callback >> >> - George > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v4 2/2] drivers: watchdog: Add support for panic notifier callback
Watchdog is not turned off in kernel panic situation.
In certain systems this might prevent the successful loading
of kdump kernel. The kdump kernel might hit a watchdog reset
while it is booting.
To avoid such scenarios add a panic notifier call back function
which can stop the watchdog. This provision can be enabled by
passing watchdog.stop_on_panic=1 via kernel command-line parameter.
Signed-off-by: George Cherian
---
drivers/watchdog/watchdog_core.c | 35
include/linux/watchdog.h | 2 ++
2 files changed, 37 insertions(+)
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index d46d8c8c01f2..f0006d90da92 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -34,6 +34,7 @@
#include /* For ida_* macros */
#include /* For IS_ERR macros */
#include /* For of_get_timeout_sec */
+#include /* For panic handler */
#include
#include "watchdog_core.h" /* For watchdog_dev_register/... */
@@ -47,6 +48,9 @@ static int stop_on_reboot = -1;
module_param(stop_on_reboot, int, 0444);
MODULE_PARM_DESC(stop_on_reboot, "Stop watchdogs on reboot (0=keep watching,
1=stop)");
+static bool stop_on_panic;
+module_param(stop_on_panic, bool, 0444);
+MODULE_PARM_DESC(stop_on_panic, "Stop watchdogs on panic (0=keep watching,
1=stop)");
/*
* Deferred Registration infrastructure.
*
@@ -155,6 +159,23 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
}
EXPORT_SYMBOL_GPL(watchdog_init_timeout);
+static int watchdog_panic_notify(struct notifier_block *nb,
+unsigned long action, void *data)
+{
+ struct watchdog_device *wdd;
+
+ wdd = container_of(nb, struct watchdog_device, panic_nb);
+ if (watchdog_active(wdd)) {
+ int ret;
+
+ ret = wdd->ops->stop(wdd);
+ if (ret)
+ return NOTIFY_BAD;
+ }
+
+ return NOTIFY_DONE;
+}
+
static int watchdog_reboot_notifier(struct notifier_block *nb,
unsigned long code, void *data)
{
@@ -334,6 +355,17 @@ static int ___watchdog_register_device(struct
watchdog_device *wdd)
wdd->id, ret);
}
+ if (stop_on_panic) {
+ if (wdd->ops->stop && !(wdd->info->options &
WDIOF_STOP_MAYSLEEP)) {
+ wdd->panic_nb.notifier_call = watchdog_panic_notify;
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &wdd->panic_nb);
+ set_bit(WDOG_STOP_ON_PANIC, &wdd->status);
+ } else {
+ pr_warn("watchdog%d: stop_on_panic not supported\n",
wdd->id);
+ }
+ }
+
return 0;
}
@@ -390,6 +422,9 @@ static void __watchdog_unregister_device(struct
watchdog_device *wdd)
if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
unregister_reboot_notifier(&wdd->reboot_nb);
+ if (test_bit(WDOG_STOP_ON_PANIC, &wdd->status))
+ atomic_notifier_chain_unregister(&panic_notifier_list,
+&wdd->panic_nb);
watchdog_dev_unregister(wdd);
ida_free(&watchdog_ida, wdd->id);
}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 99660197a36c..ef6f1136a4c5 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -108,6 +108,7 @@ struct watchdog_device {
struct notifier_block reboot_nb;
struct notifier_block restart_nb;
struct notifier_block pm_nb;
+ struct notifier_block panic_nb;
void *driver_data;
struct watchdog_core_data *wd_data;
unsigned long status;
@@ -118,6 +119,7 @@ struct watchdog_device {
#define WDOG_HW_RUNNING3 /* True if HW watchdog running
*/
#define WDOG_STOP_ON_UNREGISTER4 /* Should be stopped on
unregister */
#define WDOG_NO_PING_ON_SUSPEND5 /* Ping worker should be
stopped on suspend */
+#define WDOG_STOP_ON_PANIC 6 /* Should be stopped on panic for
loading kdump kernels */
struct list_head deferred;
};
--
2.34.1
[PATCH v4 0/2] Add stop_on_panic support for watchdog
This series adds a new kernel command line option to watchdog core to stop the watchdog on panic. This is useul in certain systems which prevents successful loading of kdump kernel due to watchdog reset. Some of the watchdog drivers stop function could sleep. For such drivers the stop_on_panic is not valid as the notifier callback happens in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info options to indicate whether the stop function would sleep. Changelog: v1 -> v2 - Remove the per driver flag setting option - Take the parameter via kernel command-line parameter to watchdog_core. v2 -> v3 - Remove the helper function watchdog_stop_on_panic() from watchdog.h. - There are no users for this. v3 -> v4 - Since the panic notifier is in atomic context, watchdog functions which sleep can't be called. - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop function sleeps. - Simplify the stop_on_panic kernel command line parsing. - Enable the panic notiffier only if the watchdog stop function doesn't sleep George Cherian (2): watchdog: Add a new flag WDIOF_STOP_MAYSLEEP drivers: watchdog: Add support for panic notifier callback drivers/watchdog/advantech_ec_wdt.c | 3 ++- drivers/watchdog/arm_smc_wdt.c | 3 ++- drivers/watchdog/armada_37xx_wdt.c | 2 +- drivers/watchdog/asm9260_wdt.c | 2 +- drivers/watchdog/bcm47xx_wdt.c | 3 ++- drivers/watchdog/bd9576_wdt.c | 2 +- drivers/watchdog/bd96801_wdt.c | 2 +- drivers/watchdog/cgbc_wdt.c | 2 +- drivers/watchdog/cros_ec_wdt.c | 5 - drivers/watchdog/da9052_wdt.c | 3 ++- drivers/watchdog/da9055_wdt.c | 4 +++- drivers/watchdog/da9062_wdt.c | 4 +++- drivers/watchdog/da9063_wdt.c | 4 +++- drivers/watchdog/db8500_wdt.c | 5 - drivers/watchdog/dw_wdt.c | 5 +++-- drivers/watchdog/f71808e_wdt.c | 3 ++- drivers/watchdog/gpio_wdt.c | 2 +- drivers/watchdog/i6300esb.c | 5 - drivers/watchdog/imx_sc_wdt.c | 2 +- drivers/watchdog/intel-mid_wdt.c| 5 - drivers/watchdog/it87_wdt.c | 5 - drivers/watchdog/jz4740_wdt.c | 5 - drivers/watchdog/kempld_wdt.c | 3 ++- drivers/watchdog/lenovo_se10_wdt.c | 5 - drivers/watchdog/max77620_wdt.c | 5 - drivers/watchdog/mei_wdt.c | 3 ++- drivers/watchdog/menf21bmc_wdt.c| 4 +++- drivers/watchdog/mlx_wdt.c | 2 +- drivers/watchdog/msc313e_wdt.c | 5 - drivers/watchdog/npcm_wdt.c | 3 ++- drivers/watchdog/omap_wdt.c | 5 - drivers/watchdog/pm8916_wdt.c | 5 +++-- drivers/watchdog/pseries-wdt.c | 2 +- drivers/watchdog/rave-sp-wdt.c | 5 - drivers/watchdog/renesas_wdt.c | 7 -- drivers/watchdog/retu_wdt.c | 5 - drivers/watchdog/rn5t618_wdt.c | 6 +++-- drivers/watchdog/rzg2l_wdt.c| 5 - drivers/watchdog/rzv2h_wdt.c| 5 - drivers/watchdog/shwdt.c| 6 +++-- drivers/watchdog/sl28cpld_wdt.c | 5 - drivers/watchdog/softdog.c | 5 - drivers/watchdog/sp805_wdt.c| 5 - drivers/watchdog/starfive-wdt.c | 3 ++- drivers/watchdog/stpmic1_wdt.c | 5 - drivers/watchdog/ts4800_wdt.c | 5 - drivers/watchdog/twl4030_wdt.c | 5 - drivers/watchdog/uniphier_wdt.c | 3 ++- drivers/watchdog/w83627hf_wdt.c | 5 - drivers/watchdog/watchdog_core.c| 35 + drivers/watchdog/wm831x_wdt.c | 5 - drivers/watchdog/wm8350_wdt.c | 5 - drivers/watchdog/xen_wdt.c | 5 - drivers/watchdog/ziirave_wdt.c | 5 - include/linux/watchdog.h| 2 ++ include/uapi/linux/watchdog.h | 1 + 56 files changed, 198 insertions(+), 58 deletions(-) -- 2.34.1
Re: [EXTERNAL] Re: [PATCH v4 0/2] Add stop_on_panic support for watchdog
Hi George, On 05.03.25 12:28, George Cherian wrote: > Hi Ahmad, >> Hi George, >> On 05.03.25 11:10, George Cherian wrote: >>> This series adds a new kernel command line option to watchdog core to >>> stop the watchdog on panic. This is useul in certain systems which prevents >>> successful loading of kdump kernel due to watchdog reset. >>> >>> Some of the watchdog drivers stop function could sleep. For such >>> drivers the stop_on_panic is not valid as the notifier callback happens >>> in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info >>> options to indicate whether the stop function would sleep. >> >> Did you consider having a reset_on_panic instead, which sets a user-specified >> timeout on panic? This would make the mechanism useful also for watchdogs > > /proc/sys/kernel/panic already provides that support. You may echo a non-zero > value > and the system tries for a soft reboot after those many seconds. But this > doesn't happen > in case of a kdump kernel load after panic. The timeout specified to the Watchdog reset_on_panic option would be programmed into the active watchdogs and not be used to trigger a software-induced reboot. >> that can't be disabled and would protect against system lock up: >> Consider a memory-corruption bug (perhaps externally via DMA), which >> partially >> overwrites both main and kdump kernel. With a disabled watchdog, the system >> may not be able to recover on its own. > > Yes, that is the reason why the kernel command-line is optional and by > default it is set to zero. > So that in cases if you have a corrupted kdump kernel then watchdog kicks in. The existing option isn't enough for the kdump kernel use case. If we (i.e. you) are going to do something about it, wouldn't it be better to have a solution that's applicable to a wider number of watchdog devices? >> If you did consider it, what made you decide against it? > watchdog.stop_on_panic=1 is specifically for systems which can't boot a kdump > kernel due to the fact > that the kdump kernel gets a watchdog reset while booting, may be due to a > shorter watchdog time. > For eg: a 32-bit watchdog down counter running at 1GHz. > reset_on_panic can guarantee only the largest watchdog timeout supported by > HW, > since there is no one to ping the watchdog. If you are serious with the watchdog use, you'll want to use the watchdog to monitor kernel startup as well. If the bootloader can set a watchdog timeout just before starting the kernel and it doesn't expire before the kernel watchdog driver takes over, why can't we do the same just before starting the dumpkernel? Thanks, Ahmad >> >> Thanks, >> Ahmad >> >>> >>> >> Changelog: >> v1 -> v2 >> - Remove the per driver flag setting option >> - Take the parameter via kernel command-line parameter to watchdog_core. >> >> v2 -> v3 >> - Remove the helper function watchdog_stop_on_panic() from watchdog.h. >> - There are no users for this. >> >> v3 -> v4 >> - Since the panic notifier is in atomic context, watchdog functions >> which sleep can't be called. >> - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop >> function sleeps. >> - Simplify the stop_on_panic kernel command line parsing. >> - Enable the panic notiffier only if the watchdog stop function doesn't >> sleep >> >> George Cherian (2): >> watchdog: Add a new flag WDIOF_STOP_MAYSLEEP >> drivers: watchdog: Add support for panic notifier callback > > - George -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 2/2] tools/perf/powerpc: Use return code from disasm_line__parse
I was able to reproduce the issue without this patch. Applied this patch on next-20250227 kernel and tested the patch and issue is fixed. Please add below tag. Tested-By: Venkat Rao Bagalkote On 04/03/25 9:11 pm, Athira Rajeev wrote: In disasm_line__parse_powerpc() , return code from function disasm_line__parse() is ignored. This will result in bad results if the disasm_line__parse fails to disasm the line. Use the return code to fix this. Signed-off-by: Athira Rajeev --- tools/perf/util/disasm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c index a53e8c4e5bca..8f0eb56c6fc6 100644 --- a/tools/perf/util/disasm.c +++ b/tools/perf/util/disasm.c @@ -976,6 +976,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar char *tmp_raw_insn, *name_raw_insn = skip_spaces(line); char *name = skip_spaces(name_raw_insn + RAW_BYTES); int disasm = 0; + int ret = 0; if (args->options->disassembler_used) disasm = 1; @@ -984,7 +985,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar return -1; if (disasm) - disasm_line__parse(name, namep, rawp); + ret = disasm_line__parse(name, namep, rawp); else *namep = ""; @@ -998,7 +999,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar if (disasm) dl->raw.raw_insn = be32_to_cpu(dl->raw.raw_insn); - return 0; + return ret; } static void annotation_line__init(struct annotation_line *al,
[PATCH] powerpc/pseries/msi: Avoid reading PCI device registers in reduced power states
When a system is being suspended to RAM, the PCI devices are also
suspended and the PPC code ends up calling pseries_msi_compose_msg() and
this triggers the BUG_ON() in __pci_read_msi_msg() because the device at
this point is in reduced power state. In reduced power state, the memory
mapped registers of the PCI device are not accessible.
To replicate the bug:
1. Make sure deep sleep is selected
# cat /sys/power/mem_sleep
s2idle [deep]
2. Make sure console is not suspended (so that dmesg logs are visible)
echo N > /sys/module/printk/parameters/console_suspend
3. Suspend the system
echo mem > /sys/power/state
To fix this behaviour, read the cached msi message of the device when the
device is not in PCI_D0 power state instead of touching the hardware.
Fixes: a5f3d2c17b07 ("powerpc/pseries/pci: Add MSI domains")
Cc: sta...@vger.kernel.org # v5.15+
Signed-off-by: Gautam Menghani
---
arch/powerpc/platforms/pseries/msi.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/msi.c
b/arch/powerpc/platforms/pseries/msi.c
index fdc2f7f38dc9..458d95c8c755 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -525,7 +525,12 @@ static struct msi_domain_info pseries_msi_domain_info = {
static void pseries_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
{
- __pci_read_msi_msg(irq_data_get_msi_desc(data), msg);
+ struct pci_dev *dev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));
+
+ if (dev->current_state == PCI_D0)
+ __pci_read_msi_msg(irq_data_get_msi_desc(data), msg);
+ else
+ get_cached_msi_msg(data->irq, msg);
}
static struct irq_chip pseries_msi_irq_chip = {
--
2.47.0
RE: [EXTERNAL] Re: [PATCH v4 0/2] Add stop_on_panic support for watchdog
Hi Ahmad, >Hi George, > On 05.03.25 11:10, George Cherian wrote: >> This series adds a new kernel command line option to watchdog core to >> stop the watchdog on panic. This is useul in certain systems which prevents >> successful loading of kdump kernel due to watchdog reset. >> >> Some of the watchdog drivers stop function could sleep. For such >> drivers the stop_on_panic is not valid as the notifier callback happens >> in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info >> options to indicate whether the stop function would sleep. > >Did you consider having a reset_on_panic instead, which sets a user-specified >timeout on panic? This would make the mechanism useful also for watchdogs /proc/sys/kernel/panic already provides that support. You may echo a non-zero value and the system tries for a soft reboot after those many seconds. But this doesn't happen in case of a kdump kernel load after panic. >that can't be disabled and would protect against system lock up: >Consider a memory-corruption bug (perhaps externally via DMA), which partially >overwrites both main and kdump kernel. With a disabled watchdog, the system >may not be able to recover on its own. Yes, that is the reason why the kernel command-line is optional and by default it is set to zero. So that in cases if you have a corrupted kdump kernel then watchdog kicks in. > >If you did consider it, what made you decide against it? watchdog.stop_on_panic=1 is specifically for systems which can't boot a kdump kernel due to the fact that the kdump kernel gets a watchdog reset while booting, may be due to a shorter watchdog time. For eg: a 32-bit watchdog down counter running at 1GHz. reset_on_panic can guarantee only the largest watchdog timeout supported by HW, since there is no one to ping the watchdog. > >Thanks, >Ahmad > >> >> > Changelog: > v1 -> v2 > - Remove the per driver flag setting option > - Take the parameter via kernel command-line parameter to watchdog_core. > > v2 -> v3 > - Remove the helper function watchdog_stop_on_panic() from watchdog.h. > - There are no users for this. > > v3 -> v4 > - Since the panic notifier is in atomic context, watchdog functions > which sleep can't be called. > - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop > function sleeps. > - Simplify the stop_on_panic kernel command line parsing. > - Enable the panic notiffier only if the watchdog stop function doesn't > sleep > > George Cherian (2): > watchdog: Add a new flag WDIOF_STOP_MAYSLEEP > drivers: watchdog: Add support for panic notifier callback - George
Re: [PATCH v4 0/2] Add stop_on_panic support for watchdog
On Wed, Mar 05, 2025 at 10:10:23AM +, George Cherian wrote: > This series adds a new kernel command line option to watchdog core to > stop the watchdog on panic. This is useul in certain systems which prevents > successful loading of kdump kernel due to watchdog reset. > > Some of the watchdog drivers stop function could sleep. For such > drivers the stop_on_panic is not valid as the notifier callback happens > in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info > options to indicate whether the stop function would sleep. Should you only enable this if the kdump is enabled in the kernel configuration? -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/2] tools/perf: Add field to annotation options to determine disassembler used
Applied this patch on next-20250227, and issue is fixed.
./perf -v
perf version 6.14.rc4.g2a432e11ebbb
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
Please add below tag.
Tested-By: Venkat Rao Bagalkote
Regards,
Venkat.
On 04/03/25 9:11 pm, Athira Rajeev wrote:
When doing "perf annotate", perf tool provides option to
use specific disassembler like llvm/objdump/capstone. The
order picked is to use llvm first and if that fails fallback
to objdump ie to use PERF_DISASM_LLVM, PERF_DISASM_CAPSTONE
and PERF_DISASM_OBJDUMP
In powerpc, when using "data type" sort keys, first preferred
approach is to read the raw instruction from the DSO. In objdump
is specified in "--objdump" option, it picks the symbol disassemble
using objdump. Currently disasm_line__parse_powerpc() function
uses length of the "line" to determine if objdump is used.
But there are few cases, where if objdump doesn't recognise the
instruction, the disassembled string will be empty.
Example:
134cdc: c4 05 82 41 beq 1352a0
134ce0: ac 00 8e 40 bne cr3,134d8c
134ce4: 0f 00 10 04 pld r9,1028308
>134ce8: d4 b0 20 e5
134cec: 16 00 40 39 li r10,22
134cf0: 48 01 21 ea ld r17,328(r1)
So depending on length of line will give bad results.
Add a new filed to annotation options structure,
"struct annotation_options" to save the disassembler used.
Use this info to determine if disassembly is done while
parsing the disasm line.
Reported-by: Tejas Manhas
Signed-off-by: Athira Rajeev
---
tools/perf/util/annotate.h | 1 +
tools/perf/util/disasm.c | 22 +-
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 98db1b88daf4..30a344afd91a 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -58,6 +58,7 @@ struct annotation_options {
full_addr;
u8 offset_level;
u8 disassemblers[MAX_DISASSEMBLERS];
+ u8 disassembler_used;
int min_pcnt;
int max_lines;
int context;
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 50c5c206b70e..a53e8c4e5bca 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -48,7 +48,7 @@ static int call__scnprintf(struct ins *ins, char *bf, size_t
size,
static void ins__sort(struct arch *arch);
static int disasm_line__parse(char *line, const char **namep, char **rawp);
-static int disasm_line__parse_powerpc(struct disasm_line *dl);
+static int disasm_line__parse_powerpc(struct disasm_line *dl, struct
annotate_args *args);
static char *expand_tabs(char *line, char **storage, size_t *storage_len);
static __attribute__((constructor)) void symbol__init_regexpr(void)
@@ -968,24 +968,24 @@ static int disasm_line__parse(char *line, const char
**namep, char **rawp)
#define PPC_OP(op) (((op) >> 26) & 0x3F)
#define RAW_BYTES 11
-static int disasm_line__parse_powerpc(struct disasm_line *dl)
+static int disasm_line__parse_powerpc(struct disasm_line *dl, struct
annotate_args *args)
{
char *line = dl->al.line;
const char **namep = &dl->ins.name;
char **rawp = &dl->ops.raw;
char *tmp_raw_insn, *name_raw_insn = skip_spaces(line);
char *name = skip_spaces(name_raw_insn + RAW_BYTES);
- int objdump = 0;
+ int disasm = 0;
- if (strlen(line) > RAW_BYTES)
- objdump = 1;
+ if (args->options->disassembler_used)
+ disasm = 1;
if (name_raw_insn[0] == '\0')
return -1;
- if (objdump) {
+ if (disasm)
disasm_line__parse(name, namep, rawp);
- } else
+ else
*namep = "";
tmp_raw_insn = strndup(name_raw_insn, 11);
@@ -995,7 +995,7 @@ static int disasm_line__parse_powerpc(struct disasm_line
*dl)
remove_spaces(tmp_raw_insn);
sscanf(tmp_raw_insn, "%x", &dl->raw.raw_insn);
- if (objdump)
+ if (disasm)
dl->raw.raw_insn = be32_to_cpu(dl->raw.raw_insn);
return 0;
@@ -1054,7 +1054,7 @@ struct disasm_line *disasm_line__new(struct annotate_args
*args)
if (args->offset != -1) {
if (arch__is(args->arch, "powerpc")) {
- if (disasm_line__parse_powerpc(dl) < 0)
+ if (disasm_line__parse_powerpc(dl, args) < 0)
goto out_free_line;
} else if (disasm_line__parse(dl->al.line, &dl->ins.name,
&dl->ops.raw) < 0)
goto out_free_line;
@@ -2289,16 +2289,2
Re: [PATCH v4 0/2] Add stop_on_panic support for watchdog
Hi George, On 05.03.25 11:10, George Cherian wrote: > This series adds a new kernel command line option to watchdog core to > stop the watchdog on panic. This is useul in certain systems which prevents > successful loading of kdump kernel due to watchdog reset. > > Some of the watchdog drivers stop function could sleep. For such > drivers the stop_on_panic is not valid as the notifier callback happens > in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info > options to indicate whether the stop function would sleep. Did you consider having a reset_on_panic instead, which sets a user-specified timeout on panic? This would make the mechanism useful also for watchdogs that can't be disabled and would protect against system lock up: Consider a memory-corruption bug (perhaps externally via DMA), which partially overwrites both main and kdump kernel. With a disabled watchdog, the system may not be able to recover on its own. If you did consider it, what made you decide against it? Thanks, Ahmad > > > Changelog: > v1 -> v2 > - Remove the per driver flag setting option > - Take the parameter via kernel command-line parameter to watchdog_core. > > v2 -> v3 > - Remove the helper function watchdog_stop_on_panic() from watchdog.h. > - There are no users for this. > > v3 -> v4 > - Since the panic notifier is in atomic context, watchdog functions > which sleep can't be called. > - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop > function sleeps. > - Simplify the stop_on_panic kernel command line parsing. > - Enable the panic notiffier only if the watchdog stop function doesn't > sleep > > George Cherian (2): > watchdog: Add a new flag WDIOF_STOP_MAYSLEEP > drivers: watchdog: Add support for panic notifier callback > > drivers/watchdog/advantech_ec_wdt.c | 3 ++- > drivers/watchdog/arm_smc_wdt.c | 3 ++- > drivers/watchdog/armada_37xx_wdt.c | 2 +- > drivers/watchdog/asm9260_wdt.c | 2 +- > drivers/watchdog/bcm47xx_wdt.c | 3 ++- > drivers/watchdog/bd9576_wdt.c | 2 +- > drivers/watchdog/bd96801_wdt.c | 2 +- > drivers/watchdog/cgbc_wdt.c | 2 +- > drivers/watchdog/cros_ec_wdt.c | 5 - > drivers/watchdog/da9052_wdt.c | 3 ++- > drivers/watchdog/da9055_wdt.c | 4 +++- > drivers/watchdog/da9062_wdt.c | 4 +++- > drivers/watchdog/da9063_wdt.c | 4 +++- > drivers/watchdog/db8500_wdt.c | 5 - > drivers/watchdog/dw_wdt.c | 5 +++-- > drivers/watchdog/f71808e_wdt.c | 3 ++- > drivers/watchdog/gpio_wdt.c | 2 +- > drivers/watchdog/i6300esb.c | 5 - > drivers/watchdog/imx_sc_wdt.c | 2 +- > drivers/watchdog/intel-mid_wdt.c| 5 - > drivers/watchdog/it87_wdt.c | 5 - > drivers/watchdog/jz4740_wdt.c | 5 - > drivers/watchdog/kempld_wdt.c | 3 ++- > drivers/watchdog/lenovo_se10_wdt.c | 5 - > drivers/watchdog/max77620_wdt.c | 5 - > drivers/watchdog/mei_wdt.c | 3 ++- > drivers/watchdog/menf21bmc_wdt.c| 4 +++- > drivers/watchdog/mlx_wdt.c | 2 +- > drivers/watchdog/msc313e_wdt.c | 5 - > drivers/watchdog/npcm_wdt.c | 3 ++- > drivers/watchdog/omap_wdt.c | 5 - > drivers/watchdog/pm8916_wdt.c | 5 +++-- > drivers/watchdog/pseries-wdt.c | 2 +- > drivers/watchdog/rave-sp-wdt.c | 5 - > drivers/watchdog/renesas_wdt.c | 7 -- > drivers/watchdog/retu_wdt.c | 5 - > drivers/watchdog/rn5t618_wdt.c | 6 +++-- > drivers/watchdog/rzg2l_wdt.c| 5 - > drivers/watchdog/rzv2h_wdt.c| 5 - > drivers/watchdog/shwdt.c| 6 +++-- > drivers/watchdog/sl28cpld_wdt.c | 5 - > drivers/watchdog/softdog.c | 5 - > drivers/watchdog/sp805_wdt.c| 5 - > drivers/watchdog/starfive-wdt.c | 3 ++- > drivers/watchdog/stpmic1_wdt.c | 5 - > drivers/watchdog/ts4800_wdt.c | 5 - > drivers/watchdog/twl4030_wdt.c | 5 - > drivers/watchdog/uniphier_wdt.c | 3 ++- > drivers/watchdog/w83627hf_wdt.c | 5 - > drivers/watchdog/watchdog_core.c| 35 + > drivers/watchdog/wm831x_wdt.c | 5 - > drivers/watchdog/wm8350_wdt.c | 5 - > drivers/watchdog/xen_wdt.c | 5 - > drivers/watchdog/ziirave_wdt.c | 5 - > include/linux/watchdog.h| 2 ++ > include/uapi/linux/watchdog.h | 1 + > 56 files changed, 198 insertions(+), 58 deletions(-) > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v4 1/2] watchdog: Add a new flag WDIOF_STOP_MAYSLEEP
Hi George, Hi Guenter, On 05.03.25 11:34, George Cherian wrote: >> why is armada_37xx_wdt also here? >> The stop function in that driver may not sleep. > Marek, > > Thanks for reviewing. > Since the stop function has a regmap_write(), I thought it might sleep. > Now that you pointed it out, I assume that it is an MMIO based regmap being > used for armada. > I will update the same in the next version Failure to add WDIOF_STOP_MAYSLEEP when it's needed can lead to kernel hanging. Failure to add an alternative WDIOF_STOP_ATOMIC would lead to the kernel option being a no-op. I think a no-op stop_on_panic (or reset_on_panic) is a saner default. Cheers, Ahmad > >> >> Marek > > -George -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v4 1/2] watchdog: Add a new flag WDIOF_STOP_MAYSLEEP
Hi George, why is armada_37xx_wdt also here? The stop function in that driver may not sleep. Marek
[PATCH v4 1/2] watchdog: Add a new flag WDIOF_STOP_MAYSLEEP
A new option flag is added to watchdog_info. This helps the watchdog
core to check whether stop functions would sleep or not.
The option flags of individual drivers are also updated accordingly.
Signed-off-by: George Cherian
---
drivers/watchdog/advantech_ec_wdt.c | 3 ++-
drivers/watchdog/arm_smc_wdt.c | 3 ++-
drivers/watchdog/armada_37xx_wdt.c | 2 +-
drivers/watchdog/asm9260_wdt.c | 2 +-
drivers/watchdog/bcm47xx_wdt.c | 3 ++-
drivers/watchdog/bd9576_wdt.c | 2 +-
drivers/watchdog/bd96801_wdt.c | 2 +-
drivers/watchdog/cgbc_wdt.c | 2 +-
drivers/watchdog/cros_ec_wdt.c | 5 -
drivers/watchdog/da9052_wdt.c | 3 ++-
drivers/watchdog/da9055_wdt.c | 4 +++-
drivers/watchdog/da9062_wdt.c | 4 +++-
drivers/watchdog/da9063_wdt.c | 4 +++-
drivers/watchdog/db8500_wdt.c | 5 -
drivers/watchdog/dw_wdt.c | 5 +++--
drivers/watchdog/f71808e_wdt.c | 3 ++-
drivers/watchdog/gpio_wdt.c | 2 +-
drivers/watchdog/i6300esb.c | 5 -
drivers/watchdog/imx_sc_wdt.c | 2 +-
drivers/watchdog/intel-mid_wdt.c| 5 -
drivers/watchdog/it87_wdt.c | 5 -
drivers/watchdog/jz4740_wdt.c | 5 -
drivers/watchdog/kempld_wdt.c | 3 ++-
drivers/watchdog/lenovo_se10_wdt.c | 5 -
drivers/watchdog/max77620_wdt.c | 5 -
drivers/watchdog/mei_wdt.c | 3 ++-
drivers/watchdog/menf21bmc_wdt.c| 4 +++-
drivers/watchdog/mlx_wdt.c | 2 +-
drivers/watchdog/msc313e_wdt.c | 5 -
drivers/watchdog/npcm_wdt.c | 3 ++-
drivers/watchdog/omap_wdt.c | 5 -
drivers/watchdog/pm8916_wdt.c | 5 +++--
drivers/watchdog/pseries-wdt.c | 2 +-
drivers/watchdog/rave-sp-wdt.c | 5 -
drivers/watchdog/renesas_wdt.c | 7 +--
drivers/watchdog/retu_wdt.c | 5 -
drivers/watchdog/rn5t618_wdt.c | 6 --
drivers/watchdog/rzg2l_wdt.c| 5 -
drivers/watchdog/rzv2h_wdt.c| 5 -
drivers/watchdog/shwdt.c| 6 --
drivers/watchdog/sl28cpld_wdt.c | 5 -
drivers/watchdog/softdog.c | 5 -
drivers/watchdog/sp805_wdt.c| 5 -
drivers/watchdog/starfive-wdt.c | 3 ++-
drivers/watchdog/stpmic1_wdt.c | 5 -
drivers/watchdog/ts4800_wdt.c | 5 -
drivers/watchdog/twl4030_wdt.c | 5 -
drivers/watchdog/uniphier_wdt.c | 3 ++-
drivers/watchdog/w83627hf_wdt.c | 5 -
drivers/watchdog/wm831x_wdt.c | 5 -
drivers/watchdog/wm8350_wdt.c | 5 -
drivers/watchdog/xen_wdt.c | 5 -
drivers/watchdog/ziirave_wdt.c | 5 -
include/uapi/linux/watchdog.h | 1 +
54 files changed, 161 insertions(+), 58 deletions(-)
diff --git a/drivers/watchdog/advantech_ec_wdt.c
b/drivers/watchdog/advantech_ec_wdt.c
index 7c380f90ca58..5b62c675c2cb 100644
--- a/drivers/watchdog/advantech_ec_wdt.c
+++ b/drivers/watchdog/advantech_ec_wdt.c
@@ -131,7 +131,8 @@ static const struct watchdog_info adv_ec_wdt_info = {
.identity = DRIVER_NAME,
.options = WDIOF_SETTIMEOUT |
WDIOF_MAGICCLOSE |
- WDIOF_KEEPALIVEPING,
+ WDIOF_KEEPALIVEPING |
+ WDIOF_STOP_MAYSLEEP,
};
static const struct watchdog_ops adv_ec_wdt_ops = {
diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c
index 8f3d0c3a005f..794cf0086912 100644
--- a/drivers/watchdog/arm_smc_wdt.c
+++ b/drivers/watchdog/arm_smc_wdt.c
@@ -90,7 +90,8 @@ static const struct watchdog_info smcwd_info = {
.identity = DRV_NAME,
.options= WDIOF_SETTIMEOUT |
WDIOF_KEEPALIVEPING |
- WDIOF_MAGICCLOSE,
+ WDIOF_MAGICCLOSE |
+ WDIOF_STOP_MAYSLEEP,
};
static const struct watchdog_ops smcwd_ops = {
diff --git a/drivers/watchdog/armada_37xx_wdt.c
b/drivers/watchdog/armada_37xx_wdt.c
index a17a7911771a..4e5ed9e1ed90 100644
--- a/drivers/watchdog/armada_37xx_wdt.c
+++ b/drivers/watchdog/armada_37xx_wdt.c
@@ -232,7 +232,7 @@ static int armada_37xx_wdt_stop(struct watchdog_device *wdt)
}
static const struct watchdog_info armada_37xx_wdt_info = {
- .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE |
WDIOF_STOP_MAYSLEEP,
.identity = "Armada 37xx Watchdog",
};
diff --git a/drivers/watchdog/asm9260_wdt.c b/drivers/watchdog/asm9260_wdt.c
index 45047e514b8e..24402206659b 100644
--- a/drivers/watchdog/asm9260_wdt.c
+++ b/drivers/watchdog/asm9260_wdt.c
@@ -181,7 +181,7 @@ static int asm9260_restart(struct watchdog_device *wdd,
unsigned long action,
static const struct watchdog_info asm9260_wdt_ident = {
.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING
- | WDIOF_MA
Re: [PATCH v4 1/2] watchdog: Add a new flag WDIOF_STOP_MAYSLEEP
On Wed, Mar 05, 2025 at 10:10:24AM +, George Cherian wrote: > A new option flag is added to watchdog_info. This helps the watchdog > core to check whether stop functions would sleep or not. > The option flags of individual drivers are also updated accordingly. ... > .options = WDIOF_SETTIMEOUT | > WDIOF_MAGICCLOSE | > - WDIOF_KEEPALIVEPING, > + WDIOF_KEEPALIVEPING | > + WDIOF_STOP_MAYSLEEP, You may heavily reduce this change if you squeeze the new option just before the last one. Currently it's a lot of unneeded churn that makes review a bit harder (each needs to be carefully checked for the correctness). -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 1/2] watchdog: Add a new flag WDIOF_STOP_MAYSLEEP
From: Marek Behún Sent: Wednesday, March 5, 2025 3:48 PM To: George Cherian Cc: li...@roeck-us.net; w...@linux-watchdog.org; jwer...@chromium.org; evanb...@chromium.org; k...@kernel.org; mazziesacco...@gmail.com; thomas.rich...@bootlin.com; l...@chromium.org; ble...@chromium.org; support.opensou...@diasemi.com; shawn...@kernel.org; s.ha...@pengutronix.de; ker...@pengutronix.de; feste...@gmail.com; a...@kernel.org; p...@crapouillou.net; alexander.usys...@intel.com; andreas.wer...@men.de; dan...@thingy.jp; romain.per...@gmail.com; avifishma...@gmail.com; tmaimo...@gmail.com; tali.per...@gmail.com; vent...@google.com; yu...@google.com; benjaminf...@google.com; ma...@linux.ibm.com; m...@ellerman.id.au; npig...@gmail.com; christophe.le...@csgroup.eu; nav...@kernel.org; mwa...@kernel.org; xingyu...@starfivetech.com; ziv...@starfivetech.com; hayashi.kunih...@socionext.com; mhira...@kernel.org; linux-watch...@vger.kernel.org; linux-ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; chrome-platf...@lists.linux.dev; i...@lists.linux.dev; linux-m...@vger.kernel.org; open...@lists.ozlabs.org; linuxppc-dev@lists.ozlabs.org; patc...@opensource.cirrus.com Subject: Re: [PATCH v4 1/2] watchdog: Add a new flag WDIOF_STOP_MAYSLEEP > Hi George, > > why is armada_37xx_wdt also here? > The stop function in that driver may not sleep. Marek, Thanks for reviewing. Since the stop function has a regmap_write(), I thought it might sleep. Now that you pointed it out, I assume that it is an MMIO based regmap being used for armada. I will update the same in the next version > > Marek -George
Re: [PATCH v4 2/2] drivers: watchdog: Add support for panic notifier callback
On Wed, Mar 05, 2025 at 10:10:25AM +, George Cherian wrote: > Watchdog is not turned off in kernel panic situation. > In certain systems this might prevent the successful loading > of kdump kernel. The kdump kernel might hit a watchdog reset > while it is booting. > > To avoid such scenarios add a panic notifier call back function > which can stop the watchdog. This provision can be enabled by > passing watchdog.stop_on_panic=1 via kernel command-line parameter. ... First of all, do we really need a new module parameter for that? Why can't it be done automatically if kdump is expected? > +static bool stop_on_panic; > +module_param(stop_on_panic, bool, 0444); > +MODULE_PARM_DESC(stop_on_panic, "Stop watchdogs on panic (0=keep watching, > 1=stop)"); + blank line. Also I do not see the documentation update. Where is it lost? > /* -- With Best Regards, Andy Shevchenko
Re: [PATCH RFC v2 02/29] x86: Create CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION
On Sat, Mar 01, 2025 at 09:23:51AM +0200, Mike Rapoport wrote: > Hi Brendan, > > On Fri, Jan 10, 2025 at 06:40:28PM +, Brendan Jackman wrote: > > Currently a nop config. Keeping as a separate commit for easy review of > > the boring bits. Later commits will use and enable this new config. > > > > This config is only added for non-UML x86_64 as other architectures do > > not yet have pending implementations. It also has somewhat artificial > > dependencies on !PARAVIRT and !KASAN which are explained in the Kconfig > > file. > > > > Co-developed-by: Junaid Shahid > > Signed-off-by: Junaid Shahid > > Signed-off-by: Brendan Jackman > > --- > > arch/alpha/include/asm/Kbuild | 1 + > > arch/arc/include/asm/Kbuild| 1 + > > arch/arm/include/asm/Kbuild| 1 + > > arch/arm64/include/asm/Kbuild | 1 + > > arch/csky/include/asm/Kbuild | 1 + > > arch/hexagon/include/asm/Kbuild| 1 + > > arch/loongarch/include/asm/Kbuild | 3 +++ > > arch/m68k/include/asm/Kbuild | 1 + > > arch/microblaze/include/asm/Kbuild | 1 + > > arch/mips/include/asm/Kbuild | 1 + > > arch/nios2/include/asm/Kbuild | 1 + > > arch/openrisc/include/asm/Kbuild | 1 + > > arch/parisc/include/asm/Kbuild | 1 + > > arch/powerpc/include/asm/Kbuild| 1 + > > arch/riscv/include/asm/Kbuild | 1 + > > arch/s390/include/asm/Kbuild | 1 + > > arch/sh/include/asm/Kbuild | 1 + > > arch/sparc/include/asm/Kbuild | 1 + > > arch/um/include/asm/Kbuild | 2 +- > > arch/x86/Kconfig | 14 ++ > > arch/xtensa/include/asm/Kbuild | 1 + > > include/asm-generic/asi.h | 5 + > > 22 files changed, 41 insertions(+), 1 deletion(-) > > I don't think this all is needed. You can put asi.h with stubs used outside > of arch/x86 in include/linux and save you the hassle of updating every > architecture. ... > If you expect other architectures might implement ASI the config would better > fit into init/Kconfig or mm/Kconfig and in arch/x86/Kconfig will define > ARCH_HAS_MITIGATION_ADDRESS_SPACE_ISOLATION. ... > > +++ b/include/asm-generic/asi.h > > @@ -0,0 +1,5 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef __ASM_GENERIC_ASI_H > > +#define __ASM_GENERIC_ASI_H > > + > > +#endif > > IMHO it should be include/linux/asi.h, with something like > > #infdef __LINUX_ASI_H > #define __LINUX_ASI_H > > #ifdef CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION > > #include > > #else /* CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION */ > > /* stubs for functions used outside arch/ */ > > #endif /* CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION */ > > #endif /* __LINUX_ASI_H */ Thanks Mike! That does indeed look way tidier. I'll try to adopt it.
Re: [Kernel 6.12.17] [PowerPC e5500] KVM HV compilation error
On Wed, Mar 5, 2025 at 3:19 PM Greg KH wrote:
> On Wed, Mar 05, 2025 at 03:14:13PM +0100, Christian Zigotzky wrote:
> > Hi All,
> >
> > The stable long-term kernel 6.12.17 cannot compile with KVM HV support for
> > e5500 PowerPC machines anymore.
> >
> > Bug report: https://github.com/chzigotzky/kernels/issues/6
> >
> > Kernel config:
> > https://github.com/chzigotzky/kernels/blob/6_12/configs/x5000_defconfig
> >
> > Error messages:
> >
> > arch/powerpc/kvm/e500_mmu_host.c: In function 'kvmppc_e500_shadow_map':
> > arch/powerpc/kvm/e500_mmu_host.c:447:9: error: implicit declaration of
> > function '__kvm_faultin_pfn' [-Werror=implicit-function-declaration]
> >pfn = __kvm_faultin_pfn(slot, gfn, FOLL_WRITE, NULL, &page);
> > ^
> > CC kernel/notifier.o
> > arch/powerpc/kvm/e500_mmu_host.c:500:2: error: implicit declaration of
> > function 'kvm_release_faultin_page'; did you mean 'kvm_read_guest_page'?
> > [-Werror=implicit-function-declaration]
> > kvm_release_faultin_page(kvm, page, !!ret, writable);
> >
> > After that, I compiled it without KVM HV support.
> >
> > Kernel config:
> > https://github.com/chzigotzky/kernels/blob/6_12/configs/e5500_defconfig
> >
> > Please check the error messages.
>
> Odd, what commit caused this problem?
48fe216d7db6b651972c1c1d8e3180cd699971b0
> Any hint as to what commit is missing to fix it?
A big-ass 90 patch series. __kvm_faultin_pfn and
kvm_release_faultin_page were introduced in 6.13, as part of a big
revamp of how KVM does page faults on all architectures.
Just revert all this crap and apply the version that I've just sent
(https://lore.kernel.org/stable/20250305144938.212918-1-pbonz...@redhat.com/):
commit 48fe216d7db6b651972c1c1d8e3180cd699971b0
KVM: e500: always restore irqs
commit 833f69be62ac366b5c23b4a6434389e470dd5c7f
KVM: PPC: e500: Use __kvm_faultin_pfn() to handle page faults
Message-ID: <20241010182427.1434605-55-sea...@google.com>
Stable-dep-of: 87ecfdbc699c ("KVM: e500: always restore irqs")
commit f2623aec7fdc2675667042c85f87502c9139c098
KVM: PPC: e500: Mark "struct page" pfn accessed before dropping mmu_lock
Message-ID: <20241010182427.1434605-54-sea...@google.com>
Stable-dep-of: 87ecfdbc699c ("KVM: e500: always restore irqs")
commit dec857329fb9a66a5bce4f9db14c97ef64725a32
KVM: PPC: e500: Mark "struct page" dirty in kvmppc_e500_shadow_map()
Message-ID: <20241010182427.1434605-53-sea...@google.com>
Stable-dep-of: 87ecfdbc699c ("KVM: e500: always restore irqs")
And this, ladies and gentlemen, is why I always include the apparently
silly Message-ID trailer. Don't you just love how someone, whether
script or human, cherry picked patches 53-55 without even wondering
what was in the 52 before. I'm not sure if it'd be worse for it to be
a human or a script... because if it's a script, surely the same level
of sophistication could have been put into figuring out whether the
thing even COMPILES.
Sasha, this wins the prize for most ridiculous automatic backport
ever. Please stop playing maintainer if you can't be bothered to read
the commit messages for random stuff that you apply.
Paolo
Re: [Kernel 6.12.17] [PowerPC e5500] KVM HV compilation error
On Wed, Mar 05, 2025 at 03:14:13PM +0100, Christian Zigotzky wrote: > Hi All, > > The stable long-term kernel 6.12.17 cannot compile with KVM HV support for > e5500 PowerPC machines anymore. > > Bug report: https://github.com/chzigotzky/kernels/issues/6 > > Kernel config: > https://github.com/chzigotzky/kernels/blob/6_12/configs/x5000_defconfig > > Error messages: > > arch/powerpc/kvm/e500_mmu_host.c: In function 'kvmppc_e500_shadow_map': > arch/powerpc/kvm/e500_mmu_host.c:447:9: error: implicit declaration of > function '__kvm_faultin_pfn' [-Werror=implicit-function-declaration] >pfn = __kvm_faultin_pfn(slot, gfn, FOLL_WRITE, NULL, &page); > ^ > CC kernel/notifier.o > arch/powerpc/kvm/e500_mmu_host.c:500:2: error: implicit declaration of > function 'kvm_release_faultin_page'; did you mean 'kvm_read_guest_page'? > [-Werror=implicit-function-declaration] > kvm_release_faultin_page(kvm, page, !!ret, writable); > > After that, I compiled it without KVM HV support. > > Kernel config: > https://github.com/chzigotzky/kernels/blob/6_12/configs/e5500_defconfig > > Please check the error messages. Odd, what commit caused this problem? Any hint as to what commit is missing to fix it? thanks, greg k-h
[Kernel 6.12.17] [PowerPC e5500] KVM HV compilation error
Hi All, The stable long-term kernel 6.12.17 cannot compile with KVM HV support for e5500 PowerPC machines anymore. Bug report: https://github.com/chzigotzky/kernels/issues/6 Kernel config: https://github.com/chzigotzky/kernels/blob/6_12/configs/x5000_defconfig Error messages: arch/powerpc/kvm/e500_mmu_host.c: In function 'kvmppc_e500_shadow_map': arch/powerpc/kvm/e500_mmu_host.c:447:9: error: implicit declaration of function '__kvm_faultin_pfn' [-Werror=implicit-function-declaration] pfn = __kvm_faultin_pfn(slot, gfn, FOLL_WRITE, NULL, &page); ^ CC kernel/notifier.o arch/powerpc/kvm/e500_mmu_host.c:500:2: error: implicit declaration of function 'kvm_release_faultin_page'; did you mean 'kvm_read_guest_page'? [-Werror=implicit-function-declaration] kvm_release_faultin_page(kvm, page, !!ret, writable); After that, I compiled it without KVM HV support. Kernel config: https://github.com/chzigotzky/kernels/blob/6_12/configs/e5500_defconfig Please check the error messages. Thanks, Christian
Re: [Kernel 6.12.17] [PowerPC e5500] KVM HV compilation error
On Wed, Mar 05, 2025, Greg KH wrote:
> On Wed, Mar 05, 2025 at 03:14:13PM +0100, Christian Zigotzky wrote:
> > Hi All,
> >
> > The stable long-term kernel 6.12.17 cannot compile with KVM HV support for
> > e5500 PowerPC machines anymore.
> >
> > Bug report: https://github.com/chzigotzky/kernels/issues/6
> >
> > Kernel config:
> > https://github.com/chzigotzky/kernels/blob/6_12/configs/x5000_defconfig
> >
> > Error messages:
> >
> > arch/powerpc/kvm/e500_mmu_host.c: In function 'kvmppc_e500_shadow_map':
> > arch/powerpc/kvm/e500_mmu_host.c:447:9: error: implicit declaration of
> > function '__kvm_faultin_pfn' [-Werror=implicit-function-declaration]
> >pfn = __kvm_faultin_pfn(slot, gfn, FOLL_WRITE, NULL, &page);
> > ^
> > CC kernel/notifier.o
> > arch/powerpc/kvm/e500_mmu_host.c:500:2: error: implicit declaration of
> > function 'kvm_release_faultin_page'; did you mean 'kvm_read_guest_page'?
> > [-Werror=implicit-function-declaration]
> > kvm_release_faultin_page(kvm, page, !!ret, writable);
> >
> > After that, I compiled it without KVM HV support.
> >
> > Kernel config:
> > https://github.com/chzigotzky/kernels/blob/6_12/configs/e5500_defconfig
> >
> > Please check the error messages.
>
> Odd, what commit caused this problem? Any hint as to what commit is
> missing to fix it?
833f69be62ac. It most definitely should be reverted. The "dependency" for
commit
87ecfdbc699c ("KVM: e500: always restore irqs") is a superficial code conflict.
Oof. The same buggy patch was queue/proposed for all stable trees from 5.4
onward,
but it look like it only landed in 6.1, 6.6, and 6.12. I'll send reverts.
commit 833f69be62ac366b5c23b4a6434389e470dd5c7f
Author: Sean Christopherson
AuthorDate: Thu Oct 10 11:23:56 2024 -0700
Commit: Greg Kroah-Hartman
CommitDate: Mon Feb 17 10:04:56 2025 +0100
KVM: PPC: e500: Use __kvm_faultin_pfn() to handle page faults
[ Upstream commit 419cfb983ca93e75e905794521afefcfa07988bb ]
Convert PPC e500 to use __kvm_faultin_pfn()+kvm_release_faultin_page(),
and continue the inexorable march towards the demise of
kvm_pfn_to_refcounted_page().
Signed-off-by: Sean Christopherson
Tested-by: Dmitry Osipenko
Signed-off-by: Paolo Bonzini
Message-ID: <20241010182427.1434605-55-sea...@google.com>
Stable-dep-of: 87ecfdbc699c ("KVM: e500: always restore irqs")
Signed-off-by: Sasha Levin
Re: [PATCH v2 1/4] powerpc/perf/core-book3s: Avoid loading platform pmu driver during dump kernel
Hi Madhavan,
kernel test robot noticed the following build errors:
[auto build test ERROR on powerpc/next]
[also build test ERROR on powerpc/fixes linus/master v6.14-rc5 next-20250305]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Madhavan-Srinivasan/powerpc-perf-hv-24x7-Avoid-loading-hv-24x7-during-dump-kernel/20250302-022531
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
patch link:
https://lore.kernel.org/r/20250301182310.6832-1-maddy%40linux.ibm.com
patch subject: [PATCH v2 1/4] powerpc/perf/core-book3s: Avoid loading platform
pmu driver during dump kernel
config: powerpc64-randconfig-001-20250305
(https://download.01.org/0day-ci/archive/20250305/202503052346.etbppobo-...@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project
cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20250305/202503052346.etbppobo-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202503052346.etbppobo-...@intel.com/
All errors (new ones prefixed by >>):
>> arch/powerpc/perf/core-book3s.c:2599:6: error: call to undeclared function
>> 'is_kdump_kernel'; ISO C99 and later do not support implicit function
>> declarations [-Wimplicit-function-declaration]
2599 | if (is_kdump_kernel() || is_fadump_active())
| ^
1 error generated.
vim +/is_kdump_kernel +2599 arch/powerpc/perf/core-book3s.c
2587
2588 static int __init init_ppc64_pmu(void)
2589 {
2590 if (cpu_has_feature(CPU_FTR_HVMODE) && pmu_override) {
2591 pr_warn("disabling perf due to pmu_override= command
line option.\n");
2592 on_each_cpu(do_pmu_override, NULL, 1);
2593 return 0;
2594 }
2595
2596 /*
2597 * If the dump kernel is active, skip loading these drivers
2598 */
> 2599 if (is_kdump_kernel() || is_fadump_active())
2600 return 0;
2601
2602 /* run through all the pmu drivers one at a time */
2603 if (!init_power5_pmu())
2604 return 0;
2605 else if (!init_power5p_pmu())
2606 return 0;
2607 else if (!init_power6_pmu())
2608 return 0;
2609 else if (!init_power7_pmu())
2610 return 0;
2611 else if (!init_power8_pmu())
2612 return 0;
2613 else if (!init_power9_pmu())
2614 return 0;
2615 else if (!init_power10_pmu())
2616 return 0;
2617 else if (!init_power11_pmu())
2618 return 0;
2619 else if (!init_ppc970_pmu())
2620 return 0;
2621 else
2622 return init_generic_compat_pmu();
2623 }
2624 early_initcall(init_ppc64_pmu);
2625
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v4] powerpc/hugetlb: Disable gigantic hugepages if fadump is active
Hello Ritesh,
On 04/03/25 10:27, Ritesh Harjani (IBM) wrote:
Sourabh Jain writes:
Hello Ritesh,
Thanks for the review.
On 02/03/25 12:05, Ritesh Harjani (IBM) wrote:
Sourabh Jain writes:
The fadump kernel boots with limited memory solely to collect the kernel
core dump. Having gigantic hugepages in the fadump kernel is of no use.
Sure got it.
Many times, the fadump kernel encounters OOM (Out of Memory) issues if
gigantic hugepages are allocated.
To address this, disable gigantic hugepages if fadump is active by
returning early from arch_hugetlb_valid_size() using
hugepages_supported(). When fadump is active, the global variable
hugetlb_disabled is set to true, which is later used by the
PowerPC-specific hugepages_supported() function to determine hugepage
support.
Returning early from arch_hugetlb_vali_size() not only disables
gigantic hugepages but also avoids unnecessary hstate initialization for
every hugepage size supported by the platform.
kernel logs related to hugepages with this patch included:
kernel argument passed: hugepagesz=1G hugepages=1
First kernel: gigantic hugepage got allocated
==
dmesg | grep -i "hugetlb"
-
HugeTLB: registered 1.00 GiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page
HugeTLB: registered 2.00 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 2.00 MiB page
$ cat /proc/meminfo | grep -i "hugetlb"
-
Hugetlb: 1048576 kB
Was this tested with patch [1] in your local tree?
[1]:
https://web.git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=d629d7a8efc33
IIUC, this patch [1] disables the boot time allocation of hugepages.
Isn't it also disabling the boot time allocation for gigantic huge pages
passed by the cmdline params like hugepagesz=1G and hugepages=2 ?
Yes, I had the patch [1] in my tree.
My understanding is that gigantic pages are allocated before normal huge
pages.
In hugepages_setup() in hugetlb.c, we have:
if (hugetlb_max_hstate && hstate_is_gigantic(parsed_hstate))
hugetlb_hstate_alloc_pages(parsed_hstate);
I believe the above code allocates memory for gigantic pages, and
hugetlb_init() is
called later because it is a subsys_initcall.
So, by the time the kernel reaches hugetlb_init(), the gigantic pages
are already
allocated. Isn't that right?
Please let me know your opinion.
Yes, you are right. We are allocating hugepages from memblock, however
this isn't getting advertized anywhere. i.e. there is no way one can
know from any user interface on whether hugepages were allocated or not.
i.e. for fadump kernel when hugepagesz= and hugepages= params are
passed, though it will allocate gigantic pages, it won't advertize this
in meminfo or anywhere else. This was adding the confusion when I tested
this (which wasn't clear from the commit msg either).
And I guess this is happening during fadump kernel because of our patch
[1], which added a check to see whether hugetlb_disabled is true in
hugepages_supported(). Due to this hugetlb_init() is now not doing the
rest of the initialization for those gigantic pages which were allocated
due to cmdline options from hugepages_setup().
[1]:
https://lore.kernel.org/linuxppc-dev/20241202054310.928610-1-sourabhj...@linux.ibm.com/
Now as we know from below that fadump can set hugetlb_disabled call in
early_setup().
i.e. fadump can mark hugetlb_disabled to true in
early_setup() -> early_init_devtree() -> fadump_reserve_mem()
And hugepages_setup() and hugepagesz_setup() gets called late in
start_kernel() -> parse_args()
And we already check for hugepages_supported() in all necessary calls in
mm/hugetlb.c. So IMO, this check should go in mm/hugetlb.c in
hugepagesz_setup() and hugepages_setup(). Because otherwise every arch
implementation will end up duplicating this by adding
hugepages_supported() check in their arch implementation of
arch_hugetlb_valid_size().
e.g. references of hugepages_supported() checks in mm/hugetlb.c
mm/hugetlb.c hugetlb_show_meminfo_node 4959 if (!hugepages_supported())
mm/hugetlb.c hugetlb_report_node_meminfo 4943 if (!hugepages_supported())
mm/hugetlb.c hugetlb_report_meminfo 4914 if (!hugepages_supported())
mm/hugetlb.c hugetlb_overcommit_handler 4848 if (!hugepages_supported())
mm/hugetlb.c hugetlb_sysctl_handler_common 4809 if (!hugepages_supported())
mm/hugetlb.c hugetlb_init 4461 if (!hugepages_supported()) {
mm/hugetlb.c dissolve_free_hugetlb_folios 2211 if (!hugepages_supported())
fs/hugetlbfs/inode.c init_hugetlbfs_fs 1604 if (!hugepages_supported()) {
Let me also see the history on why this wasn't done earlier though...
... Oh actually there is more history to this. See [2]. We already had
hugepages_supported() check in hugepages_setup() and other places
earlier which was removed to fix some other problem in ppc ;)
[2]:
https://web.git.kernel.org/
Re: [PATCH v4] powerpc/hugetlb: Disable gigantic hugepages if fadump is active
Sourabh Jain writes: > Hello Ritesh, > > > On 04/03/25 10:27, Ritesh Harjani (IBM) wrote: >> Sourabh Jain writes: >> >>> Hello Ritesh, >>> >>> Thanks for the review. >>> >>> On 02/03/25 12:05, Ritesh Harjani (IBM) wrote: Sourabh Jain writes: > The fadump kernel boots with limited memory solely to collect the kernel > core dump. Having gigantic hugepages in the fadump kernel is of no use. Sure got it. > Many times, the fadump kernel encounters OOM (Out of Memory) issues if > gigantic hugepages are allocated. > > To address this, disable gigantic hugepages if fadump is active by > returning early from arch_hugetlb_valid_size() using > hugepages_supported(). When fadump is active, the global variable > hugetlb_disabled is set to true, which is later used by the > PowerPC-specific hugepages_supported() function to determine hugepage > support. > > Returning early from arch_hugetlb_vali_size() not only disables > gigantic hugepages but also avoids unnecessary hstate initialization for > every hugepage size supported by the platform. > > kernel logs related to hugepages with this patch included: > kernel argument passed: hugepagesz=1G hugepages=1 > > First kernel: gigantic hugepage got allocated > == > > dmesg | grep -i "hugetlb" > - > HugeTLB: registered 1.00 GiB page size, pre-allocated 1 pages > HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page > HugeTLB: registered 2.00 MiB page size, pre-allocated 0 pages > HugeTLB: 0 KiB vmemmap can be freed for a 2.00 MiB page > > $ cat /proc/meminfo | grep -i "hugetlb" > - > Hugetlb: 1048576 kB Was this tested with patch [1] in your local tree? [1]: https://web.git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=d629d7a8efc33 IIUC, this patch [1] disables the boot time allocation of hugepages. Isn't it also disabling the boot time allocation for gigantic huge pages passed by the cmdline params like hugepagesz=1G and hugepages=2 ? >>> Yes, I had the patch [1] in my tree. >>> >>> My understanding is that gigantic pages are allocated before normal huge >>> pages. >>> >>> In hugepages_setup() in hugetlb.c, we have: >>> >>> if (hugetlb_max_hstate && hstate_is_gigantic(parsed_hstate)) >>> hugetlb_hstate_alloc_pages(parsed_hstate); >>> >>> I believe the above code allocates memory for gigantic pages, and >>> hugetlb_init() is >>> called later because it is a subsys_initcall. >>> >>> So, by the time the kernel reaches hugetlb_init(), the gigantic pages >>> are already >>> allocated. Isn't that right? >>> >>> Please let me know your opinion. >> Yes, you are right. We are allocating hugepages from memblock, however >> this isn't getting advertized anywhere. i.e. there is no way one can >> know from any user interface on whether hugepages were allocated or not. >> i.e. for fadump kernel when hugepagesz= and hugepages= params are >> passed, though it will allocate gigantic pages, it won't advertize this >> in meminfo or anywhere else. This was adding the confusion when I tested >> this (which wasn't clear from the commit msg either). >> >> And I guess this is happening during fadump kernel because of our patch >> [1], which added a check to see whether hugetlb_disabled is true in >> hugepages_supported(). Due to this hugetlb_init() is now not doing the >> rest of the initialization for those gigantic pages which were allocated >> due to cmdline options from hugepages_setup(). >> >> [1]: >> https://lore.kernel.org/linuxppc-dev/20241202054310.928610-1-sourabhj...@linux.ibm.com/ >> >> Now as we know from below that fadump can set hugetlb_disabled call in >> early_setup(). >> i.e. fadump can mark hugetlb_disabled to true in >> early_setup() -> early_init_devtree() -> fadump_reserve_mem() >> >> And hugepages_setup() and hugepagesz_setup() gets called late in >> start_kernel() -> parse_args() >> >> >> And we already check for hugepages_supported() in all necessary calls in >> mm/hugetlb.c. So IMO, this check should go in mm/hugetlb.c in >> hugepagesz_setup() and hugepages_setup(). Because otherwise every arch >> implementation will end up duplicating this by adding >> hugepages_supported() check in their arch implementation of >> arch_hugetlb_valid_size(). >> >> e.g. references of hugepages_supported() checks in mm/hugetlb.c >> >> mm/hugetlb.c hugetlb_show_meminfo_node 4959 if (!hugepages_supported()) >> mm/hugetlb.c hugetlb_report_node_meminfo 4943 if (!hugepages_supported()) >> mm/hugetlb.c hugetlb_report_meminfo 4914 if (!hugepages_supported()) >> mm/hugetlb.c hugetlb_overcommit_handler 4848 if (!hugepages_supported()) >> mm/hugetlb.c hugetlb_sysctl_handler_common 4809 if (!hugepages_supported()) >> mm/hugetlb.c hugetlb_init 4461 if (!hugepages_s
Re: [PATCH v4 0/2] Add stop_on_panic support for watchdog
Hi Ahmad, >Hi George, > >On 05.03.25 12:28, George Cherian wrote: > Hi Ahmad, >>> Hi George, >>> On 05.03.25 11:10, George Cherian wrote: This series adds a new kernel command line option to watchdog core to stop the watchdog on panic. This is useul in certain systems which prevents successful loading of kdump kernel due to watchdog reset. Some of the watchdog drivers stop function could sleep. For such drivers the stop_on_panic is not valid as the notifier callback happens in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info options to indicate whether the stop function would sleep. >>> >>> Did you consider having a reset_on_panic instead, which sets a >>> user-specified >>> timeout on panic? This would make the mechanism useful also for watchdogs >> >> /proc/sys/kernel/panic already provides that support. You may echo a >> non-zero value >> and the system tries for a soft reboot after those many seconds. But this >> doesn't happen >> in case of a kdump kernel load after panic. > >The timeout specified to the Watchdog reset_on_panic option would be >programmed into >the active watchdogs and not be used to trigger a software-induced reboot. Yes. > >>> that can't be disabled and would protect against system lock up: >>> Consider a memory-corruption bug (perhaps externally via DMA), which >>> partially >>> overwrites both main and kdump kernel. With a disabled watchdog, the system >>> may not be able to recover on its own. >> >> Yes, that is the reason why the kernel command-line is optional and by >> default it is set to zero. >> So that in cases if you have a corrupted kdump kernel then watchdog kicks in. > >The existing option isn't enough for the kdump kernel use case. >If we (i.e. you) are going to do something about it, wouldn't it be >better to have a solution that's applicable to a wider number of >watchdog devices? I need a slight clarification here. 1. reset_on_panic takes the number of seconds to be reloaded to watchdog HW, so that it initiates a watchdog reset after the specified timeout, if kdump kernel fails to boot or hung while booting. 2. in case reset_on_panic = 0 then it behaves like stop_on_panic=1. Is this what you meant? I would let Guenter comment on this approach. >>> If you did consider it, what made you decide against it? >> watchdog.stop_on_panic=1 is specifically for systems which can't boot a >> kdump kernel due to the fact >> that the kdump kernel gets a watchdog reset while booting, may be due to a >> shorter watchdog time. >> For eg: a 32-bit watchdog down counter running at 1GHz. >> reset_on_panic can guarantee only the largest watchdog timeout supported by >> HW, >> since there is no one to ping the watchdog. >If you are serious with the watchdog use, you'll want to use the watchdog to >monitor kernel startup as well. If the bootloader can set a watchdog timeout >just before starting the kernel and it doesn't expire before the kernel >watchdog >driver takes over, why can't we do the same just before starting the >dumpkernel? Yes, in an ideal world with ideal HW. But there are HW with issues which cannot have large enough Watchdog time. Such HW would boot from FW to kernel without watchdog enabled. And stop_on_panic does the similar for kdump kernel too. -George > >Thanks, >Ahmad > > >> >> Thanks, >> Ahmad >> >>> >>> >> Changelog: >> v1 -> v2 >> - Remove the per driver flag setting option >> - Take the parameter via kernel command-line parameter to watchdog_core. >> >> v2 -> v3 >> - Remove the helper function watchdog_stop_on_panic() from watchdog.h. >> - There are no users for this. >> >> v3 -> v4 >> - Since the panic notifier is in atomic context, watchdog functions >> which sleep can't be called. >> - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop >> function sleeps. >> - Simplify the stop_on_panic kernel command line parsing. >> - Enable the panic notiffier only if the watchdog stop function doesn't >> sleep >> >> George Cherian (2): >> watchdog: Add a new flag WDIOF_STOP_MAYSLEEP >> drivers: watchdog: Add support for panic notifier callback > > - George -- Pengutronix e.K. | | Steuerwalder Str. 21 | https://urldefense.proofpoint.com/v2/url?u=http-3A__www.pengutronix.de_&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=npgTSgHrUSLmXpBZJKVhk0lE_XNvtVDl8ZA2zBvBqPw&m=Df3J3ZRga7XxcgUdJOqYVMJ-ALX5jC3eiII4YhsAdC5pYhr1xwcqbzhIy6MCEqws&s=ybglw-WK4VGE8gHGNwMrC1_VliOv72pjDLEIm9FF_dE&e= | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v4 1/2] watchdog: Add a new flag WDIOF_STOP_MAYSLEEP
On 3/5/25 02:10, George Cherian wrote: A new option flag is added to watchdog_info. This helps the watchdog core to check whether stop functions would sleep or not. The option flags of individual drivers are also updated accordingly. Signed-off-by: George Cherian --- drivers/watchdog/advantech_ec_wdt.c | 3 ++- drivers/watchdog/arm_smc_wdt.c | 3 ++- drivers/watchdog/armada_37xx_wdt.c | 2 +- ... and many more. Sorry, I didn't expect that this would touch that many drivers. My bad. Let's do the opposite instead: Introduce WDIOF_STOP_NOSLEEP, and let drivers opt in instead of opting out. I still have to look into the other feedback. I think someone suggested to introduce a callback instead, which would stop the watchdog at runtime if needed (especially during kdump). That may be a better solution than having a module parameter. Either case, please separate driver patches from the patches introducing the new flag. Since the flag is opt-in, that should be ok - drivers supporting it can be modified over time. Thanks, Guenter
Re: [PATCH v4 1/2] watchdog: Add a new flag WDIOF_STOP_MAYSLEEP
> static const struct watchdog_ops adv_ec_wdt_ops = {
> diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c
> index 8f3d0c3a005f..794cf0086912 100644
> --- a/drivers/watchdog/arm_smc_wdt.c
> +++ b/drivers/watchdog/arm_smc_wdt.c
> @@ -90,7 +90,8 @@ static const struct watchdog_info smcwd_info = {
> .identity = DRV_NAME,
> .options= WDIOF_SETTIMEOUT |
> WDIOF_KEEPALIVEPING |
> - WDIOF_MAGICCLOSE,
> + WDIOF_MAGICCLOSE |
> + WDIOF_STOP_MAYSLEEP,
> };
I don't think this driver can sleep, unless I'm missing something?
`arm_smccc_smc()` does a synchronous call into firmware that always
returns back to the caller.
Re: [PATCH v4 1/2] watchdog: Add a new flag WDIOF_STOP_MAYSLEEP
On 3/5/25 03:01, Ahmad Fatoum wrote: Hi George, Hi Guenter, On 05.03.25 11:34, George Cherian wrote: why is armada_37xx_wdt also here? The stop function in that driver may not sleep. Marek, Thanks for reviewing. Since the stop function has a regmap_write(), I thought it might sleep. Now that you pointed it out, I assume that it is an MMIO based regmap being used for armada. I will update the same in the next version Failure to add WDIOF_STOP_MAYSLEEP when it's needed can lead to kernel hanging. Failure to add an alternative WDIOF_STOP_ATOMIC would lead to the kernel option being a no-op. I think a no-op stop_on_panic (or reset_on_panic) is a saner default. Agreed. Also, I like WDIOF_STOP_ATOMIC more than the WDIOF_STOP_NOSLEEP I had suggested in my other response. Thanks, Guenter
Re: [PATCH v4 2/2] drivers: watchdog: Add support for panic notifier callback
On 3/5/25 02:33, Andy Shevchenko wrote: On Wed, Mar 05, 2025 at 10:10:25AM +, George Cherian wrote: Watchdog is not turned off in kernel panic situation. In certain systems this might prevent the successful loading of kdump kernel. The kdump kernel might hit a watchdog reset while it is booting. To avoid such scenarios add a panic notifier call back function which can stop the watchdog. This provision can be enabled by passing watchdog.stop_on_panic=1 via kernel command-line parameter. ... First of all, do we really need a new module parameter for that? Why can't it be done automatically if kdump is expected? Sounds like a good idea to me. Guenter
