[PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer driver

[PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer driver

[Qi Liu]

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>
---
Change since v3:
- Modify the file header according to community specifications.
- Address the comments from Mathieu.
- Link:https://lore.kernel.org/linux-arm-kernel/20211118110016.40398-1-liuqi115@huawei.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        |  10 +
 drivers/hwtracing/coresight/Makefile       |   1 +
 drivers/hwtracing/coresight/ultrasoc-smb.c | 602 +++++++++++++++++++++
 drivers/hwtracing/coresight/ultrasoc-smb.h | 100 ++++
 4 files changed, 713 insertions(+)
 create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.c
 create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.h

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 514a9b8086e3..32b87df9994a 100644
--- 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
+	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");
+
+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 bool smb_buffer_cmp_pointer(struct smb_drv_data *drvdata)
+{
+	u32 wr_offset, rd_offset;
+
+	wr_offset = readl(drvdata->base + SMB_LB_WR_ADDR);
+	rd_offset = readl(drvdata->base + SMB_LB_RD_ADDR);
+	return wr_offset == rd_offset;
+}
+
+static void smb_reset_buffer_status(struct smb_drv_data *drvdata)
+{
+	writel(0xf, drvdata->base + SMB_LB_INT_STS);
+}
+
+/* Purge data remaining in hardware path to SMB. */
+static void smb_purge_data(struct smb_drv_data *drvdata)
+{
+	writel(0x1, drvdata->base + SMB_LB_PURGE);
+}
+
+static void smb_update_data_size(struct smb_drv_data *drvdata)
+{
+	struct smb_data_buffer *sdb = &drvdata->sdb;
+	u32 write_offset;
+
+	smb_purge_data(drvdata);
+	if (smb_buffer_cmp_pointer(drvdata)) {
+		if (smb_buffer_is_empty(drvdata))
+			sdb->data_size = 0;
+		else
+			sdb->data_size = sdb->buf_size;
+		return;
+	}
+
+	write_offset = readl(drvdata->base + SMB_LB_WR_ADDR) - sdb->start_addr;
+	sdb->data_size = CIRC_CNT(write_offset, sdb->rd_offset, sdb->buf_size);
+}
+
+static int smb_open(struct inode *inode, struct file *file)
+{
+	struct smb_drv_data *drvdata = container_of(file->private_data,
+						    struct smb_drv_data, miscdev);
+
+	if (local_cmpxchg(&drvdata->reading, 0, 1))
+		return -EBUSY;
+
+	return 0;
+}
+
+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;
+	}
+
+	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)
+		to_copy = sdb->buf_size - sdb->rd_offset;
+
+	if (copy_to_user(data, (void *)sdb->buf_base + sdb->rd_offset, to_copy)) {
+		dev_dbg(dev, "Failed to copy data to user\n");
+		to_copy = -EFAULT;
+		goto out;
+	}
+
+	*ppos += to_copy;
+	sdb->data_size -= to_copy;
+	sdb->rd_offset += to_copy;
+	sdb->rd_offset %= sdb->buf_size;
+	writel(sdb->start_addr + sdb->rd_offset, drvdata->base + SMB_LB_RD_ADDR);
+	dev_dbg(dev, "%d bytes copied\n", to_copy);
+out:
+	if (!sdb->data_size)
+		smb_reset_buffer_status(drvdata);
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+	return to_copy;
+}
+
+static int smb_release(struct inode *inode, struct file *file)
+{
+	struct smb_drv_data *drvdata = container_of(file->private_data,
+						    struct smb_drv_data, miscdev);
+	local_set(&drvdata->reading, 0);
+	return 0;
+}
+
+static const struct file_operations smb_fops = {
+	.owner		= THIS_MODULE,
+	.open		= smb_open,
+	.read		= smb_read,
+	.release	= smb_release,
+	.llseek		= no_llseek,
+};
+
+smb_reg(read_pos, SMB_LB_RD_ADDR);
+smb_reg(write_pos, SMB_LB_WR_ADDR);
+smb_reg(buf_status, SMB_LB_INT_STS);
+
+static ssize_t buf_size_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct smb_drv_data *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "0x%lx\n", drvdata->sdb.buf_size);
+}
+static DEVICE_ATTR_RO(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,
+};
+
+static const struct attribute_group smb_sink_group = {
+	.attrs = smb_sink_attrs,
+	.name = "status",
+};
+
+static const struct attribute_group *smb_sink_groups[] = {
+	&smb_sink_group,
+	NULL,
+};
+
+static int smb_set_perf_buffer(struct perf_output_handle *handle)
+{
+	struct cs_buffers *buf = etm_perf_sink_config(handle);
+	u32 head;
+
+	if (!buf)
+		return -EINVAL;
+
+	/* Wrap head around to the amount of space we have */
+	head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
+
+	/* Find the page to write to and offset within that page */
+	buf->cur = head / PAGE_SIZE;
+	buf->offset = head % PAGE_SIZE;
+
+	local_set(&buf->data_size, 0);
+
+	return 0;
+}
+
+static void smb_enable_hw(struct smb_drv_data *drvdata)
+{
+	writel(0x1, drvdata->base + SMB_GLOBAL_EN);
+}
+
+static void smb_disable_hw(struct smb_drv_data *drvdata)
+{
+	writel(0x0, drvdata->base + SMB_GLOBAL_EN);
+}
+
+static int smb_enable_sysfs(struct smb_drv_data *drvdata)
+{
+	if (drvdata->mode == CS_MODE_PERF)
+		return -EBUSY;
+
+	if (drvdata->mode == CS_MODE_SYSFS)
+		return 0;
+
+	smb_enable_hw(drvdata);
+	drvdata->mode = CS_MODE_SYSFS;
+	return 0;
+}
+
+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");
+		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");
+		return -EBUSY;
+	}
+	/* The sink is already enabled by this session. */
+	if (drvdata->pid == pid)
+		return 0;
+
+	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);
+	dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
+
+	return 0;
+}
+
+static int smb_disable(struct coresight_device *csdev)
+{
+	struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
+	unsigned long flags;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+
+	if (atomic_dec_return(csdev->refcnt)) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		return -EBUSY;
+	}
+
+	WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
+	smb_disable_hw(drvdata);
+
+	/*
+	 * Data remaining in hardware path will be sent to SMB after purge, so needs to
+	 * synchronize the read pointer to write pointer in perf mode.
+	 */
+	smb_purge_data(drvdata);
+	if (drvdata->mode == CS_MODE_PERF)
+		writel(readl(drvdata->base + SMB_LB_WR_ADDR), drvdata->base + SMB_LB_RD_ADDR);
+
+	/* Dissociate from the target process. */
+	drvdata->pid = -1;
+	drvdata->mode = CS_MODE_DISABLED;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
+	return 0;
+}
+
+static void *smb_alloc_buffer(struct coresight_device *csdev,
+			      struct perf_event *event, void **pages,
+			      int nr_pages, bool overwrite)
+{
+	struct cs_buffers *buf;
+	int node;
+
+	node = (event->cpu == -1) ? NUMA_NO_NODE : cpu_to_node(event->cpu);
+	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, node);
+	if (!buf)
+		return NULL;
+
+	buf->snapshot = overwrite;
+	buf->nr_pages = nr_pages;
+	buf->data_pages = pages;
+
+	return buf;
+}
+
+static void smb_free_buffer(void *config)
+{
+	struct cs_buffers *buf = config;
+
+	kfree(buf);
+}
+
+static void smb_sync_perf_buffer(struct smb_drv_data *drvdata,
+				 struct cs_buffers *buf, unsigned long data_size)
+{
+	struct smb_data_buffer *sdb = &drvdata->sdb;
+	char **dst_pages = (char **)buf->data_pages;
+	unsigned long buf_offset = buf->offset;
+	unsigned int cur = buf->cur;
+	unsigned long to_copy;
+
+	while (data_size) {
+		/* Copy parts of trace data when the read pointer will wrap around SMB buffer. */
+		if (sdb->rd_offset + PAGE_SIZE - buf_offset > sdb->buf_size)
+			to_copy = sdb->buf_size - sdb->rd_offset;
+		else
+			to_copy = min(data_size, PAGE_SIZE - buf_offset);
+
+		memcpy_fromio(dst_pages[cur] + buf_offset, sdb->buf_base + sdb->rd_offset, to_copy);
+
+		buf_offset += to_copy;
+		if (buf_offset >= PAGE_SIZE) {
+			buf_offset = 0;
+			cur++;
+			cur %= buf->nr_pages;
+		}
+		data_size -= to_copy;
+		/* ensure memcpy finished before update the read pointer */
+		sdb->rd_offset += to_copy;
+		sdb->rd_offset %= sdb->buf_size;
+	}
+
+	sdb->data_size = 0;
+	writel(sdb->start_addr + sdb->rd_offset, drvdata->base + SMB_LB_RD_ADDR);
+	smb_reset_buffer_status(drvdata);
+}
+
+static unsigned long smb_update_buffer(struct coresight_device *csdev,
+				       struct perf_output_handle *handle,
+				       void *sink_config)
+{
+	struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
+	struct smb_data_buffer *sdb = &drvdata->sdb;
+	struct cs_buffers *buf = sink_config;
+	unsigned long data_size = 0;
+	unsigned long flags;
+	bool lost = false;
+
+	if (!buf)
+		return 0;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+
+	/* Don't do anything if another tracer is using this sink. */
+	if (atomic_read(csdev->refcnt) != 1)
+		goto out;
+
+	smb_update_data_size(drvdata);
+	data_size = sdb->data_size;
+	if (data_size > handle->size) {
+		sdb->rd_offset += data_size - handle->size;
+		sdb->rd_offset %= sdb->buf_size;
+		data_size = handle->size;
+		lost = true;
+	}
+
+	smb_sync_perf_buffer(drvdata, buf, data_size);
+	if (!buf->snapshot && lost)
+		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
+
+out:
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+	return data_size;
+}
+
+static const struct coresight_ops_sink smb_cs_ops = {
+	.enable		= smb_enable,
+	.disable	= smb_disable,
+	.alloc_buffer	= smb_alloc_buffer,
+	.free_buffer	= smb_free_buffer,
+	.update_buffer	= smb_update_buffer,
+};
+
+static const struct coresight_ops cs_ops = {
+	.sink_ops	= &smb_cs_ops,
+};
+
+static int smb_init_data_buffer(struct platform_device *pdev, struct smb_data_buffer *sdb)
+{
+	struct resource *res;
+	void __iomem *base;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (IS_ERR(res)) {
+		dev_err(&pdev->dev, "SMB device failed to get resource\n");
+		return -EINVAL;
+	}
+
+	sdb->start_addr = res->start & SMB_BASE_LOW_MASK;
+	sdb->buf_size = resource_size(res);
+	if (sdb->buf_size == 0)
+		return -EINVAL;
+
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	sdb->buf_base = base;
+
+	return 0;
+}
+
+static void smb_init_hw(struct smb_drv_data *drvdata)
+{
+	u32 value;
+
+	/* First disable smb and clear the status of SMB buffer */
+	smb_reset_buffer_status(drvdata);
+	smb_disable_hw(drvdata);
+	smb_purge_data(drvdata);
+
+	/* Using smb in single-end mode, and set other configures default */
+	value = SMB_BUF_CFG_STREAMING | SMB_BUF_SINGLE_END | SMB_BUF_EN;
+	writel(value, drvdata->base + SMB_LB_CFG_LO);
+	value = SMB_MSG_FILTER(0x0, 0xff);
+	writel(value, drvdata->base + SMB_LB_CFG_HI);
+
+	writel(SMB_GLOBAL_CFG, drvdata->base + SMB_CFG_REG);
+	writel(SMB_GLB_INT_CFG, drvdata->base + SMB_GLOBAL_INT);
+	writel(SMB_BUF_INT_CFG, drvdata->base + SMB_LB_INT_CTRL);
+}
+
+static int smb_register_sink(struct platform_device *pdev, struct smb_drv_data *drvdata)
+{
+	struct coresight_platform_data *pdata = NULL;
+	struct coresight_desc desc = { 0 };
+	int ret;
+
+	pdata = coresight_get_platform_data(&pdev->dev);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
+
+	desc.type = CORESIGHT_DEV_TYPE_SINK;
+	desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
+	desc.ops = &cs_ops;
+	desc.pdata = pdata;
+	desc.dev = &pdev->dev;
+	desc.groups = smb_sink_groups;
+	desc.name = coresight_alloc_device_name(&sink_devs, &pdev->dev);
+	if (!desc.name) {
+		dev_err(&pdev->dev, "Failed to alloc coresight device name");
+		return -ENOMEM;
+	}
+
+	drvdata->csdev = coresight_register(&desc);
+	if (IS_ERR(drvdata->csdev))
+		return PTR_ERR(drvdata->csdev);
+
+	drvdata->miscdev.name = desc.name;
+	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
+	drvdata->miscdev.fops = &smb_fops;
+	ret = misc_register(&drvdata->miscdev);
+	if (ret) {
+		coresight_unregister(drvdata->csdev);
+		dev_err(&pdev->dev, "Failed to register misc: %d\n", ret);
+	}
+
+	return ret;
+}
+
+static void smb_unregister_sink(struct smb_drv_data *drvdata)
+{
+	misc_deregister(&drvdata->miscdev);
+	coresight_unregister(drvdata->csdev);
+}
+
+/*
+ * Send ultrasoc messge to control hardwares on the tracing path,
+ * using DSM calls to avoid exposing ultrasoc message format.
+ */
+static int smb_config_inport(struct device *dev, bool enable)
+{
+	u32 flag = enable ? 1 : 0;
+	union acpi_object *obj;
+	guid_t guid;
+
+	if (guid_parse("82ae1283-7f6a-4cbe-aa06-53e8fb24db18", &guid)) {
+		dev_err(dev, "Get GUID failed\n");
+		return -EINVAL;
+	}
+
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(dev), &guid, 0, flag, NULL);
+	if (!obj)
+		dev_err(dev, "ACPI handle failed\n");
+	else
+		ACPI_FREE(obj);
+
+	return 0;
+}
+
+static int smb_probe(struct platform_device *pdev)
+{
+	struct smb_drv_data *drvdata;
+	int ret;
+
+	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(drvdata->base)) {
+		dev_err(&pdev->dev, "Failed to ioremap resource\n");
+		return PTR_ERR(drvdata->base);
+	}
+
+	ret = smb_init_data_buffer(pdev, &drvdata->sdb);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to init buffer: %d\n", ret);
+		return ret;
+	}
+
+	smb_init_hw(drvdata);
+	spin_lock_init(&drvdata->spinlock);
+	drvdata->pid = -1;
+
+	ret = smb_register_sink(pdev, drvdata);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register smb sink\n");
+		return ret;
+	}
+
+	ret = smb_config_inport(&pdev->dev, true);
+	if (ret) {
+		smb_unregister_sink(drvdata);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, drvdata);
+	return 0;
+}
+
+static int smb_remove(struct platform_device *pdev)
+{
+	struct smb_drv_data *drvdata = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = smb_config_inport(&pdev->dev, false);
+	if (ret)
+		return ret;
+
+	smb_unregister_sink(drvdata);
+	return 0;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id ultrasoc_smb_acpi_match[] = {
+	{"HISI03A1", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, ultrasoc_smb_acpi_match);
+#endif
+
+static struct platform_driver smb_driver = {
+	.driver = {
+		.name = "ultrasoc-smb",
+		.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");
+MODULE_AUTHOR("Jonathan Zhou <jonathan.zhouwen@huawei.com>");
+MODULE_AUTHOR("Qi Liu <liuqi115@huawei.com>");
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h
new file mode 100644
index 000000000000..a7c76140ee46
--- /dev/null
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Siemens System Memory Buffer driver.
+ * Copyright(c) 2022, HiSilicon Limited.
+ */
+
+#ifndef _ULTRASOC_SMB_H
+#define _ULTRASOC_SMB_H
+
+#include <linux/coresight.h>
+#include <linux/list.h>
+#include <linux/miscdevice.h>
+
+#include "coresight-etm-perf.h"
+#include "coresight-priv.h"
+
+/* Offset of SMB logical buffer registers */
+#define SMB_CFG_REG			0X0
+#define SMB_GLOBAL_EN			0X4
+#define SMB_GLOBAL_INT			0X8
+#define SMB_LB_CFG_LO			0X40
+#define SMB_LB_CFG_HI			0X44
+#define SMB_LB_INT_CTRL			0X48
+#define SMB_LB_INT_STS			0X4C
+#define SMB_LB_LIMIT			0X58
+#define SMB_LB_RD_ADDR			0X5C
+#define SMB_LB_WR_ADDR			0X60
+#define SMB_LB_PURGE			0X64
+
+/* Set SMB_CFG_REG register */
+#define SMB_IDLE_PRD(period)		((((period) - 216) & 0xf) << 12)
+#define SMB_MEM_WR(credit, rate)	(((credit) << 16) | ((rate) << 18))
+#define SMB_MEM_RD(credit, rate)	(((credit) << 22) | ((rate) << 24))
+#define SMB_BURST_LEN(len)		(((len) - 1) << 4)
+#define SMB_GLOBAL_CFG			(SMB_IDLE_PRD(231) | SMB_MEM_WR(0x3, 0x0) |   \
+					 SMB_MEM_RD(0x3, 0x6) | SMB_BURST_LEN(16))
+
+/* Set SMB_GLOBAL_INT register */
+#define SMB_INT_EN			BIT(0)
+#define SMB_INT_TYPE_PULSE		BIT(1)
+#define SMB_INT_POLARITY_HIGH		BIT(2)
+#define SMB_GLB_INT_CFG			(SMB_INT_EN | SMB_INT_TYPE_PULSE | SMB_INT_POLARITY_HIGH)
+
+/* Set SMB_LB_CFG_LO register */
+#define SMB_BUF_EN			BIT(0)
+#define SMB_BUF_SINGLE_END		BIT(1)
+#define SMB_BUF_INIT			BIT(8)
+#define SMB_BUF_CONTINUOUS		BIT(11)
+#define SMB_FLOW_MASK			GENMASK(19, 16)
+#define SMB_BUF_CFG_STREAMING		(SMB_BUF_INIT | SMB_BUF_CONTINUOUS | SMB_FLOW_MASK)
+#define SMB_BASE_LOW_MASK		GENMASK(31, 0)
+
+/* Set SMB_LB_CFG_HI register */
+#define SMB_MSG_FILTER(lower, upper)	(((lower) & 0xff) | (((upper) & 0xff) << 8))
+#define SMB_BUF_INT_EN			BIT(0)
+#define SMB_BUF_NOTE_MASK		GENMASK(11, 8)
+#define SMB_BUF_INT_CFG			(SMB_BUF_INT_EN | SMB_BUF_NOTE_MASK)
+
+/**
+ * struct smb_data_buffer - Details of the buffer used by SMB
+ * @buf_base	: Memory mapped base address of SMB.
+ * @start_addr	: SMB buffer start Physical address.
+ * @buf_size	: Size of the buffer.
+ * @data_size	: Size of Trace data copy to userspace.
+ * @rd_offset	: Offset of the read pointer in the buffer.
+ */
+struct smb_data_buffer {
+	void __iomem *buf_base;
+	u32 start_addr;
+	unsigned long buf_size;
+	unsigned long data_size;
+	unsigned long rd_offset;
+};
+
+/**
+ * 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/smb" entry.
+ * @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_reg(name, offset)  coresight_simple_reg32(struct smb_drv_data, name, offset)
+
+#endif
-- 
2.24.0

Re: [PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer driver

[Suzuki K Poulose]

Hi Qi

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>


Apologies for the delay in getting to review this patch. I will take a 
look at it next. Thanks for your patience.


Kind regard
Suzuki

Re: [PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer driver

[Suzuki K Poulose]

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

> ---
> Change since v3:
> - Modify the file header according to community specifications.
> - Address the comments from Mathieu.
> - Link:https://lore.kernel.org/linux-arm-kernel/20211118110016.40398-1-liuqi115@huawei.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        |  10 +
>   drivers/hwtracing/coresight/Makefile       |   1 +
>   drivers/hwtracing/coresight/ultrasoc-smb.c | 602 +++++++++++++++++++++
>   drivers/hwtracing/coresight/ultrasoc-smb.h | 100 ++++
>   4 files changed, 713 insertions(+)
>   create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.c
>   create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.h
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 514a9b8086e3..32b87df9994a 100644
> --- 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 ?

> +	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.

> +
> +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 bool smb_buffer_cmp_pointer(struct smb_drv_data *drvdata)
> +{
> +	u32 wr_offset, rd_offset;
> +
> +	wr_offset = readl(drvdata->base + SMB_LB_WR_ADDR);
> +	rd_offset = readl(drvdata->base + SMB_LB_RD_ADDR);
> +	return wr_offset == rd_offset;
> +}
> +
> +static void smb_reset_buffer_status(struct smb_drv_data *drvdata)
> +{
> +	writel(0xf, drvdata->base + SMB_LB_INT_STS);
> +}
> +
> +/* Purge data remaining in hardware path to SMB. */
> +static void smb_purge_data(struct smb_drv_data *drvdata)
> +{
> +	writel(0x1, drvdata->base + SMB_LB_PURGE);
> +}
> +
> +static void smb_update_data_size(struct smb_drv_data *drvdata)
> +{
> +	struct smb_data_buffer *sdb = &drvdata->sdb;
> +	u32 write_offset;
> +
> +	smb_purge_data(drvdata);
> +	if (smb_buffer_cmp_pointer(drvdata)) {
> +		if (smb_buffer_is_empty(drvdata))
> +			sdb->data_size = 0;
> +		else
> +			sdb->data_size = sdb->buf_size;
> +		return;
> +	}
> +
> +	write_offset = readl(drvdata->base + SMB_LB_WR_ADDR) - sdb->start_addr;
> +	sdb->data_size = CIRC_CNT(write_offset, sdb->rd_offset, sdb->buf_size);
> +}
> +
> +static int smb_open(struct inode *inode, struct file *file)
> +{
> +	struct smb_drv_data *drvdata = container_of(file->private_data,
> +						    struct smb_drv_data, miscdev);
> +
> +	if (local_cmpxchg(&drvdata->reading, 0, 1))
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
> +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 ?

> +	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)
> +		to_copy = sdb->buf_size - sdb->rd_offset;
> +
> +	if (copy_to_user(data, (void *)sdb->buf_base + sdb->rd_offset, to_copy)) {
> +		dev_dbg(dev, "Failed to copy data to user\n");
> +		to_copy = -EFAULT;
> +		goto out;
> +	}
> +
> +	*ppos += to_copy;
> +	sdb->data_size -= to_copy;
> +	sdb->rd_offset += to_copy;
> +	sdb->rd_offset %= sdb->buf_size;
> +	writel(sdb->start_addr + sdb->rd_offset, drvdata->base + SMB_LB_RD_ADDR);
> +	dev_dbg(dev, "%d bytes copied\n", to_copy);
> +out:
> +	if (!sdb->data_size)
> +		smb_reset_buffer_status(drvdata);
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +	return to_copy;
> +}
> +
> +static int smb_release(struct inode *inode, struct file *file)
> +{
> +	struct smb_drv_data *drvdata = container_of(file->private_data,
> +						    struct smb_drv_data, miscdev);
> +	local_set(&drvdata->reading, 0);
> +	return 0;
> +}
> +
> +static const struct file_operations smb_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= smb_open,
> +	.read		= smb_read,
> +	.release	= smb_release,
> +	.llseek		= no_llseek,
> +};
> +
> +smb_reg(read_pos, SMB_LB_RD_ADDR);
> +smb_reg(write_pos, SMB_LB_WR_ADDR);
> +smb_reg(buf_status, SMB_LB_INT_STS);
> +
> +static ssize_t buf_size_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct smb_drv_data *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "0x%lx\n", drvdata->sdb.buf_size);
> +}
> +static DEVICE_ATTR_RO(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

> +
> +static const struct attribute_group smb_sink_group = {
> +	.attrs = smb_sink_attrs,
> +	.name = "status",
> +};
> +
> +static const struct attribute_group *smb_sink_groups[] = {
> +	&smb_sink_group,
> +	NULL,
> +};
> +
> +static int smb_set_perf_buffer(struct perf_output_handle *handle)
> +{
> +	struct cs_buffers *buf = etm_perf_sink_config(handle);
> +	u32 head;
> +
> +	if (!buf)
> +		return -EINVAL;
> +
> +	/* Wrap head around to the amount of space we have */
> +	head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
> +
> +	/* Find the page to write to and offset within that page */
> +	buf->cur = head / PAGE_SIZE;
> +	buf->offset = head % PAGE_SIZE;
> +
> +	local_set(&buf->data_size, 0);
> +
> +	return 0;
> +}
> +
> +static void smb_enable_hw(struct smb_drv_data *drvdata)
> +{
> +	writel(0x1, drvdata->base + SMB_GLOBAL_EN);
> +}
> +
> +static void smb_disable_hw(struct smb_drv_data *drvdata)
> +{
> +	writel(0x0, drvdata->base + SMB_GLOBAL_EN);
> +}
> +
> +static int smb_enable_sysfs(struct smb_drv_data *drvdata)
> +{
> +	if (drvdata->mode == CS_MODE_PERF)
> +		return -EBUSY;
> +
> +	if (drvdata->mode == CS_MODE_SYSFS)
> +		return 0;
> +
> +	smb_enable_hw(drvdata);
> +	drvdata->mode = CS_MODE_SYSFS;
> +	return 0;
> +}
> +
> +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.

> +		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.

> +		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 ?

> +
> +	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);
> +	dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
> +
> +	return 0;
> +}
> +
> +static int smb_disable(struct coresight_device *csdev)
> +{
> +	struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +
> +	if (atomic_dec_return(csdev->refcnt)) {

We need to increment the refcnt for every event that shares the
sink for perf mode. Please see my comments above.

> +		spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		return -EBUSY;
> +	}
> +
> +	WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> +	smb_disable_hw(drvdata);
> +
> +	/*
> +	 * Data remaining in hardware path will be sent to SMB after purge, so needs to
> +	 * synchronize the read pointer to write pointer in perf mode.

Please run checkpatch on your submission ^^^


> +	 */
> +	smb_purge_data(drvdata);
> +	if (drvdata->mode == CS_MODE_PERF)
> +		writel(readl(drvdata->base + SMB_LB_WR_ADDR), drvdata->base + SMB_LB_RD_ADDR);

Same here. Why don't we do this for CS_MODE_SYSFS ?

> +
> +	/* Dissociate from the target process. */
> +	drvdata->pid = -1;
> +	drvdata->mode = CS_MODE_DISABLED;
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
> +	return 0;
> +}
> +
> +static void *smb_alloc_buffer(struct coresight_device *csdev,
> +			      struct perf_event *event, void **pages,
> +			      int nr_pages, bool overwrite)
> +{
> +	struct cs_buffers *buf;
> +	int node;
> +
> +	node = (event->cpu == -1) ? NUMA_NO_NODE : cpu_to_node(event->cpu);
> +	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, node);
> +	if (!buf)
> +		return NULL;
> +
> +	buf->snapshot = overwrite;
> +	buf->nr_pages = nr_pages;
> +	buf->data_pages = pages;
> +
> +	return buf;
> +}
> +
> +static void smb_free_buffer(void *config)
> +{
> +	struct cs_buffers *buf = config;
> +
> +	kfree(buf);
> +}
> +
> +static void smb_sync_perf_buffer(struct smb_drv_data *drvdata,
> +				 struct cs_buffers *buf, unsigned long data_size)
> +{
> +	struct smb_data_buffer *sdb = &drvdata->sdb;
> +	char **dst_pages = (char **)buf->data_pages;
> +	unsigned long buf_offset = buf->offset;

Please could you rename this to page_offset ?

> +	unsigned int cur = buf->cur;
> +	unsigned long to_copy;
> +

> +	while (data_size) {
> +		/* Copy parts of trace data when the read pointer will wrap around SMB buffer. */

Wrap the lines please.

Please could you do :

		unsigned long page_space = (PAGE_SIZE - page_offset);


> +		if (sdb->rd_offset + PAGE_SIZE - buf_offset > sdb->buf_size)

^^		if (sdb->rd_offset + page_space > sdb->buf_size)

> +			to_copy = sdb->buf_size - sdb->rd_offset;
> +		else
> +			to_copy = min(data_size, PAGE_SIZE - buf_offset);

			to_copy = min(data_size, page_space);

> +
> +		memcpy_fromio(dst_pages[cur] + buf_offset, sdb->buf_base + sdb->rd_offset, to_copy);

		memcpy_fromio(dst_pages[cur] + page_offset, ...

> +
> +		buf_offset += to_copy;
> +		if (buf_offset >= PAGE_SIZE) {

That becomes much simpler to follow with the rename to page_offset.

> +			buf_offset = 0;
> +			cur++;
> +			cur %= buf->nr_pages;
> +		}
> +		data_size -= to_copy;
> +		/* ensure memcpy finished before update the read pointer */

Why and how ?

> +		sdb->rd_offset += to_copy;
> +		sdb->rd_offset %= sdb->buf_size;
> +	}
> +
> +	sdb->data_size = 0;
> +	writel(sdb->start_addr + sdb->rd_offset, drvdata->base + SMB_LB_RD_ADDR);
> +	smb_reset_buffer_status(drvdata);
> +}
> +
> +static unsigned long smb_update_buffer(struct coresight_device *csdev,
> +				       struct perf_output_handle *handle,
> +				       void *sink_config)
> +{
> +	struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct smb_data_buffer *sdb = &drvdata->sdb;
> +	struct cs_buffers *buf = sink_config;
> +	unsigned long data_size = 0;
> +	unsigned long flags;
> +	bool lost = false;
> +
> +	if (!buf)
> +		return 0;
> +
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +
> +	/* Don't do anything if another tracer is using this sink. */
> +	if (atomic_read(csdev->refcnt) != 1)
> +		goto out;
> +
> +	smb_update_data_size(drvdata);
> +	data_size = sdb->data_size;

Please could you add in a comment on what we do here below ?

> +	if (data_size > handle->size)  > +		sdb->rd_offset += data_size - handle->size;
> +		sdb->rd_offset %= sdb->buf_size;
> +		data_size = handle->size;
> +		lost = true;
> +	}
> +
> +	smb_sync_perf_buffer(drvdata, buf, data_size);
> +	if (!buf->snapshot && lost)
> +		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +
> +out:
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +	return data_size;
> +}
> +
> +static const struct coresight_ops_sink smb_cs_ops = {
> +	.enable		= smb_enable,
> +	.disable	= smb_disable,
> +	.alloc_buffer	= smb_alloc_buffer,
> +	.free_buffer	= smb_free_buffer,
> +	.update_buffer	= smb_update_buffer,
> +};
> +
> +static const struct coresight_ops cs_ops = {
> +	.sink_ops	= &smb_cs_ops,
> +};
> +
> +static int smb_init_data_buffer(struct platform_device *pdev, struct smb_data_buffer *sdb)
> +{
> +	struct resource *res;
> +	void __iomem *base;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);

What are these IO bases ? What does 0 stand for ? What is 1 for ?

Please could you add a documentation under

Documentation/trace/coresight/.

Doesn't need to be too detailed. But a basic description would be helpful.


> +	if (IS_ERR(res)) {
> +		dev_err(&pdev->dev, "SMB device failed to get resource\n");
> +		return -EINVAL;
> +	}
> +
> +	sdb->start_addr = res->start & SMB_BASE_LOW_MASK;
> +	sdb->buf_size = resource_size(res);
> +	if (sdb->buf_size == 0)
> +		return -EINVAL;
> +
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	sdb->buf_base = base;
> +
> +	return 0;
> +}
> +
> +static void smb_init_hw(struct smb_drv_data *drvdata)
> +{
> +	u32 value;
> +
> +	/* First disable smb and clear the status of SMB buffer */
> +	smb_reset_buffer_status(drvdata);
> +	smb_disable_hw(drvdata);
> +	smb_purge_data(drvdata);
> +
> +	/* Using smb in single-end mode, and set other configures default */
> +	value = SMB_BUF_CFG_STREAMING | SMB_BUF_SINGLE_END | SMB_BUF_EN;
> +	writel(value, drvdata->base + SMB_LB_CFG_LO);
> +	value = SMB_MSG_FILTER(0x0, 0xff);

Could we not simply say :


> +	writel(value, drvdata->base + SMB_LB_CFG_HI);
> +

	#define SMB_CFG_MSG_FILTER	0xff00

	writel(SMB_CFG_MSG_FILTER, drvdata->base + SMB_LB_CFG_HI) ?


Just like you do below for the other registers ? Anyways, none of these
values make any sense to the reader without the Reference Manual. The
best you could do is stick a sensible name for the value like you do
below.

> +	writel(SMB_GLOBAL_CFG, drvdata->base + SMB_CFG_REG);
> +	writel(SMB_GLB_INT_CFG, drvdata->base + SMB_GLOBAL_INT);
> +	writel(SMB_BUF_INT_CFG, drvdata->base + SMB_LB_INT_CTRL);
> +}
> +
> +static int smb_register_sink(struct platform_device *pdev, struct smb_drv_data *drvdata)
> +{
> +	struct coresight_platform_data *pdata = NULL;
> +	struct coresight_desc desc = { 0 };
> +	int ret;
> +
> +	pdata = coresight_get_platform_data(&pdev->dev);
> +	if (IS_ERR(pdata))
> +		return PTR_ERR(pdata);
> +
> +	desc.type = CORESIGHT_DEV_TYPE_SINK;
> +	desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> +	desc.ops = &cs_ops;
> +	desc.pdata = pdata;
> +	desc.dev = &pdev->dev;
> +	desc.groups = smb_sink_groups; > +	desc.name = coresight_alloc_device_name(&sink_devs, &pdev->dev);
> +	if (!desc.name) {
> +		dev_err(&pdev->dev, "Failed to alloc coresight device name");
> +		return -ENOMEM;
> +	}
> +
> +	drvdata->csdev = coresight_register(&desc);
> +	if (IS_ERR(drvdata->csdev))
> +		return PTR_ERR(drvdata->csdev);
> +
> +	drvdata->miscdev.name = desc.name;
> +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	drvdata->miscdev.fops = &smb_fops;
> +	ret = misc_register(&drvdata->miscdev);
> +	if (ret) {
> +		coresight_unregister(drvdata->csdev);
> +		dev_err(&pdev->dev, "Failed to register misc: %d\n", ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static void smb_unregister_sink(struct smb_drv_data *drvdata)
> +{
> +	misc_deregister(&drvdata->miscdev);
> +	coresight_unregister(drvdata->csdev);
> +}
> +
> +/*
> + * Send ultrasoc messge to control hardwares on the tracing path,
> + * using DSM calls to avoid exposing ultrasoc message format.
> + */
> +static int smb_config_inport(struct device *dev, bool enable)
> +{
> +	u32 flag = enable ? 1 : 0;
> +	union acpi_object *obj;
> +	guid_t guid;
> +
> +	if (guid_parse("82ae1283-7f6a-4cbe-aa06-53e8fb24db18", &guid)) {

Please could you add symbolic name for this UUID string and a comment on
what this will do ?

> +		dev_err(dev, "Get GUID failed\n");
> +		return -EINVAL;
> +	}
> +
> +	obj = acpi_evaluate_dsm(ACPI_HANDLE(dev), &guid, 0, flag, NULL);

What are the parameters ? What do they mean ?

> +	if (!obj)
> +		dev_err(dev, "ACPI handle failed\n");
> +	else
> +		ACPI_FREE(obj);
> +
> +	return 0;
> +}
> +
> +static int smb_probe(struct platform_device *pdev)
> +{
> +	struct smb_drv_data *drvdata;
> +	int ret;
> +
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(drvdata->base)) {
> +		dev_err(&pdev->dev, "Failed to ioremap resource\n");
> +		return PTR_ERR(drvdata->base);
> +	}
> +
> +	ret = smb_init_data_buffer(pdev, &drvdata->sdb);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to init buffer: %d\n", ret);
> +		return ret;
> +	}
> +
> +	smb_init_hw(drvdata);
> +	spin_lock_init(&drvdata->spinlock);
> +	drvdata->pid = -1;
> +
> +	ret = smb_register_sink(pdev, drvdata);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register smb sink\n");
> +		return ret;
> +	}
> +
> +	ret = smb_config_inport(&pdev->dev, true);
> +	if (ret) {
> +		smb_unregister_sink(drvdata);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, drvdata);
> +	return 0;
> +}
> +
> +static int smb_remove(struct platform_device *pdev)
> +{
> +	struct smb_drv_data *drvdata = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = smb_config_inport(&pdev->dev, false);
> +	if (ret)
> +		return ret;
> +
> +	smb_unregister_sink(drvdata);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id ultrasoc_smb_acpi_match[] = {
> +	{"HISI03A1", 0},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, ultrasoc_smb_acpi_match);
> +#endif

As mentioned above this driver must depend on the ACPI and thus
you may remove the #ifdef

> +
> +static struct platform_driver smb_driver = {
> +	.driver = {
> +		.name = "ultrasoc-smb",
> +		.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");
> +MODULE_AUTHOR("Jonathan Zhou <jonathan.zhouwen@huawei.com>");
> +MODULE_AUTHOR("Qi Liu <liuqi115@huawei.com>");
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h
> new file mode 100644
> index 000000000000..a7c76140ee46
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Siemens System Memory Buffer driver.
> + * Copyright(c) 2022, HiSilicon Limited.
> + */
> +
> +#ifndef _ULTRASOC_SMB_H
> +#define _ULTRASOC_SMB_H
> +
> +#include <linux/coresight.h>
> +#include <linux/list.h>
> +#include <linux/miscdevice.h>
> +
> +#include "coresight-etm-perf.h"
> +#include "coresight-priv.h"
> +
> +/* Offset of SMB logical buffer registers */
> +#define SMB_CFG_REG			0X0
> +#define SMB_GLOBAL_EN			0X4
> +#define SMB_GLOBAL_INT			0X8
> +#define SMB_LB_CFG_LO			0X40
> +#define SMB_LB_CFG_HI			0X44
> +#define SMB_LB_INT_CTRL			0X48
> +#define SMB_LB_INT_STS			0X4C
> +#define SMB_LB_LIMIT			0X58
> +#define SMB_LB_RD_ADDR			0X5C
> +#define SMB_LB_WR_ADDR			0X60
> +#define SMB_LB_PURGE			0X64
> +
> +/* Set SMB_CFG_REG register */
> +#define SMB_IDLE_PRD(period)		((((period) - 216) & 0xf) << 12)
> +#define SMB_MEM_WR(credit, rate)	(((credit) << 16) | ((rate) << 18))
> +#define SMB_MEM_RD(credit, rate)	(((credit) << 22) | ((rate) << 24))
> +#define SMB_BURST_LEN(len)		(((len) - 1) << 4)
> +#define SMB_GLOBAL_CFG			(SMB_IDLE_PRD(231) | SMB_MEM_WR(0x3, 0x0) |   \
> +					 SMB_MEM_RD(0x3, 0x6) | SMB_BURST_LEN(16))
> +
> +/* Set SMB_GLOBAL_INT register */
> +#define SMB_INT_EN			BIT(0)
> +#define SMB_INT_TYPE_PULSE		BIT(1)
> +#define SMB_INT_POLARITY_HIGH		BIT(2)
> +#define SMB_GLB_INT_CFG			(SMB_INT_EN | SMB_INT_TYPE_PULSE | SMB_INT_POLARITY_HIGH)
> +
> +/* Set SMB_LB_CFG_LO register */
> +#define SMB_BUF_EN			BIT(0)
> +#define SMB_BUF_SINGLE_END		BIT(1)
> +#define SMB_BUF_INIT			BIT(8)
> +#define SMB_BUF_CONTINUOUS		BIT(11)
> +#define SMB_FLOW_MASK			GENMASK(19, 16)
> +#define SMB_BUF_CFG_STREAMING		(SMB_BUF_INIT | SMB_BUF_CONTINUOUS | SMB_FLOW_MASK)
> +#define SMB_BASE_LOW_MASK		GENMASK(31, 0)
> +
> +/* Set SMB_LB_CFG_HI register */
> +#define SMB_MSG_FILTER(lower, upper)	(((lower) & 0xff) | (((upper) & 0xff) << 8))

There is only one place where this is used and it is unnecessarily
confusing. Why not simply define:

#define SMB_CFG_MSG_FILTER		0xff00


> +#define SMB_BUF_INT_EN			BIT(0)
> +#define SMB_BUF_NOTE_MASK		GENMASK(11, 8)
> +#define SMB_BUF_INT_CFG			(SMB_BUF_INT_EN | SMB_BUF_NOTE_MASK)
> +
> +/**
> + * struct smb_data_buffer - Details of the buffer used by SMB
> + * @buf_base	: Memory mapped base address of SMB.
> + * @start_addr	: SMB buffer start Physical address.
> + * @buf_size	: Size of the buffer.
> + * @data_size	: Size of Trace data copy to userspace.
> + * @rd_offset	: Offset of the read pointer in the buffer.
> + */
> +struct smb_data_buffer {
> +	void __iomem *buf_base;
> +	u32 start_addr;
> +	unsigned long buf_size;
> +	unsigned long data_size;
> +	unsigned long rd_offset;
> +};
> +
> +/**
> + * 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/smb" entry.
> + * @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_reg(name, offset)  coresight_simple_reg32(struct smb_drv_data, name, offset)
> +
> +#endif

Suzuki

Re: [PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer driver

[liuqi (BA)]

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.
>> +
>> +    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);
>> +    dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");
>> +
>> +    return 0;
>> +}
>> +
>> +static int smb_disable(struct coresight_device *csdev)
>> +{
>> +    struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&drvdata->spinlock, flags);
>> +
>> +    if (atomic_dec_return(csdev->refcnt)) {
> 
> We need to increment the refcnt for every event that shares the
> sink for perf mode. Please see my comments above.
> 
>> +        spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +        return -EBUSY;
>> +    }
>> +
>> +    WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
>> +    smb_disable_hw(drvdata);
>> +
>> +    /*
>> +     * Data remaining in hardware path will be sent to SMB after 
>> purge, so needs to
>> +     * synchronize the read pointer to write pointer in perf mode.
> 
> Please run checkpatch on your submission ^^^
> 
> 
this comment is more than 80 word but less than 120 word, so there's no 
checkpatch warning~ I'll modify it to 80 words per line next version :)
>> +     */
>> +    smb_purge_data(drvdata);
>> +    if (drvdata->mode == CS_MODE_PERF)
>> +        writel(readl(drvdata->base + SMB_LB_WR_ADDR), drvdata->base + 
>> SMB_LB_RD_ADDR);
> 
> Same here. Why don't we do this for CS_MODE_SYSFS ?
> 

when SMB is disabled, we need to do purge, in case data remaining in 
hardware path influence next trace.
when we use sysfs interface, smb_disable() is called before smb_read(), 
so the purged data will be dumped to userspace together.
But when using perf interface, smb_disable() is called after 
smb_update_buffer(), so we need to synchronize the read pointer to write 
pointer, to make sure these data won't influence next trace. Thanks.
>> +
>> +    /* Dissociate from the target process. */
>> +    drvdata->pid = -1;
>> +    drvdata->mode = CS_MODE_DISABLED;
>> +    spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +
>> +    dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
>> +    return 0;
>> +}
>> +
[...]
>> +
>> +static void smb_sync_perf_buffer(struct smb_drv_data *drvdata,
>> +                 struct cs_buffers *buf, unsigned long data_size)
>> +{
>> +    struct smb_data_buffer *sdb = &drvdata->sdb;
>> +    char **dst_pages = (char **)buf->data_pages;
>> +    unsigned long buf_offset = buf->offset;
> 
> Please could you rename this to page_offset ?
> 
yes, sure.
>> +    unsigned int cur = buf->cur;
>> +    unsigned long to_copy;
>> +
> 
>> +    while (data_size) {
>> +        /* Copy parts of trace data when the read pointer will wrap 
>> around SMB buffer. */
> 
> Wrap the lines please.
> 
> Please could you do :
> 
>          unsigned long page_space = (PAGE_SIZE - page_offset);
> 
> 
>> +        if (sdb->rd_offset + PAGE_SIZE - buf_offset > sdb->buf_size)
> 
> ^^        if (sdb->rd_offset + page_space > sdb->buf_size)
> 
>> +            to_copy = sdb->buf_size - sdb->rd_offset;
>> +        else
>> +            to_copy = min(data_size, PAGE_SIZE - buf_offset);
> 
>              to_copy = min(data_size, page_space);
> 
>> +
>> +        memcpy_fromio(dst_pages[cur] + buf_offset, sdb->buf_base + 
>> sdb->rd_offset, to_copy);
> 
>          memcpy_fromio(dst_pages[cur] + page_offset, ...
> 
>> +
>> +        buf_offset += to_copy;
>> +        if (buf_offset >= PAGE_SIZE) {
> 
> That becomes much simpler to follow with the rename to page_offset.
> 

sure, I'll do this next time. thanks.
>> +            buf_offset = 0;
>> +            cur++;
>> +            cur %= buf->nr_pages;
>> +        }
>> +        data_size -= to_copy;
>> +        /* ensure memcpy finished before update the read pointer */
> 
> Why and how ?
> 
this comment is used to express we do memcpy first, then update read 
pointer. Seems it's misleading, I'll delete it next time, thanks.
>> +        sdb->rd_offset += to_copy;
>> +        sdb->rd_offset %= sdb->buf_size;
>> +    }
>> +
>> +    sdb->data_size = 0;
>> +    writel(sdb->start_addr + sdb->rd_offset, drvdata->base + 
>> SMB_LB_RD_ADDR);
>> +    smb_reset_buffer_status(drvdata);
>> +}
>> +
>> +static unsigned long smb_update_buffer(struct coresight_device *csdev,
>> +                       struct perf_output_handle *handle,
>> +                       void *sink_config)
>> +{
>> +    struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +    struct smb_data_buffer *sdb = &drvdata->sdb;
>> +    struct cs_buffers *buf = sink_config;
>> +    unsigned long data_size = 0;
>> +    unsigned long flags;
>> +    bool lost = false;
>> +
>> +    if (!buf)
>> +        return 0;
>> +
>> +    spin_lock_irqsave(&drvdata->spinlock, flags);
>> +
>> +    /* Don't do anything if another tracer is using this sink. */
>> +    if (atomic_read(csdev->refcnt) != 1)
>> +        goto out;
>> +
>> +    smb_update_data_size(drvdata);
>> +    data_size = sdb->data_size;
> 
> Please could you add in a comment on what we do here below ?
got it, will add this, thanks.
> 
>> +    if (data_size > handle->size)  > +        sdb->rd_offset += 
>> data_size - handle->size;
>> +        sdb->rd_offset %= sdb->buf_size;
>> +        data_size = handle->size;
>> +        lost = true;
>> +    }
>> +
>> +    smb_sync_perf_buffer(drvdata, buf, data_size);
>> +    if (!buf->snapshot && lost)
>> +        perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
>> +
>> +out:
>> +    spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +    return data_size;
>> +}
>> +
>> +static const struct coresight_ops_sink smb_cs_ops = {
>> +    .enable        = smb_enable,
>> +    .disable    = smb_disable,
>> +    .alloc_buffer    = smb_alloc_buffer,
>> +    .free_buffer    = smb_free_buffer,
>> +    .update_buffer    = smb_update_buffer,
>> +};
>> +
>> +static const struct coresight_ops cs_ops = {
>> +    .sink_ops    = &smb_cs_ops,
>> +};
>> +
>> +static int smb_init_data_buffer(struct platform_device *pdev, struct 
>> smb_data_buffer *sdb)
>> +{
>> +    struct resource *res;
>> +    void __iomem *base;
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> 
> What are these IO bases ? What does 0 stand for ? What is 1 for ?
> 
> Please could you add a documentation under
> 
> Documentation/trace/coresight/.
> 
> Doesn't need to be too detailed. But a basic description would be helpful.
> 
yes, I'll add this, thanks.
> 
>> +    if (IS_ERR(res)) {
>> +        dev_err(&pdev->dev, "SMB device failed to get resource\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    sdb->start_addr = res->start & SMB_BASE_LOW_MASK;
>> +    sdb->buf_size = resource_size(res);
>> +    if (sdb->buf_size == 0)
>> +        return -EINVAL;
>> +
>> +    base = devm_ioremap_resource(&pdev->dev, res);
>> +    if (IS_ERR(base))
>> +        return PTR_ERR(base);
>> +
>> +    sdb->buf_base = base;
>> +
>> +    return 0;
>> +}
>> +
>> +static void smb_init_hw(struct smb_drv_data *drvdata)
>> +{
>> +    u32 value;
>> +
>> +    /* First disable smb and clear the status of SMB buffer */
>> +    smb_reset_buffer_status(drvdata);
>> +    smb_disable_hw(drvdata);
>> +    smb_purge_data(drvdata);
>> +
>> +    /* Using smb in single-end mode, and set other configures default */
>> +    value = SMB_BUF_CFG_STREAMING | SMB_BUF_SINGLE_END | SMB_BUF_EN;
>> +    writel(value, drvdata->base + SMB_LB_CFG_LO);
>> +    value = SMB_MSG_FILTER(0x0, 0xff);
> 
> Could we not simply say :
> 
> 
>> +    writel(value, drvdata->base + SMB_LB_CFG_HI);
>> +
> 
>      #define SMB_CFG_MSG_FILTER    0xff00
> 
>      writel(SMB_CFG_MSG_FILTER, drvdata->base + SMB_LB_CFG_HI) ?
> 
> 
> Just like you do below for the other registers ? Anyways, none of these
> values make any sense to the reader without the Reference Manual. The
> best you could do is stick a sensible name for the value like you do
> below.
> 
yes, I'll do this, thanks.

>> +    writel(SMB_GLOBAL_CFG, drvdata->base + SMB_CFG_REG);
>> +    writel(SMB_GLB_INT_CFG, drvdata->base + SMB_GLOBAL_INT);
>> +    writel(SMB_BUF_INT_CFG, drvdata->base + SMB_LB_INT_CTRL);
>> +}
[...]
>> +
>> +static void smb_unregister_sink(struct smb_drv_data *drvdata)
>> +{
>> +    misc_deregister(&drvdata->miscdev);
>> +    coresight_unregister(drvdata->csdev);
>> +}
>> +
>> +/*
>> + * Send ultrasoc messge to control hardwares on the tracing path,
>> + * using DSM calls to avoid exposing ultrasoc message format.
>> + */
>> +static int smb_config_inport(struct device *dev, bool enable)
>> +{
>> +    u32 flag = enable ? 1 : 0;
>> +    union acpi_object *obj;
>> +    guid_t guid;
>> +
>> +    if (guid_parse("82ae1283-7f6a-4cbe-aa06-53e8fb24db18", &guid)) {
> 
> Please could you add symbolic name for this UUID string and a comment on
> what this will do ?
> 
sure, I'll fix this, thanks.
>> +        dev_err(dev, "Get GUID failed\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    obj = acpi_evaluate_dsm(ACPI_HANDLE(dev), &guid, 0, flag, NULL);
> 
> What are the parameters ? What do they mean ?
> 
We need to initialize the register before tracing, but the vendor does 
not want us to expose these initialize information, so we add these set 
function in ACPI and use a ACPI handler to call this function. Thanks.

>> +    if (!obj)
>> +        dev_err(dev, "ACPI handle failed\n");
>> +    else
>> +        ACPI_FREE(obj);
>> +
>> +    return 0;
>> +}
>> +
[...]
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id ultrasoc_smb_acpi_match[] = {
>> +    {"HISI03A1", 0},
>> +    {},
>> +};
>> +MODULE_DEVICE_TABLE(acpi, ultrasoc_smb_acpi_match);
>> +#endif
> 
> As mentioned above this driver must depend on the ACPI and thus
> you may remove the #ifdef

got it, thanks
> 
>> +
>> +static struct platform_driver smb_driver = {
>> +    .driver = {
>> +        .name = "ultrasoc-smb",
>> +        .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");
>> +MODULE_AUTHOR("Jonathan Zhou <jonathan.zhouwen@huawei.com>");
>> +MODULE_AUTHOR("Qi Liu <liuqi115@huawei.com>");
>> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h 
>> b/drivers/hwtracing/coresight/ultrasoc-smb.h
>> new file mode 100644
>> index 000000000000..a7c76140ee46
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
>> @@ -0,0 +1,100 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Siemens System Memory Buffer driver.
>> + * Copyright(c) 2022, HiSilicon Limited.
>> + */
>> +
>> +#ifndef _ULTRASOC_SMB_H
>> +#define _ULTRASOC_SMB_H
>> +
>> +#include <linux/coresight.h>
>> +#include <linux/list.h>
>> +#include <linux/miscdevice.h>
>> +
>> +#include "coresight-etm-perf.h"
>> +#include "coresight-priv.h"
>> +
>> +/* Offset of SMB logical buffer registers */
>> +#define SMB_CFG_REG            0X0
>> +#define SMB_GLOBAL_EN            0X4
>> +#define SMB_GLOBAL_INT            0X8
>> +#define SMB_LB_CFG_LO            0X40
>> +#define SMB_LB_CFG_HI            0X44
>> +#define SMB_LB_INT_CTRL            0X48
>> +#define SMB_LB_INT_STS            0X4C
>> +#define SMB_LB_LIMIT            0X58
>> +#define SMB_LB_RD_ADDR            0X5C
>> +#define SMB_LB_WR_ADDR            0X60
>> +#define SMB_LB_PURGE            0X64
>> +
>> +/* Set SMB_CFG_REG register */
>> +#define SMB_IDLE_PRD(period)        ((((period) - 216) & 0xf) << 12)
>> +#define SMB_MEM_WR(credit, rate)    (((credit) << 16) | ((rate) << 18))
>> +#define SMB_MEM_RD(credit, rate)    (((credit) << 22) | ((rate) << 24))
>> +#define SMB_BURST_LEN(len)        (((len) - 1) << 4)
>> +#define SMB_GLOBAL_CFG            (SMB_IDLE_PRD(231) | 
>> SMB_MEM_WR(0x3, 0x0) |   \
>> +                     SMB_MEM_RD(0x3, 0x6) | SMB_BURST_LEN(16))
>> +
>> +/* Set SMB_GLOBAL_INT register */
>> +#define SMB_INT_EN            BIT(0)
>> +#define SMB_INT_TYPE_PULSE        BIT(1)
>> +#define SMB_INT_POLARITY_HIGH        BIT(2)
>> +#define SMB_GLB_INT_CFG            (SMB_INT_EN | SMB_INT_TYPE_PULSE | 
>> SMB_INT_POLARITY_HIGH)
>> +
>> +/* Set SMB_LB_CFG_LO register */
>> +#define SMB_BUF_EN            BIT(0)
>> +#define SMB_BUF_SINGLE_END        BIT(1)
>> +#define SMB_BUF_INIT            BIT(8)
>> +#define SMB_BUF_CONTINUOUS        BIT(11)
>> +#define SMB_FLOW_MASK            GENMASK(19, 16)
>> +#define SMB_BUF_CFG_STREAMING        (SMB_BUF_INIT | 
>> SMB_BUF_CONTINUOUS | SMB_FLOW_MASK)
>> +#define SMB_BASE_LOW_MASK        GENMASK(31, 0)
>> +
>> +/* Set SMB_LB_CFG_HI register */
>> +#define SMB_MSG_FILTER(lower, upper)    (((lower) & 0xff) | (((upper) 
>> & 0xff) << 8))
> 
> There is only one place where this is used and it is unnecessarily
> confusing. Why not simply define:
> 
> #define SMB_CFG_MSG_FILTER        0xff00
> 
> 

got it, I'll do this.

thanks a lot.
Qi
>> +#define SMB_BUF_INT_EN            BIT(0)
>> +#define SMB_BUF_NOTE_MASK        GENMASK(11, 8)
>> +#define SMB_BUF_INT_CFG            (SMB_BUF_INT_EN | SMB_BUF_NOTE_MASK)
>> +
>> +/**
>> + * struct smb_data_buffer - Details of the buffer used by SMB
>> + * @buf_base    : Memory mapped base address of SMB.
>> + * @start_addr    : SMB buffer start Physical address.
>> + * @buf_size    : Size of the buffer.
>> + * @data_size    : Size of Trace data copy to userspace.
>> + * @rd_offset    : Offset of the read pointer in the buffer.
>> + */
>> +struct smb_data_buffer {
>> +    void __iomem *buf_base;
>> +    u32 start_addr;
>> +    unsigned long buf_size;
>> +    unsigned long data_size;
>> +    unsigned long rd_offset;
>> +};
>> +
>> +/**
>> + * 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/smb" entry.
>> + * @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_reg(name, offset)  coresight_simple_reg32(struct 
>> smb_drv_data, name, offset)
>> +
>> +#endif
> 
> Suzuki
> .

Re: [PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer driver

[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

Re: [PATCH v4] drivers/coresight: Add Ultrasoc System Memory Buffer driver

[liuqi (BA)]

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@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: }
> 

Hi Suzuki，

got it, I'll increment the refcount in perf/sysfs session next version.

Thanks,
Qi
> 
> Suzuki
> .