Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754112AbdCOQCL (ORCPT ); Wed, 15 Mar 2017 12:02:11 -0400 Received: from foss.arm.com ([217.140.101.70]:48958 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754093AbdCOQAv (ORCPT ); Wed, 15 Mar 2017 12:00:51 -0400 Subject: Re: [2/2] coresight: Fix reference count for software sources To: Chunyan Zhang References: <1489073232-5093-3-git-send-email-suzuki.poulose@arm.com> Cc: Mathieu Poirier , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" From: Suzuki K Poulose Message-ID: <33ea0fe9-e93b-0cd0-9950-8443aceafc5b@arm.com> Date: Wed, 15 Mar 2017 16:00:32 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4079 Lines: 113 On 15/03/17 03:51, Chunyan Zhang wrote: > Hi Suzuki, > > On 15 March 2017 at 02:06, Suzuki K Poulose wrote: >> On 14/03/17 17:40, Mathieu Poirier wrote: >>> >>> On 14 March 2017 at 11:32, Mathieu Poirier >>> wrote: >>>> >>>> From: "Suzuki K. Poulose" >>>> >>>> For software sources (i.e STM), there could be multiple agents >>>> generating the trace data, unlike the ETMs. So we need to >>>> properly do the accounting for the active number of users >>>> to disable the device when the last user goes away. Right >>>> now, the reference counting is broken for sources as we skip >>>> the actions when we detect that the source is enabled. >>>> >>>> This patch fixes the problem by adding the refcounting for >>>> software sources, even when they are enabled. >>>> >>>> Cc: Mathieu Poirier >>>> Reported-by: Robert Walker >>>> Signed-off-by: Suzuki K Poulose >>>> --- >>>> drivers/hwtracing/coresight/coresight.c | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/hwtracing/coresight/coresight.c >>>> b/drivers/hwtracing/coresight/coresight.c >>>> index 34cd1ed..2da9e39 100644 >>>> --- a/drivers/hwtracing/coresight/coresight.c >>>> +++ b/drivers/hwtracing/coresight/coresight.c >>>> @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device *csdev) >>>> int cpu, ret = 0; >>>> struct coresight_device *sink; >>>> struct list_head *path; >>>> + enum coresight_dev_subtype_source subtype = >>>> csdev->subtype.source_subtype; >>> >>> >>> Checkpatch.pl complains about a line over 80 characters. >>> >>>> >>>> mutex_lock(&coresight_mutex); >>>> >>>> @@ -559,8 +560,16 @@ int coresight_enable(struct coresight_device *csdev) >>>> if (ret) >>>> goto out; >>>> >>>> - if (csdev->enable) >>>> + if (csdev->enable) { >>>> + /* >>>> + * There could be multiple applications driving the >>>> software >>>> + * source. So keep the refcount for each such user when >>>> the >>>> + * source is already enabled. >>>> + */ >>>> + if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) >>>> + atomic_inc(csdev->refcnt); >>>> goto out; >>>> + } >>>> >> >> Btw, should we allow the user to turn on the STM from sysfs (echo 1 > >> $STM/enable_source) ? > > If enabling STM can not be allowed via sysfs, how should we allow > users to turn on STM when they want to mmap STM to user space, and > write STM device from user space directly? For example this kind of > use case [1]. The ioct(, STP_POLICY_ID_SET) indirectly turns on the STM hardware via : stm_char_policy_set_ioctl()->stm.link (stm_generic_link)-> coresight_enable(). > >> All STM users should set their policy via ioctls and that in turn turns the >> device on. > > Yes users can set policy via ioctls to request resource of STM (i.e. > which STM channel(s) will be written), but they still need to use > sysfs to enable STM. As mentioned above, it is not necessary. > >> So it doesn't make sense for enable_source to really enable >> the hardware unless someone really opens it. > > Right, there're two ways to enable STM currently, e.g. > 1) echo .stm > /sys/class/stm_source/stm_ftrace/stm_source_link I am not familiar with the stm_source class. From a quick glance, it looks like, writing to stm_source_link triggers : stm_source_link_store()->stm_source_link_add()->(stm->data->link()). which is fine for connecting a source (ftrace,console or heartbeat) to STM. > 2) echo 1 > $STM/enable_source This is a bit awkward for an application who wants to mmap and stream data, and is quite unnecessary from my explanation above. > > That would probably make people confused, I would appreciate any > better solution. Please let me know if you have any outstanding concerns. Cheers Suzuki