linux-kernel - RE: [PATCH v1 2/7] dt-bindings: usb: hpe,gxp-udc: Add binding for gxp gadget

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: <SJ0PR84MB20853F3B0FCCF2A9583524B48D3E9@SJ0PR84MB2085.NAMPRD84.PROD.OUTLOOK.COM>
Date:   Wed, 9 Nov 2022 03:37:01 +0000
From:   "Yu, Richard" <richard.yu@....com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "Verdun, Jean-Marie" <verdun@....com>,
        "Hawkins, Nick" <nick.hawkins@....com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "Chang, Clay" <clayc@....com>
Subject: RE: [PATCH v1 2/7] dt-bindings: usb: hpe,gxp-udc: Add binding for gxp
 gadget

Hi Mr. Kozlowski,

Thank you very much for inputs.

>>>> +
>>>> +  vdevnum:
>>>> +    description:
>>>> +      virtual device number.
> 
>>> That's unusual property... Why numbering devices is part of DT (hardware description)?
> 
>>  In HPE GXP virtual EHCI controller chipset, it can support up to 8  
>> virtual devices(gadgets). Each device/gadget will be represented  by 
>> a bit in 8 bits register. For example, the interrupt register bit 0  
>> indicates the interrupt from device 0, bit 1 for device 1 ... so on.
>> When a user defines a device/gadget, he/she can define the device 
>> number as between 0 and 7. Thus, the driver can look up to the bit 
>> position. That is why we have numbering devices as part of DT.

> Wrap your lines properly, it's impossible to reply in-line to such messages.

Sorry for the improper wrapping. Hope the above fixed the problem.

> Then how do you specify two devices? You allow here only one, right?

In our current design, to specify two devices, we added the gadget 
structure into the device tree, such as  gadget0:udc@...01000{}; gadget1:udc@...02000{};....

No, we can allow up to 8 devices by adding the gadget structure,
such as gadget0:udc@...01000{}; gadget1:udc@...02000{};....gadget8:udc@...08000{};

> Which bit in which register? Your devices have separate address space, so why they cannot poke the same register, right? Then just always set it to 0...

In HPE GXP vEHCI controller, there are three register groups: standard USB EHCI registers, 
virtual device global registers, and virtual device registers.

Standard USB EHCI registers ---- We defined as "hpe,gxp-vudc" in the device tree (vuhc0) 
Virtual device global registers --- We defined as "hpe,gxp-udcg" 
Virtual device registers -- We defined as "hpe,gxp-udc"

Each virtual device will have its own separate address space. 
There is only single address space for the virtual device global registers. 

The virtual device global registers are including vDevice Global Interrupt Status register(EVGISTAT), 
vDevice Global Interrupt Enable register(EVGIEN), vEHCI FlexEndpoint Mapping register (EVFEMAP) ....
We need the vdevnum for the bit position in EVGISTAT and EVGIEN for each device.  
We write vdevnum into the EVFEMAP register to assign an EP to a specific device. 

> I might miss here something but so far it looks to me like some hacky description matching the driver, not hardware, not existing bindings.

We create "vdevnum" as device configuration parameter due to our hardware need.

>>>> +
>>>> +  fepnum:
>>>> +    description:
>>> >+      number of the flexible end-points this device is needed.
> >
>> >Similar question.
> >
> >In HPE GXP virtual EHCI Controller chipset, there is a flexible End-Point(EP) pool. 
>>Each flexible EP has its own mapping register. The mapping register 
>>bit 0 to 3 is for device number (vdevnum) and bit 4 to 7 is for EP number inside the device.
>>The device driver configures the mapping register to assign a flexible 
>>EP to a specific device.  Here, "fepnum" is the input letting the 
>>driver know how many EPs are needed for this device/gadget.

>Nope. So you create here some weird IDs to poke into syscon register.
>First, syscon has offset if you need. You could treat it maybe as bits?
>I don't know... but even then your design is poor - two devices 
>changing the same register. Even though it is sunchronized by regmap, it is conflicting, obfuscated access.

The "fepnum" is the input parameter to define how many end-points (EPs) is needed
for the device.

You are correct that all devices need to access the virtual 
device global registers during the runtime. 
Thus, we create " hpe,syscon-phandle = <&udc_system_controller>;'
for the driver getting the vDevice Global registers address.

In our current chip registers layout with the vDevice Global registers, I don’t see
a way to avoid "two devices changing the same register".

Thank you very much for your time.

Richard.