| 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: <20200323184936.GC204494@google.com>
Date: Mon, 23 Mar 2020 11:49:36 -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 v6 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth
scaling support
Hi Pradeep,
On Mon, Mar 23, 2020 at 07:55:01PM +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 v5 -> v6:
> - Added new goto jump tag to put both icc paths.
> - Removed bus vote data error check and added is_null check.
> - Addressed minor code style comments.
>
> drivers/mmc/host/sdhci-msm.c | 240 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 236 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 09ff731..469db65 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
>
> ...
>
> +/*
> + * Helper function to parse the exact OPP node
> + * Returns OPP pointer on success else NULL on error
> + */
> +static struct dev_pm_opp
> + *sdhci_msm_find_opp_for_freq(struct sdhci_msm_host *msm_host,
> + unsigned long bw)
> +{
> + struct dev_pm_opp *opp;
> + struct sdhci_host *host = mmc_priv(msm_host->mmc);
> + unsigned int freq = bw;
> + struct device *dev = &msm_host->pdev->dev;
> +
> +
delete one empty line
> + if (!freq)
> + opp = dev_pm_opp_find_peak_bw_floor(dev, &freq);
> + else
> + opp = dev_pm_opp_find_peak_bw_exact(dev, freq, true);
> +
> + /* Max bandwidth vote */
> + if (PTR_ERR(opp) == -ERANGE && freq > sdhci_msm_get_max_clock(host))
> + opp = dev_pm_opp_find_peak_bw_ceil(dev, &bw);
> +
> + if (IS_ERR(opp)) {
> + dev_err(dev, "Failed to find OPP for freq:%u err:%ld\n",
> + freq, PTR_ERR(opp));
> + return NULL;
> + }
> + return opp;
> +}
>
> ...
>
> +/*
> + * 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 i, err;
> + 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;
> + }
> + }
> +
> + err = dev_pm_opp_of_add_table(dev);
> + if (err) {
> + if (err == -ENODEV || err == -ENODATA)
> + dev_err(dev, "OPP dt properties missing:%d\n", err);
> + else
> + dev_err(dev, "OPP registration failed:%d\n", err);
> + goto put_icc;
> + }
> +
> + vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL);
> + if (!vote_data) {
> + err = -ENOMEM;
> + goto put_icc;
> + }
> + vote_data->sdhc_to_ddr = icc_paths[0];
> + vote_data->cpu_to_sdhc = icc_paths[1];
> + return vote_data;
> +
> +handle_err:
> + if (err) {
the check for 'err' is not needed, this code is only executed when an error
is encountered.
> + int other = (i == 0) ? 1 : 0;
> +
> + if (!IS_ERR_OR_NULL(icc_paths[other]))
> + icc_put(icc_paths[other]);
> + }
> + return ERR_PTR(err);
> +
> +put_icc:
> + if (err) {
> + for (i = 0; i < BUS_INTERCONNECT_PATHS; i++)
> + icc_put(icc_paths[i]);
> + }
> + return ERR_PTR(err);
The two error paths are somewhat redundant and the 'handle_err' isn't super
clear, especially since it is disconnected from the code where the error is
found.
You could have a single handler instead:
put_icc:
for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
if (!IS_ERR_OR_NULL(icc_paths[i]))
icc_put(icc_paths[i]);
}
return ERR_PTR(err);
Powered by blists - more mailing lists