| Message ID | 20190509084726.5405-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] usb: core: verify devicetree nodes for USB devices | expand |
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", ®)) > 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 >
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", ®)) > 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) >
> > 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&data=02%7C01%7Cpeter.chen% > 40nxp.c > > > om%7Cd9336abb14a745fb152508d6d4afdbb5%7C686ea1d3bc2b4c6fa92cd99c5c3 > 016 > > > 35%7C0%7C0%7C636930249105615889&sdata=CIC09rcvf%2FFp5y6oRQtJ > Hk%2Bb > > k7QjvJHECpK36LT8ocU%3D&reserved=0 > > v1: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml > > .org%2Flkml%2F2019%2F5%2F7%2F715&data=02%7C01%7Cpeter.chen% > 40nxp.c > > > om%7Cd9336abb14a745fb152508d6d4afdbb5%7C686ea1d3bc2b4c6fa92cd99c5c3 > 016 > > > 35%7C0%7C0%7C636930249105625893&sdata=RbbKufAqSKRZpY726zWne > 9cDhlK0 > > mlkBghhOmkqelY8%3D&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", ®)) > > 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
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
> > 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
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
> 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
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 <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.
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.
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
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
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", ®)) 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)
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(+)