| 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: <cf167f2c-b87b-4def-8efc-f9ff1cd49597@arm.com>
Date: Fri, 12 Jan 2024 09:29:15 +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 6/8] coresight-tpdm: Add timestamp control register
support for the CMB
On 12/01/2024 02:42, Tao Zhang wrote:
>
> On 1/8/2024 6:42 PM, Suzuki K Poulose wrote:
>> On 05/01/2024 07:49, Tao Zhang wrote:
>>>
>>> On 12/30/2023 5:39 PM, Suzuki K Poulose wrote:
>>>> On 25/12/2023 01:55, Tao Zhang wrote:
>>>>>
>>>>> On 12/20/2023 7:07 PM, Suzuki K Poulose wrote:
>>>>>> On 20/12/2023 09:51, Tao Zhang wrote:
>>>>>>>
>>>>>>> On 12/19/2023 9:51 PM, Suzuki K Poulose wrote:
>>>>>>>> On 19/12/2023 02:43, Tao Zhang wrote:
>>>>>>>>>
>>>>>>>>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote:
>>>>>>>>>> On 21/11/2023 02:24, Tao Zhang wrote:
..
>>>>>>>>>>> char *buf)
>>>>>>>>>>> {
>>>>>>>>>>> struct tpdm_drvdata *drvdata =
>>>>>>>>>>> dev_get_drvdata(dev->parent);
>>>>>>>>>>> + ssize_t size = 0;
>>>>>>>>>>> - return sysfs_emit(buf, "%u\n",
>>>>>>>>>>> - (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>>>> + if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>>>> + size = sysfs_emit(buf, "%u\n",
>>>>>>>>>>> + (unsigned int)drvdata->dsb->patt_ts);
>>>>>>>>>>> +
>>>>>>>>>>> + if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>>>> + size = sysfs_emit(buf, "%u\n",
>>>>>>>>>>> + (unsigned int)drvdata->cmb->patt_ts);
>>>>>>>>>>
>>>>>>>>>> Why does this need to show two values ? This must only show
>>>>>>>>>> ONE value.
>>>>>>>>>> How you deduce that might be based on the availability of the
>>>>>>>>>> feature
>>>>>>>>>> set. Or store the TS value in the drvdata and use that instead
>>>>>>>>>> for
>>>>>>>>>> controlling CMB/DSB.
>>>>>>>>>
>>>>>>>>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can
>>>>>>>>> I separate them
>>>>>>>>
>>>>>>>> The question really is, do we need fine grained control. i.e.,
>>>>>>>>
>>>>>>>> enable TS for DSB but not for CMB or vice versa.
>>>>>>>>
>>>>>>>> I am not an expert on the usage scenario of the same. So, if
>>>>>>>> you/Qcomm
>>>>>>>> thinks the users need separate, fine grained control for timestamp
>>>>>>>> for the DSB and CMB, then yes, follow your recommendation below.
>>>>>>>> i.e., tpdm.../dsb_patt/enable_ts
>>>>>>>>
>>>>>>>>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like
>>>>>>>>> below.
>>>>>>>>>
>>>>>>>>> tpdm0/dsb_patt/enable_dsb_ts
>>>>>>>>
>>>>>>>> You don't need enable_dsb_ts. It could be "enable_ts"
>>>>>>>>
>>>>>>>>>
>>>>>>>>> tpdm1/cmb_patt/enable_cmb_ts
>>>>>>>>>
>>>>>>>>> Is this design appropriate?
>>>>>>>>
>>>>>>>>
>>>>>>>> Otherwise, stick to single enable_ts : which enables the ts for
>>>>>>>> both
>>>>>>>> CMB/DSB. And it only ever show one value : 0 (TS is disabled for
>>>>>>>> both
>>>>>>>> CMB/DSB) 1 : TS enabled for both.
>>>>>>>
>>>>>>> We have a very special case, such as the TPDM supporting both CMB
>>>>>>> and
>>>>>>>
>>>>>>> DSB datasets. Although this case is very rare, it still exists.
>>>>>>>
>>>>>>> Can I use the data bit to instruct whether timestamp is enabled
>>>>>>> for CMB/DSB or not? For example,
>>>>>>>
>>>>>>> size = sysfs_emit(buf, "%u\n",
>>>>>>> (unsigned int)(drvdata->dsb->patt_ts << 1 |
>>>>>>> drvdata->cmb->patt_ts));
>>>>>>>
>>>>>>> Thus, this value can instruct the following situations.
>>>>>>>
>>>>>>> 0 - TS is disabled for both CMB/DSB
>>>>>>>
>>>>>>> 1 - TS is enabled for CMB
>>>>>>>
>>>>>>> 2 - TS is enabled for DSB
>>>>>>>
>>>>>>> 3 - TS is enabled for both
>>>>>>>
>>>>>>> Is this approach acceptable?
>>>>>>>
>>>>>>
>>>>>> No, please stick to separate controls for TS. Do not complicate
>>>>>> the user interface.
>>>>>>
>>>>>> i.e.,
>>>>>> tpdm0/dsb_patt/enable_ts
>>>>>> tpdm0/cmb_patt/enable_ts
>>>>>
>>>>> We need to be able to control/show dsb and cmb timestamp enablement
>>>>> independently.
>>>>>
>>>>> Can we achieve this requirement if we use a sysfs file with the
>>>>> same name?
>>>>
>>>> They are independent and in their respective directory (group) for
>>>> CMB and DSB. What am I missing ?
>>>> e.g., if you want to enable TS for DSB, you do :
>>>>
>>>> $ echo 1 > dsb_patt/enable_ts
>>>>
>>>> And that only works for DSB not for CMB.
>>>
>>> We have a special case that the TPDM supports both DSB and CMB
>>> dataset. In this special case, when we
>>>
>>> issue this command to enable timestamp, it will call enable_ts_store
>>> API, right?
>>>
>>> if (tpdm_has_dsb_dataset(drvdata))
>>> drvdata->dsb->patt_ts = !!val;
>>>
>>> if (tpdm_has_cmb_dataset(drvdata))
>>> drvdata->cmb->patt_ts = !!val;
>>
>> I don't understand. If they both are under different subgroups, why
>> should they be conflicting ? Are you not able to distinguish them, when
>> you creat those attributes ? i.e., create two different "attributes" ?
>
> Yes, although some TPDMs can support both CMB dataset and DSB dataset,
> we need to configure them separately
>
> in some scenarios. Based on your suggestion, I want to use the following
> approach to implement it.
>
> See below.
>
>>
>> See below.
>>
>>> Since this special TPDM supports both DSB and CMB dataset, both DSB
>>> patt_ts and CMB patt_ts will be set
>>>
>>> in this case even if I only configure the file in the DSB directory,
>>> right?
>>>
>>> This is the problem we have now.
>>>
>>
>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> + return size;
>>>>>>>>>>> }
>>>>>>>>>>> /*
>>>>>>>>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct
>>>>>>>>>>> device *dev,
>>>>>>>>>>> return -EINVAL;
>>>>>>>>>>> spin_lock(&drvdata->spinlock);
>>>>>>>>>>> - drvdata->dsb->patt_ts = !!val;
>>>>>>>>>>> + if (tpdm_has_dsb_dataset(drvdata))
>>>>>>>>>>> + drvdata->dsb->patt_ts = !!val;
>>>>>>>>>>> +
>>>>>>>>>>> + if (tpdm_has_cmb_dataset(drvdata))
>>>>>>>>>>> + drvdata->cmb->patt_ts = !!val;
>>>>>>>>>>> spin_unlock(&drvdata->spinlock);
>>>>>>>>>>> +
>>>>>>>>>>> return size;
>>>>>>>>>>> }
>>>>>>>>>>> static DEVICE_ATTR_RW(enable_ts);
>>
>> Do not overload the same for both DSB and CMB. Create one for each in
>> DSB and CMB ? They could share the same show/store routines, but could
>> be done with additional variable to indicate which attribute they are
>> controlling. Like the other attributes, using dev_ext_attribute or such.
>
> New approach below, please help review to see if it is acceptable?
>
> #define tpdm_patt_enable_ts_rw(name, mem) \
> (&((struct tpdm_dataset_attribute[]) { \
> { \
> __ATTR(name, 0644, enable_ts_show, \
> enable_ts_store), \
> mem, \
> } \
> })[0].attr.attr)
>
>
> #define DSB_PATT_ENABLE_TS \
> tpdm_patt_enable_ts_rw(enable_ts, \
> DSB_PATT)
>
>
> #define CMB_PATT_ENABLE_TS \
> tpdm_patt_enable_ts_rw(enable_ts, \
> CMB_PATT)
>
>
> static ssize_t enable_ts_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> struct tpdm_dataset_attribute *tpdm_attr =
> container_of(attr, struct tpdm_dataset_attribute, attr);
> ssize_t size = 0;
>
> if (tpdm_attr->mem == DSB_PATT) {
> size = sysfs_emit(buf, "%u\n",
> (unsigned int)drvdata->dsb->patt_ts);
> } else if (tpdm_attr->mem == CMB_PATT) {
> size = sysfs_emit(buf, "%u\n",
> (unsigned int)drvdata->cmb->patt_ts);
> } else
> return -EINVAL;
>
> return size;
> }
>
> static ssize_t enable_ts_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> size_t size)
> {
> struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
> struct tpdm_dataset_attribute *tpdm_attr =
> container_of(attr, struct tpdm_dataset_attribute, attr);
> unsigned long val;
>
> if ((kstrtoul(buf, 0, &val)) || (val & ~1UL))
> return -EINVAL;
>
> spin_lock(&drvdata->spinlock);
> if (tpdm_attr->mem == DSB_PATT) {
> drvdata->dsb->patt_ts = !!val;
> } else if (tpdm_attr->mem == CMB_PATT) {
> drvdata->cmb->patt_ts = !!val;
> } else
> return -EINVAL;
> spin_unlock(&drvdata->spinlock);
>
> return size;
> }
>
>
Yes, that is what I had in mind.
Thanks
Suzuki
> Best,
>
> Tao
>
>>
>>
>> Suzuki
>>
Powered by blists - more mailing lists