Quoting schowdhu@codeaurora.org (2021-04-01 07:04:07)

On 2021-03-30 01:35, Stephen Boyd wrote:

quoted

Quoting Souradeep Chowdhury (2021-03-25 01:02:33)

quoted

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 @@

[..]

quoted

quoted

+       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.

quoted

quoted

+
+       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?

quoted

quoted

+
+               dev_info(drvdata->dev, "All values written to 
enable.\n");

Debug print?

Ack

quoted

quoted

+               /* 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.

quoted

quoted

+
+               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.

quoted

quoted

+
+       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.

quoted

quoted

+       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.

quoted

quoted

+
+       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;
+}

quoted

+
+       /* 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?

quoted

quoted

+               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?