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

Message ID 20190509084726.5405-1-m.szyprowski@samsung.com
State New
Headers show
Series
  • [v3] usb: core: verify devicetree nodes for USB devices
Related show

Commit Message

Marek Szyprowski May 9, 2019, 8:47 a.m.
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(+)

-- 
2.17.1

Comments

Tobias Jakobi May 9, 2019, 7:04 p.m. | #1
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)

>
Peter Chen May 13, 2019, 9 a.m. | #2
> 

> 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
Peter Chen May 13, 2019, 9:23 a.m. | #3
> 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
Marek Szyprowski May 13, 2019, 10:03 a.m. | #4
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
Mans Rullgard May 13, 2019, 10:03 a.m. | #5
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
Mans Rullgard May 13, 2019, 10:06 a.m. | #6
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

Patch

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)