linux-kernel - Re: [PATCH v2 2/8] coresight: Convert tag clear function to take a struct cs

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2700171e-dbaa-4218-9b46-5d181f5afee7@linaro.org>
Date: Wed, 19 Mar 2025 12:30:17 +0000
From: James Clark <james.clark@...aro.org>
To: Suzuki K Poulose <suzuki.poulose@....com>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
 Mike Leach <mike.leach@...aro.org>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>, leo.yan@....com
Subject: Re: [PATCH v2 2/8] coresight: Convert tag clear function to take a
 struct cs_access



On 19/03/2025 12:00 pm, Suzuki K Poulose wrote:
> On 18/03/2025 16:21, James Clark wrote:
>> The self hosted claim tag will be reset on device probe in a later
>> commit. We'll want to do this before coresight_register() is called so
>> won't have a coresight_device and have to use cs_access instead.
>>
>> Also make them public and create locked and unlocked versions for
>> later use.
>>
>> Signed-off-by: James Clark <james.clark@...aro.org>
> 
> minor nit:
> 
> The restructuring looks a bit confusing. Could we start with :
> 
> coresight_{set,clear}_self_claim_tag_unlocked in Patch 1. Rather
> than rename what we renamed in patch 1 again here.
> 
> 

I considered collapsing these two actually, would you be ok with that?

>> ---
>>   drivers/hwtracing/coresight/coresight-core.c | 17 +++++++++++++----
>>   include/linux/coresight.h                    |  3 ++-
>>   2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ 
>> hwtracing/coresight/coresight-core.c
>> index 3f1c996d668a..9ff601e2415a 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -151,12 +151,21 @@ static inline void 
>> coresight_set_self_claim_tag(struct coresight_device *csdev)
>>       isb();
>>   }
>> -static inline void coresight_clear_self_claim_tag(struct 
>> coresight_device *csdev)
>> +void coresight_clear_self_claim_tag(struct csdev_access *csa)
>>   {
>> -    csdev_access_relaxed_write32(&csdev->access, 
>> CORESIGHT_CLAIM_SELF_HOSTED,
>> +    CS_UNLOCK(csa->base);
> 
> This looks to be hard coding MMIO based access ? Should we abstract it 
> based on the csdev_access_** ?
> 
> 
> Suzuki
> 
> 

I did something similar earlier but it ended up being huge to do it 
properly and I wasn't sure of the benefits so I deleted it. By 
'properly' I mean change all of the CS_UNLOCK() calls to be abstracted 
and delete etm4_cs_unlock() because CS_UNLOCK() would handle both. It 
ends adding in an if and indirection for every lock/unlock for devices 
that are never sysreg access anyway.

Because there's already this:

static void etm4_cs_unlock(struct etmv4_drvdata *drvdata,
			   struct csdev_access *csa)
{
	if (csa->io_mem)
		CS_UNLOCK(csa->base);

But yeah I can add the if into coresight_clear_self_claim_tag(), 
although it technically doesn't do anything now because it's never 
called by any devices that aren't MMIO. And then it makes 
etm4_cs_unlock() stand out a bit because its not a core function when it 
could be. But that's probably fine.