| Message ID | 20200805025458.2978-26-tingwei@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: allow to build coresight as modules | expand |
On 08/05/2020 03:54 AM, Tingwei Zhang wrote: > Enhance coresight developer's efficiency to debug coresight drivers. > - Kconfig becomes a tristate, to allow =m > - append -core to source file name to allow module to > be called coresight by the Makefile > - modules can have only one init/exit, so we add the etm_perf > register/unregister function calls to the core init/exit > functions. > - 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> > Tested-by: Mike Leach <mike.leach@linaro.org> > --- > drivers/hwtracing/coresight/Kconfig | 5 ++- > drivers/hwtracing/coresight/Makefile | 5 ++- > .../{coresight.c => coresight-core.c} | 42 ++++++++++++++----- > .../hwtracing/coresight/coresight-etm-perf.c | 8 +++- > .../hwtracing/coresight/coresight-etm-perf.h | 3 ++ > 5 files changed, 48 insertions(+), 15 deletions(-) > rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c} (98%) Personally, I would like to rename this to core.c dropping the "coresight-" prefix here (now that we have to do a rename). And we should do that ideally for all the other files (but not proposing it to be part of this series, and could be something that we could pursue if everyone agrees to it). We are inside the coresight directory anyways and having a prefix doesn't help with anything. The patch as such looks good to me. Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
On 08/05/2020 05:29 PM, Suzuki K Poulose wrote: > On 08/05/2020 03:54 AM, Tingwei Zhang wrote: >> Enhance coresight developer's efficiency to debug coresight drivers. >> - Kconfig becomes a tristate, to allow =m >> - append -core to source file name to allow module to >> be called coresight by the Makefile >> - modules can have only one init/exit, so we add the etm_perf >> register/unregister function calls to the core init/exit >> functions. >> - 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> >> Tested-by: Mike Leach <mike.leach@linaro.org> >> --- >> drivers/hwtracing/coresight/Kconfig | 5 ++- >> drivers/hwtracing/coresight/Makefile | 5 ++- >> .../{coresight.c => coresight-core.c} | 42 ++++++++++++++----- >> .../hwtracing/coresight/coresight-etm-perf.c | 8 +++- >> .../hwtracing/coresight/coresight-etm-perf.h | 3 ++ >> 5 files changed, 48 insertions(+), 15 deletions(-) >> rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c} >> (98%) > > Personally, I would like to rename this to core.c dropping the > "coresight-" prefix here (now that we have to do a rename). And we > should do that ideally for all the other files (but not proposing > it to be part of this series, and could be something that we could > pursue if everyone agrees to it). > > We are inside the coresight directory anyways and having a prefix > doesn't help with anything. > > The patch as such looks good to me. > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> On a second look, I believe for the sake of completion, we should set the "owner" of the etm, now that we are a module. The question is, which one should that be. It could be the "coresight" or the "coresight-etm{3,4}x". I believe the "coresight" is the better choice. Cheers Suzuki
On 2020-08-06 17:33, Suzuki K Poulose wrote: > On 08/05/2020 05:29 PM, Suzuki K Poulose wrote: >> On 08/05/2020 03:54 AM, Tingwei Zhang wrote: >>> Enhance coresight developer's efficiency to debug coresight drivers. >>> - Kconfig becomes a tristate, to allow =m >>> - append -core to source file name to allow module to >>> be called coresight by the Makefile >>> - modules can have only one init/exit, so we add the etm_perf >>> register/unregister function calls to the core init/exit >>> functions. >>> - 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> >>> Tested-by: Mike Leach <mike.leach@linaro.org> >>> --- >>> drivers/hwtracing/coresight/Kconfig | 5 ++- >>> drivers/hwtracing/coresight/Makefile | 5 ++- >>> .../{coresight.c => coresight-core.c} | 42 ++++++++++++++----- >>> .../hwtracing/coresight/coresight-etm-perf.c | 8 +++- >>> .../hwtracing/coresight/coresight-etm-perf.h | 3 ++ >>> 5 files changed, 48 insertions(+), 15 deletions(-) >>> rename drivers/hwtracing/coresight/{coresight.c => >>> coresight-core.c} (98%) >> >> Personally, I would like to rename this to core.c dropping the >> "coresight-" prefix here (now that we have to do a rename). And we >> should do that ideally for all the other files (but not proposing >> it to be part of this series, and could be something that we could >> pursue if everyone agrees to it). >> >> We are inside the coresight directory anyways and having a prefix >> doesn't help with anything. >> >> The patch as such looks good to me. >> >> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> > > On a second look, I believe for the sake of completion, we > should set the "owner" of the etm, now that we are a module. > The question is, which one should that be. It could be the > "coresight" or the "coresight-etm{3,4}x". > > I believe the "coresight" is the better choice. If you mean pmu->owner, you shouldn't really have much of a choice - it should be the module containing the actual PMU callbacks, such that they can't suddenly disappear while the PMU is in use. Allowing perf to take a reference to some other module and not actually protect itself would not be good. It should be pretty rare that the correct owner is anything other than THIS_MODULE ;) Hopefully the dependencies are such that the core module automatically holds its own reference to the individual ETM driver module(s) by the time it registers the PMU. Robin.
On 2020-08-06 18:25, Robin Murphy wrote: > On 2020-08-06 17:33, Suzuki K Poulose wrote: >> On 08/05/2020 05:29 PM, Suzuki K Poulose wrote: >>> On 08/05/2020 03:54 AM, Tingwei Zhang wrote: >>>> Enhance coresight developer's efficiency to debug coresight drivers. >>>> - Kconfig becomes a tristate, to allow =m >>>> - append -core to source file name to allow module to >>>> be called coresight by the Makefile >>>> - modules can have only one init/exit, so we add the etm_perf >>>> register/unregister function calls to the core init/exit >>>> functions. >>>> - 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> >>>> Tested-by: Mike Leach <mike.leach@linaro.org> >>>> --- >>>> drivers/hwtracing/coresight/Kconfig | 5 ++- >>>> drivers/hwtracing/coresight/Makefile | 5 ++- >>>> .../{coresight.c => coresight-core.c} | 42 >>>> ++++++++++++++----- >>>> .../hwtracing/coresight/coresight-etm-perf.c | 8 +++- >>>> .../hwtracing/coresight/coresight-etm-perf.h | 3 ++ >>>> 5 files changed, 48 insertions(+), 15 deletions(-) >>>> rename drivers/hwtracing/coresight/{coresight.c => >>>> coresight-core.c} (98%) >>> >>> Personally, I would like to rename this to core.c dropping the >>> "coresight-" prefix here (now that we have to do a rename). And we >>> should do that ideally for all the other files (but not proposing >>> it to be part of this series, and could be something that we could >>> pursue if everyone agrees to it). >>> >>> We are inside the coresight directory anyways and having a prefix >>> doesn't help with anything. >>> >>> The patch as such looks good to me. >>> >>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> >> On a second look, I believe for the sake of completion, we >> should set the "owner" of the etm, now that we are a module. >> The question is, which one should that be. It could be the >> "coresight" or the "coresight-etm{3,4}x". >> >> I believe the "coresight" is the better choice. > > If you mean pmu->owner, you shouldn't really have much of a choice - it ...by which I meant pmu->module, obviously. Oops :) Anyway, that should certainly be set not just for completeness but for correctness, since there do exist users who are adventurous enough to try unloading modules while perf is running. Robin. > should be the module containing the actual PMU callbacks, such that they > can't suddenly disappear while the PMU is in use. Allowing perf to take > a reference to some other module and not actually protect itself would > not be good. It should be pretty rare that the correct owner is anything > other than THIS_MODULE ;) > > Hopefully the dependencies are such that the core module automatically > holds its own reference to the individual ETM driver module(s) by the > time it registers the PMU. > > Robin. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 08/06/2020 06:39 PM, Robin Murphy wrote: > On 2020-08-06 18:25, Robin Murphy wrote: >> On 2020-08-06 17:33, Suzuki K Poulose wrote: >>> On 08/05/2020 05:29 PM, Suzuki K Poulose wrote: >>>> On 08/05/2020 03:54 AM, Tingwei Zhang wrote: >>>>> Enhance coresight developer's efficiency to debug coresight drivers. >>>>> - Kconfig becomes a tristate, to allow =m >>>>> - append -core to source file name to allow module to >>>>> be called coresight by the Makefile >>>>> - modules can have only one init/exit, so we add the etm_perf >>>>> register/unregister function calls to the core init/exit >>>>> functions. >>>>> - add a MODULE_DEVICE_TABLE for autoloading on boot >>>>> --- >>>>> drivers/hwtracing/coresight/Kconfig | 5 ++- >>>>> drivers/hwtracing/coresight/Makefile | 5 ++- >>>>> .../{coresight.c => coresight-core.c} | 42 >>>>> ++++++++++++++----- >>>>> .../hwtracing/coresight/coresight-etm-perf.c | 8 +++- >>>>> .../hwtracing/coresight/coresight-etm-perf.h | 3 ++ >>>>> 5 files changed, 48 insertions(+), 15 deletions(-) >>>>> rename drivers/hwtracing/coresight/{coresight.c => >>>>> coresight-core.c} (98%) >>>> >>>> Personally, I would like to rename this to core.c dropping the >>>> "coresight-" prefix here (now that we have to do a rename). And we >>>> should do that ideally for all the other files (but not proposing >>>> it to be part of this series, and could be something that we could >>>> pursue if everyone agrees to it). >>>> >>>> We are inside the coresight directory anyways and having a prefix >>>> doesn't help with anything. >>>> >>>> The patch as such looks good to me. >>>> >>>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>> >>> On a second look, I believe for the sake of completion, we >>> should set the "owner" of the etm, now that we are a module. >>> The question is, which one should that be. It could be the >>> "coresight" or the "coresight-etm{3,4}x". >>> >>> I believe the "coresight" is the better choice. >> >> If you mean pmu->owner, you shouldn't really have much of a choice - it > > ...by which I meant pmu->module, obviously. Oops :) > > Anyway, that should certainly be set not just for completeness but for > correctness, since there do exist users who are adventurous enough to > try unloading modules while perf is running. Right. It should be the coresight module, as it holds the PMU code and THIS_MODULE is sufficient. > > Robin. > >> should be the module containing the actual PMU callbacks, such that >> they can't suddenly disappear while the PMU is in use. Allowing perf >> to take a reference to some other module and not actually protect >> itself would not be good. It should be pretty rare that the correct >> owner is anything other than THIS_MODULE ;) >> >> Hopefully the dependencies are such that the core module automatically >> holds its own reference to the individual ETM driver module(s) by the >> time it registers the PMU. This is taken care of by holding module reference whenever we prepare a perf session. Suzuki
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index dfe407cde262..c1198245461d 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -3,7 +3,7 @@ # Coresight configuration # menuconfig CORESIGHT - bool "CoreSight Tracing Support" + tristate "CoreSight Tracing Support" depends on ARM || ARM64 depends on OF || ACPI select ARM_AMBA @@ -15,6 +15,9 @@ menuconfig CORESIGHT specification and configure the right series of components when a trace source gets enabled. + To compile this driver as a module, choose M here: the + module will be called coresight. + if CORESIGHT config CORESIGHT_LINKS_AND_SINKS tristate "CoreSight Link and Sink drivers" diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 0359d5a1588f..1b35b55bd420 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -2,8 +2,9 @@ # # Makefile for CoreSight drivers. # -obj-$(CONFIG_CORESIGHT) += coresight.o coresight-etm-perf.o \ - coresight-platform.o coresight-sysfs.o +obj-$(CONFIG_CORESIGHT) += coresight.o +coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \ + coresight-sysfs.o obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \ coresight-tmc-etr.o diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight-core.c similarity index 98% rename from drivers/hwtracing/coresight/coresight.c rename to drivers/hwtracing/coresight/coresight-core.c index 3b4abdda1f72..5cd200031613 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -1444,16 +1444,6 @@ int coresight_timeout(void __iomem *addr, u32 offset, int position, int value) } EXPORT_SYMBOL_GPL(coresight_timeout); -struct bus_type coresight_bustype = { - .name = "coresight", -}; - -static int __init coresight_init(void) -{ - return bus_register(&coresight_bustype); -} -postcore_initcall(coresight_init); - /* * coresight_release_platform_data: Release references to the devices connected * to the output port of this device. @@ -1662,3 +1652,35 @@ char *coresight_alloc_device_name(struct coresight_dev_list *dict, return name; } EXPORT_SYMBOL_GPL(coresight_alloc_device_name); + +struct bus_type coresight_bustype = { + .name = "coresight", +}; + +static int __init coresight_init(void) +{ + int ret; + + ret = bus_register(&coresight_bustype); + if (ret) + return ret; + + ret = etm_perf_init(); + if (ret) + bus_unregister(&coresight_bustype); + + return ret; +} + +static void __exit coresight_exit(void) +{ + etm_perf_exit(); + bus_unregister(&coresight_bustype); +} + +module_init(coresight_init); +module_exit(coresight_exit); + +MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>"); +MODULE_DESCRIPTION("Arm CoreSight tracer driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 3728c44e5763..668b3ff11576 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -591,7 +591,7 @@ void etm_perf_del_symlink_sink(struct coresight_device *csdev) csdev->ea = NULL; } -static int __init etm_perf_init(void) +int __init etm_perf_init(void) { int ret; @@ -618,4 +618,8 @@ static int __init etm_perf_init(void) return ret; } -device_initcall(etm_perf_init); + +void __exit etm_perf_exit(void) +{ + perf_pmu_unregister(&etm_pmu); +} diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 05f89723e282..3e4f2ad5e193 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -82,4 +82,7 @@ static inline void *etm_perf_sink_config(struct perf_output_handle *handle) #endif /* CONFIG_CORESIGHT */ +int __init etm_perf_init(void); +void __exit etm_perf_exit(void); + #endif