Re: [PATCH 7/7] usb: dwc3: qcom: Enable gpio-usb-conn based role-switching

[Bjorn Andersson]

On Wed 11 Mar 12:15 PDT 2020, Bryan O'Donoghue wrote:

> This patch adds the ability to receive a notification from the DRD code for
> role-switch events and in doing so it introduces a disjunction between
> gpio-usb-conn or extcon mode.
> 
> This is what we want to do, since the two methods are mutually exclusive.
> 
> Cc: Andy Gross <agross@xxxxxxxxxx>
> Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Cc: Jack Pham <jackp@xxxxxxxxxxxxxx>
> Cc: linux-arm-msm@xxxxxxxxxxxxxxx
> Cc: linux-usb@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Tested-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 6f4b2b3cffce..f6a7ede5953e 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -571,6 +571,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	struct device		*dev = &pdev->dev;
>  	struct dwc3_qcom	*qcom;
>  	struct resource		*res, *parent_res = NULL;
> +	struct dwc3		*dwc;
>  	int			ret, i;
>  	bool			ignore_pipe_clk;
>  
> @@ -669,8 +670,16 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	if (qcom->mode == USB_DR_MODE_PERIPHERAL)
>  		dwc3_qcom_vbus_overrride_enable(qcom, true);
>  
> -	/* register extcon to override sw_vbus on Vbus change later */
> -	ret = dwc3_qcom_register_extcon(qcom);
> +	if (dwc3_qcom_find_gpio_usb_connector(qcom->dwc3)) {
> +		/* Using gpio-usb-conn register a notifier for VBUS */
> +		dwc = platform_get_drvdata(qcom->dwc3);

As I was testing some other things on my qcs404 board this
suddenly failed.

The of_platform_populate() in dwc3_qcom_of_register_core() will create a
struct platform_device and attempt to probe this. But as my PHY(s) isn't
ready that returns with -EPROBE_DEFER - i.e. it will not reach the
platform_set_drvdata().

The check in dwc3_qcom_of_register_core() successfully resolves the
struct platform_device (it's sitting there waiting to be reprobed
later).

So qcom->dwc3 will be valid, but dwc here will be NULL.

> +		qcom->vbus_nb.notifier_call = dwc3_qcom_vbus_notifier;
> +		ret = dwc3_role_switch_notifier_register(dwc, &qcom->vbus_nb);

So here we pass NULL to dwc3_role_switch_notifier_register(), which
dereferences it and we get an oops.


I don't yet have a sane suggestion on how to redesign the dependency
between the two drivers in order to avoid this, but it's at least not
possible to access the child's state data from dwc3_qcom_probe().

Regards,
Bjorn

> +	} else {
> +		/* register extcon to override sw_vbus on Vbus change later */
> +		ret = dwc3_qcom_register_extcon(qcom);
> +	}
> +
>  	if (ret)
>  		goto depopulate;
>  
> @@ -702,8 +711,11 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
>  {
>  	struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
>  	struct device *dev = &pdev->dev;
> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>  	int i;
>  
> +	dwc3_role_switch_notifier_unregister(dwc, &qcom->vbus_nb);
> +
>  	of_platform_depopulate(dev);
>  
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
> -- 
> 2.25.1
>