| 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: <19682f5d-7323-d542-d243-e84a3c58e6b6@quicinc.com>
Date: Tue, 10 Jan 2023 17:34:30 +0530
From: Souradeep Chowdhury <quic_schowdhu@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: Andy Gross <agross@...nel.org>,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Alex Elder <elder@...e.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <linux-arm-msm@...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 V1 1/1] soc: qcom: dcc: Add QAD, Ctitrigger and Bootconfig
support for DCC
On 1/9/2023 9:10 PM, Bjorn Andersson wrote:
> On Mon, Jan 09, 2023 at 07:32:26PM +0530, Souradeep Chowdhury wrote:
>> Add the QAD, ctitrigger and bootconfig support for dcc driver.
>> The QAD is used for access control of the DCC configurations
>> and can be set or unset by writing to DCC_QAD_OUTPUT register.
>> The Cti-trigger is used for the cross trigger interface, on
>> enabling it the software trigger can be initiated for dcc by
>> setting the cti-trigout.
>> Bootconfig support for DCC is for configuring register values
>> to dcc at boot time, this is needed for debugging crashes that
>> happen at boot time.
>> Add the debugfs files QAD and CTI-trigger for each list folder.
>> Also add hwtrigger debugfs file which needs to be disabled on
>> enabling the cti-trigger.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@...cinc.com>
>> ---
>> Documentation/ABI/testing/debugfs-driver-dcc | 24 +++
>> drivers/soc/qcom/dcc.c | 284 ++++++++++++++++++++++++++-
>> 2 files changed, 304 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/debugfs-driver-dcc b/Documentation/ABI/testing/debugfs-driver-dcc
>> index 27ed591..f817a9f 100644
>> --- a/Documentation/ABI/testing/debugfs-driver-dcc
>> +++ b/Documentation/ABI/testing/debugfs-driver-dcc
>> @@ -125,3 +125,27 @@ Description:
>> on manual or crash induced triggers. Lists must
>> be configured and enabled sequentially, e.g. list
>> 2 can only be enabled when list 1 have so.
>> +
>> +What: /sys/kernel/debug/dcc/.../[list-number]/ctitrigger
>> +Date: January 2023
>> +Contact: Souradeep Chowdhury <quic_schowdhu@...cinc.com>
>> +Description:
>> + This debugfs interface is used for enabling the
>> + ctitrigger. Ctitrigger can be enabled by writing
>> + a '1' to the file.
>> +
>> +What: /sys/kernel/debug/dcc/.../[list-number]/QAD
>> +Date: January 2023
>> +Contact: Souradeep Chowdhury <quic_schowdhu@...cinc.com>
>> +Description:
>> + This debugfs interface is used for enabling the
>> + ctitrigger. Ctitrigger can be enabled by writing
>> + a '1' to the file.
>> +
>> +What: /sys/kernel/debug/dcc/.../[list-number]/hwtrigger
>> +Date: January 2023
>> +Contact: Souradeep Chowdhury <quic_schowdhu@...cinc.com>
>> +Description:
>> + This debugfs interface is used for enabling the
>> + hwtrigger support. Hwtrigger can be enabled by
>> + writing a '1' to the file.
>> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
>> index 5b50d63..ea44dbf 100644
>> --- a/drivers/soc/qcom/dcc.c
>> +++ b/drivers/soc/qcom/dcc.c
>> @@ -6,6 +6,7 @@
>>
>> #include <linux/bitfield.h>
>> #include <linux/bitops.h>
>> +#include <linux/bootconfig.h>
>> #include <linux/debugfs.h>
>> #include <linux/delay.h>
>> #include <linux/fs.h>
>> @@ -36,6 +37,8 @@
>> #define DCC_LL_INT_STATUS 0x1c
>> #define DCC_LL_SW_TRIGGER 0x2c
>> #define DCC_LL_BUS_ACCESS_STATUS 0x30
>> +#define DCC_CTI_TRIG 0x34
>> +#define DCC_QAD_OUTPUT 0x38
>>
>> /* Default value used if a bit 6 in the HW_INFO register is set. */
>> #define DCC_FIX_LOOP_OFFSET 16
>> @@ -114,6 +117,9 @@ struct dcc_config_entry {
>> * @nr_link_list: Total number of linkedlists supported by the DCC configuration
>> * @loop_shift: Loop offset bits range for the addresses
>> * @enable_bitmap: Bitmap to capture the enabled status of each linked list of addresses
>> + * @cti_bitmap: Bitmap to capture the cti-trigger status of each linked list of addresses
>> + * @qad_bitmap: Bitmap to capture the qad status of each linked list of addresses
>> + * @hwtrig_bitmap: Bitmap to capture the hwtrig status of each linked list of addresses
>> */
>> struct dcc_drvdata {
>> void __iomem *base;
>> @@ -131,6 +137,9 @@ struct dcc_drvdata {
>> size_t nr_link_list;
>> u8 loop_shift;
>> unsigned long *enable_bitmap;
>> + unsigned long *cti_bitmap;
>> + unsigned long *qad_bitmap;
>> + unsigned long *hwtrig_bitmap;
>> };
>>
>> struct dcc_cfg_attr {
>> @@ -148,6 +157,18 @@ struct dcc_cfg_loop_attr {
>> bool loop_start;
>> };
>>
>> +static char *replace_char(char *str, char find, char replace)
>> +{
>> + char *current_pos = strchr(str, find);
>> +
>> + while (current_pos) {
>> + *current_pos = replace;
>> + current_pos = strchr(current_pos, find);
>> + }
>> +
>> + return str;
>> +}
>> +
>> static inline u32 dcc_list_offset(int version)
>> {
>> return version == 1 ? 0x1c : version == 2 ? 0x2c : 0x34;
>> @@ -201,6 +222,10 @@ static int dcc_sw_trigger(struct dcc_drvdata *drvdata)
>> continue;
>> ll_cfg = dcc_list_readl(drvdata, i, DCC_LL_CFG);
>> tmp_ll_cfg = ll_cfg & ~DCC_TRIGGER_MASK;
>> + if (drvdata->mem_map_ver != 3)
>> + tmp_ll_cfg = ll_cfg & ~DCC_TRIGGER_MASK;
>> + else
>> + tmp_ll_cfg = ll_cfg & ~BIT(8);
>> dcc_list_writel(drvdata, tmp_ll_cfg, i, DCC_LL_CFG);
>> dcc_list_writel(drvdata, 1, i, DCC_LL_SW_TRIGGER);
>> dcc_list_writel(drvdata, ll_cfg, i, DCC_LL_CFG);
>> @@ -577,6 +602,25 @@ static int dcc_enable(struct dcc_drvdata *drvdata, unsigned int curr_list)
>> /* 5. Configure trigger */
>> dcc_list_writel(drvdata, DCC_TRIGGER_MASK,
>> curr_list, DCC_LL_CFG);
>> + if (drvdata->mem_map_ver == 3) {
>> + dcc_list_writel(drvdata, test_bit(curr_list, drvdata->qad_bitmap), curr_list,
>> + DCC_QAD_OUTPUT);
>> + dcc_list_writel(drvdata, test_bit(curr_list, drvdata->cti_bitmap), curr_list,
>> + DCC_CTI_TRIG);
>> + if (test_bit(curr_list, drvdata->hwtrig_bitmap))
>> + dcc_list_writel(drvdata, BIT(8), curr_list, DCC_LL_CFG);
>> + else
>> + dcc_list_writel(drvdata, (unsigned int)~BIT(8), curr_list, DCC_LL_CFG);
>> + } else {
>> + if (test_bit(curr_list, drvdata->hwtrig_bitmap))
>> + dcc_list_writel(drvdata, DCC_TRIGGER_MASK |
>> + test_bit(curr_list, drvdata->cti_bitmap) << 8,
>> + curr_list, DCC_LL_CFG);
>> + else
>> + dcc_list_writel(drvdata, ~DCC_TRIGGER_MASK &
>> + test_bit(curr_list, drvdata->cti_bitmap) << 8,
>> + curr_list, DCC_LL_CFG);
>> + }
>>
>> out_unlock:
>> mutex_unlock(&drvdata->mutex);
>> @@ -1103,6 +1147,168 @@ static const struct file_operations config_fops = {
>> .release = single_release,
>> };
>>
>> +static ssize_t ctitrigger_read(struct file *filp, char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + char *buf;
>> + int curr_list;
>> + struct dcc_drvdata *drvdata = filp->private_data;
>> +
>> + curr_list = dcc_filp_curr_list(filp);
>> +
>> + mutex_lock(&drvdata->mutex);
>> +
>> + if (test_bit(curr_list, drvdata->cti_bitmap))
>> + buf = "Y\n";
>> + else
>> + buf = "N\n";
>> +
>> + mutex_unlock(&drvdata->mutex);
>> +
>> + return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
>> +}
>> +
>> +static ssize_t ctitrigger_write(struct file *filp, const char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret = 0, curr_list;
>> + bool val;
>> + struct dcc_drvdata *drvdata = filp->private_data;
>> +
>> + curr_list = dcc_filp_curr_list(filp);
>> +
>> + if (test_bit(curr_list, drvdata->enable_bitmap))
>> + return -EBUSY;
>> +
>> + ret = kstrtobool_from_user(userbuf, count, &val);
>> + if (ret < 0)
>> + return 0;
>> +
>> + if (val)
>> + set_bit(curr_list, drvdata->cti_bitmap);
>> + else
>> + clear_bit(curr_list, drvdata->cti_bitmap);
>> +
>> + return count;
>> +}
>> +
>> +static const struct file_operations ctitrigger_fops = {
>> + .read = ctitrigger_read,
>> + .write = ctitrigger_write,
>> + .open = simple_open,
>> + .llseek = generic_file_llseek,
>> +};
>> +
>> +static ssize_t qad_read(struct file *filp, char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + char *buf;
>> + int curr_list;
>> +
>> + struct dcc_drvdata *drvdata = filp->private_data;
>> +
>> + curr_list = dcc_filp_curr_list(filp);
>> +
>> + mutex_lock(&drvdata->mutex);
>> +
>> + if (test_bit(curr_list, drvdata->qad_bitmap))
>> + buf = "Y\n";
>> + else
>> + buf = "N\n";
>> +
>> + mutex_unlock(&drvdata->mutex);
>> +
>> + return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
>> +}
>> +
>> +static ssize_t qad_write(struct file *filp, const char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret = 0, curr_list;
>
> First use of ret is an assignment, no need to assign it here.
Ack
>
>> + bool val;
>> + struct dcc_drvdata *drvdata = filp->private_data;
>> +
>> + curr_list = dcc_filp_curr_list(filp);
>> +
>> + if (drvdata->mem_map_ver != 3) {
>> + dev_err(drvdata->dev, "QAD is not supported\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (test_bit(curr_list, drvdata->enable_bitmap))
>> + return -EBUSY;
>> +
>> + ret = kstrtobool_from_user(userbuf, count, &val);
>> + if (ret < 0)
>> + return 0;
>
> 0 isn't a good return value here.
Ack
>
>> +
>> + if (val)
>> + set_bit(curr_list, drvdata->qad_bitmap);
>> + else
>> + clear_bit(curr_list, drvdata->qad_bitmap);
>> +
>> + return count;
>> +}
>> +
>> +static const struct file_operations qad_fops = {
>> + .read = qad_read,
>> + .write = qad_write,
>> + .open = simple_open,
>> + .llseek = generic_file_llseek,
>> +};
>> +
>> +static ssize_t hwtrigger_read(struct file *filp, char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + char *buf;
>> + int curr_list;
>> + struct dcc_drvdata *drvdata = filp->private_data;
>> +
>> + curr_list = dcc_filp_curr_list(filp);
>> +
>> + mutex_lock(&drvdata->mutex);
>> +
>> + if (test_bit(curr_list, drvdata->hwtrig_bitmap))
>> + buf = "Y\n";
>> + else
>> + buf = "N\n";
>> +
>> + mutex_unlock(&drvdata->mutex);
>> +
>> + return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
>> +}
>> +
>> +static ssize_t hwtrigger_write(struct file *filp, const char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret = 0, curr_list;
>> + bool val;
>> + struct dcc_drvdata *drvdata = filp->private_data;
>> +
>> + curr_list = dcc_filp_curr_list(filp);
>> +
>> + if (test_bit(curr_list, drvdata->enable_bitmap))
>> + return -EBUSY;
>> +
>> + ret = kstrtobool_from_user(userbuf, count, &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (val)
>> + set_bit(curr_list, drvdata->hwtrig_bitmap);
>> + else
>> + clear_bit(curr_list, drvdata->hwtrig_bitmap);
>> +
>> + return count;
>> +}
>> +
>> +static const struct file_operations hwtrigger_fops = {
>> + .read = hwtrigger_read,
>> + .write = hwtrigger_write,
>> + .open = simple_open,
>> + .llseek = generic_file_llseek,
>> +};
>> +
>> static void dcc_delete_debug_dir(struct dcc_drvdata *drvdata)
>> {
>> debugfs_remove_recursive(drvdata->dbg_dir);
>> @@ -1126,6 +1332,9 @@ static void dcc_create_debug_dir(struct dcc_drvdata *drvdata)
>> list = debugfs_create_dir(list_num, drvdata->dbg_dir);
>> debugfs_create_file("enable", 0600, list, drvdata, &enable_fops);
>> debugfs_create_file("config", 0600, list, drvdata, &config_fops);
>> + debugfs_create_file("ctitrigger", 0600, list, drvdata, &ctitrigger_fops);
>> + debugfs_create_file("QAD", 0600, list, drvdata, &qad_fops);
>> + debugfs_create_file("hwtrigger", 0600, list, drvdata, &hwtrigger_fops);
>> }
>>
>> debugfs_create_file("trigger", 0200, drvdata->dbg_dir, drvdata, &trigger_fops);
>> @@ -1185,13 +1394,60 @@ static void dcc_sram_dev_exit(struct dcc_drvdata *drvdata)
>> misc_deregister(&drvdata->sram_dev);
>> }
>>
>> -static int dcc_probe(struct platform_device *pdev)
>> +static int __init dcc_bootconfig_parse(struct dcc_drvdata *drvdata, struct xbc_node *dcc_node)
>> +{
>> + struct xbc_node *linked_list, *node;
>> + int curr_list, ret;
>> + const char *p;
>> + char *input, *token;
>> + char val[30];
>> +
>> + xbc_node_for_each_subkey(dcc_node, linked_list) {
>> + p = xbc_node_find_value(linked_list, "qcom-curr-link-list", &node);
>> + if (p) {
>
> So if I specify
>
> dcc-config {
> list1 {
> qcom-curr-link-list = 1
> }
> list2 {
>
> }
> }
>
> Then list2 should have &curr_list of 1 as well?
>
> Or did you miss the error handling here?
>
>
> PS. I think you can skip the "qcom-" prefix. Frankly, you know that
> you're in a subnode of dcc_config; use "id" and "items" ("list"?).
Ack
>
>> + ret = kstrtoint(p, 0, &curr_list);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + p = xbc_node_find_value(linked_list, "qcom-link-list", &node);
>> + if (!p)
>> + continue;
>> +
>> + xbc_array_for_each_value(node, p) {
>> + snprintf(val, sizeof(val), "%s", p);
>
> strscpy() would be less costly.
Ack
>
>> + input = replace_char(val, '_', ' ');
>> + token = strsep(&input, " ");
>> +
>> + if (!strcmp("R", token)) {
>> + ret = dcc_config_add_read(drvdata, input, curr_list);
>> + } else if (!strcmp("W", token)) {
>> + ret = dcc_config_add_write(drvdata, input, curr_list);
>> + } else if (!strcmp("RW", token)) {
>> + ret = dcc_config_add_read_write(drvdata, input, curr_list);
>> + } else if (!strcmp("L", token)) {
>> + ret = dcc_config_add_loop(drvdata, input, curr_list);
>> + } else {
>> + dev_err(drvdata->dev, "%s is not a correct input\n", token);
>> + return -EINVAL;
>> + }
>> + if (ret)
>> + return ret;
>> + }
>> + dcc_enable(drvdata, curr_list);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int __init dcc_probe(struct platform_device *pdev)
>> {
>> u32 val;
>> int ret = 0, i;
>> struct device *dev = &pdev->dev;
>> struct dcc_drvdata *drvdata;
>> struct resource *res;
>> + struct xbc_node *dcc_node;
>>
>> drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> if (!drvdata)
>> @@ -1247,13 +1503,30 @@ static int dcc_probe(struct platform_device *pdev)
>> if (!drvdata->enable_bitmap)
>> return -ENOMEM;
>>
>> + drvdata->cti_bitmap = devm_kcalloc(dev, BITS_TO_LONGS(drvdata->nr_link_list),
>> + sizeof(*drvdata->cti_bitmap), GFP_KERNEL);
>
> How about devm_bitmap_alloc()?
Ack
>
>> + if (!drvdata->cti_bitmap)
>> + return -ENOMEM;
>> +
>> + drvdata->qad_bitmap = devm_kcalloc(dev, BITS_TO_LONGS(drvdata->nr_link_list),
>> + sizeof(*drvdata->qad_bitmap), GFP_KERNEL);
>> + if (!drvdata->qad_bitmap)
>> + return -ENOMEM;
>> +
>> + drvdata->hwtrig_bitmap = devm_kcalloc(dev, BITS_TO_LONGS(drvdata->nr_link_list),
>> + sizeof(*drvdata->hwtrig_bitmap), GFP_KERNEL);
>> + if (!drvdata->hwtrig_bitmap)
>> + return -ENOMEM;
>> +
>> drvdata->cfg_head = devm_kcalloc(dev, drvdata->nr_link_list,
>> sizeof(*drvdata->cfg_head), GFP_KERNEL);
>> if (!drvdata->cfg_head)
>> return -ENOMEM;
>>
>> - for (i = 0; i < drvdata->nr_link_list; i++)
>> + for (i = 0; i < drvdata->nr_link_list; i++) {
>> INIT_LIST_HEAD(&drvdata->cfg_head[i]);
>> + set_bit(i, drvdata->hwtrig_bitmap);
>> + }
>>
>> ret = dcc_sram_dev_init(drvdata);
>> if (ret) {
>> @@ -1263,6 +1536,10 @@ static int dcc_probe(struct platform_device *pdev)
>>
>> dcc_create_debug_dir(drvdata);
>>
>> + dcc_node = xbc_find_node("dcc_config");
>> + if (dcc_node)
>> + return dcc_bootconfig_parse(drvdata, dcc_node);
>
> Wouldn't it make sense to still probe dcc, even if the configuration is
> rejected?
Ack
>
>> +
>> return 0;
>> }
>>
>> @@ -1287,14 +1564,13 @@ static const struct of_device_id dcc_match_table[] = {
>> MODULE_DEVICE_TABLE(of, dcc_match_table);
>>
>> static struct platform_driver dcc_driver = {
>> - .probe = dcc_probe,
>> .remove = dcc_remove,
>> .driver = {
>> .name = "qcom-dcc",
>> .of_match_table = dcc_match_table,
>> },
>> };
>> -module_platform_driver(dcc_driver);
>> +module_platform_driver_probe(dcc_driver, dcc_probe);
>
> This change is unrelated to the others, and I don't see it mentioned
> in your commit message.
This change is required because dcc_probe needs to defined as a part of
__init section so that it can access the bootconfig apis. If dcc is
declared as __init, it can't be a part of platform driver which is text
section.
>
> Regards,
> Bjorn
>
>>
>> MODULE_LICENSE("GPL");
>> MODULE_DESCRIPTION("Qualcomm Technologies Inc. DCC driver");
>> --
>> 2.7.4
>>
Powered by blists - more mailing lists