Re: [PATCH v7 09/25] coresight: etm3x: allow etm3x to be built as a module

[Tingwei Zhang <tingweiz@codeaurora.org>]

On Thu, Aug 06, 2020 at 01:44:56PM +0800, Sai Prakash Ranjan wrote:
> Hi Mike,
> 
> On 2020-08-05 18:24, Mike Leach wrote:
> >Hi,
> >
> >On Wed, 5 Aug 2020 at 12:05, Suzuki K Poulose <suzuki.poulose@arm.com>
> >wrote:
> >>
> >>On 08/05/2020 03:54 AM, Tingwei Zhang wrote:
> >>> From: Kim Phillips <kim.phillips@arm.com>
> >>>
> >>> Allow to build coresight-etm3x as a module, for ease of development.
> >>>
> >>> - Kconfig becomes a tristate, to allow =m
> >>> - append -core to source file name to allow module to
> >>>    be called coresight-etm3x by the Makefile
> >>> - add an etm_remove function, for module unload
> >>> - add a MODULE_DEVICE_TABLE for autoloading on boot
> >>>
> >>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>> Cc: Leo Yan <leo.yan@linaro.org>
> >>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >>> Cc: Randy Dunlap <rdunlap@infradead.org>
> >>> Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> Cc: Russell King <linux@armlinux.org.uk>
> >>> Signed-off-by: Kim Phillips <kim.phillips@arm.com>
> >>> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> >>> Reviewed-by: Mike Leach <mike.leach@linaro.org>
> >>> ---
> >>>   drivers/hwtracing/coresight/Kconfig           |  5 +++-
> >>>   drivers/hwtracing/coresight/Makefile          |  3 ++-
> >>>   ...resight-etm3x.c => coresight-etm3x-core.c} | 27
> ++++++++++++++++++-
> >>>   3 files changed, 32 insertions(+), 3 deletions(-)
> >>>   rename drivers/hwtracing/coresight/{coresight-etm3x.c =>
> coresight-etm3x-core.c} (97%)
> >>>
> >>> diff --git a/drivers/hwtracing/coresight/Kconfig
> b/drivers/hwtracing/coresight/Kconfig
> >>> index 6433f835fc97..8fd9fd139cf3 100644
> >>> --- a/drivers/hwtracing/coresight/Kconfig
> >>> +++ b/drivers/hwtracing/coresight/Kconfig
> >>> @@ -65,7 +65,7 @@ config CORESIGHT_SINK_ETBV10
> >>>         special enhancement or added features.
> >>>
> >>>   config CORESIGHT_SOURCE_ETM3X
> >>> -     bool "CoreSight Embedded Trace Macrocell 3.x driver"
> >>> +     tristate "CoreSight Embedded Trace Macrocell 3.x driver"
> >>>       depends on !ARM64
> >>>       select CORESIGHT_LINKS_AND_SINKS
> >>>       help
> >>> @@ -74,6 +74,9 @@ config CORESIGHT_SOURCE_ETM3X
> >>>         This is primarily useful for instruction level tracing.
> Depending
> >>>         the ETM version data tracing may also be available.
> >>>
> >>> +       To compile this driver as a module, choose M here: the
> >>> +       module will be called coresight-etm3x.
> >>> +
> >>>   config CORESIGHT_SOURCE_ETM4X
> >>>       bool "CoreSight Embedded Trace Macrocell 4.x driver"
> >>>       depends on ARM64
> >>> diff --git a/drivers/hwtracing/coresight/Makefile
> b/drivers/hwtracing/coresight/Makefile
> >>> index 19497d1d92bf..d619cfd0abd8 100644
> >>> --- a/drivers/hwtracing/coresight/Makefile
> >>> +++ b/drivers/hwtracing/coresight/Makefile
> >>> @@ -11,7 +11,8 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) +=
> coresight-tpiu.o
> >>>   obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
> >>>   obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
> >>>                                          coresight-replicator.o
> >>> -obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
> coresight-etm-cp14.o \
> >>> +obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
> >>> +coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
> >>>                                       coresight-etm3x-sysfs.o
> >>>   obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> >>>                                       coresight-etm4x-sysfs.o
> >>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c
> b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> >>> similarity index 97%
> >>> rename from drivers/hwtracing/coresight/coresight-etm3x.c
> >>> rename to drivers/hwtracing/coresight/coresight-etm3x-core.c
> >>> index bf22dcfd3327..82b333c40006 100644
> >>> --- a/drivers/hwtracing/coresight/coresight-etm3x.c
> >>> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> >>> @@ -895,6 +895,23 @@ static int etm_probe(struct amba_device *adev,
> const struct amba_id *id)
> >>>       return ret;
> >>>   }
> >>>
> >>> +static int __exit etm_remove(struct amba_device *adev)
> >>> +{
> >>> +     struct etm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
> >>> +
> >>> +     etm_perf_symlink(drvdata->csdev, false);
> >>> +
> >>> +     if (--etm_count == 0) {
> >>
> >>Could there be multiple instances of remove running in parallel ? I
> >>believe we need some sort of a protection here to avoid racing.
> >>
> >>Or even better, I would recommend leaving the notifiers registered
> >>at module_init and removed at the cleanup of the module, just like
> >>we are doing for etm4x driver, and get rid of this silly scheme.
> >
> >I would agree that this needs addressing but this is an independent
> >problem that could be better served by a separate patchset, rather
> >than add feature creep to this set. The same schema is used in the CTI
> >driver as well and needs fixing there.
> >
> >>Like here :
> >>
> >>
> https://lkml.kernel.org/r/20200729051310.18436-1-saiprakash.ranjan@codeaur
> ora.org
> >>
> >
> >This conflicts with the init / exit fns created for module loading in
> >the etm4x part of this set..
> >
> >A decision needs to be made on which set gets applied first - my view
> >is that the module set could go first, then a set fixing the PM
> >registration issues for all three affected drivers to be applied next.
> 
> The etm4_count race is a bug fix which in my opinion should go first because
> this feature would likely to be merged for 5.10 and till then we would have
> a bug lying around if the fix patch for the race does not go in right?
> 

Agree with you, Sai.  I'll rebase this series to your change.

> Thanks,
> Sai
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel