LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: balbi@kernel.org, gregkh@linuxfoundation.org,
	linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, jackp@codeaurora.org,
	robh@kernel.org, Andy Gross <agross@kernel.org>,
	Lee Jones <lee.jones@linaro.org>,
	Philipp Zabel <p.zabel@pengutronix.de>
Subject: Re: [PATCH 7/7] usb: dwc3: qcom: Enable gpio-usb-conn based role-switching
Date: Mon, 16 Mar 2020 23:31:01 -0700	[thread overview]
Message-ID: <20200317063101.GA2778164@ripper> (raw)
In-Reply-To: <20200311191501.8165-8-bryan.odonoghue@linaro.org>

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@kernel.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Jack Pham <jackp@codeaurora.org>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  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
> 

  reply	other threads:[~2020-03-17  6:32 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-11 19:14 [PATCH 0/7] DWC3/Qualcomm connector " Bryan O'Donoghue
2020-03-11 19:14 ` [PATCH 1/7] usb: dwc3: Registering a role switch in the DRD code Bryan O'Donoghue
2020-03-11 19:14 ` [PATCH 2/7] dt-bindings: usb: dwc3: Add a gpio-usb-connector example Bryan O'Donoghue
2020-03-19  1:08   ` Stephen Boyd
2020-03-19 15:22     ` Bryan O'Donoghue
2020-03-19 16:40       ` Stephen Boyd
2020-03-19 18:03         ` Bryan O'Donoghue
2020-03-19 20:30           ` Stephen Boyd
2020-03-11 19:14 ` [PATCH 3/7] dt-bindings: usb: dwc3: Add a usb-role-switch to the example Bryan O'Donoghue
2020-03-11 19:14 ` [PATCH 4/7] usb: dwc3: qcom: Add support for usb-conn-gpio connectors Bryan O'Donoghue
2020-03-11 19:14 ` [PATCH 5/7] usb: dwc3: " Bryan O'Donoghue
2020-03-11 19:15 ` [PATCH 6/7] usb: dwc3: Add support for a role-switch notifier Bryan O'Donoghue
2020-07-23  8:13   ` Vincent Whitchurch
2020-03-11 19:15 ` [PATCH 7/7] usb: dwc3: qcom: Enable gpio-usb-conn based role-switching Bryan O'Donoghue
2020-03-17  6:31   ` Bjorn Andersson [this message]
2020-03-17 15:22     ` Bryan O'Donoghue

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200317063101.GA2778164@ripper \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@kernel.org \
    --cc=balbi@kernel.org \
    --cc=bryan.odonoghue@linaro.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jackp@codeaurora.org \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=robh@kernel.org \
    --subject='Re: [PATCH 7/7] usb: dwc3: qcom: Enable gpio-usb-conn based role-switching' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).