| 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: <5d1bc111-54f1-c603-e8f4-ef28cc7cef81@arm.com>
Date: Thu, 24 Mar 2022 12:37:30 +0000
From: Suzuki Kuruppassery Poulose <suzuki.poulose@....com>
To: "liuqi (BA)" <liuqi115@...wei.com>, mathieu.poirier@...aro.org,
mike.leach@...aro.org
Cc: coresight@...ts.linaro.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linuxarm@...wei.com
Subject: Re: [PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer
driver
On 14/03/2022 10:04, liuqi (BA) wrote:
>
>
> On 2022/3/10 6:49, Suzuki K Poulose wrote:
>> Hi
>>
>> On 28/01/2022 06:17, 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>
>>
>> Please find my comments below. In general :
>>
>> 1) Please run checkpatch and fix the warnings
>> 2) Add documentation for :
>> - A brief description of the device and the firmware bindings
>> (see comments below )
>> - Sysfs attributes
>> 3) Other minor comments on the code
>>
> Hi Suzuki,
>
> thanks a lot for your review! and some replies inline.
>>> ---
> [...]
>>> --- a/drivers/hwtracing/coresight/Kconfig
>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>> @@ -201,4 +201,14 @@ 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_LINKS_AND_SINKS
>>
>> Given that this driver absolutely only works with ACPI, please could
>> you add that dependency here ?
>
> yes, I'll add this, thanks.
>>
>>> + 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..344dba8d6ff8 100644
>>> --- a/drivers/hwtracing/coresight/Makefile
>>> +++ b/drivers/hwtracing/coresight/Makefile
>>> @@ -27,3 +27,4 @@ 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
>>> +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..a18d061aab61
>>> --- /dev/null
>>> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
>>> @@ -0,0 +1,602 @@
>>> +// SPDX-License-Identifier: MIT/GPL
>>> +/*
>>> + * Siemens System Memory Buffer driver.
>>> + * Copyright(c) 2022, HiSilicon Limited.
>>> + */
>>> +
>>> +#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, "smb");
>>
>> minor nit: This name could be confused with SMB (samba) under linux
>> especially when it appears under /dev/smbN as a misc device.
>>
> got it, I'll rename this device. How about /dev/ultra_smbN?
>>> +
>>> +static bool smb_buffer_is_empty(struct smb_drv_data *drvdata)
>>> +{
>>> + u32 buf_status = readl(drvdata->base + SMB_LB_INT_STS);
>>> +
>>> + return buf_status & BIT(0) ? false : true;
>>> +}
>>> +
>
> [...]
>
>>> +
>>> +static ssize_t smb_read(struct file *file, char __user *data, size_t
>>> len, loff_t *ppos)
>>> +{
>>> + struct smb_drv_data *drvdata = container_of(file->private_data,
>>> + struct smb_drv_data, miscdev);
>>> + struct smb_data_buffer *sdb = &drvdata->sdb;
>>> + struct device *dev = &drvdata->csdev->dev;
>>> + unsigned long flags;
>>> + int to_copy = 0;
>>> +
>>> + spin_lock_irqsave(&drvdata->spinlock, flags);
>>> +
>>> + if (!sdb->data_size) {
>>> + smb_update_data_size(drvdata);
>>> + if (!sdb->data_size)
>>> + goto out;
>>> + }
>>> +
>>> + if (atomic_read(drvdata->csdev->refcnt)) {
>>> + to_copy = -EBUSY;
>>> + goto out;
>>> + }
>>> +
>>
>> Shouldn't this be performed *before* updating the data_size above ?
>> Looking at the smb_update_data_size() it "purges" the data. Is that
>> acceptable ?
>
> ah, yes, we should get this before updating the data_size. I'll fix
> this, thanks.
>>
>>> + to_copy = min(sdb->data_size, len);
>>> +
>>> + /* Copy parts of trace data when the read pointer will wrap
>>> around SMB buffer. */
>>> + if (sdb->rd_offset + to_copy > sdb->buf_size)
>
> [...]
>>> +static struct attribute *smb_sink_attrs[] = {
>>> + &dev_attr_read_pos.attr,
>>> + &dev_attr_write_pos.attr,
>>> + &dev_attr_buf_status.attr,
>>> + &dev_attr_buf_size.attr,
>>> + NULL,
>>> +};
>>
>> Please could you also update the sysfs ABI files with a brief
>> description of the above attributes ?
>>
>> e.g., Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc
>>
> yes, sure. I'll do this.
>
>>> +
>>> +static const struct attribute_group smb_sink_group = {
>>> + .attrs = smb_sink_attrs,
>>> + .name = "status",
>>> +};
>>> +
> [...]
>>> +
>>> +static int smb_enable_perf(struct smb_drv_data *drvdata, void *data)
>>> +{
>>> + struct device *dev = &drvdata->csdev->dev;
>>> + struct perf_output_handle *handle = data;
>>> + pid_t pid;
>>> +
>>> + if (drvdata->mode == CS_MODE_SYSFS) {
>>> + dev_err(dev, "Device is already in used by sysfs\n");
>>
>> Please could you convert this to dev_dbg() ? An -EBUSY should
>> be sufficient to indicate that the resource is busy.
>>
> got it, thanks, will fix this.
>
>>> + return -EBUSY;
>>> + }
>>> +
>>> + /* Get a handle on the pid of the target process*/
>>> + pid = task_pid_nr(handle->event->owner);
>>> + if (drvdata->pid != -1 && drvdata->pid != pid) {
>>> + dev_err(dev, "Device is already in used by other session\n");
>>
>> Same here.
>>
> ok.
>>> + return -EBUSY;
>>> + }
>>> + /* The sink is already enabled by this session. */
>>> + if (drvdata->pid == pid)
>>> + return 0;
>>
>> If the sink is shared by multiple events of the same group, don't
>> we need a refcount to make sure we don't disable the buffer when
>> an event goes away ?
>>
>
> we increase this refcount in smb_enable(), if smb_enable_sysfs() or
> smb_enable_perf() is enabled successfully, we increase the refcount.
>
> So, when the sink is shared by multiple events of the same group,
> refcount will be increased too. thanks.
I don't think this the case, at least by looking at the code. You need
to increment the refcount when you find that *this* session is going
to share the sink.
>>> +
>>> + if (smb_set_perf_buffer(handle))
>>> + return -EINVAL;
>>> +
>>> + smb_enable_hw(drvdata);
>>> + drvdata->pid = pid;
>>> + drvdata->mode = CS_MODE_PERF;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int smb_enable(struct coresight_device *csdev, u32 mode, void
>>> *data)
>>> +{
>>> + struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
>>> + unsigned long flags;
>>> + int ret = -EINVAL;
>>> +
>>> + /* Do nothing if trace data is reading by other interface now. */
>>> + if (local_read(&drvdata->reading))
>>> + return -EBUSY;
>>> +
>>> + spin_lock_irqsave(&drvdata->spinlock, flags);
>>> +
>>> + if (mode == CS_MODE_SYSFS)
>>> + ret = smb_enable_sysfs(drvdata);
>>> +
>>> + if (mode == CS_MODE_PERF)
>>> + ret = smb_enable_perf(drvdata, data);
>>> +
>>> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
>>> +
>>> + if (ret)
>>> + return ret;
>>> +
>>> + atomic_inc(csdev->refcnt);
Shouldn't this be done within the spinlock ? Othewise, we could have:
CPU0: smb_enable() {
CPU0: spin_lock()
CPU0: ...
CPU0: spin_unlock()...
CPU0: atomic_inc(refcnt) == 1;
CPU0: }
...
CPU0: smb_disable() { # Note CPU0 started to disable the session
CPU1: smb_enable() {
CPU1: spin_lock()
CPU1: ...
CPU1: spin_unlock()...
CPU0: spin_lock();
# Before the CPU1 increments the counter, CPU0 could see the recnt == 1
# and go ahead to disable the HW. Where this must have been 2.
CPU0: if (!atomic_dec_return()) == 0
CPU0: disable_hw();
CPU1: atomic_inc(refcnt) == 1;
CPU0: spin_unlock()
CPU1: }
Suzuki
Powered by blists - more mailing lists