diff mbox series

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

Message ID 20200805025458.2978-10-tingwei@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series coresight: allow to build coresight as modules | expand

Commit Message

Tingwei Zhang Aug. 5, 2020, 2:54 a.m. UTC
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%)

Comments

Suzuki K Poulose Aug. 5, 2020, 11:09 a.m. UTC | #1
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.
Like here :

https://lkml.kernel.org/r/20200729051310.18436-1-saiprakash.ranjan@codeaurora.org


Cheers
Suzuki
Mike Leach Aug. 5, 2020, 12:54 p.m. UTC | #2
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@codeaurora.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 change to making all the drivers module loadable  provides the
init functions needed to initalise the PM outside the probe functions,
just like the patch mentioned above and makes a change to all three
drivers easy to create.

Regards

Mike
>
> Cheers
> Suzuki
Suzuki K Poulose Aug. 5, 2020, 2:58 p.m. UTC | #3
On 08/05/2020 01:54 PM, 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>


>>>        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

I agree that it is an independent problem, but this series makes it more
exposed, i.e, calling the probe/remove at runtime in any order.
So, pushing this series before that is fixed is going to cause trouble,
if they both end up in different releases.

> 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@codeaurora.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.

I think we should do the opposite. Fixing the hotplug is fairly straight
forward, and need not block on this large series.

Cheers
Suzuki
Sai Prakash Ranjan Aug. 6, 2020, 5:44 a.m. UTC | #4
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@codeaurora.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?

Thanks,
Sai
Tingwei Zhang Aug. 6, 2020, 11:43 a.m. UTC | #5
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
diff mbox series

Patch

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) {
+		cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
+		if (hp_online)
+			cpuhp_remove_state_nocalls(hp_online);
+	}
+
+	coresight_unregister(drvdata->csdev);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static int etm_runtime_suspend(struct device *dev)
 {
@@ -937,6 +954,8 @@  static const struct amba_id etm_ids[] = {
 	{ 0, 0},
 };
 
+MODULE_DEVICE_TABLE(amba, etm_ids);
+
 static struct amba_driver etm_driver = {
 	.drv = {
 		.name	= "coresight-etm3x",
@@ -945,6 +964,12 @@  static struct amba_driver etm_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe		= etm_probe,
+	.remove         = etm_remove,
 	.id_table	= etm_ids,
 };
-builtin_amba_driver(etm_driver);
+module_amba_driver(etm_driver);
+
+MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>");
+MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>");
+MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace driver");
+MODULE_LICENSE("GPL v2");