[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJ9a7VhA0b_o_aYJisfey0XK7FadNfxkUapTQc3nE611RiyVRg@mail.gmail.com>
Date: Tue, 21 Mar 2023 15:12:10 +0000
From: Mike Leach <mike.leach@...aro.org>
To: James Clark <james.clark@....com>
Cc: coresight@...ts.linaro.org,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Suzuki K Poulose <suzuki.poulose@....com>,
Leo Yan <leo.yan@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/9] coresight: Fix CTI module refcount leak by making
it a helper device
Hi James
On Fri, 10 Mar 2023 at 16:06, James Clark <james.clark@....com> wrote:
>
> Changes since v1:
>
> * Don't dereference handle in tmc_etr_get_buffer() when not in perf mode.
> * Fix some W=1 warnings
> * Add a commit to rename child/output in terms of local/remote
>
> -------------------
>
> Currently there is a refcount leak in CTI when using system wide mode
> or tracing multithreaded applications. See the last commit for a
> reproducer. This prevents the module from being unloaded.
>
> Historically there have been a few issues and fixes attempted around
> here which have resulted in some extra logic and a member to keep
> track of CTI being enabled 'struct coresight_device->ect_enabled'.
> The fix in commit 665c157e0204 ("coresight: cti: Fix hang in
> cti_disable_hw()") was also related to CTI having its own
> enable/disable path which came later than other devices.
>
> If we make CTI a helper device and enable helper devices adjacent to
> the path we get very similar enable/disable behavior to now, but with
> more reuse of the existing reference counting logic in the coresight
> core code. This also affects CATU which can have a little bit of
> its hard coded enable/disable code removed.
>
> Enabling CATU on the generic path does require that input connections
> are tracked so that it can get its associated ETR buffer.
>
> Applies to coresight/next (669c4614236a7) but also requires the
> realloc_array patch here [1].
>
> Also available in full here [2].
>
> [1]: https://lore.kernel.org/linux-arm-kernel/20230306152723.3090195-1-james.clark@arm.com/
> [2]: https://gitlab.arm.com/linux-arm/linux-jc/-/tree/james-cs-cti-module-refcount-fix-v2
>
> James Clark (9):
> coresight: Use enum type for cs_mode wherever possible
> coresight: Change name of pdata->conns
> coresight: Rename nr_outports to nr_outconns
> coresight: Rename connection members to allow for input connections
> coresight: Dynamically add connections
> coresight: Store in-connections as well as out-connections
> coresight: Refactor out buffer allocation function for ETR
> coresight: Enable and disable helper devices adjacent to the path
> coresight: Fix CTI module refcount leak by making it a helper device
>
> drivers/hwtracing/coresight/coresight-catu.c | 34 +-
> drivers/hwtracing/coresight/coresight-core.c | 312 +++++++++++-------
> .../hwtracing/coresight/coresight-cti-core.c | 56 ++--
> .../hwtracing/coresight/coresight-cti-sysfs.c | 4 +-
> drivers/hwtracing/coresight/coresight-cti.h | 4 +-
> drivers/hwtracing/coresight/coresight-etb10.c | 3 +-
> .../coresight/coresight-etm3x-core.c | 6 +-
> .../coresight/coresight-etm4x-core.c | 6 +-
> .../hwtracing/coresight/coresight-platform.c | 178 +++++++---
> drivers/hwtracing/coresight/coresight-priv.h | 9 +-
> drivers/hwtracing/coresight/coresight-stm.c | 6 +-
> drivers/hwtracing/coresight/coresight-sysfs.c | 9 +-
> .../hwtracing/coresight/coresight-tmc-etf.c | 2 +-
> .../hwtracing/coresight/coresight-tmc-etr.c | 89 ++---
> drivers/hwtracing/coresight/coresight-tmc.h | 2 +
> drivers/hwtracing/coresight/coresight-tpdm.c | 4 +-
> drivers/hwtracing/coresight/coresight-tpiu.c | 3 +-
> drivers/hwtracing/coresight/coresight-trbe.c | 3 +-
> drivers/hwtracing/coresight/ultrasoc-smb.c | 3 +-
> drivers/hwtracing/coresight/ultrasoc-smb.h | 2 +-
> include/linux/coresight.h | 109 +++---
> 21 files changed, 530 insertions(+), 314 deletions(-)
>
> --
> 2.34.1
>
Looking at this overall - given that the only use of the in_conn is to
reference the connecting device from the helper,
i.e. coresight-catu.c:405: tmp = csdev->pdata->in_conns[i].remote_dev;
would it not be simpler to :
a)
in coresight_connection add a field:
struct coresight_device *origin_dev;
which mimics the origin / target model we already have in coresight_sysfs_link
then
b) the in_conns could simply be references to out_conn object from
origin_dev, rather than a complete coresight_connection with reversed
values, thus simplifying the in_conns handling code, and removing the
unused reversed feilds in the current in_conn object.
e.g. tmp = csdev->pdata->in_conns[i]->origin_dev
The remainder of the code would remain much the same, just adjusted
for in_conns as refs rather than independent conn objects
Regards
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Powered by blists - more mailing lists