| 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
| ||
|
Date: Fri, 06 Sep 2019 14:02:50 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: adrian.hunter@...el.com, georgi.djakov@...aro.org,
ppvk@...eaurora.org, robh+dt@...nel.org, ulf.hansson@...aro.org
Cc: asutoshd@...eaurora.org, vbadigan@...eaurora.org,
stummala@...eaurora.org, sayalil@...eaurora.org,
rampraka@...eaurora.org, linux-mmc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org,
Subhash Jadavani <subhashj@...eaurora.org>,
Andy Gross <agross@...nel.org>
Subject: Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting
Quoting ppvk@...eaurora.org (2019-09-06 05:51:54)
> +Georgi Djakov
>
> On 2019-09-06 18:17, Pradeep P V K wrote:
> > diff --git a/drivers/mmc/host/sdhci-msm.c
> > b/drivers/mmc/host/sdhci-msm.c
> > index b75c82d..71515ca 100644
> > --- a/drivers/mmc/host/sdhci-msm.c
> > +++ b/drivers/mmc/host/sdhci-msm.c
> > #define msm_host_writel(msm_host, val, host, offset) \
> > msm_host->var_ops->msm_writel_relaxed(val, host, offset)
> >
> > +#define SDHC_DDR "sdhc-ddr"
> > +#define CPU_SDHC "cpu-sdhc"
Do you really need these defines? They're not used more than once or
twice and just seem to make the code harder to read.
> > +
> > struct sdhci_msm_offset {
> > u32 core_hc_mode;
> > u32 core_mci_data_cnt;
> > @@ -228,6 +232,31 @@ struct sdhci_msm_variant_info {
> > const struct sdhci_msm_offset *offset;
> > };
> >
> > +struct msm_bus_vectors {
> > + uint64_t ab;
> > + uint64_t ib;
> > +};
> > +
> > +struct msm_bus_path {
> > + unsigned int num_paths;
> > + struct msm_bus_vectors *vec;
> > +};
> > +
> > +struct sdhci_msm_bus_vote_data {
> > + const char *name;
> > + unsigned int num_usecase;
> > + struct msm_bus_path *usecase;
> > +
> > + unsigned int *bw_vecs;
> > + unsigned int bw_vecs_size;
> > +
> > + struct icc_path *sdhc_ddr;
> > + struct icc_path *cpu_sdhc;
> > +
> > + uint32_t curr_vote;
> > +
Please use u32 instead of uint32_t. Same comment for u64.
> > +};
> > +
> > struct sdhci_msm_host {
> > struct platform_device *pdev;
> > void __iomem *core_mem; /* MSM SDCC mapped address */
> > @@ -1678,6 +1714,341 @@ static void
> > sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
> > pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
> > }
> >
> > +static int sdhci_msm_dt_get_array(struct device *dev, const char
> > *prop_name,
> > + u32 **bw_vecs, int *len, u32 size)
> > +{
> > + int ret = 0;
> > + struct device_node *np = dev->of_node;
> > + size_t sz;
> > + u32 *arr = NULL;
> > +
> > + if (!of_get_property(np, prop_name, len)) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > + sz = *len = *len / sizeof(*arr);
> > + if (sz <= 0 || (size > 0 && (sz > size))) {
> > + dev_err(dev, "%s invalid size\n", prop_name);
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> > + if (!arr) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + ret = of_property_read_u32_array(np, prop_name, arr, sz);
> > + if (ret < 0) {
> > + dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> > + goto out;
> > + }
> > + *bw_vecs = arr;
> > +out:
> > + if (ret)
> > + *len = 0;
> > + return ret;
> > +}
> > +
> > +/* Returns required bandwidth in Bytes per Sec */
> > +static unsigned long sdhci_get_bw_required(struct sdhci_host *host,
> > + struct mmc_ios *ios)
> > +{
> > + unsigned long bw;
> > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > +
> > + bw = msm_host->clk_rate;
> > +
> > + if (ios->bus_width == MMC_BUS_WIDTH_4)
> > + bw /= 2;
> > + else if (ios->bus_width == MMC_BUS_WIDTH_1)
> > + bw /= 8;
> > +
> > + return bw;
> > +}
> > +
> > +static int sdhci_msm_bus_get_vote_for_bw(struct sdhci_msm_host *host,
> > + unsigned int bw)
> > +{
> > + struct sdhci_msm_bus_vote_data *bvd = host->bus_vote_data;
> > +
> > + unsigned int *table = bvd->bw_vecs;
Should probably be a const bw_vecs pointer so that it can't be modified
after the fact.
> > + unsigned int size = bvd->bw_vecs_size;
> > + int i;
> > +
> > + for (i = 0; i < size; i++) {
> > + if (bw <= table[i])
> > + break;
return i;
> > + }
> > +
return i - 1;
> > + if (i && (i == size))
> > + i--;
> > +
> > + return i;
And then this is useless.....
> > +}
> > +
> > +/*
> > + * This function must be called with host lock acquired.
> > + * Caller of this function should also ensure that msm bus client
> > + * handle is not null.
If it was NULL it would be pretty sad.
> > + */
> > +static inline int sdhci_msm_bus_set_vote(struct sdhci_msm_host
> > *msm_host,
> > + int vote,
> > + unsigned long *flags)
> > +{
> > + struct sdhci_host *host = platform_get_drvdata(msm_host->pdev);
> > + struct sdhci_msm_bus_vote_data *bvd = msm_host->bus_vote_data;
> > + struct msm_bus_path *usecase = bvd->usecase;
> > + struct msm_bus_vectors *vec = usecase[vote].vec;
> > + int ddr_rc = 0, cpu_rc = 0;
Why initialize to 0?
> > +
> > + if (vote != bvd->curr_vote) {
> > + spin_unlock_irqrestore(&host->lock, *flags);
> > + pr_debug("%s: vote:%d sdhc_ddr ab:%llu ib:%llu cpu_sdhc ab:%llu
> > ib:%llu\n",
> > + mmc_hostname(host->mmc), vote, vec[0].ab,
> > + vec[0].ib, vec[1].ab, vec[1].ib);
> > + ddr_rc = icc_set_bw(bvd->sdhc_ddr, vec[0].ab, vec[0].ib);
> > + cpu_rc = icc_set_bw(bvd->cpu_sdhc, vec[1].ab, vec[1].ib);
> > + spin_lock_irqsave(&host->lock, *flags);
This is some tricky spin-lockery.
> > + if (ddr_rc || cpu_rc) {
> > + pr_err("%s: icc_set() failed\n",
> > + mmc_hostname(host->mmc));
> > + goto out;
> > + }
> > + bvd->curr_vote = vote;
> > + }
> > +out:
> > + return cpu_rc;
> > +}
> > +
> > +/*
> > + * Internal work. Work to set 0 bandwidth for msm bus.
> > + */
> > +static void sdhci_msm_bus_work(struct work_struct *work)
> > +{
> > + struct sdhci_msm_host *msm_host;
> > + struct sdhci_host *host;
> > + unsigned long flags;
> > +
> > + msm_host = container_of(work, struct sdhci_msm_host,
> > + bus_vote_work.work);
> > + host = platform_get_drvdata(msm_host->pdev);
> > +
> > + /* Check handle and return */
This comment is useless, please remove it.
> > + if (!msm_host->bus_vote_data->sdhc_ddr ||
> > + !msm_host->bus_vote_data->cpu_sdhc)
> > + return;
> > + spin_lock_irqsave(&host->lock, flags);
> > + /* don't vote for 0 bandwidth if any request is in progress */
> > + if (!host->mmc->ongoing_mrq)
> > + sdhci_msm_bus_set_vote(msm_host, 0, &flags);
> > + else
> > + pr_warn("Transfer in progress.Skipping bus voting to 0\n");
Missing space after the full stop. Also, useless warning so just remove
it?
> > + spin_unlock_irqrestore(&host->lock, flags);
> > +}
> > +
> > +/*
> > + * This function cancels any scheduled delayed work and sets the bus
> > + * vote based on bw (bandwidth) argument.
> > + */
> > +static void sdhci_msm_bus_cancel_work_and_set_vote(struct sdhci_host
> > *host,
> > + unsigned int bw)
> > +{
> > + int vote;
> > + unsigned long flags;
> > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > +
> > + cancel_delayed_work_sync(&msm_host->bus_vote_work);
> > + spin_lock_irqsave(&host->lock, flags);
Why does the lock need to be held? Is the interconnect framework not
consistent with itself?
> > + vote = sdhci_msm_bus_get_vote_for_bw(msm_host, bw);
> > + sdhci_msm_bus_set_vote(msm_host, vote, &flags);
> > + spin_unlock_irqrestore(&host->lock, flags);
> > +}
> > +
> > +
> > +#define MSM_MMC_BUS_VOTING_DELAY 200 /* msecs */
> > +#define VOTE_ZERO 0
> > +
> > +/* This function queues a work which will set the bandwidth requiement
> > to 0 */
This comment style is wrong. Also s/requiement/requirement/
> > +static void sdhci_msm_bus_queue_work(struct sdhci_host *host)
> > +{
> > + unsigned long flags;
> > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > +
> > + spin_lock_irqsave(&host->lock, flags);
> > + if (msm_host->bus_vote_data->curr_vote != VOTE_ZERO)
> > + queue_delayed_work(system_wq,
> > + &msm_host->bus_vote_work,
> > + msecs_to_jiffies(MSM_MMC_BUS_VOTING_DELAY));
> > + spin_unlock_irqrestore(&host->lock, flags);
Ok it seems that the sdhci code isn't consistent with itself?
> > +}
> > +
> > +static struct sdhci_msm_bus_vote_data
> > *sdhci_msm_get_bus_vote_data(struct device
> > + *dev, struct sdhci_msm_host *host)
> > +
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct device_node *of_node = dev->of_node;
> > + struct sdhci_msm_bus_vote_data *bvd = NULL;
> > + struct msm_bus_path *usecase = NULL;
> > + int ret = 0, i = 0, j, k, num_paths, len;
> > + const uint32_t *vec_arr = NULL;
> > + bool mem_err = false;
> > +
> > + if (!pdev) {
> > + dev_err(dev, "Null platform device!\n");
> > + return NULL;
> > + }
> > +
> > + bvd = devm_kzalloc(dev, sizeof(struct sdhci_msm_bus_vote_data),
sizeof(*bvd) is better and shorter!
> > + GFP_KERNEL);
> > + if (!bvd) {
> > + ret = -ENOMEM;
> > + dev_err(dev, "No sufficient memory!\n");
Don't print errors for allocation failures
> > + return bvd;
> > + }
> > + ret = sdhci_msm_dt_get_array(dev, "qcom,bus-bw-vectors-bps",
> > + &bvd->bw_vecs, &bvd->bw_vecs_size, 0);
> > + if (ret) {
> > + dev_info(dev, "No dt property of bus bw. voting defined!\n");
> > + dev_info(dev, "Skipping Bus BW voting now!!\n");
Is this debug junk? Why does the user care?
> > + host->skip_bus_bw_voting = true;
> > + if (ret != -EINVAL && ret != -ENOMEM)
> > + goto free;
> > + goto err;
> > + }
> > +
> > + ret = of_property_read_string(of_node, "qcom,msm-bus,name",
> > &bvd->name);
> > + if (ret) {
> > + dev_err(dev, "Error: (%d) Bus name missing!\n", ret);
> > + goto err;
> > + }
> > +
> > + ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
> > + &bvd->num_usecase);
> > + if (ret) {
> > + dev_err(dev, "Error: num-usecases not found\n");
> > + goto err;
> > + }
> > +
> > + usecase = devm_kzalloc(dev, (sizeof(struct msm_bus_path) *
> > + bvd->num_usecase), GFP_KERNEL);
> > + if (!usecase)
> > + goto err;
> > +
> > + ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
> > + &num_paths);
> > + if (ret) {
> > + dev_err(dev, "Error: num_paths not found\n");
> > + goto out;
> > + }
> > +
> > + vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps",
Why are all the properties qcom specific?
> > &len);
> > + if (vec_arr == NULL) {
A more consistent style is if (!vec_arr)
> > + dev_err(dev, "Error: Vector array not found\n");
> > + goto out;
> > + }
> > +
> > + for (i = 0; i < bvd->num_usecase; i++) {
> > + usecase[i].num_paths = num_paths;
> > + usecase[i].vec = devm_kzalloc(dev, num_paths *
Use devm_kcalloc().
> > + sizeof(struct msm_bus_vectors),
> > + GFP_KERNEL);
> > + if (!usecase[i].vec) {
> > + mem_err = true;
> > + dev_err(dev, "Error: Failed to alloc mem for vectors\n");
> > + goto out;
> > + }
> > + for (j = 0; j < num_paths; j++) {
> > + int idx = ((i * num_paths) + j) * 2;
> > +
> > + usecase[i].vec[j].ab = (uint64_t)
> > + be32_to_cpu(vec_arr[idx]);
> > + usecase[i].vec[j].ib = (uint64_t)
> > + be32_to_cpu(vec_arr[idx + 1]);
> > + }
> > + }
> > +
> > + bvd->usecase = usecase;
> > + return bvd;
> > +out:
> > + if (mem_err) {
Should be possible to not have this flag.
> > + for (k = i - 1; k >= 0; k--)
> > + devm_kfree(dev, usecase[k].vec);
> > + }
> > + devm_kfree(dev, usecase);
> > +free:
> > + devm_kfree(dev, bvd->bw_vecs);
> > +err:
> > + devm_kfree(dev, bvd);
You don't need to devm_kfree() anything, just let probe fail it.
> > + bvd = NULL;
> > + return bvd;
> > +}
> > +
> > +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
> > + struct platform_device *pdev)
> > +{
> > + struct sdhci_msm_bus_vote_data *bsd;
> > + struct device *dev = &pdev->dev;
> > +
> > + bsd = sdhci_msm_get_bus_vote_data(dev, host);
> > + if (!bsd) {
> > + dev_err(&pdev->dev, "Failed: getting bus_scale data\n");
> > + return PTR_ERR(bsd);
> > + }
> > + host->bus_vote_data = bsd;
> > +
> > + bsd->sdhc_ddr = of_icc_get(&pdev->dev, SDHC_DDR);
> > + if (IS_ERR(bsd->sdhc_ddr)) {
> > + dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
We don't need "Error: " prefix. That's what the kernel log level is for.
> > + PTR_ERR(bsd->sdhc_ddr), SDHC_DDR);
> > + return PTR_ERR(bsd->sdhc_ddr);
> > + }
> > +
> > + bsd->cpu_sdhc = of_icc_get(&pdev->dev, CPU_SDHC);
> > + if (IS_ERR(bsd->cpu_sdhc)) {
> > + dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
> > + PTR_ERR(bsd->cpu_sdhc), CPU_SDHC);
> > + return PTR_ERR(bsd->cpu_sdhc);
> > + }
> > +
> > + INIT_DELAYED_WORK(&host->bus_vote_work, sdhci_msm_bus_work);
Why is it in a workqueue context? Is there any reason it can' be done in
whatever calling context it is?
> > +
> > + return 0;
> > +}
> > +
> > +static void sdhci_msm_bus_unregister(struct device *dev,
> > + struct sdhci_msm_host *host)
> > +{
> > + struct sdhci_msm_bus_vote_data *bsd = host->bus_vote_data;
> > + int i;
> > +
> > + icc_put(bsd->sdhc_ddr);
> > + icc_put(bsd->cpu_sdhc);
Is there a devm_icc_get() API? If not, please add it and use it.
> > +
> > + for (i = 0; i < bsd->num_usecase; i++)
> > + devm_kfree(dev, bsd->usecase[i].vec);
> > + devm_kfree(dev, bsd->usecase);
> > + devm_kfree(dev, bsd->bw_vecs);
> > + devm_kfree(dev, bsd);
Again, not sure we need any devm_kfree() stuff.
> > +}
> > +
> > +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
> > +{
> > + struct mmc_ios *ios = &host->mmc->ios;
> > + unsigned int bw;
> > +
> > + bw = sdhci_get_bw_required(host, ios);
> > + if (enable)
> > + sdhci_msm_bus_cancel_work_and_set_vote(host, bw);
> > + else
> > + sdhci_msm_bus_queue_work(host);
> > +}
> > +
> > static const struct sdhci_msm_variant_ops mci_var_ops = {
> > .msm_readl_relaxed = sdhci_msm_mci_variant_readl_relaxed,
> > .msm_writel_relaxed = sdhci_msm_mci_variant_writel_relaxed,
> > @@ -1839,6 +2210,13 @@ static int sdhci_msm_probe(struct
> > platform_device *pdev)
> > dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret);
> > }
> >
> > + ret = sdhci_msm_bus_register(msm_host, pdev);
> > + if (ret && !msm_host->skip_bus_bw_voting)
Can this be a check for a NULL icc handle instead of the bool?
Powered by blists - more mailing lists