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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <708a5bbd-2bad-4f94-8fd1-6bd10825ba71@linaro.org>
Date: Wed, 1 Oct 2025 14:44:06 +0100
From: James Clark <james.clark@...aro.org>
To: Leo Yan <leo.yan@....com>
Cc: Suzuki K Poulose <suzuki.poulose@....com>,
 Mike Leach <mike.leach@...aro.org>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jonathan Corbet <corbet@....net>, coresight@...ts.linaro.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 5/6] coresight: Add format attribute for setting the
 timestamp interval



On 01/10/2025 2:28 pm, Leo Yan wrote:
> On Wed, Oct 01, 2025 at 01:40:37PM +0100, James Clark wrote:
> 
> [...]
> 
>>>> @@ -103,6 +111,9 @@ static struct attribute *etm_config_formats_attr[] = {
>>>>    	&format_attr_configid.attr,
>>>>    	&format_attr_branch_broadcast.attr,
>>>>    	&format_attr_cc_threshold.attr,
>>>> +#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
>>>> +	&format_attr_ts_level.attr,
>>>> +#endif
>>>
>>> By using .visible() callback for attrs, we can improve a bit code
>>> without spreading "#ifdef IS_ENABLED()" in this file. E.g.,
>>>
>>>      static umode_t format_attr_is_visible(struct kobject *kobj,
>>>                                      struct attribute *attr, int n)
>>>      {
>>>           struct device *dev = kobj_to_dev(kobj);
>>>
>>>           if (attr == &format_attr_ts_level.attr &&
>>> 	    !IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X))
>>>                   return 0;
>>>
>>>           return attr->mode;
>>>      }
>>>
>>> Otherwise, LGTM:
>>>
>>> Reviewed-by: Leo Yan <leo.yan@....com>
>>>
>>
>> Unfortunately that won't work because you'd have to always include
>> coresight-etm4x.h. This file is compiled for both arm32 and arm64 so it
>> would break the arm32 build.
>>
>> I could define the TTR_CFG_FLD_ts_level_* stuff somewhere else but then it
>> becomes messier than just doing the #ifdefs here.
> 
> ATTR_CFG_FLD_ts_level_* is only used in coresight-etm4x-core.c, it is not
> used in coresight-etm-perf.c. Thus, we don't need to include
> coresight-etm4x.h in coresight-etm-perf.c. Do I miss anything?

Yes, GEN_PMU_FORMAT_ATTR() uses them but it makes it hard to see.

> 
> A similiar case is the attr 'cc_threshold' is only used by ETMv4, it is
> exported always. It is not bad for me to always expose these attrs but
> in the are ignored in the ETMv3 driver - so we even don't need to
> bother adding .visible() callback.
> 

I disagree with always showing them. I think they should be hidden if 
they're not used, or at least return an error to avoid confusing users. 
It also wastes config bits if they're allocated but never used.

Either way, this was done because of the header mechanics which can only 
be avoided by adding more changes than just the #ifdefs. There are also 
already ETM4 #ifdefs in the file.

> Thanks,
> Leo


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ