Re: [PATCH] of: property: Fix fw_devlink handling of interrupt-map

[Anup Patel]

On Wed, May 29, 2024 at 8:47 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Wed, May 29, 2024 at 01:00:07PM +0100, Marc Zyngier wrote:
> > > > > In the RISC-V world, there have been quite a few QEMU releases
> > > > > where the generated DT node of the interrupt controller does not
> > > > > have the "#address-cells" property. This patch breaks the kernel
> > > > > for all such QEMU releases.
> > > >
> > > > Congratulations, you've forked DT. News at 11.
> > >
> > > Can you elaborate how ?
> >
> > You've stated it yourself. You are relying on a behaviour that
> > deviates from the standard by having DTs with missing properties
> >
> > And since we can't travel back it time to fix this, the only solution
> > I can see is to support both behaviours by quirking it.
>
> I'm not convinced that there is any actual production hardware that
> would get broken by your patch, just QEMU, so I think it should get
> fixed to output devicetrees that are spec compliant rather than add some
> riscv-specific hacks that we can't even gate on the "qemu,aplic"
> compatible because QEMU doesn't use the compatible created for it...

I also did further digging and it turns out the "#address-cells"
is missing only for APLIC DT nodes and this issue only impacts
APLIC DT node creation in QEMU RISC-V virt machine. We
should just go ahead and fix QEMU.

>
> Spec violations aside, the QEMU aplic nodes in the DT contain a bunch
> of other issues, including using properties that changed in the
> upstreaming process. Here's the issues with Alistair's current riscv
> tree for QEMU w/ -smp 4 -M virt,aia=aplic,dumpdtb=$(qemu_dtb) -cpu max -m 1G -nographic
>
> qemu.dtb: aplic@d000000: $nodename:0: 'aplic@d000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@d000000: compatible:0: 'riscv,aplic' is not one of ['qemu,aplic']
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@d000000: compatible: ['riscv,aplic'] is too short
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@d000000: Unevaluated properties are not allowed ('compatible' was unexpected)
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@c000000: $nodename:0: 'aplic@c000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@c000000: compatible:0: 'riscv,aplic' is not one of ['qemu,aplic']
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@c000000: compatible: ['riscv,aplic'] is too short
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> qemu.dtb: aplic@c000000: Unevaluated properties are not allowed ('compatible', 'riscv,delegate' were unexpected)
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
>
> I guess noone updated QEMU to comply with the bindings that actually got
> upstreamed for the aplic?

Yes, we never bothered to update the QEMU DT generation after
AIA DT bindings were accepted. Thanks for catching.

Regards,
Anup