[PATCH] usb: core: verify devicetree nodes for disabled interfaces

[PATCH] usb: core: verify devicetree nodes for disabled interfaces

[Marek Szyprowski]

Commit 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
add support for disabling given USB device interface by adding nodes to
the USB host controller device. The mentioned commit however identifies
the given USB interface node only by the 'reg' property in the host
controller children nodes and then checks for their the 'status'. The USB
device interface nodes however also has to have a 'compatible' property as
described in Documentation/devicetree/bindings/usb/usb-device.txt. This is
important, because USB host controller might have 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. This conflicts with the proposed
approach and verifying for the presence of the compatible property fixes
this issue without changing the devicetree bindings and the way the PHY
controllers are handled by Exynos EHCI/OHCI drivers.

Reported-by: Markus Reichl <m.reichl@fivetechno.de>
Fixes: 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/core/message.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 82239f27c4cc..cd455c4add25 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -2007,6 +2007,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		struct usb_interface *intf = cp->interface[i];
 
 		if (intf->dev.of_node &&
+		    of_device_is_compatible(intf->dev.of_node, NULL) &&
 		    !of_device_is_available(intf->dev.of_node)) {
 			dev_info(&dev->dev, "skipping disabled interface %d\n",
 				 intf->cur_altsetting->desc.bInterfaceNumber);
-- 
2.17.1

Re: [PATCH] usb: core: verify devicetree nodes for disabled interfaces

[Måns Rullgård]

Marek Szyprowski <m.szyprowski@samsung.com> writes:

> Commit 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
> add support for disabling given USB device interface by adding nodes to
> the USB host controller device. The mentioned commit however identifies
> the given USB interface node only by the 'reg' property in the host
> controller children nodes and then checks for their the 'status'. The USB
> device interface nodes however also has to have a 'compatible' property as
> described in Documentation/devicetree/bindings/usb/usb-device.txt. This is
> important, because USB host controller might have 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. This conflicts with the proposed
> approach and verifying for the presence of the compatible property fixes
> this issue without changing the devicetree bindings and the way the PHY
> controllers are handled by Exynos EHCI/OHCI drivers.
>
> Reported-by: Markus Reichl <m.reichl@fivetechno.de>
> Fixes: 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/core/message.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 82239f27c4cc..cd455c4add25 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -2007,6 +2007,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>  		struct usb_interface *intf = cp->interface[i];
>
>  		if (intf->dev.of_node &&
> +		    of_device_is_compatible(intf->dev.of_node, NULL) &&
>  		    !of_device_is_available(intf->dev.of_node)) {
>  			dev_info(&dev->dev, "skipping disabled interface %d\n",
>  				 intf->cur_altsetting->desc.bInterfaceNumber);
> -- 

This doesn't look right.  of_device_is_compatible() with a NULL compat
argument always returns zero.

There also seems to be a broader incompatibility between the generic USB
bindings and the use of child nodes in the Exynos bindings.

-- 
Måns Rullgård

Re: [PATCH] usb: core: verify devicetree nodes for disabled interfaces

[Marek Szyprowski]

Hi

On 2019-05-07 15:24, Måns Rullgård wrote:
> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>> Commit 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
>> add support for disabling given USB device interface by adding nodes to
>> the USB host controller device. The mentioned commit however identifies
>> the given USB interface node only by the 'reg' property in the host
>> controller children nodes and then checks for their the 'status'. The USB
>> device interface nodes however also has to have a 'compatible' property as
>> described in Documentation/devicetree/bindings/usb/usb-device.txt. This is
>> important, because USB host controller might have 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. This conflicts with the proposed
>> approach and verifying for the presence of the compatible property fixes
>> this issue without changing the devicetree bindings and the way the PHY
>> controllers are handled by Exynos EHCI/OHCI drivers.
>>
>> Reported-by: Markus Reichl <m.reichl@fivetechno.de>
>> Fixes: 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/usb/core/message.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>> index 82239f27c4cc..cd455c4add25 100644
>> --- a/drivers/usb/core/message.c
>> +++ b/drivers/usb/core/message.c
>> @@ -2007,6 +2007,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>>   		struct usb_interface *intf = cp->interface[i];
>>
>>   		if (intf->dev.of_node &&
>> +		    of_device_is_compatible(intf->dev.of_node, NULL) &&
>>   		    !of_device_is_available(intf->dev.of_node)) {
>>   			dev_info(&dev->dev, "skipping disabled interface %d\n",
>>   				 intf->cur_altsetting->desc.bInterfaceNumber);
>> -- 
> This doesn't look right.  of_device_is_compatible() with a NULL compat
> argument always returns zero.

Right, my fault. I missed that. of_find_property(intf->dev.of_node, 
"compatible", NULL) is the correct check.


> There also seems to be a broader incompatibility between the generic USB
> bindings and the use of child nodes in the Exynos bindings.

True, but frankly I have no idea how to fix this in a better way.

Exynos EHCI/OHCI bindings predates the generic USB device bindings and 
now we can only mitigate the damage caused by this conflict.

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

[PATCH v2] usb: core: verify devicetree nodes for disabled interfaces

[Marek Szyprowski]

Commit 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
add support for disabling given USB device interface by adding nodes to
the USB host controller device. The mentioned commit however identifies
the given USB interface node only by the 'reg' property in the host
controller children nodes and then checks for their the 'status'. The USB
device interface nodes however also has to have a 'compatible' property as
described in Documentation/devicetree/bindings/usb/usb-device.txt. This is
important, because USB host controller might have 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. This conflicts with the proposed
approach and verifying for the presence of the compatible property fixes
this issue without changing the bindings and the way the PHY controllers
are handled by Exynos EHCI/OHCI drivers.

Reported-by: Markus Reichl <m.reichl@fivetechno.de>
Fixes: 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/core/message.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index e844bb7b5676..6f7d047392bd 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -2009,6 +2009,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		struct usb_interface *intf = cp->interface[i];
 
 		if (intf->dev.of_node &&
+		    of_find_property(intf->dev.of_node, "compatible", NULL) &&
 		    !of_device_is_available(intf->dev.of_node)) {
 			dev_info(&dev->dev, "skipping disabled interface %d\n",
 				 intf->cur_altsetting->desc.bInterfaceNumber);
-- 
2.17.1

Re: [PATCH v2] usb: core: verify devicetree nodes for disabled interfaces

[Måns Rullgård]

Marek Szyprowski <m.szyprowski@samsung.com> writes:

> Commit 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
> add support for disabling given USB device interface by adding nodes to
> the USB host controller device. The mentioned commit however identifies
> the given USB interface node only by the 'reg' property in the host
> controller children nodes and then checks for their the 'status'. The USB
> device interface nodes however also has to have a 'compatible' property as
> described in Documentation/devicetree/bindings/usb/usb-device.txt. This is
> important, because USB host controller might have 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. This conflicts with the proposed
> approach and verifying for the presence of the compatible property fixes
> this issue without changing the bindings and the way the PHY controllers
> are handled by Exynos EHCI/OHCI drivers.
>
> Reported-by: Markus Reichl <m.reichl@fivetechno.de>
> Fixes: 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/core/message.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index e844bb7b5676..6f7d047392bd 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -2009,6 +2009,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>  		struct usb_interface *intf = cp->interface[i];
>
>  		if (intf->dev.of_node &&
> +		    of_find_property(intf->dev.of_node, "compatible", NULL) &&
>  		    !of_device_is_available(intf->dev.of_node)) {
>  			dev_info(&dev->dev, "skipping disabled interface %d\n",
>  				 intf->cur_altsetting->desc.bInterfaceNumber);
> -- 

I don't think this is the right approach.  We don't want to be adding
such checks everywhere the of_node is used.  A better way might be to
not set of_node at all in the absence of a proper "compatible" string.

Then there's the problem of how to resolve the incompatibility between
the generic USB and Exynos bindings.  One possible fix could be to use
a child node of the controller node to represent the root hub.  Since
the driver currently doesn't work at all if a devicetree has nodes for
USB devices, there should be no compatibility concerns.

-- 
Måns Rullgård

Re: [PATCH v2] usb: core: verify devicetree nodes for disabled interfaces

[Marek Szyprowski]

Hi

On 2019-05-08 13:46, Måns Rullgård wrote:
> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>> Commit 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
>> add support for disabling given USB device interface by adding nodes to
>> the USB host controller device. The mentioned commit however identifies
>> the given USB interface node only by the 'reg' property in the host
>> controller children nodes and then checks for their the 'status'. The USB
>> device interface nodes however also has to have a 'compatible' property as
>> described in Documentation/devicetree/bindings/usb/usb-device.txt. This is
>> important, because USB host controller might have 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. This conflicts with the proposed
>> approach and verifying for the presence of the compatible property fixes
>> this issue without changing the bindings and the way the PHY controllers
>> are handled by Exynos EHCI/OHCI drivers.
>>
>> Reported-by: Markus Reichl <m.reichl@fivetechno.de>
>> Fixes: 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/usb/core/message.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>> index e844bb7b5676..6f7d047392bd 100644
>> --- a/drivers/usb/core/message.c
>> +++ b/drivers/usb/core/message.c
>> @@ -2009,6 +2009,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>>   		struct usb_interface *intf = cp->interface[i];
>>
>>   		if (intf->dev.of_node &&
>> +		    of_find_property(intf->dev.of_node, "compatible", NULL) &&
>>   		    !of_device_is_available(intf->dev.of_node)) {
>>   			dev_info(&dev->dev, "skipping disabled interface %d\n",
>>   				 intf->cur_altsetting->desc.bInterfaceNumber);
>> -- 
> I don't think this is the right approach.  We don't want to be adding
> such checks everywhere the of_node is used.  A better way might be to
> not set of_node at all in the absence of a proper "compatible" string.

Right, this will be a better approach. I've just checked the code and a 
simple check for 'compatible' property presence can be easily added in 
drivers/usb/core/of.c in usb_of_get_device_node() and 
usb_of_get_interface_node() functions.

The second check could be added in drivers/usb/core/hub.c in 
usb_new_device() - to ensure that the device's vid/pid matches of_node 
compatible string.

Is this okay? Or just add a latter one?

> Then there's the problem of how to resolve the incompatibility between
> the generic USB and Exynos bindings.  One possible fix could be to use
> a child node of the controller node to represent the root hub.  Since
> the driver currently doesn't work at all if a devicetree has nodes for
> USB devices, there should be no compatibility concerns.

So far we don't have any use case for adding devicetree nodes for usb 
devices under Exynos EHCI/OHCI hcd, so this shouldn't be a problem for now.

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

Re: [PATCH v2] usb: core: verify devicetree nodes for disabled interfaces

[Måns Rullgård]

Marek Szyprowski <m.szyprowski@samsung.com> writes:

> Hi
>
> On 2019-05-08 13:46, Måns Rullgård wrote:
>> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>>> Commit 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
>>> add support for disabling given USB device interface by adding nodes to
>>> the USB host controller device. The mentioned commit however identifies
>>> the given USB interface node only by the 'reg' property in the host
>>> controller children nodes and then checks for their the 'status'. The USB
>>> device interface nodes however also has to have a 'compatible' property as
>>> described in Documentation/devicetree/bindings/usb/usb-device.txt. This is
>>> important, because USB host controller might have 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. This conflicts with the proposed
>>> approach and verifying for the presence of the compatible property fixes
>>> this issue without changing the bindings and the way the PHY controllers
>>> are handled by Exynos EHCI/OHCI drivers.
>>>
>>> Reported-by: Markus Reichl <m.reichl@fivetechno.de>
>>> Fixes: 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   drivers/usb/core/message.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>>> index e844bb7b5676..6f7d047392bd 100644
>>> --- a/drivers/usb/core/message.c
>>> +++ b/drivers/usb/core/message.c
>>> @@ -2009,6 +2009,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>>>   		struct usb_interface *intf = cp->interface[i];
>>>
>>>   		if (intf->dev.of_node &&
>>> +		    of_find_property(intf->dev.of_node, "compatible", NULL) &&
>>>   		    !of_device_is_available(intf->dev.of_node)) {
>>>   			dev_info(&dev->dev, "skipping disabled interface %d\n",
>>>   				 intf->cur_altsetting->desc.bInterfaceNumber);
>>> -- 
>> I don't think this is the right approach.  We don't want to be adding
>> such checks everywhere the of_node is used.  A better way might be to
>> not set of_node at all in the absence of a proper "compatible" string.
>
> Right, this will be a better approach. I've just checked the code and a 
> simple check for 'compatible' property presence can be easily added in 
> drivers/usb/core/of.c in usb_of_get_device_node() and 
> usb_of_get_interface_node() functions.
>
> The second check could be added in drivers/usb/core/hub.c in 
> usb_new_device() - to ensure that the device's vid/pid matches of_node 
> compatible string.
>
> Is this okay? Or just add a latter one?

I'm not sure where the best place to check is.  Someone else will have
to weigh in on that.

>> Then there's the problem of how to resolve the incompatibility between
>> the generic USB and Exynos bindings.  One possible fix could be to use
>> a child node of the controller node to represent the root hub.  Since
>> the driver currently doesn't work at all if a devicetree has nodes for
>> USB devices, there should be no compatibility concerns.
>
> So far we don't have any use case for adding devicetree nodes for usb 
> devices under Exynos EHCI/OHCI hcd, so this shouldn't be a problem for now.

None that you know of, that is.  Regardless, the bindings are
inconsistent, and that needs to be fixed.

-- 
Måns Rullgård

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

[Marek Szyprowski]

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.

Reported-by: Markus Reichl <m.reichl@fivetechno.de>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
v3:
- replaced ad hoc checks by proper test for proper value of the
  compatible string in drivers/usb/core/of.c
v2: https://lkml.org/lkml/2019/5/8/321
v1: https://lkml.org/lkml/2019/5/7/715
---
 drivers/usb/core/hub.c |  3 +++
 drivers/usb/core/of.c  | 31 +++++++++++++++++++++++++++++++
 include/linux/usb/of.h |  2 ++
 3 files changed, 36 insertions(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 2f94568ba385..6f2438522d09 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -22,6 +22,7 @@
 #include <linux/usb.h>
 #include <linux/usbdevice_fs.h>
 #include <linux/usb/hcd.h>
+#include <linux/usb/of.h>
 #include <linux/usb/otg.h>
 #include <linux/usb/quirks.h>
 #include <linux/workqueue.h>
@@ -5023,6 +5024,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 		if (status < 0)
 			goto loop;
 
+		usb_of_validate_device_node(udev);
+
 		if (udev->quirks & USB_QUIRK_DELAY_INIT)
 			msleep(2000);
 
diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
index 651708d8c908..2b6f16753edc 100644
--- a/drivers/usb/core/of.c
+++ b/drivers/usb/core/of.c
@@ -30,6 +30,12 @@ struct device_node *usb_of_get_device_node(struct usb_device *hub, int port1)
 	for_each_child_of_node(hub->dev.of_node, node) {
 		if (of_property_read_u32(node, "reg", &reg))
 			continue;
+		/*
+		 * idVendor and idProduct are not yet known, so check only
+		 * a presence of the compatible property.
+		 */
+		if (!of_find_property(node, "compatible", NULL))
+			continue;
 
 		if (reg == port1)
 			return node;
@@ -39,6 +45,31 @@ struct device_node *usb_of_get_device_node(struct usb_device *hub, int port1)
 }
 EXPORT_SYMBOL_GPL(usb_of_get_device_node);
 
+/**
+ * usb_of_validate_device_node() - validate dt node of the probed USB device
+ * @udev: USB device
+ *
+ * Validate devicetree node for the USB device. Compatible string must match
+ * device's idVendor and idProduct, otherwise the of_node will be put and set
+ * to NULL.
+ */
+void usb_of_validate_device_node(struct usb_device *udev)
+{
+	char compatible[13];
+
+	if (!udev->dev.of_node)
+		return;
+
+	snprintf(compatible, sizeof(compatible), "usb%x,%x",
+		 le16_to_cpu(udev->descriptor.idVendor),
+		 le16_to_cpu(udev->descriptor.idProduct));
+
+	if (of_device_is_compatible(udev->dev.of_node, compatible) == 0) {
+		of_node_put(udev->dev.of_node);
+		udev->dev.of_node = NULL;
+	}
+}
+
 /**
  * usb_of_has_combined_node() - determine whether a device has a combined node
  * @udev: USB device
diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
index dba55ccb9b53..9969efda03ad 100644
--- a/include/linux/usb/of.h
+++ b/include/linux/usb/of.h
@@ -24,6 +24,7 @@ bool usb_of_has_combined_node(struct usb_device *udev);
 struct device_node *usb_of_get_interface_node(struct usb_device *udev,
 		u8 config, u8 ifnum);
 struct device *usb_of_get_companion_dev(struct device *dev);
+void usb_of_validate_device_node(struct usb_device *udev);
 #else
 static inline enum usb_dr_mode
 of_usb_get_dr_mode_by_phy(struct device_node *np, int arg0)
@@ -57,6 +58,7 @@ static inline struct device *usb_of_get_companion_dev(struct device *dev)
 {
 	return NULL;
 }
+static inline void usb_of_validate_device_node(struct usb_device *udev) { }
 #endif
 
 #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
-- 
2.17.1

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

[Måns Rullgård]

Marek Szyprowski <m.szyprowski@samsung.com> 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.
>
> Reported-by: Markus Reichl <m.reichl@fivetechno.de>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> v3:
> - replaced ad hoc checks by proper test for proper value of the
>   compatible string in drivers/usb/core/of.c
> v2: https://lkml.org/lkml/2019/5/8/321
> v1: https://lkml.org/lkml/2019/5/7/715
> ---
>  drivers/usb/core/hub.c |  3 +++
>  drivers/usb/core/of.c  | 31 +++++++++++++++++++++++++++++++
>  include/linux/usb/of.h |  2 ++
>  3 files changed, 36 insertions(+)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 2f94568ba385..6f2438522d09 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -22,6 +22,7 @@
>  #include <linux/usb.h>
>  #include <linux/usbdevice_fs.h>
>  #include <linux/usb/hcd.h>
> +#include <linux/usb/of.h>
>  #include <linux/usb/otg.h>
>  #include <linux/usb/quirks.h>
>  #include <linux/workqueue.h>
> @@ -5023,6 +5024,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>  		if (status < 0)
>  			goto loop;
>
> +		usb_of_validate_device_node(udev);
> +
>  		if (udev->quirks & USB_QUIRK_DELAY_INIT)
>  			msleep(2000);
>
> diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> index 651708d8c908..2b6f16753edc 100644
> --- a/drivers/usb/core/of.c
> +++ b/drivers/usb/core/of.c
> @@ -30,6 +30,12 @@ struct device_node *usb_of_get_device_node(struct usb_device *hub, int port1)
>  	for_each_child_of_node(hub->dev.of_node, node) {
>  		if (of_property_read_u32(node, "reg", &reg))
>  			continue;
> +		/*
> +		 * idVendor and idProduct are not yet known, so check only
> +		 * a presence of the compatible property.
> +		 */

This function could be called from anywhere, so that comment seems a bit
misplaced.

> +		if (!of_find_property(node, "compatible", NULL))
> +			continue;

What if there is a node with a "compatible" property for some entirely
different purpose?  Since that won't be caught, why bother with this
test at all?

>  		if (reg == port1)
>  			return node;
> @@ -39,6 +45,31 @@ struct device_node *usb_of_get_device_node(struct usb_device *hub, int port1)
>  }
>  EXPORT_SYMBOL_GPL(usb_of_get_device_node);
>
> +/**
> + * usb_of_validate_device_node() - validate dt node of the probed USB device
> + * @udev: USB device
> + *
> + * Validate devicetree node for the USB device. Compatible string must match
> + * device's idVendor and idProduct, otherwise the of_node will be put and set
> + * to NULL.
> + */
> +void usb_of_validate_device_node(struct usb_device *udev)
> +{
> +	char compatible[13];
> +
> +	if (!udev->dev.of_node)
> +		return;
> +
> +	snprintf(compatible, sizeof(compatible), "usb%x,%x",
> +		 le16_to_cpu(udev->descriptor.idVendor),
> +		 le16_to_cpu(udev->descriptor.idProduct));
> +
> +	if (of_device_is_compatible(udev->dev.of_node, compatible) == 0) {
> +		of_node_put(udev->dev.of_node);
> +		udev->dev.of_node = NULL;
> +	}
> +}
> +
>  /**
>   * usb_of_has_combined_node() - determine whether a device has a combined node
>   * @udev: USB device
> diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> index dba55ccb9b53..9969efda03ad 100644
> --- a/include/linux/usb/of.h
> +++ b/include/linux/usb/of.h
> @@ -24,6 +24,7 @@ bool usb_of_has_combined_node(struct usb_device *udev);
>  struct device_node *usb_of_get_interface_node(struct usb_device *udev,
>  		u8 config, u8 ifnum);
>  struct device *usb_of_get_companion_dev(struct device *dev);
> +void usb_of_validate_device_node(struct usb_device *udev);
>  #else
>  static inline enum usb_dr_mode
>  of_usb_get_dr_mode_by_phy(struct device_node *np, int arg0)
> @@ -57,6 +58,7 @@ static inline struct device *usb_of_get_companion_dev(struct device *dev)
>  {
>  	return NULL;
>  }
> +static inline void usb_of_validate_device_node(struct usb_device *udev) { }
>  #endif
>
>  #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
> -- 
> 2.17.1
>

-- 
Måns Rullgård

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

[Peter Chen]

 
> 
> Marek Szyprowski <m.szyprowski@samsung.com> 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?
I do not understand how 01fdf179f4b0 affect your boards, some nodes under
the USB controller with status is not "okay", but still want to be enumerated?

Peter

> > Reported-by: Markus Reichl <m.reichl@fivetechno.de>
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> > v3:
> > - replaced ad hoc checks by proper test for proper value of the
> >   compatible string in drivers/usb/core/of.c
> > v2:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml
> > .org%2Flkml%2F2019%2F5%2F8%2F321&amp;data=02%7C01%7Cpeter.chen%
> 40nxp.c
> >
> om%7Cd9336abb14a745fb152508d6d4afdbb5%7C686ea1d3bc2b4c6fa92cd99c5c3
> 016
> >
> 35%7C0%7C0%7C636930249105615889&amp;sdata=CIC09rcvf%2FFp5y6oRQtJ
> Hk%2Bb
> > k7QjvJHECpK36LT8ocU%3D&amp;reserved=0
> > v1:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml
> > .org%2Flkml%2F2019%2F5%2F7%2F715&amp;data=02%7C01%7Cpeter.chen%
> 40nxp.c
> >
> om%7Cd9336abb14a745fb152508d6d4afdbb5%7C686ea1d3bc2b4c6fa92cd99c5c3
> 016
> >
> 35%7C0%7C0%7C636930249105625893&amp;sdata=RbbKufAqSKRZpY726zWne
> 9cDhlK0
> > mlkBghhOmkqelY8%3D&amp;reserved=0
> > ---
> >  drivers/usb/core/hub.c |  3 +++
> >  drivers/usb/core/of.c  | 31 +++++++++++++++++++++++++++++++
> > include/linux/usb/of.h |  2 ++
> >  3 files changed, 36 insertions(+)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index
> > 2f94568ba385..6f2438522d09 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/usb.h>
> >  #include <linux/usbdevice_fs.h>
> >  #include <linux/usb/hcd.h>
> > +#include <linux/usb/of.h>
> >  #include <linux/usb/otg.h>
> >  #include <linux/usb/quirks.h>
> >  #include <linux/workqueue.h>
> > @@ -5023,6 +5024,8 @@ static void hub_port_connect(struct usb_hub *hub, int
> port1, u16 portstatus,
> >  		if (status < 0)
> >  			goto loop;
> >
> > +		usb_of_validate_device_node(udev);
> > +
> >  		if (udev->quirks & USB_QUIRK_DELAY_INIT)
> >  			msleep(2000);
> >
> > diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c index
> > 651708d8c908..2b6f16753edc 100644
> > --- a/drivers/usb/core/of.c
> > +++ b/drivers/usb/core/of.c
> > @@ -30,6 +30,12 @@ struct device_node *usb_of_get_device_node(struct
> usb_device *hub, int port1)
> >  	for_each_child_of_node(hub->dev.of_node, node) {
> >  		if (of_property_read_u32(node, "reg", &reg))
> >  			continue;
> > +		/*
> > +		 * idVendor and idProduct are not yet known, so check only
> > +		 * a presence of the compatible property.
> > +		 */
> 
> This function could be called from anywhere, so that comment seems a bit
> misplaced.
> 
> > +		if (!of_find_property(node, "compatible", NULL))
> > +			continue;
> 
> What if there is a node with a "compatible" property for some entirely different
> purpose?  Since that won't be caught, why bother with this test at all?
> 
> >  		if (reg == port1)
> >  			return node;
> > @@ -39,6 +45,31 @@ struct device_node *usb_of_get_device_node(struct
> > usb_device *hub, int port1)  }
> > EXPORT_SYMBOL_GPL(usb_of_get_device_node);
> >
> > +/**
> > + * usb_of_validate_device_node() - validate dt node of the probed USB
> > +device
> > + * @udev: USB device
> > + *
> > + * Validate devicetree node for the USB device. Compatible string
> > +must match
> > + * device's idVendor and idProduct, otherwise the of_node will be put
> > +and set
> > + * to NULL.
> > + */
> > +void usb_of_validate_device_node(struct usb_device *udev) {
> > +	char compatible[13];
> > +
> > +	if (!udev->dev.of_node)
> > +		return;
> > +
> > +	snprintf(compatible, sizeof(compatible), "usb%x,%x",
> > +		 le16_to_cpu(udev->descriptor.idVendor),
> > +		 le16_to_cpu(udev->descriptor.idProduct));
> > +
> > +	if (of_device_is_compatible(udev->dev.of_node, compatible) == 0) {
> > +		of_node_put(udev->dev.of_node);
> > +		udev->dev.of_node = NULL;
> > +	}
> > +}
> > +
> >  /**
> >   * usb_of_has_combined_node() - determine whether a device has a combined
> node
> >   * @udev: USB device
> > diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h index
> > dba55ccb9b53..9969efda03ad 100644
> > --- a/include/linux/usb/of.h
> > +++ b/include/linux/usb/of.h
> > @@ -24,6 +24,7 @@ bool usb_of_has_combined_node(struct usb_device
> > *udev);  struct device_node *usb_of_get_interface_node(struct usb_device *udev,
> >  		u8 config, u8 ifnum);
> >  struct device *usb_of_get_companion_dev(struct device *dev);
> > +void usb_of_validate_device_node(struct usb_device *udev);
> >  #else
> >  static inline enum usb_dr_mode
> >  of_usb_get_dr_mode_by_phy(struct device_node *np, int arg0) @@ -57,6
> > +58,7 @@ static inline struct device *usb_of_get_companion_dev(struct
> > device *dev)  {
> >  	return NULL;
> >  }
> > +static inline void usb_of_validate_device_node(struct usb_device
> > +*udev) { }
> >  #endif
> >
> >  #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
> > --
> > 2.17.1
> >
> 
> --
> Måns Rullgård

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

[Marek Szyprowski]

Hi Peter,

On 2019-05-10 05:10, Peter Chen wrote:
>
>> Marek Szyprowski <m.szyprowski@samsung.com> 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.

> I do not understand how 01fdf179f4b0 affect your boards, some nodes under
> the USB controller with status is not "okay", but still want to be enumerated?

Please look at the ehci node in arch/arm/boot/dts/exynos4.dtsi and then 
at the changes to that node in arch/arm/boot/dts/exynos4412-odroidx.dts. 
Exynos EHCI controller has 3 subnodes, which matches to the physical 
ports of it and allows the driver to enable given PHY ports depending on 
which physical port is used on the particular board. All ports cannot 
not be enabled by default, because PHY controller has limited resources 
and shares them between USB host and USB device ports.


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

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

[Peter Chen]

 
> 
> On 2019-05-10 05:10, Peter Chen wrote:
> >
> >> Marek Szyprowski <m.szyprowski@samsung.com> 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?

Peter

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

[Marek Szyprowski]

Hi Peter,

On 2019-05-13 11:00, Peter Chen wrote:
>> On 2019-05-10 05:10, Peter Chen wrote:
>>>> Marek Szyprowski <m.szyprowski@samsung.com> 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?

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

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

[Peter Chen]

 
> On 2019-05-13 11:00, Peter Chen wrote:
> >> On 2019-05-10 05:10, Peter Chen wrote:
> >>>> Marek Szyprowski <m.szyprowski@samsung.com> 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?

Peter

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

[Marek Szyprowski]

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@samsung.com> 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?

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

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

[Måns Rullgård]

Marek Szyprowski <m.szyprowski@samsung.com> 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@samsung.com> 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.

-- 
Måns Rullgård

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@samsung.com> 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@samsung.com> 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

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

[Måns Rullgård]

Marek Szyprowski <m.szyprowski@samsung.com> writes:

> Hi Peter,
>
> On 2019-05-10 05:10, Peter Chen wrote:
>>
>>> Marek Szyprowski <m.szyprowski@samsung.com> 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.
>
>> I do not understand how 01fdf179f4b0 affect your boards, some nodes
>> under the USB controller with status is not "okay", but still want to
>> be enumerated?
>
> Please look at the ehci node in arch/arm/boot/dts/exynos4.dtsi and then 
> at the changes to that node in arch/arm/boot/dts/exynos4412-odroidx.dts. 
> Exynos EHCI controller has 3 subnodes, which matches to the physical 
> ports of it and allows the driver to enable given PHY ports depending on 
> which physical port is used on the particular board. All ports cannot 
> not be enabled by default, because PHY controller has limited resources 
> and shares them between USB host and USB device ports.

It seems like what's happening is that the Exynos port/phy nodes are
mistaken for standard USB device nodes attached to the root hub.  The
problem is that hub port numbering starts at 1 while the Exynos nodes
start from 0.  This causes attached devices to be associated with the
wrong DT node.

Ignoring backwards compatibility, I can see a few ways of fixing this:

- Add another child node, along side the port@N nodes, of the host
  controller to represent the root hub.  Nodes for attached devices
  would then be descendants of this new node.

- Change the Exynos HCD binding to use a more standard "phys" property
  and get rid of the child nodes for this purpose.

- Move the port@N nodes below a new dedicated child node of the HCD.

The first is probably the easiest to implement since it doesn't require
any nasty hacks to avoid breaking existing device trees.

-- 
Måns Rullgård

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

[Marek Szyprowski]

Hi Måns

On 2019-05-13 12:03, Måns Rullgård wrote:
> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>> Hi Peter,
>>
>> On 2019-05-10 05:10, Peter Chen wrote:
>>>
>>>> Marek Szyprowski <m.szyprowski@samsung.com> 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.
>>
>>> I do not understand how 01fdf179f4b0 affect your boards, some nodes
>>> under the USB controller with status is not "okay", but still want to
>>> be enumerated?
>> Please look at the ehci node in arch/arm/boot/dts/exynos4.dtsi and then
>> at the changes to that node in arch/arm/boot/dts/exynos4412-odroidx.dts.
>> Exynos EHCI controller has 3 subnodes, which matches to the physical
>> ports of it and allows the driver to enable given PHY ports depending on
>> which physical port is used on the particular board. All ports cannot
>> not be enabled by default, because PHY controller has limited resources
>> and shares them between USB host and USB device ports.
> It seems like what's happening is that the Exynos port/phy nodes are
> mistaken for standard USB device nodes attached to the root hub.  The
> problem is that hub port numbering starts at 1 while the Exynos nodes
> start from 0.  This causes attached devices to be associated with the
> wrong DT node.
>
> Ignoring backwards compatibility, I can see a few ways of fixing this:
>
> - Add another child node, along side the port@N nodes, of the host
>    controller to represent the root hub.  Nodes for attached devices
>    would then be descendants of this new node.
>
> - Change the Exynos HCD binding to use a more standard "phys" property
>    and get rid of the child nodes for this purpose.
>
> - Move the port@N nodes below a new dedicated child node of the HCD.
>
> The first is probably the easiest to implement since it doesn't require
> any nasty hacks to avoid breaking existing device trees.

I've posted a patch, which disables creating USB device nodes for Exynos 
HCD devices (by zeroing their of_node pointer). Then I will try to apply 
the second approach from the above list, but merging it to mainline will 
require a few more steps and some time.

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

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

[Tobias Jakobi]

Hello Marek,

I can confirm that this restores USB operation on my X2, so you can my Tested-by
if you want.

With best wishes,
Tobias


Marek Szyprowski wrote:
> 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.
> 
> Reported-by: Markus Reichl <m.reichl@fivetechno.de>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> v3:
> - replaced ad hoc checks by proper test for proper value of the
>   compatible string in drivers/usb/core/of.c
> v2: https://lkml.org/lkml/2019/5/8/321
> v1: https://lkml.org/lkml/2019/5/7/715
> ---
>  drivers/usb/core/hub.c |  3 +++
>  drivers/usb/core/of.c  | 31 +++++++++++++++++++++++++++++++
>  include/linux/usb/of.h |  2 ++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 2f94568ba385..6f2438522d09 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -22,6 +22,7 @@
>  #include <linux/usb.h>
>  #include <linux/usbdevice_fs.h>
>  #include <linux/usb/hcd.h>
> +#include <linux/usb/of.h>
>  #include <linux/usb/otg.h>
>  #include <linux/usb/quirks.h>
>  #include <linux/workqueue.h>
> @@ -5023,6 +5024,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>  		if (status < 0)
>  			goto loop;
>  
> +		usb_of_validate_device_node(udev);
> +
>  		if (udev->quirks & USB_QUIRK_DELAY_INIT)
>  			msleep(2000);
>  
> diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> index 651708d8c908..2b6f16753edc 100644
> --- a/drivers/usb/core/of.c
> +++ b/drivers/usb/core/of.c
> @@ -30,6 +30,12 @@ struct device_node *usb_of_get_device_node(struct usb_device *hub, int port1)
>  	for_each_child_of_node(hub->dev.of_node, node) {
>  		if (of_property_read_u32(node, "reg", &reg))
>  			continue;
> +		/*
> +		 * idVendor and idProduct are not yet known, so check only
> +		 * a presence of the compatible property.
> +		 */
> +		if (!of_find_property(node, "compatible", NULL))
> +			continue;
>  
>  		if (reg == port1)
>  			return node;
> @@ -39,6 +45,31 @@ struct device_node *usb_of_get_device_node(struct usb_device *hub, int port1)
>  }
>  EXPORT_SYMBOL_GPL(usb_of_get_device_node);
>  
> +/**
> + * usb_of_validate_device_node() - validate dt node of the probed USB device
> + * @udev: USB device
> + *
> + * Validate devicetree node for the USB device. Compatible string must match
> + * device's idVendor and idProduct, otherwise the of_node will be put and set
> + * to NULL.
> + */
> +void usb_of_validate_device_node(struct usb_device *udev)
> +{
> +	char compatible[13];
> +
> +	if (!udev->dev.of_node)
> +		return;
> +
> +	snprintf(compatible, sizeof(compatible), "usb%x,%x",
> +		 le16_to_cpu(udev->descriptor.idVendor),
> +		 le16_to_cpu(udev->descriptor.idProduct));
> +
> +	if (of_device_is_compatible(udev->dev.of_node, compatible) == 0) {
> +		of_node_put(udev->dev.of_node);
> +		udev->dev.of_node = NULL;
> +	}
> +}
> +
>  /**
>   * usb_of_has_combined_node() - determine whether a device has a combined node
>   * @udev: USB device
> diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> index dba55ccb9b53..9969efda03ad 100644
> --- a/include/linux/usb/of.h
> +++ b/include/linux/usb/of.h
> @@ -24,6 +24,7 @@ bool usb_of_has_combined_node(struct usb_device *udev);
>  struct device_node *usb_of_get_interface_node(struct usb_device *udev,
>  		u8 config, u8 ifnum);
>  struct device *usb_of_get_companion_dev(struct device *dev);
> +void usb_of_validate_device_node(struct usb_device *udev);
>  #else
>  static inline enum usb_dr_mode
>  of_usb_get_dr_mode_by_phy(struct device_node *np, int arg0)
> @@ -57,6 +58,7 @@ static inline struct device *usb_of_get_companion_dev(struct device *dev)
>  {
>  	return NULL;
>  }
> +static inline void usb_of_validate_device_node(struct usb_device *udev) { }
>  #endif
>  
>  #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
>