linux-kernel - Re: [PATCH v11 0/4] Add AP6275P wireless support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <67d67f15-4631-44ba-bc05-c8da6a1af1bf@broadcom.com>
Date: Mon, 19 Aug 2024 21:35:12 +0200
From: Arend van Spriel <arend.vanspriel@...adcom.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>,
 jacobe.zang@...ion.com
Cc: bhelgaas@...gle.com, brcm80211-dev-list.pdl@...adcom.com,
 brcm80211@...ts.linux.dev, christophe.jaillet@...adoo.fr,
 conor+dt@...nel.org, davem@...emloft.net, devicetree@...r.kernel.org,
 duoming@....edu.cn, edumazet@...gle.com, gregkh@...uxfoundation.org,
 krzk+dt@...nel.org, kuba@...nel.org, kvalo@...nel.org,
 linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org, megi@....cz,
 minipli@...ecurity.net, netdev@...r.kernel.org, pabeni@...hat.com,
 robh@...nel.org, saikrishnag@...vell.com, stern@...land.harvard.edu,
 yajun.deng@...ux.dev
Subject: Re: [PATCH v11 0/4] Add AP6275P wireless support



On 8/19/2024 6:42 PM, Sebastian Reichel wrote:
> Hi,
> 
> I tested this on RK3588 EVB1 and the driver is working fine. The DT
> bindings are not correct, though:
> 
> linux/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dtb: wifi@0,0:
> compatible: 'oneOf' conditional failed, one must be fixed:
> 
> ['pci14e4,449d', 'brcm,bcm4329-fmac'] is too long
> 'pci14e4,449d' is not one of ['brcm,bcm43143-fmac', 'brcm,bcm4341b0-fmac',
> 'brcm,bcm4341b4-fmac', 'brcm,bcm4341b5-fmac', 'brcm,bcm4329-fmac',
> 'brcm,bcm4330-fmac', 'brcm,bcm4334-fmac', 'brcm,bcm43340-fmac',
> 'brcm,bcm4335-fmac', 'brcm,bcm43362-fmac', 'brcm,bcm4339-fmac',
> 'brcm,bcm43430a0-fmac', 'brcm,bcm43430a1-fmac', 'brcm,bcm43455-fmac',
> 'brcm,bcm43456-fmac', 'brcm,bcm4354-fmac', 'brcm,bcm4356-fmac',
> 'brcm,bcm4359-fmac', 'brcm,bcm4366-fmac', 'cypress,cyw4373-fmac',
> 'cypress,cyw43012-fmac', 'infineon,cyw43439-fmac']
> from schema $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml#
> 
> It's easy to see the problem in the binding. It does not expect a
> fallback string after the PCI ID based compatible. Either the
> pci14e4,449d entry must be added to the first enum in the binding,
> which has the fallback compatible, or the fallback compatible
> should not be added to DTS.

Never understood why we ended up with such a large list. When the 
binding was introduced there was one compatible, ie. brcm,bcm4329-fmac. 
People wanted all the other flavors because it described a specific wifi 
chip and no other reason whatsoever. The PCI ID based compatible do 
obfuscate that info so those are even less useful in my opinion.

> If the fallback compatible is missing in DTS, the compatible check in
> brcmf_of_probe() fails and the lpo clock is not requested resulting
> in the firmware startup failing. So that would require further
> driver changes.

Right. The text based bindings file in 5.12 kernel clearly says:

Required properties:

  - compatible : Should be "brcm,bcm4329-fmac".

In 5.13 kernel this was replaced by the json-schema yaml file. The PCI 
ID based enum which was added later does also list brcm,bcm4329-fmac so 
why does that not work for the compatible list ['pci14e4,449d', 
'brcm,bcm4329-fmac']? Looking at the compatible property in yaml which I 
stripped a bit for brevity:

properties:
   compatible:
     oneOf:
       - items:
           - enum:
               - brcm,bcm43143-fmac
               - brcm,bcm4329-fmac
               - infineon,cyw43439-fmac
           - const: brcm,bcm4329-fmac
       - enum:
           - brcm,bcm4329-fmac
           - pci14e4,43dc  # BCM4355
           - pci14e4,4464  # BCM4364
           - pci14e4,4488  # BCM4377
           - pci14e4,4425  # BCM4378
           - pci14e4,4433  # BCM4387

So how should I read this. Searching for some sort of syntax description 
I found [1] which has an example schema with description that has a 
similarly complicated compatible property. From that I think the above 
should be changed to:

  properties:
    compatible:
      oneOf:
        - items:
            - enum:
                - brcm,bcm43143-fmac
-              - brcm,bcm4329-fmac
                - infineon,cyw43439-fmac
            - const: brcm,bcm4329-fmac
+      - items:
            - enum:
-              - brcm,bcm4329-fmac
                - pci14e4,43dc  # BCM4355
                - pci14e4,4464  # BCM4364
                - pci14e4,4488  # BCM4377
                - pci14e4,4425  # BCM4378
                - pci14e4,4433  # BCM4387
+          - const: brcm,bcm4329-fmac
+      - const: brcm,bcm4329-fmac

This poses a constraint in which the last string in the compatible list 
is always 'brcm,bcm4329-fmac' even if it is the only string. At least 
that is my understanding so if my understanding is wrong feel free to 
correct me on this.

[1] https://docs.kernel.org/devicetree/bindings/writing-schema.html
> Greetings,
> 
> -- Sebastian