| 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: <23892559-a300-d886-d945-1a01c4d9ac09@huawei.com>
Date: Wed, 24 Nov 2021 15:12:39 +0800
From: "liuqi (BA)" <liuqi115@...wei.com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
CC: <suzuki.poulose@....com>, <mike.leach@...aro.org>,
<coresight@...ts.linaro.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linuxarm@...wei.com>
Subject: Re: [PATCH v3] drivers/coresight: Add Ultrasoc System Memory Buffer
driver
On 2021/11/24 2:30, Mathieu Poirier wrote:
> Good morning,
>
> On Thu, Nov 18, 2021 at 07:00:16PM +0800, Qi Liu wrote:
>> This patch adds driver for Ultrasoc SMB(System Memory Buffer)
>> device. SMB provides a way to buffer messages from ETM, and
>> store these CPU instructions in system memory.
>>
>> SMB is developed by Ultrasoc technology, which is acquired by
>> Siemens, and we still use "Ultrasoc" to name driver.
>>
>> Signed-off-by: Qi Liu <liuqi115@...wei.com>
>> Tested-by: JunHao He <hejunhao2@...ilicon.com>
>>
>> ---
>>
>> Change since v2:
>> - Move ultrasoc driver to drivers/hwtracing/coresight.
>> - Link:https://lists.linaro.org/pipermail/coresight/2021-November/007310.html
>>
>> Change since v1:
>> - Drop the document of Ultrasoc according to Mathieu's comment.
>> - Add comments to explain some private hardware settings.
>> - Address the comments from Mathieu.
>> - Link: https://lists.linaro.org/pipermail/coresight/2021-August/006842.html
>>
>> Change since RFC:
>> - Move ultrasoc driver to drivers/hwtracing/coresight/ultrasoc.
>> - Remove ultrasoc-axi-com.c, as AXI-COM doesn't need to be configured in
>> basic tracing function.
>> - Remove ultrasoc.c as SMB does not need to register with the ultrasoc core.
>> - Address the comments from Mathieu and Suzuki.
>> - Link: https://lists.linaro.org/pipermail/coresight/2021-June/006535.html
>>
>> drivers/hwtracing/coresight/Kconfig | 12 +
>> drivers/hwtracing/coresight/Makefile | 2 +
>> drivers/hwtracing/coresight/ultrasoc-smb.c | 607 +++++++++++++++++++++
>> drivers/hwtracing/coresight/ultrasoc-smb.h | 116 ++++
>> 4 files changed, 737 insertions(+)
>> create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.c
>> create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.h
>
> This set is giving me obvious checkpatch warnings, something that is quite
> disappointing since 1) you had plenty of time to fix those and 2) someone with
> your kind of upstream experience should know better.
>
> Despite the above I have decided to review this patch but next time I will simply
> ignore your submission.
>
Hi Mathieu,
Sorry for my fault, I'll pay attention to these things latter, thanks.
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index 514a9b8086e3..d24a5d95153a 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -201,4 +201,16 @@ config CORESIGHT_TRBE
>>
>> To compile this driver as a module, choose M here: the module will be
>> called coresight-trbe.
>> +
>> +config ULTRASOC_SMB
>> + tristate "Ultrasoc system memory buffer drivers"
>> + depends on ARM64 && CORESIGHT
>
> You don't need to depend on CORESIGHT since this is already enclosed in "if
> CORESIGHT". But like other sink, this should depend on
> CORESIGHT_LINKS_AND_SINKS.
>
Got it, will fix this.
>> + help
>> + This driver provides support for the Ultrasoc system memory buffer (SMB).
>> + SMB is responsible for receiving the trace data from Coresight ETM devices
>> + and storing them to a system buffer.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called ultrasoc-smb.
>> +
>> endif
>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> index b6c4a48140ec..2899e5a5d5cd 100644
>> --- a/drivers/hwtracing/coresight/Makefile
>> +++ b/drivers/hwtracing/coresight/Makefile
>> @@ -27,3 +27,5 @@ obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o
>> obj-$(CONFIG_CORESIGHT_TRBE) += coresight-trbe.o
>> coresight-cti-y := coresight-cti-core.o coresight-cti-platform.o \
>> coresight-cti-sysfs.o
>> +
>
> Extra newline.
got it, thanks.
>
>> +obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
>> new file mode 100644
>> index 000000000000..b477fc51fbbb
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
>> @@ -0,0 +1,607 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright (C) 2021 HiSilicon Limited.
>> + *
>> + * Code herein communicates with and accesses proprietary hardware which is
>> + * licensed intellectual property (IP) belonging to Siemens Digital Industries
>> + * Software Ltd.
>> + *
>> + * Siemens Digital Industries Software Ltd. asserts and reserves all rights to
>> + * their intellectual property. This paragraph may not be removed or modified
>> + * in any way without permission from Siemens Digital Industries Software Ltd.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/circ_buf.h>
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "ultrasoc-smb.h"
>> +
>> +DEFINE_CORESIGHT_DEVLIST(sink_devs, "sink_smb");
>
> DEFINE_CORESIGHT_DEVLIST(sink_devs, "smb");
> got it, will change this.
> More comments at the very bottom of the file.
>
>> +
[...]
>> +static const struct acpi_device_id ultrasoc_smb_acpi_match[] = {
>> + {"HISI03A1", 0},
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(acpi, ultrasoc_smb_acpi_match);
>
> Please enclose this in #ifdef CONFIG_ACPI as it is done for the funnel and
> replicator drivers.
>
got it, thanks.
>> +
>> +static struct platform_driver smb_driver = {
>> + .driver = {
>> + .name = "ultrasoc,smb",
>
> "ultrasoc-smb"
>
got it.
>> + .acpi_match_table = ACPI_PTR(ultrasoc_smb_acpi_match),
>> + .suppress_bind_attrs = true,
>> + },
>> + .probe = smb_probe,
>> + .remove = smb_remove,
>> +};
>> +module_platform_driver(smb_driver);
>> +
>> +MODULE_DESCRIPTION("Ultrasoc smb driver");
>> +MODULE_LICENSE("Dual MIT/GPL");
>
> The dual licence is not reflected properly in the SPDX tag at the top of the
> file. Please read Documentation/process/license-rules.rst.
>
I'll fix this next version, thanks.
>> +MODULE_AUTHOR("Jonathan Zhou <jonathan.zhouwen@...wei.com>");
>> +MODULE_AUTHOR("Qi Liu <liuqi115@...wei.com>");
[...]
>> +/**
>> + * struct smb_drv_data - specifics associated to an SMB component
>> + * @base: Memory mapped base address for SMB component.
>> + * @csdev: Component vitals needed by the framework.
>> + * @sdb: Data buffer for SMB.
>> + * @miscdev: Specifics to handle "/dev/xyz.tmc" entry.
>
> What tmc entry would this be?
here is a typo, misdev should be an entry to handle /dev/smbx, will fix
this, thanks.
>
>> + * @spinlock: Only one at a time pls.
>> + * @reading: Synchronise user space access to SMB buffer.
>> + * @pid: Process ID of the process being monitored by the session
>> + * that is using this component.
>> + * @mode: how this SMB is being used, perf mode or sysfs mode.
>> + */
>> +struct smb_drv_data {
>> + void __iomem *base;
>> + struct coresight_device *csdev;
>> + struct smb_data_buffer sdb;
>> + struct miscdevice miscdev;
>> + spinlock_t spinlock;
>> + local_t reading;
>> + pid_t pid;
>> + u32 mode;
>> +};
>> +
>> +#define smb_simple_func(type, name, offset) \
>> +static ssize_t name##_show(struct device *_dev, \
>> + struct device_attribute *attr, char *buf) \
>> +{ \
>> + type *drvdata = dev_get_drvdata(_dev->parent); \
>> + u32 val = readl(drvdata->base + offset); \
>> + \
>> + return sysfs_emit(buf, "0x%x\n", val); \
>> +} \
>> +static DEVICE_ATTR_RO(name)
>
> I'm pretty sure you can re-use coresight_tmc_reg() instead of writing
> another macro. I suggest to look at coresight_tmc_reg() in coresight-tmc-core.c
> for an example.
>
got it, I've check coresight_tmc_reg and will use this next version.
> I am out of time for today - I will continue tomorrow.
Thanks for reviewing this patch.
Qi
>
> Thanks,
> Mathieu
>
>> +
>> +#endif
>> --
>> 2.33.0
>>
> .
>
Powered by blists - more mailing lists