Messages in this thread | | | Date | Thu, 24 Mar 2022 12:37:30 +0000 | Subject | Re: [PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer driver | From | Suzuki Kuruppassery Poulose <> |
| |
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@huawei.com> >>> Tested-by: JunHao He <hejunhao2@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: }
Suzuki
| |