| 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: Thu, 01 Apr 2021 17:50:38 -0700
From: Stephen Boyd <swboyd@...omium.org>
To: schowdhu@...eaurora.org
Cc: Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
Sibi Sankar <sibis@...eaurora.org>,
Rajendra Nayak <rnayak@...eaurora.org>, vkoul@...nel.org
Subject: Re: [PATCH V2 2/5] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC)
Quoting schowdhu@...eaurora.org (2021-04-01 07:04:07)
> On 2021-03-30 01:35, Stephen Boyd wrote:
> > Quoting Souradeep Chowdhury (2021-03-25 01:02:33)
> >> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
> >> new file mode 100644
> >> index 0000000..a55d8ca7
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/dcc.c
> >> @@ -0,0 +1,1549 @@
[..]
> >
> >> + void __iomem *base;
> >> + u32 reg_size;
> >> + struct device *dev;
> >> + struct mutex mutex;
> >
> > In particular what this mutex is protecting.
>
> Ack. The mutex is used to protect the access as well as manipulation of
> the main instance of dcc_drvdata structure
> initialized during probe time. This structure contains the useful driver
> data information and is set using the call
> platform_set_drvdata(pdev, drvdata) which links this data to the
> platform device and hence needs to be protected via
> mutex locks. The same convention is followed across other similar
> drivers exposing userspace like the llcc driver.
The region that the mutex is protecting seems quite large. That's
probably because I don't understand the driver.
> >
> >> +
> >> + mutex_lock(&drvdata->mutex);
> >> +
> >> + for (curr_list = 0; curr_list < drvdata->nr_link_list;
> >> curr_list++) {
> >> + if (!drvdata->enable[curr_list])
> >> + continue;
> >> + ll_cfg = dcc_readl(drvdata, DCC_LL_CFG(curr_list));
> >> + tmp_ll_cfg = ll_cfg & ~BIT(9);
> >> + dcc_writel(drvdata, tmp_ll_cfg,
> >> DCC_LL_CFG(curr_list));
> >> + dcc_writel(drvdata, 1, DCC_LL_SW_TRIGGER(curr_list));
> >> + dcc_writel(drvdata, ll_cfg, DCC_LL_CFG(curr_list));
> >> + }
> >
> > Does the mutex need to be held while waiting for ready?
>
> Yes, to maintain consistency because inside the dcc_ready function,
> there is access to dcc_drvdata structure set
> on the platform device.
Is the drvdata going to be modified somewhere else?
> >> +
> >> + dev_info(drvdata->dev, "All values written to
> >> enable.\n");
> >
> > Debug print?
>
> Ack
>
> >
> >> + /* Make sure all config is written in sram */
> >> + mb();
> >
> > This won't work as intended.
>
> This was called to prevent instruction reordering if the driver runs on
> multiple
> CPU cores. As the hardware manipulation has to be done sequentially
> before the
> trigger is set. Kindly let me know the concern in this case.
Device I/O with the proper accessors is sequential even if the process
moves to a different CPU. Is that what you're worried about? The comment
says "make sure it is written to sram", which should be achieved by
reading some register back from the device after all the writes so that
the driver knows the writes have been posted to the device. I believe
this mb() is doing nothing.
>
> >
> >> +
> >> + drvdata->enable[list] = true;
> >> +
> >> + /* 5. Configure trigger */
> >> + dcc_writel(drvdata, BIT(9), DCC_LL_CFG(list));
> >> + }
> >> +
> >> +err:
> >> + mutex_unlock(&drvdata->mutex);
> >> + return ret;
> >> +}
> >> +
> >> +static void dcc_disable(struct dcc_drvdata *drvdata)
> >> +{
> >> + int curr_list;
> >> +
> >> + mutex_lock(&drvdata->mutex);
> >> +
> >> + if (!dcc_ready(drvdata))
> >> + dev_err(drvdata->dev, "DCC is not ready Disabling
> >> DCC...\n");
> >
> > Is that two sentences? And a debug print?
>
> Ack.
>
> >
> >> +
> >> + for (curr_list = 0; curr_list < drvdata->nr_link_list;
> >> curr_list++) {
> >> + if (!drvdata->enable[curr_list])
> >> + continue;
> >> + dcc_writel(drvdata, 0, DCC_LL_CFG(curr_list));
> >> + dcc_writel(drvdata, 0, DCC_LL_BASE(curr_list));
> >> + dcc_writel(drvdata, 0, DCC_FD_BASE(curr_list));
> >> + dcc_writel(drvdata, 0, DCC_LL_LOCK(curr_list));
> >> + drvdata->enable[curr_list] = false;
> >> + }
> >> + memset_io(drvdata->ram_base, 0, drvdata->ram_size);
> >> + drvdata->ram_cfg = 0;
> >> + drvdata->ram_start = 0;
> >> + mutex_unlock(&drvdata->mutex);
> >> +}
> >> +
> >> +static ssize_t curr_list_show(struct device *dev,
> >> + struct device_attribute *attr, char *buf)
> >> +{
> >> + int ret;
> >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev);
> >> +
> >> + mutex_lock(&drvdata->mutex);
> >> + if (drvdata->curr_list == DCC_INVALID_LINK_LIST) {
> >> + dev_err(dev, "curr_list is not set.\n");
> >> + ret = -EINVAL;
> >> + goto err;
> >> + }
> >> +
> >> + ret = scnprintf(buf, PAGE_SIZE, "%d\n", drvdata->curr_list);
> >> +err:
> >> + mutex_unlock(&drvdata->mutex);
> >> + return ret;
> >> +}
> >> +
> >> +static ssize_t curr_list_store(struct device *dev,
> >> + struct
> >> device_attribute *attr,
> >> + const char *buf,
> >> size_t size)
> >> +{
> >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev);
> >> + unsigned long val;
> >> + u32 lock_reg;
> >> + bool dcc_enable = false;
> >> +
> >> + if (kstrtoul(buf, 16, &val))
> >> + return -EINVAL;
> >> +
> >> + if (val >= drvdata->nr_link_list)
> >> + return -EINVAL;
> >> +
> >> + mutex_lock(&drvdata->mutex);
> >> +
> >> + dcc_enable = is_dcc_enabled(drvdata);
> >> + if (drvdata->curr_list != DCC_INVALID_LINK_LIST && dcc_enable)
> >> {
> >> + dev_err(drvdata->dev, "DCC is enabled, please disable
> >> it first.\n");
> >> + mutex_unlock(&drvdata->mutex);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + lock_reg = dcc_readl(drvdata, DCC_LL_LOCK(val));
> >> + if (lock_reg & 0x1) {
> >> + dev_err(drvdata->dev, "DCC linked list is already
> >> configured\n");
> >> + mutex_unlock(&drvdata->mutex);
> >> + return -EINVAL;
> >> + }
> >> + drvdata->curr_list = val;
> >> + mutex_unlock(&drvdata->mutex);
> >> +
> >> + return size;
> >> +}
> >> +
> >> +static DEVICE_ATTR_RW(curr_list);
> >> +
> >> +
> >> +static ssize_t trigger_store(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *buf, size_t size)
> >> +{
> >> + int ret = 0;
> >> + unsigned long val;
> >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev);
> >> +
> >> + if (kstrtoul(buf, 16, &val))
> >> + return -EINVAL;
> >> + if (val != 1)
> >> + return -EINVAL;
> >> +
> >> + ret = dcc_sw_trigger(drvdata);
> >> + if (!ret)
> >> + ret = size;
> >> +
> >> + return ret;
> >> +}
> >> +static DEVICE_ATTR_WO(trigger);
> >> +
> >> +static ssize_t enable_show(struct device *dev,
> >> + struct device_attribute *attr, char *buf)
> >> +{
> >> + int ret;
> >> + bool dcc_enable = false;
> >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev);
> >> +
> >> + mutex_lock(&drvdata->mutex);
> >> + if (drvdata->curr_list >= drvdata->nr_link_list) {
> >> + dev_err(dev, "Select link list to program using
> >> curr_list\n");
> >> + ret = -EINVAL;
> >> + goto err;
> >> + }
> >> +
> >> + dcc_enable = is_dcc_enabled(drvdata);
> >> +
> >> + ret = scnprintf(buf, PAGE_SIZE, "%u\n",
> >> + (unsigned int)dcc_enable);
> >> +err:
> >> + mutex_unlock(&drvdata->mutex);
> >
> > What does the mutex being held serve here?
>
> As mentioned earlier, the mutex has been used while accessing
> dcc_drvdata structure.
>
And what purpose does it serve? I suppose curr_list can be modified? But
then when this function returns it could be disabled before userspace
sees the value so I'm still lost why we care to hold the lock this long.
> >
> >> + return ret;
> >> +}
> >> +
> >> +static ssize_t enable_store(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *buf, size_t size)
> >> +{
> >> + int ret = 0;
> >> + unsigned long val;
> >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev);
> >> +
> >> + if (kstrtoul(buf, 16, &val))
> >> + return -EINVAL;
> >> +
> >> + if (val)
> >> + ret = dcc_enable(drvdata);
> >> + else
> >> + dcc_disable(drvdata);
> >> +
> >> + if (!ret)
> >> + ret = size;
> >> +
> >> + return ret;
> >> +
> >> +}
> >> +
> >> +static DEVICE_ATTR_RW(enable);
> >> +
> >> +static ssize_t config_show(struct device *dev,
> >> + struct device_attribute *attr, char *buf)
> >> +{
> >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev);
> >> + struct dcc_config_entry *entry;
> >> + char local_buf[64];
> >> + int len = 0, count = 0;
> >> +
> >> + buf[0] = '\0';
> >
> > Why?
>
> The strlcat function is used here to concatenate the buffer with the
> config values.
> The strlcat function in C needs a NULL terminated string both as it's
> source and
> destination. That's why this has been initialized with NULL termination.
>
sysfs files shall be one value per file, i.e. something that a machine
reads. This function looks like a debugfs function.
>
> >
> >> +
> >> + mutex_lock(&drvdata->mutex);
> >> + if (drvdata->curr_list >= drvdata->nr_link_list) {
> >> + dev_err(dev, "Select link list to program using
> >> curr_list\n");
> >> + count = -EINVAL;
> >> + goto err;
> >> + }
> >> +
> >> + list_for_each_entry(entry,
> >> + &drvdata->cfg_head[drvdata->curr_list], list) {
> >> + switch (entry->desc_type) {
> >> + case DCC_READ_WRITE_TYPE:
> >> + len = snprintf(local_buf, 64, "Index: 0x%x,
> >> mask: 0x%x, val: 0x%x\n",
> >> + entry->index, entry->mask,
> >> entry->write_val);
> >> + break;
> >> + case DCC_LOOP_TYPE:
> >> + len = snprintf(local_buf, 64, "Index: 0x%x,
> >> Loop: %d\n",
> >> + entry->index, entry->loop_cnt);
> >> + break;
> >> + case DCC_WRITE_TYPE:
> >> + len = snprintf(local_buf, 64,
> >> + "Write Index: 0x%x, Base: 0x%x,
> >> Offset: 0x%x, len: 0x%x APB: %d\n",
> >> + entry->index, entry->base,
> >> entry->offset, entry->len,
> >> + entry->apb_bus);
> >> + break;
> >> + default:
> >> + len = snprintf(local_buf, 64,
> >> + "Read Index: 0x%x, Base: 0x%x, Offset:
> >> 0x%x, len: 0x%x APB: %d\n",
> >> + entry->index, entry->base,
> >> entry->offset,
> >> + entry->len, entry->apb_bus);
> >> + }
> >> +
> >> + if ((count + len) > PAGE_SIZE) {
> >> + dev_err(dev, "DCC: Couldn't write complete
> >> config\n");
> >> + break;
> >> + }
> >> + strlcat(buf, local_buf, PAGE_SIZE);
> >> + count += len;
> >> + }
> >> +
> >> +err:
> >> + mutex_unlock(&drvdata->mutex);
> >> + return count;
> >> +}
> >
> >> +
> >> + /* EOF check */
> >> + if (drvdata->ram_size <= *ppos)
> >> + return 0;
> >> +
> >> + if ((*ppos + len) > drvdata->ram_size)
> >> + len = (drvdata->ram_size - *ppos);
> >> +
> >> + buf = kzalloc(len, GFP_KERNEL);
> >> + if (!buf)
> >> + return -ENOMEM;
> >> +
> >> + memcpy_fromio(buf, drvdata->ram_base + *ppos, len);
> >> +
> >> + if (copy_to_user(data, buf, len)) {
> >
> > Is there any sort of memcpy_fromio_to_user() API? That would avoid the
> > extra buffer allocation by copying to userspace in the readl loop.
>
> No. For directly copying io data to userspace we need to use direct I/O
> which is used for
> special cases like tape drivers. In this case the complexity of using it
> outweighs it's
> advantages. Also this is a fixed transfer of data in the form of
> dcc_sram content rather
> than bulk transfers.
Tape drivers? Huh? Can you please look into adding a
memcpy_fromio_to_user() API that does this without allocating memory for
a buffer?
>
> >
> >> + dcc->loopoff = DCC_FIX_LOOP_OFFSET;
> >> + else
> >> + dcc->loopoff = get_bitmask_order((dcc->ram_size +
> >> + dcc->ram_offset) / 4 - 1);
> >> +
> >> + mutex_init(&dcc->mutex);
> >> + dcc->enable = devm_kcalloc(dev, dcc->nr_link_list,
> >> + sizeof(bool), GFP_KERNEL);
> >> + if (!dcc->enable)
> >> + return -ENOMEM;
> >> +
> >> + dcc->configured = devm_kcalloc(dev, dcc->nr_link_list,
> >> + sizeof(bool), GFP_KERNEL);
> >> + if (!dcc->configured)
> >> + return -ENOMEM;
> >> +
> >> + dcc->nr_config = devm_kcalloc(dev, dcc->nr_link_list,
> >> + sizeof(u32), GFP_KERNEL);
> >> + if (!dcc->nr_config)
> >> + return -ENOMEM;
> >> +
> >> + dcc->cfg_head = devm_kcalloc(dev, dcc->nr_link_list,
> >> + sizeof(struct list_head), GFP_KERNEL);
> >> + if (!dcc->cfg_head)
> >> + return -ENOMEM;
> >
> > These are a lot of allocations. Any chance we can do one instead of
> > this
> > many?
>
> All these variable have predefined requirement of sizes
> so they need to be allocated separately.
Gather requirements, do some addition, and then allocate one chunk of
memory?
Powered by blists - more mailing lists