From: Johan Hovold <johan@kernel.org>
To: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Cc: Bjorn Andersson <andersson@kernel.org>,
Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Philipp Zabel <p.zabel@pengutronix.de>,
Andy Gross <agross@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Felipe Balbi <balbi@kernel.org>,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
quic_pkondeti@quicinc.com, quic_ppratap@quicinc.com,
quic_wcheng@quicinc.com, quic_jackp@quicinc.com,
quic_harshq@quicinc.com, ahalaney@redhat.com
Subject: Re: [PATCH v8 6/9] usb: dwc3: qcom: Add multiport controller support for qcom wrapper
Date: Wed, 17 May 2023 18:37:30 +0200 [thread overview]
Message-ID: <ZGUCykpDFt9zgeTU@hovoldconsulting.com> (raw)
In-Reply-To: <bc347624-4539-4a3a-9399-9b4e272cdb32@quicinc.com>
On Tue, May 16, 2023 at 07:49:14AM +0530, Krishna Kurapati PSSNV wrote:
>
>
> On 5/16/2023 3:57 AM, Bjorn Andersson wrote:
> > On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> >> -#define PWR_EVNT_IRQ_STAT_REG 0x58
> >> +#define PWR_EVNT_IRQ1_STAT_REG 0x58
> >> +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc
> >> +#define PWR_EVNT_IRQ3_STAT_REG 0x228
> >> +#define PWR_EVNT_IRQ4_STAT_REG 0x238
> >> #define PWR_EVNT_LPM_IN_L2_MASK BIT(4)
> >> #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5)
> >>
> >> @@ -93,6 +96,13 @@ struct dwc3_qcom {
> >> struct icc_path *icc_path_apps;
> >> };
> >>
> >> +static u32 pwr_evnt_irq_stat_reg_offset[4] = {
> >> + PWR_EVNT_IRQ1_STAT_REG,
> >> + PWR_EVNT_IRQ2_STAT_REG,
> >> + PWR_EVNT_IRQ3_STAT_REG,
> >> + PWR_EVNT_IRQ4_STAT_REG,
> >
> > Seems to be excessive indentation of these...
> >
> > Can you also please confirm that these should be counted starting at 1 -
> > given that you otherwise talk about port0..N-1?
> I am fine with either way. Since this just denoted 4 different ports,
> I named them starting with 1. Either ways, we will run through array
> from (0-3), so we must be fine.
Actually, the USB ports are indexed from 1, so the above naming may or
may not be correct depending on how they are defined.
> >> +};
> >> +
> >> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> >> {
> >> u32 reg;
> >> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> >> {
> >> u32 val;
> >> int i, ret;
> >> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >>
> >> if (qcom->is_suspended)
> >> return 0;
> >>
> >> - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> >> - if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> >> - dev_err(qcom->dev, "HS-PHY not in L2\n");
> >> + for (i = 0; i < dwc->num_usb2_ports; i++) {
> >
> > In the event that the dwc3 core fails to acquire or enable e.g. clocks
> > its drvdata will be NULL. If you then hit a runtime pm transition in the
> > dwc3-qcom glue you will dereference NULL here. (You can force this issue
> > by e.g. returning -EINVAL from dwc3_clk_enable()).
> >
> > So if you're peaking into qcom->dwc3 you need to handle the fact that
> > dwc might be NULL, here and in resume below.
> >
> Thanks for catching this. You are right, there were instances where the
> we saw probe for dwc3 being deferred while the probe for dwc3-qcom was
> still successful [1]. In this case, if the dwc3 probe never happened and
> system tries to enter suspend, we might hit a NULL pointer dereference.
I don't think we should be adding more of these layering violations. A
parent device driver has no business messing with the driver data for a
child device which may or may not even have probed yet.
I added a FIXME elsewhere in the driver about fixing up the current
instances that have already snuck in (which in some sense is even worse
by accessing driver data of a grandchild device).
We really need to try sort this mess out and how to properly handle the
interactions between these layers (e.g. glue, dwc3 core and xhci). This
will likely involve adding callbacks from the child to the parent, for
example, when the child is suspending.
Johan
next prev parent reply other threads:[~2023-05-17 16:37 UTC|newest]
Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-14 5:49 [PATCH v8 0/9] Add multiport support for DWC3 controllers Krishna Kurapati
2023-05-14 5:49 ` [PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport Krishna Kurapati
2023-05-14 9:46 ` Krzysztof Kozlowski
2023-05-16 10:59 ` Johan Hovold
2023-05-17 11:10 ` Krishna Kurapati PSSNV
2023-05-17 11:44 ` Johan Hovold
2023-05-17 12:19 ` Krishna Kurapati PSSNV
2023-05-17 12:55 ` Johan Hovold
2023-05-14 5:49 ` [PATCH v8 2/9] dt-bindings: usb: Add bindings for multiport properties on DWC3 controller Krishna Kurapati
2023-05-14 5:49 ` [PATCH v8 3/9] usb: dwc3: core: Access XHCI address space temporarily to read port info Krishna Kurapati
2023-05-15 21:08 ` Bjorn Andersson
2023-05-16 2:12 ` Krishna Kurapati PSSNV
2023-05-16 22:39 ` Thinh Nguyen
2023-05-16 12:11 ` Johan Hovold
2023-05-16 15:02 ` Krishna Kurapati PSSNV
2023-05-17 3:10 ` Krishna Kurapati PSSNV
2023-05-17 3:21 ` Thinh Nguyen
2023-05-17 7:46 ` Johan Hovold
2023-05-17 23:21 ` Thinh Nguyen
2023-06-07 11:56 ` Johan Hovold
2023-05-17 7:35 ` Johan Hovold
2023-05-17 12:21 ` Krishna Kurapati PSSNV
2023-05-17 15:10 ` Johan Hovold
2023-05-14 5:49 ` [PATCH v8 4/9] usb: dwc3: core: Skip setting event buffers for host only controllers Krishna Kurapati
2023-05-15 21:19 ` Bjorn Andersson
2023-05-16 12:17 ` Johan Hovold
2023-05-16 14:28 ` Krishna Kurapati PSSNV
2023-05-14 5:49 ` [PATCH v8 5/9] usb: dwc3: core: Refactor PHY logic to support Multiport Controller Krishna Kurapati
2023-05-15 21:47 ` Bjorn Andersson
2023-05-16 2:31 ` Krishna Kurapati PSSNV
2023-05-17 16:17 ` Johan Hovold
2023-05-14 5:49 ` [PATCH v8 6/9] usb: dwc3: qcom: Add multiport controller support for qcom wrapper Krishna Kurapati
2023-05-15 22:27 ` Bjorn Andersson
2023-05-16 2:19 ` Krishna Kurapati PSSNV
2023-05-17 16:37 ` Johan Hovold [this message]
2023-05-20 17:48 ` Krishna Kurapati PSSNV
2023-06-07 11:37 ` Johan Hovold
2023-06-07 19:51 ` Krishna Kurapati PSSNV
2023-06-08 9:42 ` Johan Hovold
2023-06-08 15:23 ` Krishna Kurapati PSSNV
2023-06-08 17:57 ` Thinh Nguyen
2023-06-09 8:18 ` Johan Hovold
2023-06-09 18:16 ` Thinh Nguyen
2023-06-15 4:20 ` Krishna Kurapati PSSNV
2023-06-15 21:08 ` Thinh Nguyen
2023-06-21 7:38 ` Johan Hovold
2023-06-22 4:39 ` Krishna Kurapati PSSNV
2023-06-21 7:34 ` Johan Hovold
2023-06-22 22:41 ` Thinh Nguyen
2023-05-26 2:55 ` Bjorn Andersson
2023-05-26 15:25 ` Krishna Kurapati PSSNV
2023-06-07 11:44 ` Johan Hovold
2023-06-07 19:55 ` Krishna Kurapati PSSNV
2023-06-08 9:44 ` Johan Hovold
2023-06-07 12:16 ` Johan Hovold
2023-06-27 15:43 ` Johan Hovold
2023-07-02 19:05 ` Krishna Kurapati PSSNV
2023-07-14 9:00 ` Johan Hovold
2023-07-14 10:38 ` Krishna Kurapati PSSNV
2023-07-21 11:16 ` Johan Hovold
2023-07-21 12:10 ` Konrad Dybcio
2023-07-21 12:54 ` Johan Hovold
2023-08-11 16:48 ` Konrad Dybcio
2023-08-12 8:58 ` Krishna Kurapati PSSNV
2023-05-14 5:49 ` [PATCH v8 7/9] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280 Krishna Kurapati
2023-05-15 14:26 ` Johan Hovold
2023-05-15 15:32 ` Krishna Kurapati PSSNV
2023-05-16 10:54 ` Johan Hovold
2023-05-16 14:24 ` Krishna Kurapati PSSNV
2023-05-16 14:42 ` Johan Hovold
2023-05-16 14:44 ` Krishna Kurapati PSSNV
2023-05-14 5:49 ` [PATCH v8 8/9] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports Krishna Kurapati
2023-05-14 5:49 ` [PATCH v8 9/9] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller Krishna Kurapati
2023-05-15 2:40 ` [PATCH v8 0/9] Add multiport support for DWC3 controllers Bjorn Andersson
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=ZGUCykpDFt9zgeTU@hovoldconsulting.com \
--to=johan@kernel.org \
--cc=Thinh.Nguyen@synopsys.com \
--cc=agross@kernel.org \
--cc=ahalaney@redhat.com \
--cc=andersson@kernel.org \
--cc=balbi@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski+dt@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=quic_harshq@quicinc.com \
--cc=quic_jackp@quicinc.com \
--cc=quic_kriskura@quicinc.com \
--cc=quic_pkondeti@quicinc.com \
--cc=quic_ppratap@quicinc.com \
--cc=quic_wcheng@quicinc.com \
--cc=robh+dt@kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).