Received-SPF: pass (google.com: domain of linux-kernel+bounces-31004-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <52631d2d-a2bb-4e86-af59-ff67bbf0b32a@arm.com>
Date: Fri, 19 Jan 2024 10:07:43 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 5/8] coresight: Remove the 'enable' field.
Content-Language: en-US
To: James Clark <james.clark@arm.com>, coresight@lists.linaro.org
Cc: Mike Leach <mike.leach@linaro.org>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Maxime Coquelin <mcoquelin.stm32@gmail.com>,
 Alexandre Torgue <alexandre.torgue@foss.st.com>,
 linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
 linux-stm32@st-md-mailman.stormreply.com, Tao Zhang
 <quic_taozha@quicinc.com>, Mao Jinlong <quic_jinlmao@quicinc.com>
References: <20231212155407.1429121-1-james.clark@arm.com>
 <20231212155407.1429121-6-james.clark@arm.com>
 <82e9dbed-281c-4a87-8c0b-a2b1cb0a2247@arm.com>
 <cb65b58f-5c6a-ad99-095c-70b9f013b3e5@arm.com>
From: Suzuki K Poulose <suzuki.poulose@arm.com>
In-Reply-To: <cb65b58f-5c6a-ad99-095c-70b9f013b3e5@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 19/01/2024 09:59, James Clark wrote:
> 
> 
> On 08/01/2024 14:42, Suzuki K Poulose wrote:
>> Hi James
>>
>> +Cc: Tao Zhang <quic_taozha@quicinc.com>
>> +Cc: Mao Jinlong <quic_jinlmao@quicinc.com>
>>
>> On 12/12/2023 15:54, James Clark wrote:
>>> 'enable', which probably should have been 'enabled', is only ever read
>>> in the core code in relation to controlling sources, and specifically
>>> only sources in sysfs mode. Confusingly it's not labelled as such and
>>> relying on it can be a source of bugs like the one fixed by
>>> commit 078dbba3f0c9 ("coresight: Fix crash when Perf and sysfs modes are
>>> used concurrently").
>>>
>>> Most importantly, it can only be used when the coresight_mutex is held
>>> which is only done when enabling and disabling paths in sysfs mode, and
>>> not Perf mode.
>>
>>
>> I think we may be able to relax this a bit for the syfs. The sole reason
>> for holding the mutex is for the "build_path" (and get_enabled_sink)
>> where we need to make sure that no devices are removed/added. We hold
>> necessary refcount on the device and the module (via
>> coresight_grab_device()). After which, we should be able to release the
>> mutex and perform the rest without it in coresight_enable()
>>
> 
> After looking at the per-sink trace ID maps a bit more, I'm not sure if
> it will be worth the mental effort and risk to relax the sysfs locking
> right now.
> 
> We also currently have other things like writing to the global
> tracer_path which are outside of build_path/get_enabled_sink. But for
> the per-sink maps change we'll also have some tracking for sysfs mode
> about which sink map was used for each source and sink. And this state
> will be accessed across multiple sources, and after building the path,
> so it makes sense to leave the locking as-is for now IMO.
> 
> I also can't see a realistic gain from doing it, most sysfs use cases
> would be done from a single threaded script. Maybe in the future we
> could do the change to move the per-device locks into struct
> coresight_device, and then the core code can use them for things that
> need to be locked, but don't need the full coresight_mutex. And then
> that would also work for the per-sink case. But at the moment each
> device has its own lock so that's difficult.

Ok, we could come back to this after the per-sink trace id pool work.
My observation was about the inconsistency between the perf vs sysfs 
mode as you mentioned in the above code.

Suzuki