[PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer driver
liuqi (BA)
liuqi115 at huawei.com
Fri Mar 25 01:25:39 PDT 2022
On 2022/3/24 20:37, Suzuki Kuruppassery Poulose wrote:
> 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 at huawei.com>
>>>> Tested-by: JunHao He <hejunhao2 at hisilicon.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: }
>
Hi Suzuki，
got it, I'll increment the refcount in perf/sysfs session next version.
Thanks,
Qi
>
> Suzuki
> .
More information about the linux-arm-kernel
mailing list