Re: [PATCH v2 04/17] dt-bindings: timer: arm,arch_timer: Fix requirements for interrupt description

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The schema permits omitting the EL3 secure timer but fails to make `interrupt-names` a required property, which allows configurations that fatally misroute interrupts and hang the system.
- [Medium] The static `items` array with `minItems: 2` structurally fails to enforce the stated architectural rules and causes positional misalignment for valid configurations.
- [Medium] The commit incorrectly asserts that the EL1 virtual timer IRQ is 'always required', which misrepresents ARMv7 hardware and contradicts existing device trees.
--

commit 01733bbba71a7bef01e5c0820740d8ab21f684f8
Author: Marc Zyngier <maz@kernel.org>

dt-bindings: timer: arm,arch_timer: Fix requirements for interrupt description

This commit updates the arm,arch_timer binding to clarify the requirements
for timer interrupts based on the implemented architecture. It raises the
minimum number of interrupts to two and uses architectural terminology.

> Follow the architecture by making it explicit that:
> - the EL1 secure timer irq is required if EL3 is implemented
> - the EL1 physical timer irq is always required
> - the EL1 virtual timer irq is always required

Is this description architecturally accurate for ARMv7?

In ARMv7, Virtualization Extensions are optional, so hardware lacking these
extensions does not implement a virtual timer. Existing device trees, such
as arch/arm/boot/dts/rockchip/rv1108.dtsi, accurately reflect this by
providing exactly two physical interrupts (secure and non-secure) and
completely omitting the virtual timer.

Establishing the EL1 virtual timer as always required seems to contradict
these valid hardware configurations.

> diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml

[ ... ]

> @@ -33,13 +28,13 @@ properties:
>            - const: arm,armv7-timer
>  
>    interrupts:
> -    minItems: 1
> +    minItems: 2
>      items:
> -      - description: secure timer irq
> -      - description: non-secure timer irq
> -      - description: virtual timer irq
> -      - description: hypervisor timer irq
> -      - description: hypervisor virtual timer irq
> +      - description: EL1 secure physical timer irq, if EL3 is implemented
> +      - description: EL1 non-secure physical timer irq
> +      - description: EL1 virtual timer irq

Will this static array structure cause positional misalignment and fail to
enforce the stated requirements?

JSON Schema array validation evaluates strictly by index. If a platform
lacks EL3 and correctly omits the secure timer IRQ, its first provided
interrupt (the non-secure physical timer) shifts to index 0. The schema
would then incorrectly validate it against the secure physical timer
description.

Additionally, because the EL1 virtual timer is listed as the 3rd item,
minItems: 2 only enforces the presence of the first two items. A device
with EL3 that erroneously omits the virtual timer would silently pass
validation.

Should this be restructured using a oneOf block to define the specific
valid sequences?

Since this patch explicitly documents that the secure timer can be omitted,
does this create an issue where missing interrupt-names can lead to a
system hang?

If a device tree provides the two required interrupts (physical and virtual)
and omits interrupt-names, it will pass schema validation with minItems: 2.
However, drivers/clocksource/arm_arch_timer.c maps interrupts by strict
index if interrupt-names is absent (0 to secure, 1 to non-secure).

This causes the driver to incorrectly map the virtual IRQ as the non-secure
physical interrupt. The system will then request the virtual IRQ from the
GIC but program the physical timer registers, causing unhandled physical
interrupts and a system hang on boot due to missing timer ticks.

Should interrupt-names be made a required property to prevent this valid
schema from causing a runtime misconfiguration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514150945.3917510-1-maz@kernel.org?part=4