| 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: <a2bd3bbf-5512-971a-95a1-3220f31814a2@arm.com>
Date: Fri, 2 Jun 2023 09:45:34 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Tao Zhang <quic_taozha@...cinc.com>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Konrad Dybcio <konradybcio@...il.com>,
Mike Leach <mike.leach@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Cc: Jinlong Mao <quic_jinlmao@...cinc.com>,
Leo Yan <leo.yan@...aro.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
Tingwei Zhang <quic_tingweiz@...cinc.com>,
Yuanfang Zhang <quic_yuanfang@...cinc.com>,
Trilok Soni <quic_tsoni@...cinc.com>,
Hao Zhang <quic_hazha@...cinc.com>,
linux-arm-msm@...r.kernel.org, andersson@...nel.org
Subject: Re: [PATCH v4 07/11] coresight-tpdm: Add nodes for dsb edge control
On 02/06/2023 09:21, Tao Zhang wrote:
>
> On 6/1/2023 8:14 PM, Suzuki K Poulose wrote:
>> On 27/04/2023 10:00, Tao Zhang wrote:
>>> Add the nodes to set value for DSB edge control and DSB edge
>>> control mask. Each DSB subunit TPDM has maximum of n(n<16) EDCR
>>> resgisters to configure edge control. DSB edge detection control
>>> 00: Rising edge detection
>>> 01: Falling edge detection
>>> 10: Rising and falling edge detection (toggle detection)
>>> And each DSB subunit TPDM has maximum of m(m<8) ECDMR registers to
>>> configure mask. Eight 32 bit registers providing DSB interface
>>> edge detection mask control.
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@...cinc.com>
>>> ---
>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 32 +++++
>>> drivers/hwtracing/coresight/coresight-tpdm.c | 135
>>> ++++++++++++++++++++-
>>> drivers/hwtracing/coresight/coresight-tpdm.h | 21 ++++
>>> 3 files changed, 187 insertions(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> index 348e167..a57f000 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>> @@ -60,3 +60,35 @@ Description:
>>> Bit[3] : Set to 0 for low performance mode.
>>> Set to 1 for high performance mode.
>>> Bit[4:8] : Select byte lane for high performance mode.
>>> +
>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl
>>> +Date: March 2023
>>> +KernelVersion 6.3
>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@...cinc.com>, Tao Zhang
>>> (QUIC) <quic_taozha@...cinc.com>
>>> +Description:
>>> + Read/Write a set of the edge control registers of the DSB
>>> + in TPDM.
>>> +
>>> + Expected format is the following:
>>> + <integer1> <integer2> <integer3>
>>> +
>>> + Where:
>>> + <integer1> : Start EDCR register number
>>> + <integer2> : End EDCR register number
>>> + <integer3> : The value need to be written
>>> +
>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_edge_ctrl_mask
>>> +Date: March 2023
>>> +KernelVersion 6.3
>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@...cinc.com>, Tao Zhang
>>> (QUIC) <quic_taozha@...cinc.com>
>>> +Description:
>>> + Read/Write a set of the edge control mask registers of the
>>> + DSB in TPDM.
>>> +
>>> + Expected format is the following:
>>> + <integer1> <integer2> <integer3>
>>> +
>>> + Where:
>>> + <integer1> : Start EDCMR register number
>>> + <integer2> : End EDCMR register number
>>> + <integer3> : The value need to be written
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c
>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> index 1bacaa5..a40e458 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>> @@ -80,7 +80,14 @@ static void set_trigger_type(struct tpdm_drvdata
>>> *drvdata, u32 *val)
>>> static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>>> {
>>> - u32 val;
>>> + u32 val, i;
>>> +
>>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++)
>>> + writel_relaxed(drvdata->dsb->edge_ctrl[i],
>>> + drvdata->base + TPDM_DSB_EDCR(i));
>>> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++)
>>> + writel_relaxed(drvdata->dsb->edge_ctrl_mask[i],
>>> + drvdata->base + TPDM_DSB_EDCMR(i));
>>
>> Do all TPDM DSBs have MAX_EDCR registers ? Or some have less than that ?
>> If it is latter, do we need special care to avoid writing to inexistent
>> registers ?
>>
> You are right, not all DSB TPDMs have MAX_EDCR registers. In our design,
> the inexistent register addresses
>
> are not occupied and safe for accessing.
>
> Currently we don't have a good way to know the quantity of EDCR/EDCMR
> registers for DSB TPDMs.
>
> The only way we can think of is to set it in device tree manually.
>
> Do you have other suggestion for this?
>
>>> val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
>>> /* Set trigger timestamp */
>>> @@ -313,6 +320,130 @@ static ssize_t dsb_mode_store(struct device *dev,
>>> }
>>> static DEVICE_ATTR_RW(dsb_mode);
>>> +static ssize_t dsb_edge_ctrl_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> + ssize_t size = 0;
>>> + int i;
>>> +
>>> + spin_lock(&drvdata->spinlock);
>>> + for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>>> + size += sysfs_emit_at(buf, size,
>>> + "Index:0x%x Val:0x%x\n", i,
>>> + drvdata->dsb->edge_ctrl[i]);
>>
>> It may be safe, but please add a check to make sure that we don't
>> overflow. At least bail out when we hit a return of 0, indicating
>> reached the end of buffer.
>>
> Can I add the following check to replace the current code??
>
> int ret = 0;
>
>
> for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) {
>
> ret = sysfs_emit_at(buf, size, "Index:0x%x Val:0x%x\n", i,
> drvdata->dsb->edge_ctrl[i]);
>
> if (!ret) {
>
> dev_warn(drvdata->dev, "The buffer has been overflowed\n");
You don't need this, it already triggers a WARN() in sysfs_emit_at().
So you could do:
for (....) {
unsigned long bytes = sysfs_emit_at(buf, size, ....);
if (bytes <= 0)
break;
size += bytes;
}
>
> spin_unlock(&drvdata->spinlock);
>
> return size;
>
> } else
>
> size += ret;
>
> }
>
>>> + }
>>> + spin_unlock(&drvdata->spinlock);
>>> + return size;
>>> +}
>>> +
>>> +/*
>>> + * value 1: Start EDCR register number
>>> + * value 2: End EDCR register number
>>> + * value 3: The value need to be written
>>> + * The EDCR registers can include up to 16 32-bit registers, and each
>>> + * one can be configured to control up to 16 edge detections(2 bits
>>> + * control one edge detection). So a total 256 edge detections can be
>>> + * configured. So the starting number(value 1) and ending
>>> number(value 2)
>>> + * cannot be greater than 256, and value 1 should be less than value 2.
>>> + * The following values are the rage of value 3.
>>> + * 0 - Rising edge detection
>>> + * 1 - Falling edge detection
>>> + * 2 - Rising and falling edge detection (toggle detection)
>>> + */
>>> +static ssize_t dsb_edge_ctrl_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf,
>>> + size_t size)
>>> +{
>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> + unsigned long val, mask, start, end, edge_ctrl, edge_ctrl_shift;
>>> + int i, reg;
>>> +
>>> + if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3)
>>> + return -EINVAL;
>>> + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) ||
>>> + (start > end) || (edge_ctrl > 0x2))
>>> + return -EPERM;
>>> +
>>> + spin_lock(&drvdata->spinlock);
>>> + for (i = start; i <= end; i++) {
>>> + /*
>>> + * There are 2 bit per DSB Edge Control line.
>>> + * Thus we have 16 lines in a 32bit word.
>>> + */
>>> + reg = EDCR_TO_WORD_IDX(i);
>>> + mask = EDCR_TO_WORD_MASK(i);
>>> + val = drvdata->dsb->edge_ctrl[reg];
>>
>>> + edge_ctrl_shift = EDCR_TO_WORD_VAL(edge_ctrl, i);
>>> + bitmap_replace(&val, &val, &edge_ctrl_shift, &mask, 32);
>>
>> Could we simply do :
>>
>> reg &= ~mask;
>> reg |= FIELD_PREP(mask, edge_ctrl);
>>
> Perhaps "FIELD_PREP" cannot be used here since "mask" must be constant
> in this macro.
Ah, you are right. Sorry about that.
>
> But in our code, the variable "mask" is not constant.
Still I think using the bitmap_replace is an overkill. We could simply
do:
val &= ~mask;
val |= EDCR_TO_WORD_VAL(edge_ctrl, i);
Suzuki
>>
>>> + drvdata->dsb->edge_ctrl[reg] = val;
>>> + }
>>> + spin_unlock(&drvdata->spinlock);
>>> +
>>> + return size;
>>> +}
>>> +static DEVICE_ATTR_RW(dsb_edge_ctrl);
>>> +
>>> +static ssize_t dsb_edge_ctrl_mask_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> + ssize_t size = 0;
>>> + int i;
>>> +
>>> + spin_lock(&drvdata->spinlock);
>>> + for (i = 0; i < TPDM_DSB_MAX_EDCMR; i++) {
>>> + size += sysfs_emit_at(buf, size,
>>> + "Index:0x%x Val:0x%x\n", i,
>>> + drvdata->dsb->edge_ctrl_mask[i]);
>>> + }
>>> + spin_unlock(&drvdata->spinlock);
>>> + return size;
>>> +}
>>> +
>>> +/*
>>> + * value 1: Start EDCMR register number
>>> + * value 2: End EDCMR register number
>>> + * value 3: The value need to be written
>>> + */
>>> +static ssize_t dsb_edge_ctrl_mask_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf,
>>> + size_t size)
>>> +{
>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> + unsigned long start, end, val;
>>> + u32 set;
>>> + int i, reg;
>>> +
>>> + if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3)
>>> + return -EINVAL;
>>> + if ((start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES)
>>> + || (start > end) || (val & ~1UL))
>>> + return -EPERM;
>>> +
>>> + spin_lock(&drvdata->spinlock);
>>> + for (i = start; i <= end; i++) {
>>> + /*
>>> + * There is 1 bit per DSB Edge Control Mark line.
>>> + * Thus we have 32 lines in a 32bit word.
>>> + */
>>> + reg = EDCMR_TO_WORD_IDX(i);
>>> + set = drvdata->dsb->edge_ctrl_mask[reg];
>>> + if (val)
>>> + set |= BIT(EDCR_TO_WORD_SHIFT(i));
>>> + else
>>> + set &= ~BIT(EDCR_TO_WORD_SHIFT(i));
>>> + drvdata->dsb->edge_ctrl_mask[reg] = set;
>>> + }
>>> + spin_unlock(&drvdata->spinlock);
>>> + return size;
>>> +}
>>> +static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);
>>> +
>>> static ssize_t dsb_trig_type_show(struct device *dev,
>>> struct device_attribute *attr, char *buf)
>>> {
>>> @@ -385,6 +516,8 @@ static DEVICE_ATTR_RW(dsb_trig_ts);
>>> static struct attribute *tpdm_dsb_attrs[] = {
>>> &dev_attr_dsb_mode.attr,
>>> + &dev_attr_dsb_edge_ctrl.attr,
>>> + &dev_attr_dsb_edge_ctrl_mask.attr,
>>> &dev_attr_dsb_trig_ts.attr,
>>> &dev_attr_dsb_trig_type.attr,
>>> NULL,
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h
>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>> index 79df07e..f25dcdec 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>> @@ -12,6 +12,8 @@
>>> /* DSB Subunit Registers */
>>> #define TPDM_DSB_CR (0x780)
>>> #define TPDM_DSB_TIER (0x784)
>>> +#define TPDM_DSB_EDCR(n) (0x808 + (n * 4))
>>> +#define TPDM_DSB_EDCMR(n) (0x848 + (n * 4))
>>> /* Enable bit for DSB subunit */
>>> #define TPDM_DSB_CR_ENA BIT(0)
>>> @@ -34,6 +36,15 @@
>>> #define TPDM_DSB_TEST_MODE GENMASK(10, 9)
>>> #define TPDM_DSB_HPSEL GENMASK(6, 2)
>>> +#define EDCRS_PER_WORD 16
>>> +#define EDCR_TO_WORD_IDX(r) ((r) / EDCRS_PER_WORD)
>>> +#define EDCR_TO_WORD_SHIFT(r) ((r % EDCRS_PER_WORD) * 2)
>>> +#define EDCR_TO_WORD_VAL(val, r) (val << EDCR_TO_WORD_SHIFT(r))
>>> +#define EDCR_TO_WORD_MASK(r) EDCR_TO_WORD_VAL(0x3, r)
>>
>> minor nit: add a new line here please
> Sure, I will update this in the next patch series.
>>
>>> +#define EDCMRS_PER_WORD 32
>>> +#define EDCMR_TO_WORD_IDX(r) ((r) / EDCMRS_PER_WORD)
>>> +#define EDCMR_TO_WORD_SHIFT(r) ((r) % EDCMRS_PER_WORD)
>>> +
>>> /* TPDM integration test registers */
>>> #define TPDM_ITATBCNTRL (0xEF0)
>>> #define TPDM_ITCNTRL (0xF00)
>>> @@ -60,14 +71,24 @@
>>> #define TPDM_PIDR0_DS_IMPDEF BIT(0)
>>> #define TPDM_PIDR0_DS_DSB BIT(1)
>>> +#define TPDM_DSB_MAX_LINES 256
>>> +/* MAX number of EDCR registers */
>>> +#define TPDM_DSB_MAX_EDCR 16
>>> +/* MAX number of EDCMR registers */
>>> +#define TPDM_DSB_MAX_EDCMR 8
>>> +
>>> /**
>>> * struct dsb_dataset - specifics associated to dsb dataset
>>> * @mode: DSB programming mode
>>> + * @edge_ctrl: Save value for edge control
>>> + * @edge_ctrl_mask: Save value for edge control mask
>>> * @trig_ts: Enable/Disable trigger timestamp.
>>> * @trig_type: Enable/Disable trigger type.
>>> */
>>> struct dsb_dataset {
>>> u32 mode;
>>> + u32 edge_ctrl[TPDM_DSB_MAX_EDCR];
>>> + u32 edge_ctrl_mask[TPDM_DSB_MAX_EDCMR];
>>
>> minor nit: Please align it with the fields below.
>
> Sure, I will update this in the next patch series.
>
>
> Best,
>
> Tao
>
>>
>>> bool trig_ts;
>>> bool trig_type;
>>> };
>>
>> Suzuki
Powered by blists - more mailing lists