linux-kernel - Re: [RFC v4 3/5] usb: dwc3: core: Do not setup 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]

Message-ID: <20230119023216.q73djy4zdolhg325@synopsys.com>
Date:   Thu, 19 Jan 2023 02:32:21 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Jack Pham <quic_jackp@...cinc.com>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Krishna Kurapati <quic_kriskura@...cinc.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>,
        "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_wcheng@...cinc.com" <quic_wcheng@...cinc.com>,
        "quic_harshq@...cinc.com" <quic_harshq@...cinc.com>
Subject: Re: [RFC v4 3/5] usb: dwc3: core: Do not setup event buffers for host
 only controllers

On Wed, Jan 18, 2023, Jack Pham wrote:
> Hi Thinh,
> 
> On Thu, Jan 19, 2023 at 12:38:51AM +0000, Thinh Nguyen wrote:
> > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > Multiport controllers being host-only capable do not have GEVNTADDR
> 
> Multiport may not be relevant here.  Host-only is though.
> 
> > > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event
> > 
> > I think you should reword "present" to something else. They're still
> > present
> 
> In our case we have an instance where the IP is statically configured
> via coreConsultant with DWC_USB31_MODE==1 (host only) and we did observe
> that none of the registers pertaining to device mode (including GEVNT*
> and of course all the D* ones) are even *present* in the register map.
> If we try to access them we encounter some kind of access error or stall
> (or translation fault as described).  So the approach here is to first
> verify by checking the HWPARAMS0 register if the HW is even capable of
> device mode in the first place.

I see.

> 
> > but those registers are to be set while operating in device
> > mode. The rest looks fine.
> 
> Are you suggesting only touching the GEVNT* registers when *operating*
> in device mode, even in the case of a dual-role capable controller?  In
> that case would it make more sense to additionally move the calls to
> dwc3_event_buffers_{setup,cleanup} out of core.c and into
> dwc3_gadget_{init,exit} perhaps?  That way we avoid them completely

While it shouldn't be a problem for DRD, it may be cleaner to do that.

> unless and until we switch into peripheral mode (assuming controller
> supports that, which we should already have checks for).  Moreover, if
> the devicetree dr_mode property is set to host-only we'd also avoid
> calling these.
> 
> > > buffers during core_init can cause an SMMU Fault. Avoid event buffers
> > > setup if the GHWPARAMS0 tells that the controller is host-only.
> > > 
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
> > > ---
> > >  drivers/usb/dwc3/core.c | 23 +++++++++++++++--------
> > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 7e0a9a598dfd..f61ebddaecc0 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
> > >  
> > >  static void dwc3_core_exit(struct dwc3 *dwc)
> > >  {
> > > -	int i;
> > > +	int		i;
> > > +	unsigned int	hw_mode;
> > >  
> > > -	dwc3_event_buffers_cleanup(dwc);
> > > +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > > +	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST)
> 
> If we stick with this approach, we probably could just check
> dwc->dr_mode instead as probe should have already set that to be an
> intersection between the values given in devicetree "dr_mode" and the
> HWPARAMS0 capability.
> 

What we have here should not break DRD, so it's fine either way.

Thanks,
Thinh