Re: [PATCH v3] usb: core: verify devicetree nodes for USB devices

[Marek Szyprowski]

Hi MÃns

On 2019-05-13 12:06, MÃns RullgÃrd wrote:
> Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> writes:
>
>> Hi Peter,
>>
>> On 2019-05-13 11:23, Peter Chen wrote:
>>>> On 2019-05-13 11:00, Peter Chen wrote:
>>>>>> On 2019-05-10 05:10, Peter Chen wrote:
>>>>>>>> Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> writes:
>>>>>>>>> Commit 69bec7259853 ("USB: core: let USB device know device node")
>>>>>>>>> added support for attaching devicetree node for USB devices. The
>>>>>>>>> mentioned commit however identifies the given USB device node only
>>>>>>>>> by the
>>>>>> 'reg'
>>>>>>>>> property in the host controller children nodes. The USB device
>>>>>>>>> node however also has to have a 'compatible' property as described
>>>>>>>>> in Documentation/devicetree/bindings/usb/usb-device.txt. Lack for
>>>>>>>>> the 'compatible' property check might result in assigning a
>>>>>>>>> devicetree node, which is not intended to be the proper node for the given
>>>> USB device.
>>>>>>>>> This is important especially when USB host controller has
>>>>>>>>> child-nodes for other purposes. For example, Exynos EHCI and OHCI
>>>>>>>>> drivers already define child-nodes for each physical root hub port
>>>>>>>>> and assigns respective PHY controller and parameters for them.
>>>>>>>>> Those binding predates support for USB devicetree nodes.
>>>>>>>>>
>>>>>>>>> Checking for the proper compatibility string allows to mitigate
>>>>>>>>> the conflict between USB device devicetree nodes and the bindings
>>>>>>>>> for USB controllers with child nodes. It also fixes the
>>>>>>>>> side-effect of the other commits, like 01fdf179f4b0 ("usb: core:
>>>>>>>>> skip interfaces disabled in devicetree"), which incorrectly
>>>>>>>>> disables some devices on Exynos based boards.
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> The purpose of your patch is do not set of_node for device under USB
>>>>>>> controller,
>>>>>> right?
>>>>>>
>>>>>> Right.
>>>>>>
>>>>> Do you mind doing it at function exynos_ehci_get_phy of ehci-exynos.c?
>>>> I don't mind fixing it in ehci-exynos, but frankly so far I have no
>>>> idea how to do it.  The problem is that newly created USB devices
>>>> get of-node pointer pointing to a node which if not intended for
>>>> them. How this can be fixed in ehci-exynos?
>>>>
>>>    
>>> Can't be workaround by setting of_node as NULL for EHCI controller or
>>> for PHY node at exynos_ehci_get_phy?
>> Ah, such workaround? I will check, but this will need to be done with
>> care, because have a side effect for other subsystems like regulators or
>> clocks.
>>
>> BTW, What's wrong with proper, full verification of USB device nodes?
> Your approach so far doesn't address the actual problem of a conflict
> between the generic USB DT bindings and those for the Exynos host
> controller.  If you fix that, the validation issue goes away.

Well, the issue caused by the Exynos binding conflict will go away, but 
there still will be a chance that wrong node might be assigned to the 
USB device, especially if partially incorrect data will be stored in the 
device tree. For example, it may happen that on some boards the 
different USB chip is mounted, which require different parameters. The 
code only relies on the reg property and device number, while the 
bindings define compatible string with proper exact vendor/product ids.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland