On Mon, Jul 03, 2023, Krishna Kurapati PSSNV wrote:
>
>
> On 6/27/2023 5:16 AM, Thinh Nguyen wrote:
> > On Mon, Jun 26, 2023, Thinh Nguyen wrote:
> > > On Sat, Jun 24, 2023, Krishna Kurapati PSSNV wrote:
> > > >
> > > >
> > > > On 6/24/2023 3:57 AM, Thinh Nguyen wrote:
> > > > > On Wed, Jun 21, 2023, Krishna Kurapati wrote:
> > > > > > On some SoC's like SA8295P where the tertiary controller is host-only
> > > > > > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible.
> > > > > > Trying to access them leads to a crash.
> > > > > >
> > > > > > For DRD/Peripheral supported controllers, event buffer setup is done
> > > > > > again in gadget_pullup. Skip setup or cleanup of event buffers if
> > > > > > controller is host-only capable.
> > > > > >
> > > > > > Suggested-by: Johan Hovold <johan@xxxxxxxxxx>
> > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/usb/dwc3/core.c | 11 +++++++++++
> > > > > > 1 file changed, 11 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > > index 32ec05fc242b..e1ebae54454f 100644
> > > > > > --- a/drivers/usb/dwc3/core.c
> > > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > > @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc)
> > > > > > static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length)
> > > > > > {
> > > > > > struct dwc3_event_buffer *evt;
> > > > > > + unsigned int hw_mode;
> > > > > > +
> > > > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > > > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST)
> > > > > > + return 0;
> > > > >
> > > > > This is a little awkward. Returning 0 here indicates that this function
> > > > > was successful, and the event buffers were allocated based on the
> > > > > function name. Do this check outside of dwc3_alloc_one_event_buffer()
> > > > > and specifically set dwc->ev_buf = NULL if that's the case.
> > > > >
> > > > Hi Thinh,
> > > >
> > > > Apologies, I didn't understand the comment properly.
> > > >
> > > > I thought we were supposed to return 0 here if we fulfill the goal of this
> > > > function (allocate if we are drd/gadget and don't allocate if we are host
> > > > mode only).
> > >
> > > The name of the function implies that it returns 0 if it allocated the
> > > event buffer. If there are multiple conditions to the function returning
> > > 0 here, then we should document it.
> > >
> > > >
> > > > If we return a non zero error here, probe would fail as this call will be
> > > > done only for host only controllers during probe and nowhere else.
> > > >
> > > > Are you suggesting we move this check to dwc3_alloc_one_event_buffer call
> > > > ?
> > > >
> > > > Regards,
> > > > Krishna,
> > > >
> > >
> > > I'm suggesting to move the check to the caller of
> > > dwc3_alloc_one_event_buffer(). Something like this so that it's
> > > self-documented:
> > >
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 0beaab932e7d..bba82792bf6f 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -1773,6 +1773,7 @@ static int dwc3_probe(struct platform_device *pdev)
> > > struct resource *res, dwc_res;
> > > void __iomem *regs;
> > > struct dwc3 *dwc;
> > > + unsigned int hw_mode;
> > > int ret;
> > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
> > > @@ -1854,11 +1855,16 @@ static int dwc3_probe(struct platform_device *pdev)
> > > pm_runtime_forbid(dev);
> > > - ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
> > > - if (ret) {
> > > - dev_err(dwc->dev, "failed to allocate event buffers\n");
> > > - ret = -ENOMEM;
> > > - goto err_allow_rpm;
> > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
> > > + dwc->ev_buf = NULL;
> > > + } else {
> > > + ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
> > > + if (ret) {
> > > + dev_err(dwc->dev, "failed to allocate event buffers\n");
> > > + ret = -ENOMEM;
> > > + goto err_allow_rpm;
> > > + }
> > > }
> > > dwc->edev = dwc3_get_extcon(dwc);
> > >
> >
> > Actually, please ignore. there's already a document there, I missed that
> > for some reason. What you did is fine. Though, I don't see the condition
> > for ev_buf = NULL anywhere. Can you add that for clarity?
> >
> > Thanks,
> > Thinh
>
> Hi Thinh,
>
> Did you mean adding "dwc->ev_buf = NULL" specifically ?
>
Yes, please initialize dwc->ev_buf to NULL somewhere if it's host mode.
Thanks,
Thinh
