[1/1] usb: dwc3: Do not get extcon device when usb-role-switch is used

  The change breaks device tree based platforms with PHY device and use
usb-role-switch instead of an extcon switch. extcon_find_edev_by_node()
will return EPROBE_DEFER if it can not find a device so probing without
an extcon device will be deferred indefinitely. Fix this by
explicitly checking for usb-role-switch.
At least the out-of-tree USB3 support on Apple silicon based platforms
using dwc3 with tipd USB Type-C and PD controller is affected by this
issue.

Fixes: d182c2e1bc92 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")
Cc: stable@kernel.org
Signed-off-by: Janne Grunau <j@jannau.net>
---
 drivers/usb/dwc3/core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
  

On Sun, Nov 06, 2022 at 10:48:04PM +0100, Janne Grunau wrote:
> The change breaks device tree based platforms with PHY device and use
> usb-role-switch instead of an extcon switch. extcon_find_edev_by_node()
> will return EPROBE_DEFER if it can not find a device so probing without
> an extcon device will be deferred indefinitely. Fix this by
> explicitly checking for usb-role-switch.
> At least the out-of-tree USB3 support on Apple silicon based platforms
> using dwc3 with tipd USB Type-C and PD controller is affected by this
> issue.

We don't care about out-of-tree modules, do we?

OTOH, the problem you are trying to workaround is probably in a (mis)use of
deferred probe somewhere.

Btw, does it prevent the system boot or you just see the extcon in the list of
deferred devices after booting?
  

On Mon, Nov 7, 2022, at 12:25, Andy Shevchenko wrote:
> On Sun, Nov 06, 2022 at 10:48:04PM +0100, Janne Grunau wrote:
>> The change breaks device tree based platforms with PHY device and use
>> usb-role-switch instead of an extcon switch. extcon_find_edev_by_node()
>> will return EPROBE_DEFER if it can not find a device so probing without
>> an extcon device will be deferred indefinitely. Fix this by
>> explicitly checking for usb-role-switch.
>> At least the out-of-tree USB3 support on Apple silicon based platforms
>> using dwc3 with tipd USB Type-C and PD controller is affected by this
>> issue.
>
> We don't care about out-of-tree modules, do we?
>
> OTOH, the problem you are trying to workaround is probably in a (mis)use of
> deferred probe somewhere.
>
> Btw, does it prevent the system boot or you just see the extcon in the list of
> deferred devices after booting?

Which extcon? The commit description already mentions that the issue is that there
is no extcon and that the dwc3 probe gets stuck with EPROBE_DEFER forever.

This happens because the code after Janne's new check looks for the PHY and then
just assumes that if the PHY has a "port" that the other end always is an extcon.
It then tries extcon_find_edev_by_node which will always fail with EPROBE_DEFER
if that node never registers an extcon.

If "usb-role-switch" is used and configured in the DT there is no extcon.
There actually cannot ever be a working extcon with "usb-role-switch" because
the very first thing dwc3_drd_init does is to look for a role switch partner
and then skip the entire extcon setup:

int dwc3_drd_init(struct dwc3 *dwc)
{
	int ret, irq;

	if (ROLE_SWITCH &&
	    device_property_read_bool(dwc->dev, "usb-role-switch"))
		return dwc3_setup_role_switch(dwc);
[....]


This entire issue was actually first fixed in ab7aa2866d29, then broken
again in 0f0101719138 due to a merge resolution, then fixed again with
7a84e7353e23 (where we actually had a brief discussion about this already
on the ML) and then broken again in d182c2e1bc92.

Janne's fix is much less subtle and should hopefully survive this time.

For the patch:

Reviewed-by: Sven Peter <sven@svenpeter.dev>


Best,

Sven
  

Hi,

On Sun, Nov 06, 2022, Janne Grunau wrote:
> The change breaks device tree based platforms with PHY device and use
> usb-role-switch instead of an extcon switch. extcon_find_edev_by_node()
> will return EPROBE_DEFER if it can not find a device so probing without
> an extcon device will be deferred indefinitely. Fix this by
> explicitly checking for usb-role-switch.
> At least the out-of-tree USB3 support on Apple silicon based platforms
> using dwc3 with tipd USB Type-C and PD controller is affected by this
> issue.
> 
> Fixes: d182c2e1bc92 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")
> Cc: stable@kernel.org
> Signed-off-by: Janne Grunau <j@jannau.net>
> ---
>  drivers/usb/dwc3/core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c0e7c76dc5c8..1f348bc867c2 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1710,6 +1710,16 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
>  	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
>  		return extcon_get_extcon_dev(name);
>  
> +	/*
> +	 * Check explicitly if "usb-role-switch" is used since
> +	 * extcon_find_edev_by_node() can not be used to check the absence of

can not -> can't or cannot

> +	 * an extcon device. In the absence of an device it will always return

"a" device,

> +	 * EPROBE_DEFER.
> +	 */
> +	if (IS_ENABLED(CONFIG_USB_ROLE_SWITCH) &&
> +	    device_property_read_bool(dev, "usb-role-switch"))
> +		return NULL;
> +
>  	/*
>  	 * Try to get an extcon device from the USB PHY controller's "port"
>  	 * node. Check if it has the "port" node first, to avoid printing the
> -- 
> 2.37.3
> 

Looks like the "port" node check from c824c73a5e08 ("usb: dwc3: drd:
Avoid error when extcon is missing") did not account for this platform
setup.

This looks fine to me. Should we cleanup the incomplete check and stale
comment right after this too (as a separate patch)?

Thanks,
Thinh
  

On Tue, Nov 08, 2022 at 02:02:19AM +0000, Thinh Nguyen wrote:
> Hi,
> 
> On Sun, Nov 06, 2022, Janne Grunau wrote:
> > The change breaks device tree based platforms with PHY device and use
> > usb-role-switch instead of an extcon switch. extcon_find_edev_by_node()
> > will return EPROBE_DEFER if it can not find a device so probing without
> > an extcon device will be deferred indefinitely. Fix this by
> > explicitly checking for usb-role-switch.
> > At least the out-of-tree USB3 support on Apple silicon based platforms
> > using dwc3 with tipd USB Type-C and PD controller is affected by this
> > issue.
> > 
> > Fixes: d182c2e1bc92 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")
> > Cc: stable@kernel.org
> > Signed-off-by: Janne Grunau <j@jannau.net>
> > ---
> >  drivers/usb/dwc3/core.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index c0e7c76dc5c8..1f348bc867c2 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1710,6 +1710,16 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
> >  	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
> >  		return extcon_get_extcon_dev(name);
> >  
> > +	/*
> > +	 * Check explicitly if "usb-role-switch" is used since
> > +	 * extcon_find_edev_by_node() can not be used to check the absence of
> 
> can not -> can't or cannot

"can not" is fine.

> 
> > +	 * an extcon device. In the absence of an device it will always return
> 
> "a" device,

Not an issue.

> > +	 * EPROBE_DEFER.
> > +	 */
> > +	if (IS_ENABLED(CONFIG_USB_ROLE_SWITCH) &&
> > +	    device_property_read_bool(dev, "usb-role-switch"))
> > +		return NULL;
> > +
> >  	/*
> >  	 * Try to get an extcon device from the USB PHY controller's "port"
> >  	 * node. Check if it has the "port" node first, to avoid printing the
> > -- 
> > 2.37.3
> > 
> 
> Looks like the "port" node check from c824c73a5e08 ("usb: dwc3: drd:
> Avoid error when extcon is missing") did not account for this platform
> setup.
> 
> This looks fine to me. Should we cleanup the incomplete check and stale
> comment right after this too (as a separate patch)?

Is this a Reviewed-by: ?

thanks,

greg k-h
  

On Tue, Nov 08, 2022, Greg Kroah-Hartman wrote:
> On Tue, Nov 08, 2022 at 02:02:19AM +0000, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Sun, Nov 06, 2022, Janne Grunau wrote:
> > > The change breaks device tree based platforms with PHY device and use
> > > usb-role-switch instead of an extcon switch. extcon_find_edev_by_node()
> > > will return EPROBE_DEFER if it can not find a device so probing without
> > > an extcon device will be deferred indefinitely. Fix this by
> > > explicitly checking for usb-role-switch.
> > > At least the out-of-tree USB3 support on Apple silicon based platforms
> > > using dwc3 with tipd USB Type-C and PD controller is affected by this
> > > issue.
> > > 
> > > Fixes: d182c2e1bc92 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")
> > > Cc: stable@kernel.org
> > > Signed-off-by: Janne Grunau <j@jannau.net>
> > > ---
> > >  drivers/usb/dwc3/core.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index c0e7c76dc5c8..1f348bc867c2 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -1710,6 +1710,16 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
> > >  	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0)
> > >  		return extcon_get_extcon_dev(name);
> > >  
> > > +	/*
> > > +	 * Check explicitly if "usb-role-switch" is used since
> > > +	 * extcon_find_edev_by_node() can not be used to check the absence of
> > 
> > can not -> can't or cannot
> 
> "can not" is fine.
> 
> > 
> > > +	 * an extcon device. In the absence of an device it will always return
> > 
> > "a" device,
> 
> Not an issue.
> 
> > > +	 * EPROBE_DEFER.
> > > +	 */
> > > +	if (IS_ENABLED(CONFIG_USB_ROLE_SWITCH) &&
> > > +	    device_property_read_bool(dev, "usb-role-switch"))
> > > +		return NULL;
> > > +
> > >  	/*
> > >  	 * Try to get an extcon device from the USB PHY controller's "port"
> > >  	 * node. Check if it has the "port" node first, to avoid printing the
> > > -- 
> > > 2.37.3
> > > 
> > 
> > Looks like the "port" node check from c824c73a5e08 ("usb: dwc3: drd:
> > Avoid error when extcon is missing") did not account for this platform
> > setup.
> > 
> > This looks fine to me. Should we cleanup the incomplete check and stale
> > comment right after this too (as a separate patch)?
> 
> Is this a Reviewed-by: ?
> 

Please pick up this fix.

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh