| 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
| ||
|
Date: Mon, 15 Jan 2024 09:20:56 +0000
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>,
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>, Song Chai <quic_songchai@...cinc.com>,
linux-arm-msm@...r.kernel.org, andersson@...nel.org
Subject: Re: [PATCH v3 8/8] coresight-tpdm: Add msr register support for CMB
On 15/01/2024 06:20, Tao Zhang wrote:
>
> On 1/12/2024 5:43 PM, Suzuki K Poulose wrote:
>> On 12/01/2024 09:12, Tao Zhang wrote:
>>>
>>> On 12/20/2023 5:06 PM, Tao Zhang wrote:
>>>>
>>>> On 12/19/2023 10:09 PM, Suzuki K Poulose wrote:
>>>>> On 19/12/2023 06:58, Tao Zhang wrote:
>>>>>>
>>>>>> On 12/18/2023 7:02 PM, Suzuki K Poulose wrote:
>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
>>>>>>>> Add the nodes for CMB subunit MSR(mux select register) support.
>>>>>>>> CMB MSRs(mux select registers) is to separate mux,arbitration,
>>>>>>>> ,interleaving,data packing control from stream filtering control.
>>>>>>>>
>>>>>>>> Reviewed-by: James Clark <james.clark@....com>
>>>>>>>> Signed-off-by: Tao Zhang <quic_taozha@...cinc.com>
>>>>>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@...cinc.com>
>>>>>>>> ---
>>>>>>>> .../testing/sysfs-bus-coresight-devices-tpdm | 8 ++
>>>>>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 86
>>>>>>>> +++++++++++++++++++
>>>>>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 16 +++-
>>>>>>>> 3 files changed, 109 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> index e0b77107be13..914f3fd81525 100644
>>>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>>>>>> @@ -249,3 +249,11 @@ Description:
>>>>>>>> Accepts only one of the 2 values - 0 or 1.
>>>>>>>> 0 : Disable the timestamp of all trace packets.
>>>>>>>> 1 : Enable the timestamp of all trace packets.
>>>>>>>> +
>>>>>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/cmb_msr/msr[0:31]
>>>>>>>> +Date: September 2023
>>>>>>>> +KernelVersion 6.7
>>>>>>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@...cinc.com>, Tao
>>>>>>>> Zhang (QUIC) <quic_taozha@...cinc.com>
>>>>>>>> +Description:
>>>>>>>> + (RW) Set/Get the MSR(mux select register) for the CMB
>>>>>>>> subunit
>>>>>>>> + TPDM.
>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> index f6cda5616e84..7e331ea436cc 100644
>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>>>>>> @@ -86,6 +86,11 @@ static ssize_t
>>>>>>>> tpdm_simple_dataset_show(struct device *dev,
>>>>>>>> return -EINVAL;
>>>>>>>> return sysfs_emit(buf, "0x%x\n",
>>>>>>>> drvdata->cmb->patt_mask[tpdm_attr->idx]);
>>>>>>>> + case CMB_MSR:
>>>>>>>> + if (tpdm_attr->idx >= drvdata->cmb_msr_num)
>>>>>>>> + return -EINVAL;
>>>>>>>> + return sysfs_emit(buf, "0x%x\n",
>>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx]);
>>>>>>>> }
>>>>>>>> return -EINVAL;
>>>>>>>> }
>>>>>>>> @@ -162,6 +167,12 @@ static ssize_t
>>>>>>>> tpdm_simple_dataset_store(struct device *dev,
>>>>>>>> else
>>>>>>>> ret = -EINVAL;
>>>>>>>> break;
>>>>>>>> + case CMB_MSR:
>>>>>>>> + if (tpdm_attr->idx < drvdata->cmb_msr_num)
>>>>>>>> + drvdata->cmb->msr[tpdm_attr->idx] = val;
>>>>>>>> + else
>>>>>>>> + ret = -EINVAL;
>>>>>>>
>>>>>>>
>>>>>>> minor nit: Could we not break from here instead of adding return
>>>>>>> -EINVAL
>>>>>>> for each case ? (I understand it has been done for the existing
>>>>>>> cases.
>>>>>>> But I think we should clean up all of that, including the ones
>>>>>>> you added
>>>>>>> in Patch 5. Similarly for the dataset_show()
>>>>>>
>>>>>> Sure, do I also need to change the DSB corresponding code? If so,
>>>>>> how about
>>>>>>
>>>>>> if I add a new patch to the next patch series to change the
>>>>>> previous existing cases?
>>>>>
>>>>> You could fix the existing cases as a preparatory patch of the next
>>>>> version of this series. I can pick it up and push it to next as I
>>>>> see fit.
>>>>
>>>> Got it. I will update this to the next patch series.
>>>
>>> Hi Suzuki,
>>>
>>>
>>> Since the dataset data is configured with spin lock protection, it
>>> needs to be unlock before return.
>>>
>>> List my modification below. Would you mind help review to see if it
>>> is good for you.
>>>
>>> static ssize_t tpdm_simple_dataset_store(struct device *dev,
>>> struct device_attribute *attr,
>>> const char *buf,
>>> size_t size)
>>> {
>>> unsigned long val;
>>>
>>> struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> struct tpdm_dataset_attribute *tpdm_attr =
>>> container_of(attr, struct tpdm_dataset_attribute, attr);
>>>
>>> if (kstrtoul(buf, 0, &val))
>>> return -EINVAL;
>>>
>>> spin_lock(&drvdata->spinlock);
>>
>> Use guard() to avoid explicit unlock on return and then you don't need
>> the spin_unlock() everywhere. It would be done on return from the
>> function implicitly.
>>
>>
>>> switch (tpdm_attr->mem) {
>>> case DSB_TRIG_PATT:
>>
>> With guard() in place:
>>
>> ret = -EINVAL;
>>
>> switch () {
>> case XXX:
>>
>> if (tpdm_attr->idx < TPDM_XXXX_IDX) {
>> drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
>> ret = size;
>> }
>> break;
>> case ...
>> ...
>> }
>>
>> return ret;
>
> Thanks for your suggestion. I will update the code like below.
>
> I will update it in the next version of the patch series if it meets
> your expectation.
>
> static ssize_t tpdm_simple_dataset_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> size_t size)
> {
> unsigned long val;
> ssize_t ret = -EINVAL;
>
> struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> struct tpdm_dataset_attribute *tpdm_attr =
> container_of(attr, struct tpdm_dataset_attribute, attr);
>
> if (kstrtoul(buf, 0, &val))
> return ret;
>
> guard(spinlock)(&drvdata->spinlock);
> switch (tpdm_attr->mem) {
> case DSB_TRIG_PATT:
> if (tpdm_attr->idx < TPDM_DSB_MAX_PATT) {
> drvdata->dsb->trig_patt[tpdm_attr->idx] = val;
> ret =size;
> }
> break;
> case ...
>
> ...
> }
> return ret;
> }
>
Yes that looks good to me. Please rebase this on to for-next/queue
branch on the coresight repository.
Suzuki
Powered by blists - more mailing lists