linux-kernel - Re: [PATCH v5] usb: host: xhci-plat: Set XHCI_SKIP_PHY

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 9 Feb 2022 11:23:52 +0530
From:   Pavan Kondeti <quic_pkondeti@...cinc.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Sandeep Maheswaram <quic_c_sanm@...cinc.com>,
        Felipe Balbi <balbi@...nel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Doug Anderson <dianders@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_pkondeti@...cinc.com>,
        <quic_ppratap@...cinc.com>
Subject: Re: [PATCH v5] usb: host: xhci-plat: Set XHCI_SKIP_PHY_INIT quirk
 for DWC3 controller

On Tue, Feb 08, 2022 at 11:11:53AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 08, 2022 at 03:34:22PM +0530, Sandeep Maheswaram wrote:
> > Hi Greg,
> > 
> > On 1/27/2022 10:28 AM, Sandeep Maheswaram wrote:
> > > 
> > > On 1/26/2022 5:55 PM, Greg Kroah-Hartman wrote:
> > > > On Fri, Jan 07, 2022 at 10:27:59AM +0530, Sandeep Maheswaram wrote:
> > > > > On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote:
> > > > > > > Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice.
> > > > > > > Runtime suspend of phy drivers was failing from DWC3
> > > > > > > driver as runtime
> > > > > > > usage value is 2 because the phy is initialized from
> > > > > > > DWC3 and HCD core.
> > > > > > > DWC3 manages phy in their core drivers. Set this quirk to avoid phy
> > > > > > > initialization in HCD core.
> > > > > > > 
> > > > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@...cinc.com>
> > > > > > > ---
> > > > > > > v5:
> > > > > > > Added comment to explain the change done.
> > > > > > > v4:
> > > > > > > Changed pdev->dev.parent->of_node to sysdev->of_node
> > > > > > > 
> > > > > > >    drivers/usb/host/xhci-plat.c | 8 ++++++++
> > > > > > >    1 file changed, 8 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/host/xhci-plat.c
> > > > > > > b/drivers/usb/host/xhci-plat.c
> > > > > > > index c1edcc9..e6014d4 100644
> > > > > > > --- a/drivers/usb/host/xhci-plat.c
> > > > > > > +++ b/drivers/usb/host/xhci-plat.c
> > > > > > > @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct
> > > > > > > platform_device *pdev)
> > > > > > >                         &xhci->imod_interval);
> > > > > > >        }
> > > > > > > +    /*
> > > > > > > +     * Set XHCI_SKIP_PHY_INIT quirk to avoid phy
> > > > > > > initialization twice.
> > > > > > > +     * DWC3 manages phy in their core drivers. Set this
> > > > > > > quirk to avoid phy
> > > > > > > +     * initialization in HCD core.
> > > > > > > +     */
> > > > > > > +    if (of_device_is_compatible(sysdev->of_node, "snps,dwc3"))
> > > > > > > +        xhci->quirks |= XHCI_SKIP_PHY_INIT;
> > > > > > > +
> > > > > > Why is this function caring about dwc3 stuff?  Shoudn't this be a
> > > > > > "generic" device property instead of this device-specific one?
> > > > > > 
> > > > > > thanks,
> > > > > > 
> > > > > > greg k-h
> > > > > This quirk is set only if required for some controllers (eg:
> > > > > dwc3 & cdns3).
> > > > > 
> > > > > Please check below commit.
> > > > > 
> > > > > https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/
> > > > > 
> > > > That commit has nothing to do with a specific "dwc3" quirk anywhere.
> > > > Why not set this flag in the specific platform xhci driver instead where
> > > > it belongs?
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > There is no specific xhci platform driver for dwc3 controllers.
> > > 
> > > dwc3 controllers use xhci-plat driver .
> > > 
> > > We can add this quirk in usb/dwc3/host.c as cdns3 does but that requires
> > > tying dwc3 and xhci driver .
> > > 
> > > https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/
> > > 
> > > 
> > > Regards
> > > 
> > > Sandeep
> > > 
> > > 
> > Can you suggest any other method to set this quirk for dwc3 controllers.
> 
> No idea, sorry.

Sandeep,

I agree with Greg's comments here. The compatible based check to detect dwc3
controller is not elegant. Your proposal of adding a device tree param is
overkill, I believe.

Greg already gave us a pointer here [1] which I feel is the best approach going
forward. We know that xhci-plat is being used by drivers like dwc3, cdns3 and
these drivers need to expose their xhci quirks. As Greg suggested, why can't
we move xhci quirks definition to include/linux/usb/xhci-quriks.h and directly
access from the glue drivers? The attached is the patch (completely untested)
for your reference. It will prepare the setup for you to add the private data
and quirks in the dwc3 host glue driver.

Thanks,
Pavan

[1]
https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/



View attachment "0001-usb-xhci-refactor-quirks-and-plat-private-data.patch" of type "text/x-diff" (9328 bytes)