| Message ID | 20200908150124.31336-1-pgwipeout@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | phy: tegra: xusb: fix xusb backwards compatibility | expand |
Hi Peter, Thanks for your patch. If an OTG capable port doesn't have a way to detect its initial role based on cable detection, I think it's somehow dangerous to give the port to host controller. When the port is enabled for host role, platform starts supplying VBUS to the connector. If the connector happens to be connected to a host role port which is supplying VBUS as well, there could be some damage to the board which doesn't have proper protection. Thanks, JC On 9/8/20 11:01 PM, Peter Geis wrote: > Prior to implementing role switch support, all enabled ports enumerated > as host devices. > With role switch support enabled, device trees with otg ports which have > not been updated with usb-role-switch support now bail out during > enumeration. > This disables all xhci ports tied to the affected phy. > > Retain backwards compatibility by forcing host mode on otg ports which > are missing the usb-role-switch flag. > Disable ports explicitly defined as peripheral mode that are missing the > usb-role-switch flag. > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > Reported-by: Matias Zuniga <matias.nicolas.zc@gmail.com> > > Fixes: e8f7d2f409a1 ("phy: tegra: xusb: Add usb-phy support") > --- > drivers/phy/tegra/xusb.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c > index de4a46fe1763..c36dce13e0c6 100644 > --- a/drivers/phy/tegra/xusb.c > +++ b/drivers/phy/tegra/xusb.c > @@ -734,10 +734,12 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2) > err = tegra_xusb_setup_usb_role_switch(port); > if (err < 0) > return err; > + } else if (usb2->mode == USB_DR_MODE_PERIPHERAL) { > + dev_err(&port->dev, "mandatory usb-role-switch not found for %s mode, disabling port\n", modes[usb2->mode]); > + usb2->mode = USB_DR_MODE_UNKNOWN; > } else { > - dev_err(&port->dev, "usb-role-switch not found for %s mode", > - modes[usb2->mode]); > - return -EINVAL; > + dev_warn(&port->dev, "usb-role-switch not found for %s mode, forcing host\n", modes[usb2->mode]); > + usb2->mode = USB_DR_MODE_HOST; > } > } > >
On Wed, Sep 9, 2020 at 10:18 PM JC Kuo <jckuo@nvidia.com> wrote: > > Hi Peter, > Thanks for your patch. If an OTG capable port doesn't have a way to detect its > initial role based on cable detection, I think it's somehow dangerous to give > the port to host controller. When the port is enabled for host role, platform > starts supplying VBUS to the connector. If the connector happens to be connected > to a host role port which is supplying VBUS as well, there could be some damage > to the board which doesn't have proper protection. Thank you for your input. Unfortunately I see a few issues with this concept. First, it doesn't address that the patch this fix addresses breaks all usb for older device trees, which already are implemented with a stable ABI. As the mailing list has made abundantly clear, if at all possible we need to prevent breaking backwards compatibility. Specifically this breaks Tegra124 devices with mainlined device trees. It was reported against the tegra124-nyan, but from the looks of it the following mainlined boards will be affected: tegra124-apalis tegra124-apalis-v1.2 tegra124-nyan tegra124-venice2 Second, from an engineering perspective it doesn't make much sense. USB operates at 5v, so even if VBUS is enabled and a device is plugged in that also provides 5v, the total potential will remain the same. USB-C can drive higher, but only at the request of the controller chip, which will have protections in place. There is nothing stopping a user from plugging in a device cable to a host only port. There are poorly designed devices such as powered hubs that can back feed power into the USB port. Even peripheral ports lacking VBUS can function as host ports with an external power supply. Third, and I hate to bring other drivers into this, most drivers have yet to make usb-role-switch a mandatory flag. The presence of dr_mode = <otg> alone shows the port is capable of both modes. The usb-role-switch API is completely new, having only come around in 5.4 with an intel driver. This patch prevents us from breaking all usb functionality, while retaining the driver's behavior prior to the patch it addresses. If you have a better method for accomplishing both goals, I'm happy to hear any ideas. > > Thanks, > JC > > On 9/8/20 11:01 PM, Peter Geis wrote: > > Prior to implementing role switch support, all enabled ports enumerated > > as host devices. > > With role switch support enabled, device trees with otg ports which have > > not been updated with usb-role-switch support now bail out during > > enumeration. > > This disables all xhci ports tied to the affected phy. > > > > Retain backwards compatibility by forcing host mode on otg ports which > > are missing the usb-role-switch flag. > > Disable ports explicitly defined as peripheral mode that are missing the > > usb-role-switch flag. > > > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > > Reported-by: Matias Zuniga <matias.nicolas.zc@gmail.com> > > > > Fixes: e8f7d2f409a1 ("phy: tegra: xusb: Add usb-phy support") > > --- > > drivers/phy/tegra/xusb.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c > > index de4a46fe1763..c36dce13e0c6 100644 > > --- a/drivers/phy/tegra/xusb.c > > +++ b/drivers/phy/tegra/xusb.c > > @@ -734,10 +734,12 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2) > > err = tegra_xusb_setup_usb_role_switch(port); > > if (err < 0) > > return err; > > + } else if (usb2->mode == USB_DR_MODE_PERIPHERAL) { > > + dev_err(&port->dev, "mandatory usb-role-switch not found for %s mode, disabling port\n", modes[usb2->mode]); > > + usb2->mode = USB_DR_MODE_UNKNOWN; > > } else { > > - dev_err(&port->dev, "usb-role-switch not found for %s mode", > > - modes[usb2->mode]); > > - return -EINVAL; > > + dev_warn(&port->dev, "usb-role-switch not found for %s mode, forcing host\n", modes[usb2->mode]); > > + usb2->mode = USB_DR_MODE_HOST; > > } > > } > > > >
diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c index de4a46fe1763..c36dce13e0c6 100644 --- a/drivers/phy/tegra/xusb.c +++ b/drivers/phy/tegra/xusb.c @@ -734,10 +734,12 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2) err = tegra_xusb_setup_usb_role_switch(port); if (err < 0) return err; + } else if (usb2->mode == USB_DR_MODE_PERIPHERAL) { + dev_err(&port->dev, "mandatory usb-role-switch not found for %s mode, disabling port\n", modes[usb2->mode]); + usb2->mode = USB_DR_MODE_UNKNOWN; } else { - dev_err(&port->dev, "usb-role-switch not found for %s mode", - modes[usb2->mode]); - return -EINVAL; + dev_warn(&port->dev, "usb-role-switch not found for %s mode, forcing host\n", modes[usb2->mode]); + usb2->mode = USB_DR_MODE_HOST; } }
Prior to implementing role switch support, all enabled ports enumerated as host devices. With role switch support enabled, device trees with otg ports which have not been updated with usb-role-switch support now bail out during enumeration. This disables all xhci ports tied to the affected phy. Retain backwards compatibility by forcing host mode on otg ports which are missing the usb-role-switch flag. Disable ports explicitly defined as peripheral mode that are missing the usb-role-switch flag. Signed-off-by: Peter Geis <pgwipeout@gmail.com> Reported-by: Matias Zuniga <matias.nicolas.zc@gmail.com> Fixes: e8f7d2f409a1 ("phy: tegra: xusb: Add usb-phy support") --- drivers/phy/tegra/xusb.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)