| 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: <ff154c61-69de-4791-a8a1-8c1ff2a496d1@quicinc.com>
Date: Thu, 11 Jan 2024 11:28:01 +0800
From: Jinlong Mao <quic_jinlmao@...cinc.com>
To: James Clark <james.clark@....com>,
Suzuki K Poulose
<suzuki.poulose@....com>,
Mike Leach <mike.leach@...aro.org>
CC: <coresight@...ts.linaro.org>, <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
"Tingwei
Zhang" <quic_tingweiz@...cinc.com>,
Yuanfang Zhang
<quic_yuanfang@...cinc.com>,
Tao Zhang <quic_taozha@...cinc.com>, Leo Yan
<leo.yan@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Subject: Re: [PATCH] coresight: Add coresight name support
On 12/28/2023 7:26 PM, James Clark wrote:
>
>
> On 28/12/2023 09:33, Mao Jinlong wrote:
>> Add coresight name support for custom names which will be
>> easy to identify the device by the name.
>>
>
> I suppose this is more of a V2 because the subject is the same as the
> one sent earlier this year. But it looks like the discussion on the
> previous one wasn't resolved.
I will make next patch as V2
>
> With the main issues to solve being:
>
> * It would be nice to use the existing root node name instead of adding
> a new property. But at the same time DT nodes are supposed to have
> generic names.
>
> * This only works for DT and not ACPI
>
> To me it seems like adding the new property is just a "cheat" to get
> around not being allowed to have a specific name for the root node. But
> if we admit that we need a name I don't see the benefit of not putting
> the name where the node is already named.
>
> Using the root node name at this point would also undo the hard coded
> per-cpu naming of the CTI and ETM devices, so maybe it would be nice,
> but it's just too late. That means that a new field is necessary.
> Although that field could be a boolean like "use-root-name-for-display"
> or something like that. In the end it probably doesn't really make a
> difference whether it's that or a name string.
>
> And maybe the answer to the ACPI question is just that if anyone needs
> it, they can add it in the future. It doesn't seem like it would
> conflict with anything we do here.
>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@...cinc.com>
>> ---
>> .../hwtracing/coresight/coresight-cti-core.c | 20 ++++++++------
>> drivers/hwtracing/coresight/coresight-dummy.c | 10 ++++---
>> .../hwtracing/coresight/coresight-platform.c | 27 +++++++++++++++++++
>> drivers/hwtracing/coresight/coresight-tpdm.c | 10 ++++---
>> include/linux/coresight.h | 1 +
>> 5 files changed, 53 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
>> index 3999d0a2cb60..60a1e76064a9 100644
>> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
>> @@ -902,14 +902,18 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>> /* default to powered - could change on PM notifications */
>> drvdata->config.hw_powered = true;
>>
>> - /* set up device name - will depend if cpu bound or otherwise */
>> - if (drvdata->ctidev.cpu >= 0)
>> - cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
>> - drvdata->ctidev.cpu);
>> - else
>> - cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
>
> Can we put the new name stuff inside coresight_alloc_device_name()? Then
> it happens by default for every device.
>
> I know Suzuki said previously to do it per-device, but the new DT
> property is just "coresight-name", so it's generic. Rather than being
> specific like "cti-name". So I don't see the benefit of duplicating the
> code at this point if we do decide to do it.
I will add it inside the coresight_alloc_device_name.
>
>> - if (!cti_desc.name)
>> - return -ENOMEM;
>> + cti_desc.name = coresight_get_device_name(dev);
>> + if (!cti_desc.name) {
>> + /* set up device name - will depend if cpu bound or otherwise */
>> + if (drvdata->ctidev.cpu >= 0)
>> + cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
>> + drvdata->ctidev.cpu);
>> + else {
>> + cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
>> + if (!cti_desc.name)
>> + return -ENOMEM;
>> + }
>> + }
>
>>
>> /* setup CPU power management handling for CPU bound CTI devices. */
>> ret = cti_pm_setup(drvdata);
>> diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c
>> index e4deafae7bc2..b19cd400df79 100644
>> --- a/drivers/hwtracing/coresight/coresight-dummy.c
>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>> @@ -76,10 +76,12 @@ static int dummy_probe(struct platform_device *pdev)
>> struct coresight_desc desc = { 0 };
>>
>> if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {
>> -
>> - desc.name = coresight_alloc_device_name(&source_devs, dev);
>> - if (!desc.name)
>> - return -ENOMEM;
>> + desc.name = coresight_get_device_name(dev);
>> + if (!desc.name) {
>> + desc.name = coresight_alloc_device_name(&source_devs, dev);
>> + if (!desc.name)
>> + return -ENOMEM;
>> + }
>>
>> desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>> desc.subtype.source_subtype =
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>> index 9d550f5697fa..284aa22a06b7 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -183,6 +183,18 @@ static int of_coresight_get_cpu(struct device *dev)
>> return cpu;
>> }
>>
>> +static const char *of_coresight_get_device_name(struct device *dev)
>> +{
>> + const char *name = NULL;
>> +
>> + if (!dev->of_node)
>> + return NULL;
>> +
>> + of_property_read_string(dev->of_node, "coresight-name", &name);
>
> Do you need to update the binding docs with this new property?
Yes. I will update the binding doc.
>
> Also a minor nit: Maybe "display-name" is better? "Coresight" is
> implied, and the node is already named, although that node name isn't
> used for display purposes, but this one is.
From Suzuki's comments, "device-name" should be better. What do you think ?
Thanks
Jinlong Mao
Powered by blists - more mailing lists