| 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: <686c07a8-1b53-b60b-940b-bcb659aaf026@quicinc.com>
Date: Fri, 30 Sep 2022 12:37:10 +0530
From: Souradeep Chowdhury <quic_schowdhu@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: Andy Gross <agross@...nel.org>, Rob Herring <robh+dt@...nel.org>,
"Alex Elder" <elder@...e.org>,
Krzysztof Kozlowski <krzk@...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" <quic_saipraka@...cinc.com>,
Sibi Sankar <quic_sibis@...cinc.com>,
Rajendra Nayak <quic_rjendra@...cinc.com>, <vkoul@...nel.org>
Subject: Re: [PATCH V14 2/7] soc: qcom: dcc: Add driver support for Data
Capture and Compare unit(DCC)
On 9/29/2022 9:53 PM, Bjorn Andersson wrote:
> On Wed, Sep 28, 2022 at 10:41:12PM +0530, Souradeep Chowdhury wrote:
> [..]
>> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
> [..]
>> +/**
>> + * struct dcc_drvdata - configuration information related to a dcc device
>> + * @base: Base Address of the dcc device
>> + * @dev: The device attached to the driver data
>> + * @mutex: Lock to protect access and manipulation of dcc_drvdata
>> + * @ram_base: Base address for the SRAM dedicated for the dcc device
>> + * @ram_size: Total size of the SRAM dedicated for the dcc device
>> + * @ram_offset: Offset to the SRAM dedicated for dcc device
>> + * @mem_map_ver: Memory map version of DCC hardware
>> + * @ram_cfg: Used for address limit calculation for dcc
>> + * @ram_start: Starting address of DCC SRAM
>> + * @sram_dev: Miscellaneous device equivalent of dcc SRAM
>> + * @cfg_head: Points to the head of the linked list of addresses
>> + * @dbg_dir: The dcc debugfs directory under which all the debugfs files are placed
>> + * @nr_link_list: Total number of linkedlists supported by the DCC configuration
>> + * @loopoff: Loop offset bits range for the addresses
>> + * @enable; This contains an array of linkedlist enable flags
>> + */
>> +struct dcc_drvdata {
>> + void __iomem *base;
>> + void *ram_base;
>> + struct device *dev;
>> + struct mutex mutex;
>> + size_t ram_size;
>> + size_t ram_offset;
>> + int mem_map_ver;
>> + phys_addr_t ram_cfg;
>> + phys_addr_t ram_start;
>> + struct miscdevice sram_dev;
>> + struct list_head *cfg_head;
>> + struct dentry *dbg_dir;
>> + size_t nr_link_list;
>> + u8 loopoff;
>> + bool *enable;
> It's idiomatic to carry such information in a bitmap, and if
> DCC_MAX_LINK_LIST applies to all versions (not obvious from the code
> below) then replacing this with just a fixed unsigned long would be a
> good move.
>
> Use set_bit(), clear_bit() and test_bit() as needed to access the
> individual bits.
Ack
>
>> +};
>> +
>> +struct dcc_cfg_attr {
>> + u32 addr;
>> + u32 prev_addr;
>> + u32 prev_off;
>> + u32 link;
>> + u32 sram_offset;
>> +};
>> +
>> +struct dcc_cfg_loop_attr {
>> + u32 loop;
>> + u32 loop_cnt;
>> + u32 loop_len;
>> + u32 loop_off;
>> + bool loop_start;
>> +};
>> +
> [..]
>> +static bool is_dcc_enabled(struct dcc_drvdata *drvdata)
>> +{
>> + int list;
>> +
>> + for (list = 0; list < DCC_MAX_LINK_LIST; list++)
> It's possible that there's only dcc->nr_link_list entries in the enable
> array.
Ack
>
>> + if (drvdata->enable[list])
>> + return true;
>> +
>> + return false;
>> +}
> [..]
>> +static int dcc_probe(struct platform_device *pdev)
>> +{
>> + u32 val;
>> + int ret = 0, i, size;
>> + struct device *dev = &pdev->dev;
>> + struct dcc_drvdata *dcc;
>> + struct resource *res;
>> +
>> + dcc = devm_kzalloc(dev, sizeof(*dcc), GFP_KERNEL);
>> + if (!dcc)
>> + return -ENOMEM;
>> +
>> + dcc->dev = &pdev->dev;
>> + platform_set_drvdata(pdev, dcc);
>> +
>> + dcc->base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(dcc->base))
>> + return PTR_ERR(dcc->base);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + if (!res)
>> + return -ENODEV;
>> +
>> + dcc->ram_base = memremap(res->start, resource_size(res), MEMREMAP_WB);
>> + if (!dcc->ram_base)
>> + return -ENODEV;
>> +
>> + dcc->ram_size = resource_size(res);
>> +
>> + dcc->ram_offset = (size_t)of_device_get_match_data(&pdev->dev);
>> +
>> + val = dcc_readl(dcc, DCC_HW_INFO);
>> +
>> + if (FIELD_GET(DCC_VER_INFO_MASK, val)) {
>> + dcc->mem_map_ver = 3;
>> + dcc->nr_link_list = dcc_readl(dcc, DCC_LL_NUM_INFO);
>> + if (dcc->nr_link_list == 0)
>> + return -EINVAL;
>> + } else if ((val & DCC_VER_MASK2) == DCC_VER_MASK2) {
>> + dcc->mem_map_ver = 2;
>> + dcc->nr_link_list = dcc_readl(dcc, DCC_LL_NUM_INFO);
>> + if (dcc->nr_link_list == 0)
>> + return -EINVAL;
>> + } else {
>> + dcc->mem_map_ver = 1;
>> + dcc->nr_link_list = DCC_MAX_LINK_LIST;
>> + }
>> +
>> + /* Either set the fixed loop offset or calculate it
>> + * from ram_size. Max consecutive addresses the
>> + * dcc can loop is equivalent to the ram size
>> + */
>> + if (val & DCC_LOOP_OFFSET_MASK)
>> + dcc->loopoff = DCC_FIX_LOOP_OFFSET;
>> + else
>> + dcc->loopoff = get_bitmask_order((dcc->ram_size +
>> + dcc->ram_offset) / 4 - 1);
>> +
>> + mutex_init(&dcc->mutex);
>> + /* Allocate space for all entries at once */
>> + size = sizeof(*dcc->enable) + sizeof(*dcc->cfg_head);
>> +
>> + dcc->enable = devm_kcalloc(dev, dcc->nr_link_list, size, GFP_KERNEL);
>> + if (!dcc->enable)
>> + return -ENOMEM;
>> +
>> + dcc->cfg_head = (struct list_head *)(dcc->enable + dcc->nr_link_list);
> Turning enable into a unsigned long bitmap, will clean this stuff up as
> well, as you won't have the need/urge to allocate the two components at
> once and then do pointer math on them...
Ack
>
> Regards,
> Bjorn
>
>> +
>> + for (i = 0; i < dcc->nr_link_list; i++)
>> + INIT_LIST_HEAD(&dcc->cfg_head[i]);
>> +
>> + ret = dcc_sram_dev_init(dcc);
>> + if (ret) {
>> + dev_err(dcc->dev, "DCC: sram node not registered.\n");
>> + return ret;
>> + }
>> +
>> + ret = dcc_create_debug_dir(dcc);
>> + if (ret) {
>> + dev_err(dcc->dev, "DCC: debugfs files not created.\n");
>> + dcc_sram_dev_exit(dcc);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
Powered by blists - more mailing lists