| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20221109165615.00006060@Huawei.com>
Date: Wed, 9 Nov 2022 16:56:15 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Junhao He <hejunhao3@...wei.com>
CC: <mathieu.poirier@...aro.org>, <suzuki.poulose@....com>,
<mike.leach@...aro.org>, <leo.yan@...aro.org>,
<john.garry@...wei.com>, <coresight@...ts.linaro.org>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <lpieralisi@...nel.org>,
<linuxarm@...wei.com>, <liuqi115@...wei.com>,
<f.fangjian@...wei.com>, <prime.zeng@...ilicon.com>
Subject: Re: [PATCH v12 1/2] drivers/coresight: Add UltraSoc System Memory
Buffer driver
On Wed, 9 Nov 2022 21:50:07 +0800
Junhao He <hejunhao3@...wei.com> wrote:
> From: Qi Liu <liuqi115@...wei.com>
>
> 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 trace" in system memory.
>
> SMB is developed by UltraSoc technology, which is acquired by
> Siemens, and we still use "UltraSoc" to name driver.
>
> Signed-off-by: Qi Liu <liuqi115@...wei.com>
> Signed-off-by: Junhao He <hejunhao3@...wei.com>
> Tested-by: JunHao He <hejunhao3@...wei.com>
Hi JunHao,
One trivial side effect of dropping the ACPI dependency.
Also, I think (at the cost of a slightly lengthening of lines)
you can rename the register fields to avoid any potential
confusion between GLB and LB registers.
With those fixed feel free to add
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
...
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> new file mode 100644
> index 000000000000..ea2552a98d28
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
...
> +static const struct acpi_device_id ultrasoc_smb_acpi_match[] = {
> + {"HISI03A1", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, ultrasoc_smb_acpi_match);
> +
> +static struct platform_driver smb_driver = {
> + .driver = {
> + .name = "ultrasoc-smb",
> + .acpi_match_table = ACPI_PTR(ultrasoc_smb_acpi_match),
Now the driver build isn't dependent on CONFIG_ACPI
if !CONFIG_ACPI ACPI_PTR() doesn't reference the parameter.
As such you'll get unused warnings.
1 options to fix this
a) Drop ACPI_PTR() and just have .acpi_match_data = ultrasoc_smb_acpi_match
b) ifdef magic around the acpi_match table.
In theory the first option results in bloat, but in this case I doubt we care.
> + .suppress_bind_attrs = true,
> + },
> + .probe = smb_probe,
> + .remove = smb_remove,
> +};
> +module_platform_driver(smb_driver);
> +
> +MODULE_DESCRIPTION("UltraSoc SMB CoreSight driver");
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_AUTHOR("Jonathan Zhou <jonathan.zhouwen@...wei.com>");
> +MODULE_AUTHOR("Qi Liu <liuqi115@...wei.com>");
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h
> new file mode 100644
> index 000000000000..2e2f9f8fe54b
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/*
> + * Siemens System Memory Buffer driver.
> + * Copyright(c) 2022, HiSilicon Limited.
> + */
> +
> +#ifndef _ULTRASOC_SMB_H
> +#define _ULTRASOC_SMB_H
> +
> +#include <linux/miscdevice.h>
> +#include <linux/mutex.h>
> +
> +/* Offset of SMB global registers */
> +#define SMB_GLB_CFG_REG 0x00
> +#define SMB_GLB_EN_REG 0x04
> +#define SMB_GLB_INT_REG 0x08
> +
> +/* Offset of SMB logical buffer registers */
> +#define SMB_LB_CFG_LO_REG 0x40
> +#define SMB_LB_CFG_HI_REG 0x44
> +#define SMB_LB_INT_CTRL_REG 0x48
> +#define SMB_LB_INT_STS_REG 0x4c
> +#define SMB_LB_RD_ADDR_REG 0x5c
> +#define SMB_LB_WR_ADDR_REG 0x60
> +#define SMB_LB_PURGE_REG 0x64
> +
> +/* Set global config register */
> +#define SMB_CFG_BURST_LEN_MSK GENMASK(11, 4)
Given there are several CFG registers, possibly worth
prefix of SMB_GLB_CFG_ ...
> +#define SMB_CFG_IDLE_PRD_MSK GENMASK(15, 12)
> +#define SMB_CFG_MEM_WR_MSK GENMASK(21, 16)
> +#define SMB_CFG_MEM_RD_MSK GENMASK(27, 22)
> +#define SMB_GLB_CFG_DEFAULT (FIELD_PREP(SMB_CFG_BURST_LEN_MSK, 0xf) | \
> + FIELD_PREP(SMB_CFG_IDLE_PRD_MSK, 0xf) | \
> + FIELD_PREP(SMB_CFG_MEM_WR_MSK, 0x3) | \
> + FIELD_PREP(SMB_CFG_MEM_RD_MSK, 0x1b))
> +
> +/* Set global interrupt control register */
> +#define SMB_INT_EN BIT(0)
Again, multiple INT registers, so SMB_INT_GLB_* perhaps?
> +#define SMB_INT_PULSE BIT(1) /* Interrupt type: 1 - Pulse */
> +#define SMB_INT_ACT_H BIT(2) /* Interrupt polarity: 1 - Active high */
> +#define SMB_GLB_INT_CFG (SMB_INT_EN | SMB_INT_PULSE | SMB_INT_ACT_H)
> +
> +/* Set logical buffer config register lower 32 bits */
> +#define SMB_CFG_LO_EN BIT(0)
SMB_LB_CFG_...
etc for other cases.
> +#define SMB_CFG_LO_SINGLE_END BIT(1)
> +#define SMB_CFG_LO_INIT BIT(8)
> +#define SMB_CFG_LO_CONT BIT(11)
> +#define SMB_CFG_LO_FLOW_MSK GENMASK(19, 16)
> +#define SMB_LB_CFG_LO_DEFAULT (SMB_CFG_LO_EN | SMB_CFG_LO_SINGLE_END | \
> + SMB_CFG_LO_INIT | SMB_CFG_LO_CONT | \
> + FIELD_PREP(SMB_CFG_LO_FLOW_MSK, 0xf))
> +
> +/* Set logical buffer config register upper 32 bits */
> +#define SMB_CFG_HI_RANGE_UP_MSK GENMASK(15, 8)
> +#define SMB_LB_CFG_HI_DEFAULT FIELD_PREP(SMB_CFG_HI_RANGE_UP_MSK, 0xff)
> +
> +/* Set logical buffer interrupt control register */
> +#define SMB_INT_CTRL_EN BIT(0)
> +#define SMB_INT_CTRL_BUF_NOTE_MSK GENMASK(11, 8)
> +#define SMB_LB_INT_CTRL_CFG (SMB_INT_CTRL_EN | \
> + FIELD_PREP(SMB_INT_CTRL_BUF_NOTE_MSK, 0xf))
> +
> +#define SMB_LB_INT_STS_NOT_EMPTY_MSK BIT(0)
> +#define SMB_LB_STS_RESET_MSK GENMASK(3, 0)
> +#define SMB_LB_INT_BUF_STS_RESET FIELD_PREP(SMB_LB_STS_RESET_MSK, 0xf)
> +#define SMB_LB_PURGE_PURGED BIT(0)
> +#define SMB_GLB_EN_HW_ENABLE BIT(0)
> +
> +#define SMB_REG_ADDR_RES 0
> +#define SMB_BUF_ADDR_RES 1
> +#define SMB_BUF_ADDR_LO_MSK GENMASK(31, 0)
...
> +
> +/**
> + * struct smb_drv_data - specifics associated to an SMB component
> + * @base: Memory mapped base address for SMB component.
> + * @csdev: Component vitals needed by the framework.
> + * @sdb: Data buffer for SMB.
> + * @miscdev: Specifics to handle "/dev/xyz.smb" entry.
> + * @mutex: Control data access to one at a time.
> + * @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.
Trivial, but for consistency should be: How this...
> + */
> +struct smb_drv_data {
> + void __iomem *base;
> + struct coresight_device *csdev;
> + struct smb_data_buffer sdb;
> + struct miscdevice miscdev;
> + struct mutex mutex;
> + local_t reading;
> + pid_t pid;
> + u32 mode;
> +};
> +
> +#endif
Powered by blists - more mailing lists