[RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[Mateusz Nowicki]

Hi all,

this follows up on Marek's report about the stm32mp2 watchdog [1].

A quick recap: the arm,smc-wdt driver talks to the firmware through one
SMC function id that it reads from the device tree, and there is no way
to ask the firmware which id it uses. On the STM32MP2 SoCs that id
changed over time. ST's OP-TEE from ecosystem v5 expects 0xb200005a,
their 4.0.0 fork accepts both, and the 4.8.0 fork as well as upstream
OP-TEE only accept 0xbc000000.

My earlier patch, commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id
of the SMC watchdog on stm32mp2"), switched the dtsi to 0xbc000000. That
made the watchdog work with current OP-TEE, but it broke boards that run
the older firmware. So today 6.18.y uses one id and 7.4 the other. When
the kernel and the firmware disagree, nobody pets the watchdog that the
bootloader started, and the board resets after 32 seconds. Marek hit
this even with a recovery kernel, which is about the worst place for it.

The idea from that thread: let arm,smc-id be a list. The driver tries
the ids one by one and keeps the first one the firmware accepts. Older
kernels only read the first cell, so they behave exactly as they do
now. For stm32mp2 the list is <0xbc000000 0xb200005a>. Calling an id
that belongs to some other firmware service would be bad, so the
binding says the list may only hold ids known to be this watchdog.

Why RFC: arm,smc-id already exists in firmware/arm,scmi.yaml as a plain
uint32, and dt_binding_check complains when another binding turns it
into an array:

  arm-smc-wdt.yaml: arm,smc-id: multiple incompatible types: uint32-array
  arm,scmi.yaml: arm,smc-id: multiple incompatible types: uint32

DT folks, is there a way to keep the list in arm,smc-id that you would
be fine with? If not, I can move the extra ids into a new property,
something like:

  arm,smc-id = <0xbc000000>;
  arm,smc-alt-ids = <0xb200005a>;

The driver side would stay the same, it would just read the fallback
ids from the other property.

One more question: should patches 2 and 3 go to stable as well? Then
6.18.y would also work with both firmware generations. The 6.18.y
version of patch 3 would turn its 0xb200005a into the same list.

[1] https://lore.kernel.org/all/1b14e1e4-962f-4391-91fd-b09143641643@nabladev.com/

Mateusz Nowicki (3):
  dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids
  watchdog: arm_smc_wdt: try every id of arm,smc-id
  arm64: dts: st: list both watchdog SMC ids on stm32mp23 and stm32mp25

 .../bindings/watchdog/arm-smc-wdt.yaml         |  9 +++++++--
 arch/arm64/boot/dts/st/stm32mp231.dtsi         |  2 +-
 arch/arm64/boot/dts/st/stm32mp251.dtsi         |  2 +-
 drivers/watchdog/arm_smc_wdt.c                 | 18 ++++++++++++------
 4 files changed, 21 insertions(+), 10 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.56.0

[RFC PATCH 1/3] dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids

[Mateusz Nowicki]

The SMC function id of the watchdog service is not discoverable and
differs between firmware versions for the same SoC. Let arm,smc-id hold
a list of ids that the driver tries in order, so a SoC dtsi can name
every id used by firmware for that SoC. A single id stays valid.

Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---
 .../devicetree/bindings/watchdog/arm-smc-wdt.yaml        | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
index 8e9d0b7e8244..9844b23ef51c 100644
--- a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
@@ -18,9 +18,14 @@ properties:
       - arm,smc-wdt
 
   arm,smc-id:
-    $ref: /schemas/types.yaml#/definitions/uint32
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 4
     description: |
-      The ATF smc function id used by the firmware.
+      The ATF smc function id used by the firmware, or a list of ids for
+      firmware versions that use different ones. The ids are tried in order
+      until the firmware accepts the init call, so only ids that are known
+      to be this watchdog service on the platform belong in the list.
       Defaults to 0x82003D06 if unset.
 
 required:
-- 
2.56.0

Re: [RFC PATCH 1/3] dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: watchdog: arm-smc-wdt: unsafe SMC ID probing list in arm,smc-id
--

commit dea383b087f6f2623ae4ede98f366e18fd9b9ed8
Author: Mateusz Nowicki <mateusz.nowicki@posteo.net>

dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids

This commit changes the arm,smc-id property in the arm-smc-wdt binding from a
single uint32 to a uint32-array of up to 4 items so the driver can probe a
list of candidate SMC function IDs in order across different firmware
versions.

> diff --git a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
> index 8e9d0b7e82444..9844b23ef51ce 100644
> --- a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
> @@ -18,9 +18,14 @@ properties:
>        - arm,smc-wdt
>  
>    arm,smc-id:
> -    $ref: /schemas/types.yaml#/definitions/uint32
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    maxItems: 4
>      description: |
> -      The ATF smc function id used by the firmware.
> +      The ATF smc function id used by the firmware, or a list of ids for
> +      firmware versions that use different ones. The ids are tried in order
> +      until the firmware accepts the init call, so only ids that are known
> +      to be this watchdog service on the platform belong in the list.
>        Defaults to 0x82003D06 if unset.

[Severity: Medium]
Is it safe to define arm,smc-id as a list of candidate SMC function IDs to be
sequentially probed in EL3/S-EL1 secure firmware?

In drivers/watchdog/arm_smc_wdt.c, smcwd_probe() invokes smcwd_call(wdd,
SMCWD_INIT, 0, &res), which issues arm_smccc_smc() with a raw function ID and
arguments a1=0, a2=0 without any UUID or version handshake:

drivers/watchdog/arm_smc_wdt.c:smcwd_probe() {
	if (of_property_read_u32(pdev->dev.of_node, "arm,smc-id",
				 &smc_func_id))
		smc_func_id = 0x82003D06;
	watchdog_set_drvdata(wdd, (void *)(uintptr_t)smc_func_id);

	err = smcwd_call(wdd, SMCWD_INIT, 0, &res);
}

Because SiP and Trusted OS SMCCC function IDs are vendor- and
firmware-specific, an ID used for the watchdog in one firmware release is not
guaranteed to be unimplemented across all other firmware implementations for
that SoC. Could probing a list of candidate IDs invoke an unrelated
privileged SMC handler on firmware versions that assign one of the earlier
IDs to another service, causing unintended secure-world side effects or
binding the watchdog driver to the wrong service if it returns 0?

Also, existing Devicetree consumers (such as older kernels calling
of_property_read_u32() on arm,smc-id) only read the first cell and will fail
if the active firmware uses a subsequent ID in the array. Since a running
firmware image implements a single watchdog SMC ID, should differences
between mutually exclusive firmware builds be handled by the bootloader
updating the Devicetree, a DT overlay, or board-specific DTS files rather
than encoding a trial-and-error probing list in the SoC dtsi?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104161553.30115-1-mateusz.nowicki@posteo.net?part=1

[RFC PATCH 2/3] watchdog: arm_smc_wdt: try every id of arm,smc-id

[Mateusz Nowicki]

The SMC function id of the watchdog service is not discoverable, and
firmware for the same SoC does not always use the same one: on the
STM32MP2 SoCs it is 0xbc000000 (the OP-TEE default for the platform) or
0xb200005a. With a single id in the SoC dtsi, every board whose firmware
uses the other id loses its watchdog.

Read arm,smc-id as a list and use the first id for which SMCWD_INIT
succeeds. Go to the next id on any error: OP-TEE answers an unknown
function id with 0xffffffff in a0, which smcwd_call() reports as -EIO,
not -ENODEV. A missing property still means 0x82003D06.

Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---
 drivers/watchdog/arm_smc_wdt.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c
index c73ed1dcdfcb..993c8f92c871 100644
--- a/drivers/watchdog/arm_smc_wdt.c
+++ b/drivers/watchdog/arm_smc_wdt.c
@@ -113,8 +113,9 @@ static const struct watchdog_ops smcwd_timeleft_ops = {
 
 static int smcwd_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	struct watchdog_device *wdd;
-	int err;
+	int err, i, n;
 	struct arm_smccc_res res;
 	u32 smc_func_id;
 
@@ -123,12 +124,17 @@ static int smcwd_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, wdd);
 
-	if (of_property_read_u32(pdev->dev.of_node, "arm,smc-id",
-				 &smc_func_id))
-		smc_func_id = 0x82003D06;
-	watchdog_set_drvdata(wdd, (void *)(uintptr_t)smc_func_id);
+	/* The id is not discoverable: take the first one of the list the firmware accepts */
+	n = of_property_count_u32_elems(np, "arm,smc-id");
+	for (i = 0; i < max(n, 1); i++) {
+		if (of_property_read_u32_index(np, "arm,smc-id", i, &smc_func_id))
+			smc_func_id = 0x82003D06;
+		watchdog_set_drvdata(wdd, (void *)(uintptr_t)smc_func_id);
 
-	err = smcwd_call(wdd, SMCWD_INIT, 0, &res);
+		err = smcwd_call(wdd, SMCWD_INIT, 0, &res);
+		if (!err)
+			break;
+	}
 	if (err < 0)
 		return err;
 
-- 
2.56.0

[RFC PATCH 3/3] arm64: dts: st: list both watchdog SMC ids on stm32mp23 and stm32mp25

[Mateusz Nowicki]

Commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id of the SMC watchdog
on stm32mp2") changed the watchdog SMC id from 0xb200005a to
0xbc000000, the OP-TEE default for the platform. That fixed the watchdog
with such firmware but broke it on boards whose firmware still uses
0xb200005a. List both ids, the driver takes the first one the firmware
accepts. Kernels that read a single id keep using 0xbc000000.

Fixes: ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id of the SMC watchdog on stm32mp2")
Reported-by: Marek Vasut <marex@nabladev.com>
Closes: https://lore.kernel.org/all/1b14e1e4-962f-4391-91fd-b09143641643@nabladev.com/
Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---
 arch/arm64/boot/dts/st/stm32mp231.dtsi | 2 +-
 arch/arm64/boot/dts/st/stm32mp251.dtsi | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/st/stm32mp231.dtsi b/arch/arm64/boot/dts/st/stm32mp231.dtsi
index fefbf6f4f153..7526e86f5a1d 100644
--- a/arch/arm64/boot/dts/st/stm32mp231.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp231.dtsi
@@ -35,7 +35,7 @@ arm-pmu {
 
 	arm_wdt: watchdog {
 		compatible = "arm,smc-wdt";
-		arm,smc-id = <0xbc000000>;
+		arm,smc-id = <0xbc000000 0xb200005a>;
 		status = "disabled";
 	};
 
diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
index dac3d2d23ff0..143d83216530 100644
--- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
@@ -36,7 +36,7 @@ arm-pmu {
 
 	arm_wdt: watchdog {
 		compatible = "arm,smc-wdt";
-		arm,smc-id = <0xbc000000>;
+		arm,smc-id = <0xbc000000 0xb200005a>;
 		status = "disabled";
 	};
 
-- 
2.56.0

Re: [RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[Guenter Roeck]

On 11/4/26 08:16, Mateusz Nowicki wrote:
> Hi all,
> 
> this follows up on Marek's report about the stm32mp2 watchdog [1].
> 
> A quick recap: the arm,smc-wdt driver talks to the firmware through one
> SMC function id that it reads from the device tree, and there is no way
> to ask the firmware which id it uses. On the STM32MP2 SoCs that id
> changed over time. ST's OP-TEE from ecosystem v5 expects 0xb200005a,
> their 4.0.0 fork accepts both, and the 4.8.0 fork as well as upstream
> OP-TEE only accept 0xbc000000.
> 
> My earlier patch, commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id
> of the SMC watchdog on stm32mp2"), switched the dtsi to 0xbc000000. That
> made the watchdog work with current OP-TEE, but it broke boards that run
> the older firmware. So today 6.18.y uses one id and 7.4 the other. When
> the kernel and the firmware disagree, nobody pets the watchdog that the
> bootloader started, and the board resets after 32 seconds. Marek hit
> this even with a recovery kernel, which is about the worst place for it.
> 

So the DT on the board is updated to use the new ID but the firmware itself
isn't ? That seems a bit odd; I would have assumed that the DT on a board
is kept in sync with its firmware. Is that not the case here ?

> The idea from that thread: let arm,smc-id be a list. The driver tries
> the ids one by one and keeps the first one the firmware accepts. Older
> kernels only read the first cell, so they behave exactly as they do
> now. For stm32mp2 the list is <0xbc000000 0xb200005a>. Calling an id
> that belongs to some other firmware service would be bad, so the
> binding says the list may only hold ids known to be this watchdog.
> 

Would it possibly make sense to check keep a list of known IDs in the
driver ? It already checks 0x82003D06, so it might as well check a sequence.

Thanks,
Guenter

> Why RFC: arm,smc-id already exists in firmware/arm,scmi.yaml as a plain
> uint32, and dt_binding_check complains when another binding turns it
> into an array:
> 
>    arm-smc-wdt.yaml: arm,smc-id: multiple incompatible types: uint32-array
>    arm,scmi.yaml: arm,smc-id: multiple incompatible types: uint32
> 
> DT folks, is there a way to keep the list in arm,smc-id that you would
> be fine with? If not, I can move the extra ids into a new property,
> something like:
> 
>    arm,smc-id = <0xbc000000>;
>    arm,smc-alt-ids = <0xb200005a>;
> 
> The driver side would stay the same, it would just read the fallback
> ids from the other property.
> 
> One more question: should patches 2 and 3 go to stable as well? Then
> 6.18.y would also work with both firmware generations. The 6.18.y
> version of patch 3 would turn its 0xb200005a into the same list.
> 
> [1] https://lore.kernel.org/all/1b14e1e4-962f-4391-91fd-b09143641643@nabladev.com/
> 
> Mateusz Nowicki (3):
>    dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids
>    watchdog: arm_smc_wdt: try every id of arm,smc-id
>    arm64: dts: st: list both watchdog SMC ids on stm32mp23 and stm32mp25
> 
>   .../bindings/watchdog/arm-smc-wdt.yaml         |  9 +++++++--
>   arch/arm64/boot/dts/st/stm32mp231.dtsi         |  2 +-
>   arch/arm64/boot/dts/st/stm32mp251.dtsi         |  2 +-
>   drivers/watchdog/arm_smc_wdt.c                 | 18 ++++++++++++------
>   4 files changed, 21 insertions(+), 10 deletions(-)
> 
> 
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4

Re: [RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[mateusz.nowicki]

On 11/4/26 Guenter Roeck wrote:
> On 11/4/26 08:16, Mateusz Nowicki wrote:
>> My earlier patch, commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id
>> of the SMC watchdog on stm32mp2"), switched the dtsi to 0xbc000000. 
>> That
>> made the watchdog work with current OP-TEE, but it broke boards that 
>> run
>> the older firmware. So today 6.18.y uses one id and 7.4 the other. 
>> When
>> the kernel and the firmware disagree, nobody pets the watchdog that 
>> the
>> bootloader started, and the board resets after 32 seconds. Marek hit
>> this even with a recovery kernel, which is about the worst place for 
>> it.
>> 
> 
> So the DT on the board is updated to use the new ID but the firmware 
> itself
> isn't ? That seems a bit odd; I would have assumed that the DT on a 
> board
> is kept in sync with its firmware. Is that not the case here ?

Not necessarily. On these boards the DTB is built from the kernel tree
and installed with the kernel, U-Boot just loads it from the boot
partition. TF-A and OP-TEE sit in the FIP and are updated separately, so
updating the kernel alone changes the ID. That's how Marek got hit: he
updated OP-TEE, but his recovery kernel still had the old DTB.

>> The idea from that thread: let arm,smc-id be a list. The driver tries
>> the ids one by one and keeps the first one the firmware accepts. Older
>> kernels only read the first cell, so they behave exactly as they do
>> now. For stm32mp2 the list is <0xbc000000 0xb200005a>. Calling an id
>> that belongs to some other firmware service would be bad, so the
>> binding says the list may only hold ids known to be this watchdog.
>> 
> 
> Would it possibly make sense to check keep a list of known IDs in the
> driver ? It already checks 0x82003D06, so it might as well check a 
> sequence.

Fine with me, and it avoids the binding issue. I'd just rather not try
the ST IDs on every platform, so how about making the list per
compatible:

   compatible = "st,stm32mp25-smc-wdt", "arm,smc-wdt";

with { 0xbc000000, 0xb200005a } as match data for it. arm,smc-id stays a
single u32 and, if present, is tried first. Older kernels just match
"arm,smc-wdt" and behave as today.

If that works for you and the DT folks, I'll send v2.

Thanks,
Mateusz

Re: [RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[Guenter Roeck]

On 11/4/26 10:10, mateusz.nowicki@posteo.net wrote:
> On 11/4/26 Guenter Roeck wrote:
>> On 11/4/26 08:16, Mateusz Nowicki wrote:
>>> My earlier patch, commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id
>>> of the SMC watchdog on stm32mp2"), switched the dtsi to 0xbc000000. That
>>> made the watchdog work with current OP-TEE, but it broke boards that run
>>> the older firmware. So today 6.18.y uses one id and 7.4 the other. When
>>> the kernel and the firmware disagree, nobody pets the watchdog that the
>>> bootloader started, and the board resets after 32 seconds. Marek hit
>>> this even with a recovery kernel, which is about the worst place for it.
>>>
>>
>> So the DT on the board is updated to use the new ID but the firmware itself
>> isn't ? That seems a bit odd; I would have assumed that the DT on a board
>> is kept in sync with its firmware. Is that not the case here ?
> 
> Not necessarily. On these boards the DTB is built from the kernel tree
> and installed with the kernel, U-Boot just loads it from the boot
> partition. TF-A and OP-TEE sit in the FIP and are updated separately, so
> updating the kernel alone changes the ID. That's how Marek got hit: he
> updated OP-TEE, but his recovery kernel still had the old DTB.
> 

If the DT does not really reflect the HW, is installed as part of the kernel
image, and the ID can change across FW versions, it seems to me that it does
not even make sense to store the ID in devicetree.

>>> The idea from that thread: let arm,smc-id be a list. The driver tries
>>> the ids one by one and keeps the first one the firmware accepts. Older
>>> kernels only read the first cell, so they behave exactly as they do
>>> now. For stm32mp2 the list is <0xbc000000 0xb200005a>. Calling an id
>>> that belongs to some other firmware service would be bad, so the
>>> binding says the list may only hold ids known to be this watchdog.
>>>
>>
>> Would it possibly make sense to check keep a list of known IDs in the
>> driver ? It already checks 0x82003D06, so it might as well check a sequence.
> 
> Fine with me, and it avoids the binding issue. I'd just rather not try
> the ST IDs on every platform, so how about making the list per
> compatible:
> 
>    compatible = "st,stm32mp25-smc-wdt", "arm,smc-wdt";
> 
> with { 0xbc000000, 0xb200005a } as match data for it. arm,smc-id stays a
> single u32 and, if present, is tried first. Older kernels just match
> "arm,smc-wdt" and behave as today.
> 
> If that works for you and the DT folks, I'll send v2.
> 
Works for me.

Thanks,
Guenter