linux-kernel - Re: [PATCH v1 1/4] usb: ehci-exynos: Use devm_clk_get

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: <aee270dc-80b2-48c1-a7cb-b1692b5a7677@wanadoo.fr>
Date: Sat, 2 Mar 2024 19:41:15 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Anand Moon <linux.amoon@...il.com>
Cc: Alan Stern <stern@...land.harvard.edu>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Alim Akhtar <alim.akhtar@...sung.com>, linux-usb@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/4] usb: ehci-exynos: Use devm_clk_get_enabled()
 helpers

Le 02/03/2024 à 17:35, Anand Moon a écrit :
> Hi Christophe,
> 
> On Sat, 2 Mar 2024 at 21:19, Christophe JAILLET
> <christophe.jaillet@...adoo.fr> wrote:
>>
>> Le 01/03/2024 à 20:38, Anand Moon a écrit :
>>> The devm_clk_get_enabled() helpers:
>>>       - call devm_clk_get()
>>>       - call clk_prepare_enable() and register what is needed in order to
>>>        call clk_disable_unprepare() when needed, as a managed resource.
>>>
>>> This simplifies the code and avoids the calls to clk_disable_unprepare().
>>>
>>> While at it, use dev_err_probe consistently, and use its return value
>>> to return the error code.
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@...il.com>
>>> ---
>>>    drivers/usb/host/ehci-exynos.c | 30 +++++-------------------------
>>>    1 file changed, 5 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
>>> index f644b131cc0b..05aa3d9c2a3b 100644
>>> --- a/drivers/usb/host/ehci-exynos.c
>>> +++ b/drivers/usb/host/ehci-exynos.c
>>> @@ -159,19 +159,12 @@ static int exynos_ehci_probe(struct platform_device *pdev)
>>>
>>>        err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci);
>>>        if (err)
>>> -             goto fail_clk;
>>> -
>>> -     exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost");
>>> -
>>> -     if (IS_ERR(exynos_ehci->clk)) {
>>> -             dev_err(&pdev->dev, "Failed to get usbhost clock\n");
>>> -             err = PTR_ERR(exynos_ehci->clk);
>>> -             goto fail_clk;
>>> -     }
>>> +             goto fail_io;
>>>
>>> -     err = clk_prepare_enable(exynos_ehci->clk);
>>> -     if (err)
>>> -             goto fail_clk;
>>> +     exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost");
>>> +     if (IS_ERR(exynos_ehci->clk))
>>> +             return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk),
>>> +                               "Failed to get usbhost clock\n");
>>>
>>>        hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>>>        if (IS_ERR(hcd->regs)) {
>>> @@ -223,8 +216,6 @@ static int exynos_ehci_probe(struct platform_device *pdev)
>>>        exynos_ehci_phy_disable(&pdev->dev);
>>>        pdev->dev.of_node = exynos_ehci->of_node;
>>>    fail_io:
>>> -     clk_disable_unprepare(exynos_ehci->clk);
>>> -fail_clk:
>>>        usb_put_hcd(hcd);
>>>        return err;
>>>    }
>>> @@ -240,8 +231,6 @@ static void exynos_ehci_remove(struct platform_device *pdev)
>>>
>>>        exynos_ehci_phy_disable(&pdev->dev);
>>>
>>> -     clk_disable_unprepare(exynos_ehci->clk);
>>> -
>>>        usb_put_hcd(hcd);
>>>    }
>>>
>>> @@ -249,7 +238,6 @@ static void exynos_ehci_remove(struct platform_device *pdev)
>>>    static int exynos_ehci_suspend(struct device *dev)
>>>    {
>>>        struct usb_hcd *hcd = dev_get_drvdata(dev);
>>> -     struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
>>>
>>>        bool do_wakeup = device_may_wakeup(dev);
>>>        int rc;
>>> @@ -260,25 +248,17 @@ static int exynos_ehci_suspend(struct device *dev)
>>>
>>>        exynos_ehci_phy_disable(dev);
>>>
>>> -     clk_disable_unprepare(exynos_ehci->clk);
>>
>> Hi,
>>
>> I don't think that removing clk_[en|dis]abble from the suspend and
>> resume function is correct.
>>
>> The goal is to stop some hardware when the system is suspended, in order
>> to save some power.
> Yes correct,
>>
>> Why did you removed it?
>>
> 
> devm_clk_get_enabled  function register callback for clk_prepare_enable
> and clk_disable_unprepare, so when the clock resource is not used it should get
> disabled.

Same explanation as in the other patch.

The registered function is called when the driver is *unloaded*, not 
when it magically knows that some things can be disabled or enabled.

CJ

> 
> [0] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-devres.c#L75
> 
> I have also tested with rtc suspend & resume and did not find any issue.
> 
>> CJ
> 
> Thanks
> -Anand
>>
>>> -
>>>        return rc;
>>>    }
>>>
>>>    static int exynos_ehci_resume(struct device *dev)
>>>    {
>>>        struct usb_hcd *hcd = dev_get_drvdata(dev);
>>> -     struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
>>>        int ret;
>>>
>>> -     ret = clk_prepare_enable(exynos_ehci->clk);
>>> -     if (ret)
>>> -             return ret;
>>> -
>>>        ret = exynos_ehci_phy_enable(dev);
>>>        if (ret) {
>>>                dev_err(dev, "Failed to enable USB phy\n");
>>> -             clk_disable_unprepare(exynos_ehci->clk);
>>>                return ret;
>>>        }
>>>
>>
> 
>