| 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
| ||
|
Message-ID: <20200312204935.GF144492@google.com>
Date: Thu, 12 Mar 2020 13:49:35 -0700
From: Matthias Kaehlcke <mka@...omium.org>
To: Pradeep P V K <ppvk@...eaurora.org>
Cc: bjorn.andersson@...aro.org, adrian.hunter@...el.com,
robh+dt@...nel.org, ulf.hansson@...aro.org,
asutoshd@...eaurora.org, stummala@...eaurora.org,
sayalil@...eaurora.org, rampraka@...eaurora.org,
vbadigan@...eaurora.org, sboyd@...nel.org,
georgi.djakov@...aro.org, linux-mmc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, agross@...nel.org,
linux-mmc-owner@...r.kernel.org,
Subhash Jadavani <subhashj@...eaurora.org>
Subject: Re: [RFC v5 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth
scaling support
Hi Pradeep,
On Thu, Mar 12, 2020 at 11:31:50AM +0530, Pradeep P V K wrote:
> Add interconnect bandwidths for SDHC driver using OPP framework that
> is required by SDHC driver based on the clock frequency and bus width
> of the card. Otherwise, the system clocks may run at minimum clock
> speed and thus affecting the performance.
>
> This change is based on
> [RFC] mmc: host: sdhci-msm: Use the interconnect API
> (https://lkml.org/lkml/2018/10/11/499) and
>
> [PATCH v6] Introduce Bandwidth OPPs for interconnects
> (https://lkml.org/lkml/2019/12/6/740)
>
> Co-developed-by: Sahitya Tummala <stummala@...eaurora.org>
> Signed-off-by: Sahitya Tummala <stummala@...eaurora.org>
> Co-developed-by: Subhash Jadavani <subhashj@...eaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@...eaurora.org>
> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@...eaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@...eaurora.org>
> Co-developed-by: Pradeep P V K <ppvk@...eaurora.org>
> Signed-off-by: Pradeep P V K <ppvk@...eaurora.org>
> ---
>
> RFC v4 -> v5:
> - Rewrote the icc interconnect get handlers and its error handling
> and allocated vote data after handling all icc get handler errors.
> - Removed explicit error check on ICC handlers.
> - Addressed minor code style comments.
>
> drivers/mmc/host/sdhci-msm.c | 231 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 227 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 09ff731..5fe8fad 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
>
> ...
>
> +/*
> + * This function sets the interconnect bus bandwidth
> + * vote based on bw (bandwidth) argument.
> + */
> +#define BUS_INTERCONNECT_PATHS 2 /* 1. sdhc -> ddr 2. cpu -> sdhc */
> +static void sdhci_msm_bus_set_vote(struct sdhci_host *host,
> + unsigned int bw)
> +{
> + int i, err;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> + struct sdhci_msm_bus_vote_data *vote_data = msm_host->bus_vote_data;
> + struct device *dev = &msm_host->pdev->dev;
> + struct dev_pm_opp *opp;
> + unsigned long freq = bw;
> + unsigned long peak_bw[BUS_INTERCONNECT_PATHS] = {0};
> + unsigned long avg_bw[BUS_INTERCONNECT_PATHS] = {0};
> +
> + if (bw == vote_data->curr_freq)
> + return;
> +
> + for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
> + opp = sdhci_msm_find_opp_for_freq(msm_host, freq);
> + if (opp) {
> + avg_bw[i] = dev_pm_opp_get_bw(opp, &peak_bw[i]);
> + freq += 1; /* Next bandwidth vote */
> + dev_pm_opp_put(opp);
> + }
> + }
> + pr_debug("%s: freq:%d sdhc_to_ddr avg_bw:%lu peak_bw:%lu cpu_to_sdhc avg_bw:%lu peak_bw:%lu\n",
> + mmc_hostname(host->mmc), bw, avg_bw[0], peak_bw[0],
> + avg_bw[1], peak_bw[1]);
> + err = icc_set_bw(vote_data->sdhc_to_ddr, 0, peak_bw[0]);
> + if (err) {
> + dev_err(dev, "icc_set() failed for 'sdhc-ddr' path err:%d\n",
> + err);
nit: the alignment is odd, either align with 'dev' or a tab after 'dev_err'
> + return;
> + }
> + err = icc_set_bw(vote_data->cpu_to_sdhc, 0, peak_bw[1]);
> + if (err) {
> + dev_err(dev, "icc_set() failed for 'cpu-sdhc' path err:%d\n",
> + err);
ditto
> + return;
> + }
> + vote_data->curr_freq = bw;
> +}
> +
> +/*
> + * Helper function to register for OPP and interconnect
> + * frameworks.
> + */
> +static struct sdhci_msm_bus_vote_data
> + *sdhci_msm_bus_register(struct sdhci_msm_host *host,
> + struct platform_device *pdev)
> +{
> + struct sdhci_msm_bus_vote_data *vote_data;
> + struct device *dev = &pdev->dev;
> + int ret, i, err;
nit: you can get rid of 'ret' and use 'err' instead.
> + struct icc_path *icc_paths[BUS_INTERCONNECT_PATHS];
> + const char *path_names[] = {
> + "sdhc-ddr",
> + "cpu-sdhc",
> + };
> +
> + for (i = 0; i < BUS_INTERCONNECT_PATHS; i++)
> + icc_paths[i] = of_icc_get(&pdev->dev, path_names[i]);
> +
> + if (!icc_paths[0] && !icc_paths[1]) {
> + dev_info(&pdev->dev, "ICC DT property is missing.Skip vote!!\n");
> + return NULL;
> + }
> +
> + for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
> + if (!icc_paths[i]) {
> + dev_err(&pdev->dev, "interconnect path '%s' is not configured\n",
> + path_names[i]);
> + err = -EINVAL;
> + goto handle_err;
> + }
> + if (IS_ERR(icc_paths[i])) {
> + err = PTR_ERR(icc_paths[i]);
> +
> + if (err != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "interconnect path '%s' is invalid:%d\n",
> + path_names[i], err);
> + goto handle_err;
> + }
> + }
> +
> + ret = dev_pm_opp_of_add_table(dev);
> + if (ret) {
> + if (ret == -ENODEV || ret == -ENODATA)
> + dev_err(dev, "OPP dt properties missing:%d\n", ret);
> + else
> + dev_err(dev, "OPP registration failed:%d\n", ret);
need to call icc_put() for the two paths?
> + return ERR_PTR(ret);
> + }
> +
> + vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL);
> + if (!vote_data)
ditto
probably you want to do this with a jump to an error handler below.
> + return ERR_PTR(-ENOMEM);
> +
> + vote_data->sdhc_to_ddr = icc_paths[0];
> + vote_data->cpu_to_sdhc = icc_paths[1];
> + return vote_data;
nit: add empty line
> +handle_err:
> + if (err) {
> + int other = (i == 0) ? 1 : 0;
> +
> + if (!IS_ERR_OR_NULL(icc_paths[other]))
> + icc_put(icc_paths[other]);
> + }
doing this at the end (as opposed to my suggestion from
https://patchwork.kernel.org/patch/11388409/#23165321) has the advantage of
keeping the above loop cleaner from error handling cruft, on the downside it
is probably easier to understand right away in the context of the loop. I
guess you can do it either way.
It might get a bit more messy if you also handle the case where both paths are
valid. If that gets too involved I'd suggest to hnadle the above case inside
the loop.
> + return ERR_PTR(err);
> +}
> +
> +static void sdhci_msm_bus_unregister(struct device *dev,
> + struct sdhci_msm_host *host)
> +{
> + struct sdhci_msm_bus_vote_data *vote_data = host->bus_vote_data;
> +
> + if (IS_ERR_OR_NULL(vote_data))
I think 'if (!vote_data)' would be sufficient, since _probe() aborts in
case of an error.
> + return;
> +
> + icc_put(vote_data->sdhc_to_ddr);
> + icc_put(vote_data->cpu_to_sdhc);
> +}
> +
> +static void sdhci_msm_bus_voting(struct sdhci_host *host, bool enable)
> +{
> + struct mmc_ios *ios = &host->mmc->ios;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> + unsigned int bw;
> +
> + if (IS_ERR_OR_NULL(msm_host->bus_vote_data))
> + return;
ditto
Powered by blists - more mailing lists