Re: [PATCH v8 6/9] usb: dwc3: qcom: Add multiport controller support for qcom wrapper

[Krishna Kurapati PSSNV]

On 5/16/2023 3:57 AM, Bjorn Andersson wrote:
> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> > QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS
> > ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's
> > for all the ports during suspend/resume.
> >
> > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> > ---
> >   drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------
> >   1 file changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 959fc925ca7c..7a9bce66295d 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -37,7 +37,10 @@
> >   #define PIPE3_PHYSTATUS_SW			BIT(3)
> >   #define PIPE_UTMI_CLK_DIS			BIT(8)
> >   
> > -#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?
Hi Bjorn,

  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.

> > +};
> > +
> >   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 will fix this in v9.

[1]: 
https://patchwork.kernel.org/project/linux-usb/patch/1657809067-25815-1-git-send-email-quic_kriskura@xxxxxxxxxxx/

Regards,
Krishna,