linux-kernel - Re: [PATCH v8 20/34] mmc: sdhci-tegra: Add runtime PM and OPP support

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: <5a15647c-bc6a-294d-61c8-91efa33c681e@gmail.com>
Date:   Wed, 25 Aug 2021 12:45:01 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Viresh Kumar <vireshk@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        Mikko Perttunen <mperttunen@...dia.com>,
        Peter Chen <peter.chen@...nel.org>,
        Mark Brown <broonie@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        Nishanth Menon <nm@...com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Richard Weinberger <richard@....at>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Lucas Stach <dev@...xeye.de>, Stefan Agner <stefan@...er.ch>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-usb@...r.kernel.org,
        linux-staging@...ts.linux.dev, linux-spi@...r.kernel.org,
        linux-pwm@...r.kernel.org, linux-mtd@...ts.infradead.org,
        linux-mmc@...r.kernel.org, linux-media@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-clk@...r.kernel.org
Subject: Re: [PATCH v8 20/34] mmc: sdhci-tegra: Add runtime PM and OPP support

20.08.2021 14:35, Thierry Reding пишет:
> On Fri, Aug 20, 2021 at 01:37:13AM +0300, Dmitry Osipenko wrote:
>> 19.08.2021 20:03, Thierry Reding пишет:
>>> On Tue, Aug 17, 2021 at 04:27:40AM +0300, Dmitry Osipenko wrote:
>>>> The SDHCI on Tegra belongs to the core power domain and we're going to
>>>> enable GENPD support for the core domain. Now SDHCI must be resumed using
>>>> runtime PM API in order to initialize the SDHCI power state. The SDHCI
>>>> clock rate must be changed using OPP API that will reconfigure the power
>>>> domain performance state in accordance to the rate. Add runtime PM and OPP
>>>> support to the SDHCI driver.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci-tegra.c | 146 ++++++++++++++++++++++++---------
>>>>  1 file changed, 105 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
>>>> index 387ce9cdbd7c..a3583359c972 100644
>>>> --- a/drivers/mmc/host/sdhci-tegra.c
>>>> +++ b/drivers/mmc/host/sdhci-tegra.c
>>>> @@ -15,6 +15,8 @@
>>>>  #include <linux/of.h>
>>>>  #include <linux/of_device.h>
>>>>  #include <linux/pinctrl/consumer.h>
>>>> +#include <linux/pm_opp.h>
>>>> +#include <linux/pm_runtime.h>
>>>>  #include <linux/regulator/consumer.h>
>>>>  #include <linux/reset.h>
>>>>  #include <linux/mmc/card.h>
>>>> @@ -24,6 +26,8 @@
>>>>  #include <linux/gpio/consumer.h>
>>>>  #include <linux/ktime.h>
>>>>  
>>>> +#include <soc/tegra/common.h>
>>>> +
>>>>  #include "sdhci-pltfm.h"
>>>>  #include "cqhci.h"
>>>>  
>>>> @@ -123,6 +127,12 @@
>>>>  					 SDHCI_TRNS_BLK_CNT_EN | \
>>>>  					 SDHCI_TRNS_DMA)
>>>>  
>>>> +enum {
>>>> +	TEGRA_CLK_BULK_SDHCI,
>>>> +	TEGRA_CLK_BULK_TMCLK,
>>>> +	TEGRA_CLK_BULK_NUM,
>>>> +};
>>>> +
>>>>  struct sdhci_tegra_soc_data {
>>>>  	const struct sdhci_pltfm_data *pdata;
>>>>  	u64 dma_mask;
>>>> @@ -171,6 +181,8 @@ struct sdhci_tegra {
>>>>  	bool enable_hwcq;
>>>>  	unsigned long curr_clk_rate;
>>>>  	u8 tuned_tap_delay;
>>>> +
>>>> +	struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM];
>>>
>>> This doesn't seem worth it to me. There's a lot of churn in this driver
>>> that's only needed to convert this to the clk_bulk API and it makes the
>>> code a lot more difficult to read, in my opinion.
>>>
>>> It looks like the only benefit that this gives us is that runtime
>>> suspend and resume become a few lines shorter.
>>
>> The driver probe code looks cleaner with that. You should be looking at
>> the final result and not at the patch to see it.
> 
> I did look at the final result and didn't find it cleaner at all. =)

There is UAF bug in this patch that was spotted by kasan. The
sdhci_tegra_soc_data isn't resource-managed, but clk_bulk_data is. I'm
now thinking that it should be okay to keep tmclk always-on, so I'll
replace the bulk clks back with the sdhci clk in v9.