linux-kernel - Re: [PATCH v1] USB: dwc3: only call usb_phy_set

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: <GV0P278MB0589921FFF5487D2F94D3FF2E8A4A@GV0P278MB0589.CHEP278.PROD.OUTLOOK.COM>
Date:   Sat, 4 Nov 2023 11:51:22 +0000
From:   Stefan Eichenberger <stefan.eichenberger@...adex.com>
To:     Johan Hovold <johan@...nel.org>,
        Francesco Dolcini <francesco@...cini.it>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <balbi@...com>,
        Kishon Vijay Abraham I <kishon@...com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Francesco Dolcini <francesco.dolcini@...adex.com>
Subject: Re: [PATCH v1] USB: dwc3: only call usb_phy_set_suspend in
 suspend/resume

Hi Johan,

> From: Johan Hovold <johan@...nel.org>
> 
> On Fri, Nov 03, 2023 at 11:22:36AM +0100, Francesco Dolcini wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> >
> > Currently we have the following two call chains:
> > dwc3_probe -> dwc3_core_init -> dwc3_phy_init -> usb_phy_init
> > dwc3_probe -> dwc3_core_init -> dwc3_phy_power_on -> usb_phy_set_suspend
> >
> > If we look at phy-generic we see the following calls:
> > usb_gen_phy_init -> regulator_enable
> > usb_gen_phy_init -> clk_prepare_enable
> >
> > If we call usb_phy_set_suspend we call the following in phy-generic:
> > nop_set_suspend -> clk_prepare_enable
> > and we sent a patch to also call:
> > nop_set_suspend -> regulator_enable
> >
> > Because clk_prepare_enable and regulator_enable do reference counting we
> > increased the reference counter of the clock and regulator to two. If we
> > want to put the system into suspend we only decrease the reference
> > counters by one and therefore the clock and regulator stay on.
> 
> No, this does not seem to be a correct description of the current
> implementation.
> 
> The driver always calls both usb_phy_set_suspend() and
> usb_phy_init()/usb_phy_shutdown() so those usage counters would still be
> balanced (e.g. see dwc3_core_init() and dwc3_core_exit()).
> 

You are right I missunderstood that part.

> When reviewing the driver I did find a bug in the xhci-plat driver which
> is likely the cause for the imbalance you're seeing. I just sent a fix
> here in case you want to give it a try:
> 
>         https://lore.kernel.org/lkml/20231103164323.14294-1-johan+linaro@kernel.org/

I tested it and it solves the issue we have. Thanks a lot for the fix!
Before the use count for our regulator always went up to 2 and now it is
1 as expected.
root@...din-imx8mp-14773241:~# cat /sys/kernel/debug/regulator/CTRL_SLEEP_MOCI#/use_count
1

Also when going to suspend the regulator is turned off now. With the
suspend patch applied from us the use count will be one more but
everything still works as expected.

> 
> But, also, why are you using legacy PHYs? Which platform is this for?

We have an external hub that we want to turn off when the system goes
into suspend. For the i.MX8MM we use the phy-generic driver to achieve
this. When I saw that the dwc3 driver would support the phy-generic via
usb-phy, I thought we could use the same approach for the i.MX8MP and,
in the future, the AM62. Maybe I misunderstood, would the right solution
be to add a suspend function to the fsl,imx8mp-usb-phy driver and use
vbus instead? But what would we do for the AM62, as it doesn't have a
phy driver if I'm not mistaken.

Regards,
Stefan