linux-kernel - Re: [PATCH v9 04/10] usb: dwc3: core: Skip setting event buffers for host only controllers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 26 Jun 2023 23:34:28 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Felipe Balbi <balbi@...nel.org>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        Johan Hovold <johan@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "quic_pkondeti@...cinc.com" <quic_pkondeti@...cinc.com>,
        "quic_ppratap@...cinc.com" <quic_ppratap@...cinc.com>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>,
        "quic_harshq@...cinc.com" <quic_harshq@...cinc.com>,
        "ahalaney@...hat.com" <ahalaney@...hat.com>,
        "quic_shazhuss@...cinc.com" <quic_shazhuss@...cinc.com>
Subject: Re: [PATCH v9 04/10] usb: dwc3: core: Skip setting event buffers for
 host only controllers

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@...nel.org>
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
> > > ---
> > >   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);

--

Thanks,
Thinh