Allow to build coresight as modules. This enhances developer
efficiency by allowing the development to take place exclusively
on the target, and without needing to reboot in between changes.
depends on:
- "amba: Export amba_bustype"
https://patchwork.kernel.org/patch/10387433/
Queued for 4.18 here:
http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=for-next&id=e9ac68c34f70a0c8d51ee63d259f7c8e79b362c1
- "coresight: Don't use contextID with PID namespaces"
https://www.spinics.net/lists/arm-kernel/msg652755.html
Queued for 4.18 here:
http://git.linaro.org/kernel/coresight.git/log/?h=next
those depends plus this series, available here:
- git://linux-arm.org/linux-kp.git, coresight-modules branch
Changes from v3:
- rebased on coresight/next
- re-split per-module-wise (Mathieu P)
- some unnecessary changes were removed as a consequence
- git bisectability maintained
- module get/put in coresight_build_path/release_path, respectively
instead of enable/disable functions (Suzuki P)
- this prevents drivers from being unloaded when they are part of
the path in use, either by sysfs or perf mode.
- rename few source files e.g., coresight.c -> coresight-core.c in
order to have the resultant module be more intuitively called coresight.ko
instead of coresight-core.ko.
Changes from v2:
- split into multiple patches, for reviewer clarity (Greg K-H)
Changes from v1:
- remove depends on coresight that are in the if CORESIGHT block
(Randy Dunlap)
- rebased and removed coresight_vpid_to_pid relocation on this
new series this patch now depends on:
https://www.spinics.net/lists/arm-kernel/msg652755.html
That new series eliminates patch 2/4 from this series (patches
1 and 4 of this series remain the same).
- actually call tmc_read_unprepare() in tmc_remove() this time,
instead of open-coding the kfree, dma_free_coherent calls.
Changes from versions previously sent to coresight mailing list:
- tmc_remove: free buffer used by TMC-ETR and TMC-ETF by calling
tmc_read_unprepare()
- fixed an unbalanced pm_runtime_enable in coresight-replicator
- etm[4]_remove(): call cpuhp_remove_state_nocalls() and
etm_perf_symlink(.., false) to clear up cpuhp and symlink state
- add module parent checks for all enable/disable functions, source
and sink modules
- refactored device ptr dereferences by introducing a new parent_dev
variable
- corrected replicator author
- whitespace fix in funnel driver
- added user Kconfig help text with the names of the modules.
- Addressed Mathieu's comments:
- renamed coresight-link-sink-tmc coresight-tmc-core
- prevent ability to crash the system by removing drivers from an
active path by adding try_module_get() and module_put() calls in
funnel and replicator drivers' enable and disable functions (thanks for
figuring that out, Mathieu).
- Addressed most of Mathieu's comments:
- rm __inits causing linker section mismatch errors
- barrier_pkt made static, moved to coresight_priv.h
- rm unnecessary tmc_* EXPORT_SYMBOL leftovers
- add some missing MODULE_AUTHORs
Kim Phillips (14):
coresight: cpu_debug: minor module fixups
coresight: use IS_ENABLED for CONFIGs that may be modules
coresight: move shared barrier_pkt[] to coresight_priv.h
coresight: export coresight_timeout and etm_perf_symlink
coresight: get/put module in coresight_build/release_path
coresight: allow stm to be built as a module
coresight: allow dynamic-replicator to be built as a module
coresight: allow etm3x to be built as a module
coresight: allow etm4x to be built as a module
coresight: allow etb to be built as a module
coresight: allow tpiu to be built as a module
coresight: allow tmc to be built as a module
coresight: allow funnel and replicator drivers to be built as modules
coresight: allow the coresight core driver to be built as a module
drivers/hwtracing/coresight/Kconfig | 48 +++++++++++++++----
drivers/hwtracing/coresight/Makefile | 20 ++++----
.../{coresight.c => coresight-core.c} | 23 ++++-----
.../hwtracing/coresight/coresight-cpu-debug.c | 2 +
.../coresight/coresight-dynamic-replicator.c | 19 +++++++-
drivers/hwtracing/coresight/coresight-etb10.c | 20 +++++++-
.../hwtracing/coresight/coresight-etm-perf.c | 18 ++++++-
.../hwtracing/coresight/coresight-etm-perf.h | 2 +-
...resight-etm3x.c => coresight-etm3x-core.c} | 24 +++++++++-
...resight-etm4x.c => coresight-etm4x-core.c} | 25 +++++++++-
.../hwtracing/coresight/coresight-funnel.c | 18 ++++++-
drivers/hwtracing/coresight/coresight-priv.h | 10 +++-
.../coresight/coresight-replicator.c | 22 ++++++++-
drivers/hwtracing/coresight/coresight-stm.c | 20 +++++++-
.../{coresight-tmc.c => coresight-tmc-core.c} | 22 ++++++++-
drivers/hwtracing/coresight/coresight-tpiu.c | 19 +++++++-
include/linux/coresight.h | 2 +-
17 files changed, 270 insertions(+), 44 deletions(-)
rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c} (98%)
rename drivers/hwtracing/coresight/{coresight-etm3x.c => coresight-etm3x-core.c} (97%)
rename drivers/hwtracing/coresight/{coresight-etm4x.c => coresight-etm4x-core.c} (97%)
rename drivers/hwtracing/coresight/{coresight-tmc.c => coresight-tmc-core.c} (95%)
--
2.17.0
Allow to build coresight as a module. This enhances
coresight developer efficiency by allowing the development to
take place exclusively on the target, and without needing to
reboot in between changes.
- 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 core bus
register/unregister function calls to the etm_perf init/exit
functions, since coresight.c does not have etm_pmu defined.
- add a MODULE_DEVICE_TABLE for autoloading on boot
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 5 ++++-
drivers/hwtracing/coresight/Makefile | 7 +++++--
.../coresight/{coresight.c => coresight-core.c} | 6 ------
.../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++++-
4 files changed, 25 insertions(+), 10 deletions(-)
rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c} (99%)
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 181a44ea2d61..c05b265f7731 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -2,7 +2,7 @@
# Coresight configuration
#
menuconfig CORESIGHT
- bool "CoreSight Tracing Support"
+ tristate "CoreSight Tracing Support"
select ARM_AMBA
select PERF_EVENTS
help
@@ -12,6 +12,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 45d7a0f34170..ed2d4bcb017b 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -2,8 +2,11 @@
#
# Makefile for CoreSight drivers.
#
-obj-$(CONFIG_CORESIGHT) += coresight.o coresight-etm-perf.o
-obj-$(CONFIG_OF) += of_coresight.o
+obj-$(CONFIG_CORESIGHT) += coresight.o
+coresight-objs := coresight-core.o coresight-etm-perf.o
+ifeq ($(CONFIG_OF), y)
+coresight-objs += of_coresight.o
+endif
obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
coresight-tmc-objs := 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 99%
rename from drivers/hwtracing/coresight/coresight.c
rename to drivers/hwtracing/coresight/coresight-core.c
index 1c941351f1d1..f96258de1e9b 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -948,12 +948,6 @@ struct bus_type coresight_bustype = {
.name = "coresight",
};
-static int __init coresight_init(void)
-{
- return bus_register(&coresight_bustype);
-}
-postcore_initcall(coresight_init);
-
struct coresight_device *coresight_register(struct coresight_desc *desc)
{
int i;
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 0fe7e43ea1c4..ceac9aee4a82 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -472,6 +472,10 @@ static int __init etm_perf_init(void)
{
int ret;
+ ret = bus_register(&coresight_bustype);
+ if (ret)
+ return ret;
+
etm_pmu.capabilities = PERF_PMU_CAP_EXCLUSIVE;
etm_pmu.attr_groups = etm_pmu_attr_groups;
@@ -494,4 +498,15 @@ static int __init etm_perf_init(void)
return ret;
}
-device_initcall(etm_perf_init);
+postcore_initcall(etm_perf_init);
+
+static void __exit etm_perf_exit(void)
+{
+ perf_pmu_unregister(&etm_pmu);
+ bus_unregister(&coresight_bustype);
+}
+module_exit(etm_perf_exit);
+
+MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight tracer driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
Allow to build coresight-etm4x 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-etm4x by the Makefile
- add an etm4_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 5 +++-
drivers/hwtracing/coresight/Makefile | 4 +--
...resight-etm4x.c => coresight-etm4x-core.c} | 25 ++++++++++++++++++-
3 files changed, 30 insertions(+), 4 deletions(-)
rename drivers/hwtracing/coresight/{coresight-etm4x.c => coresight-etm4x-core.c} (97%)
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 56675e4f5a15..c32673f9177a 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -64,7 +64,7 @@ config CORESIGHT_SOURCE_ETM3X
module will be called coresight-etm3x.
config CORESIGHT_SOURCE_ETM4X
- bool "CoreSight Embedded Trace Macrocell 4.x driver"
+ tristate "CoreSight Embedded Trace Macrocell 4.x driver"
depends on ARM64
select CORESIGHT_LINKS_AND_SINKS
help
@@ -73,6 +73,9 @@ config CORESIGHT_SOURCE_ETM4X
for instruction level tracing. Depending on the implemented version
data tracing may also be available.
+ To compile this driver as a module, choose M here: the
+ module will be called coresight-etm4x.
+
config CORESIGHT_DYNAMIC_REPLICATOR
tristate "CoreSight Programmable Replicator driver"
depends on CORESIGHT_LINKS_AND_SINKS
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index c3281e37e542..1623ed266b04 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -14,8 +14,8 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
coresight-etm3x-objs := coresight-etm3x-core.o coresight-etm-cp14.o \
coresight-etm3x-sysfs.o
-obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
- coresight-etm4x-sysfs.o
+obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o
+coresight-etm4x-objs := coresight-etm4x-core.o coresight-etm4x-sysfs.o
obj-$(CONFIG_CORESIGHT_DYNAMIC_REPLICATOR) += coresight-dynamic-replicator.o
obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
similarity index 97%
rename from drivers/hwtracing/coresight/coresight-etm4x.c
rename to drivers/hwtracing/coresight/coresight-etm4x-core.c
index 1d94ebec027b..0f4828c2268c 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1046,6 +1046,20 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
return ret;
}
+static int __exit etm4_remove(struct amba_device *adev)
+{
+ struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+ etm_perf_symlink(drvdata->csdev, false);
+
+ cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
+ cpuhp_remove_state_nocalls(hp_online);
+
+ coresight_unregister(drvdata->csdev);
+
+ return 0;
+}
+
#define ETM4x_AMBA_ID(pid) \
{ \
.id = pid, \
@@ -1061,12 +1075,21 @@ static const struct amba_id etm4_ids[] = {
{},
};
+MODULE_DEVICE_TABLE(amba, etm4_ids);
+
static struct amba_driver etm4x_driver = {
.drv = {
.name = "coresight-etm4x",
+ .owner = THIS_MODULE,
.suppress_bind_attrs = true,
},
.probe = etm4_probe,
+ .remove = etm4_remove,
.id_table = etm4_ids,
};
-builtin_amba_driver(etm4x_driver);
+module_amba_driver(etm4x_driver);
+
+MODULE_AUTHOR("Pratik Patel <[email protected]>");
+MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace v4 driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
Allow to build coresight-funnel and coresight-replicator as modules,
for ease of development.
- Kconfig becomes a tristate, to allow =m
- add funnel_remove and replicator_remove functions,
for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 5 ++++-
.../hwtracing/coresight/coresight-funnel.c | 18 ++++++++++++++-
.../coresight/coresight-replicator.c | 22 ++++++++++++++++++-
3 files changed, 42 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index ae05b4b89ff9..181a44ea2d61 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -14,13 +14,16 @@ menuconfig CORESIGHT
if CORESIGHT
config CORESIGHT_LINKS_AND_SINKS
- bool "CoreSight Link and Sink drivers"
+ tristate "CoreSight Link and Sink drivers"
help
This enables support for CoreSight link and sink drivers that are
responsible for transporting and collecting the trace data
respectively. Link and sinks are dynamically aggregated with a trace
entity at run time to form a complete trace path.
+ To compile these drivers as modules, choose M here: the
+ modules will be called coresight-funnel and coresight-replicator.
+
config CORESIGHT_LINK_AND_SINK_TMC
tristate "Coresight generic TMC driver"
depends on CORESIGHT_LINKS_AND_SINKS
diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
index 448145a36675..861ac641a716 100644
--- a/drivers/hwtracing/coresight/coresight-funnel.c
+++ b/drivers/hwtracing/coresight/coresight-funnel.c
@@ -211,6 +211,15 @@ static int funnel_probe(struct amba_device *adev, const struct amba_id *id)
return PTR_ERR_OR_ZERO(drvdata->csdev);
}
+static int __exit funnel_remove(struct amba_device *adev)
+{
+ struct funnel_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+ coresight_unregister(drvdata->csdev);
+
+ return 0;
+}
+
#ifdef CONFIG_PM
static int funnel_runtime_suspend(struct device *dev)
{
@@ -250,6 +259,8 @@ static const struct amba_id funnel_ids[] = {
{ 0, 0},
};
+MODULE_DEVICE_TABLE(amba, funnel_ids);
+
static struct amba_driver funnel_driver = {
.drv = {
.name = "coresight-funnel",
@@ -258,6 +269,11 @@ static struct amba_driver funnel_driver = {
.suppress_bind_attrs = true,
},
.probe = funnel_probe,
+ .remove = funnel_remove,
.id_table = funnel_ids,
};
-builtin_amba_driver(funnel_driver);
+module_amba_driver(funnel_driver);
+
+MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight Funnel Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index 8d2eaaab6c2f..1fb469d7cf50 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -112,6 +112,17 @@ static int replicator_probe(struct platform_device *pdev)
return ret;
}
+static int __exit replicator_remove(struct platform_device *pdev)
+{
+ struct replicator_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
+
+ coresight_unregister(drvdata->csdev);
+
+ pm_runtime_disable(&pdev->dev);
+
+ return 0;
+}
+
#ifdef CONFIG_PM
static int replicator_runtime_suspend(struct device *dev)
{
@@ -144,13 +155,22 @@ static const struct of_device_id replicator_match[] = {
{}
};
+MODULE_DEVICE_TABLE(of, replicator_match);
+
static struct platform_driver replicator_driver = {
.probe = replicator_probe,
+ .remove = replicator_remove,
.driver = {
.name = "coresight-replicator",
.of_match_table = replicator_match,
+ .owner = THIS_MODULE,
.pm = &replicator_dev_pm_ops,
.suppress_bind_attrs = true,
},
};
-builtin_platform_driver(replicator_driver);
+module_platform_driver(replicator_driver);
+
+MODULE_AUTHOR("Pratik Patel <[email protected]>");
+MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight Replicator Driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
Allow to build coresight-tpiu as a module, for ease of development.
- Kconfig becomes a tristate, to allow =m
- add a tpiu_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 5 ++++-
drivers/hwtracing/coresight/coresight-tpiu.c | 19 ++++++++++++++++++-
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 3c3256425a7f..2f44adb82361 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -32,7 +32,7 @@ config CORESIGHT_LINK_AND_SINK_TMC
special enhancement or added features.
config CORESIGHT_SINK_TPIU
- bool "Coresight generic TPIU driver"
+ tristate "Coresight generic TPIU driver"
depends on CORESIGHT_LINKS_AND_SINKS
help
This enables support for the Trace Port Interface Unit driver,
@@ -42,6 +42,9 @@ config CORESIGHT_SINK_TPIU
connected to an external host for use case capturing more traces than
the on-board coresight memory can handle.
+ To compile this driver as a module, choose M here: the
+ module will be called coresight-tpiu.
+
config CORESIGHT_SINK_ETBV10
tristate "Coresight ETBv1.0 driver"
depends on CORESIGHT_LINKS_AND_SINKS
diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
index 01b7457fe8fc..ab53ce1a9ce3 100644
--- a/drivers/hwtracing/coresight/coresight-tpiu.c
+++ b/drivers/hwtracing/coresight/coresight-tpiu.c
@@ -164,6 +164,15 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)
return PTR_ERR_OR_ZERO(drvdata->csdev);
}
+static int __exit tpiu_remove(struct amba_device *adev)
+{
+ struct tpiu_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+ coresight_unregister(drvdata->csdev);
+
+ return 0;
+}
+
#ifdef CONFIG_PM
static int tpiu_runtime_suspend(struct device *dev)
{
@@ -207,6 +216,8 @@ static const struct amba_id tpiu_ids[] = {
{ 0, 0},
};
+MODULE_DEVICE_TABLE(amba, tpiu_ids);
+
static struct amba_driver tpiu_driver = {
.drv = {
.name = "coresight-tpiu",
@@ -215,6 +226,12 @@ static struct amba_driver tpiu_driver = {
.suppress_bind_attrs = true,
},
.probe = tpiu_probe,
+ .remove = tpiu_remove,
.id_table = tpiu_ids,
};
-builtin_amba_driver(tpiu_driver);
+module_amba_driver(tpiu_driver);
+
+MODULE_AUTHOR("Pratik Patel <[email protected]>");
+MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight TPIU (Trace Port Interface Unit) driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
Allow to build coresight-etb10 as a module, for ease of development.
- Kconfig becomes a tristate, to allow =m
- add an etb_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 5 ++++-
drivers/hwtracing/coresight/coresight-etb10.c | 20 ++++++++++++++++++-
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index c32673f9177a..3c3256425a7f 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -43,13 +43,16 @@ config CORESIGHT_SINK_TPIU
the on-board coresight memory can handle.
config CORESIGHT_SINK_ETBV10
- bool "Coresight ETBv1.0 driver"
+ tristate "Coresight ETBv1.0 driver"
depends on CORESIGHT_LINKS_AND_SINKS
help
This enables support for the Embedded Trace Buffer version 1.0 driver
that complies with the generic implementation of the component without
special enhancement or added features.
+ To compile this driver as a module, choose M here: the
+ module will be called coresight-etb10.
+
config CORESIGHT_SOURCE_ETM3X
tristate "CoreSight Embedded Trace Macrocell 3.x driver"
depends on !ARM64
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 78e71bf728fc..5b9d264e35be 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -708,6 +708,16 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)
return ret;
}
+static int __exit etb_remove(struct amba_device *adev)
+{
+ struct etb_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+ misc_deregister(&drvdata->miscdev);
+ coresight_unregister(drvdata->csdev);
+
+ return 0;
+}
+
#ifdef CONFIG_PM
static int etb_runtime_suspend(struct device *dev)
{
@@ -742,6 +752,8 @@ static const struct amba_id etb_ids[] = {
{ 0, 0},
};
+MODULE_DEVICE_TABLE(amba, etb_ids);
+
static struct amba_driver etb_driver = {
.drv = {
.name = "coresight-etb10",
@@ -751,6 +763,12 @@ static struct amba_driver etb_driver = {
},
.probe = etb_probe,
+ .remove = etb_remove,
.id_table = etb_ids,
};
-builtin_amba_driver(etb_driver);
+module_amba_driver(etb_driver);
+
+MODULE_AUTHOR("Pratik Patel <[email protected]>");
+MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight Embedded Trace Buffer driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
Checking for ifdef CONFIG_x fails if CONFIG_x=m. Use IS_ENABLED
that is true for both built-ins and modules, instead. Required
when building coresight components as modules.
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/coresight-etm-perf.h | 2 +-
drivers/hwtracing/coresight/coresight-priv.h | 2 +-
include/linux/coresight.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index 4197df4faf5e..539b250df455 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -43,7 +43,7 @@ struct etm_filters {
};
-#ifdef CONFIG_CORESIGHT
+#if IS_ENABLED(CONFIG_CORESIGHT)
int etm_perf_symlink(struct coresight_device *csdev, bool link);
#else
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 1a6cf3589866..158c720119dd 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -144,7 +144,7 @@ struct list_head *coresight_build_path(struct coresight_device *csdev,
struct coresight_device *sink);
void coresight_release_path(struct list_head *path);
-#ifdef CONFIG_CORESIGHT_SOURCE_ETM3X
+#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X)
extern int etm_readl_cp14(u32 off, unsigned int *val);
extern int etm_writel_cp14(u32 off, u32 val);
#else
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index e5421b83e4e6..548fa56b29bd 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -236,7 +236,7 @@ struct coresight_ops {
const struct coresight_ops_source *source_ops;
};
-#ifdef CONFIG_CORESIGHT
+#if IS_ENABLED(CONFIG_CORESIGHT)
extern struct coresight_device *
coresight_register(struct coresight_desc *desc);
extern void coresight_unregister(struct coresight_device *csdev);
--
2.17.0
barrier_pkt[] is used in the etb and tmc-etf coresight
components. Change barrier_pkt[] to a static definition,
so as to allow them to be built as modules.
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/coresight-priv.h | 8 +++++++-
drivers/hwtracing/coresight/coresight.c | 7 -------
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 158c720119dd..e76f19ca9e04 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -57,7 +57,13 @@ static DEVICE_ATTR_RO(name)
#define coresight_simple_reg64(type, name, lo_off, hi_off) \
__coresight_simple_func(type, NULL, name, lo_off, hi_off)
-extern const u32 barrier_pkt[4];
+/*
+ * When losing synchronisation a new barrier packet needs to be inserted at the
+ * beginning of the data collected in a buffer. That way the decoder knows that
+ * it needs to look for another sync sequence.
+ */
+static const u32 barrier_pkt[4] = { 0x7fffffff, 0x7fffffff,
+ 0x7fffffff, 0x7fffffff };
#define CORESIGHT_BARRIER_PKT_SIZE (sizeof(barrier_pkt))
enum etm_addr_type {
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 4969b329511c..0cbc2948defc 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -46,13 +46,6 @@ static DEFINE_PER_CPU(struct list_head *, tracer_path);
*/
static struct list_head *stm_path;
-/*
- * When losing synchronisation a new barrier packet needs to be inserted at the
- * beginning of the data collected in a buffer. That way the decoder knows that
- * it needs to look for another sync sequence.
- */
-const u32 barrier_pkt[4] = {0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff};
-
static int coresight_id_match(struct device *dev, void *data)
{
int trace_id, i_trace_id;
--
2.17.0
Allow to build coresight-tmc 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-tmc by the Makefile
- add an tmc_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 5 ++++-
drivers/hwtracing/coresight/Makefile | 6 ++---
.../{coresight-tmc.c => coresight-tmc-core.c} | 22 ++++++++++++++++++-
3 files changed, 28 insertions(+), 5 deletions(-)
rename drivers/hwtracing/coresight/{coresight-tmc.c => coresight-tmc-core.c} (95%)
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 2f44adb82361..ae05b4b89ff9 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -22,7 +22,7 @@ config CORESIGHT_LINKS_AND_SINKS
entity at run time to form a complete trace path.
config CORESIGHT_LINK_AND_SINK_TMC
- bool "Coresight generic TMC driver"
+ tristate "Coresight generic TMC driver"
depends on CORESIGHT_LINKS_AND_SINKS
help
This enables support for the Trace Memory Controller driver.
@@ -31,6 +31,9 @@ config CORESIGHT_LINK_AND_SINK_TMC
complies with the generic implementation of the component without
special enhancement or added features.
+ To compile this driver as a module, choose M here: the
+ module will be called coresight-tmc.
+
config CORESIGHT_SINK_TPIU
tristate "Coresight generic TPIU driver"
depends on CORESIGHT_LINKS_AND_SINKS
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 1623ed266b04..45d7a0f34170 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -4,9 +4,9 @@
#
obj-$(CONFIG_CORESIGHT) += coresight.o coresight-etm-perf.o
obj-$(CONFIG_OF) += of_coresight.o
-obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o \
- coresight-tmc-etf.o \
- coresight-tmc-etr.o
+obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
+coresight-tmc-objs := coresight-tmc-core.o coresight-tmc-etf.o \
+ coresight-tmc-etr.o
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 \
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
similarity index 95%
rename from drivers/hwtracing/coresight/coresight-tmc.c
rename to drivers/hwtracing/coresight/coresight-tmc-core.c
index 1b817ec1192c..ec4867b81be8 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -474,6 +474,19 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
return ret;
}
+static int __exit tmc_remove(struct amba_device *adev)
+{
+ struct tmc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+ /* free ETB/ETF or ETR memory */
+ tmc_read_unprepare(drvdata);
+
+ misc_deregister(&drvdata->miscdev);
+ coresight_unregister(drvdata->csdev);
+
+ return 0;
+}
+
static const struct amba_id tmc_ids[] = {
{
.id = 0x000bb961,
@@ -498,6 +511,8 @@ static const struct amba_id tmc_ids[] = {
{ 0, 0},
};
+MODULE_DEVICE_TABLE(amba, tmc_ids);
+
static struct amba_driver tmc_driver = {
.drv = {
.name = "coresight-tmc",
@@ -505,6 +520,11 @@ static struct amba_driver tmc_driver = {
.suppress_bind_attrs = true,
},
.probe = tmc_probe,
+ .remove = tmc_remove,
.id_table = tmc_ids,
};
-builtin_amba_driver(tmc_driver);
+module_amba_driver(tmc_driver);
+
+MODULE_AUTHOR("Pratik Patel <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight Trace Memory Controller driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
Allow to build coresight-stm as a module, for ease of development.
- Kconfig becomes a tristate, to allow =m
- add a stm_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 5 ++++-
drivers/hwtracing/coresight/coresight-stm.c | 20 +++++++++++++++++++-
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index d1209f5acf76..8c5e5407b7dd 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -79,8 +79,8 @@ config CORESIGHT_DYNAMIC_REPLICATOR
trace data based on the traceid.
config CORESIGHT_STM
- bool "CoreSight System Trace Macrocell driver"
depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
+ tristate "CoreSight System Trace Macrocell driver"
select CORESIGHT_LINKS_AND_SINKS
select STM
help
@@ -89,6 +89,9 @@ config CORESIGHT_STM
logging useful software events or data coming from various entities
in the system, possibly running different OSs
+ To compile this driver as a module, choose M here: the
+ module will be called coresight-stm.
+
config CORESIGHT_CPU_DEBUG
tristate "CoreSight CPU Debug driver"
depends on ARM || ARM64
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index c46c70aec1d5..c465e6bc66c8 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -882,6 +882,17 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id)
return ret;
}
+static int __exit stm_remove(struct amba_device *adev)
+{
+ struct stm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+ coresight_unregister(drvdata->csdev);
+
+ stm_unregister_device(&drvdata->stm);
+
+ return 0;
+}
+
#ifdef CONFIG_PM
static int stm_runtime_suspend(struct device *dev)
{
@@ -922,6 +933,8 @@ static const struct amba_id stm_ids[] = {
{ 0, 0},
};
+MODULE_DEVICE_TABLE(amba, stm_ids);
+
static struct amba_driver stm_driver = {
.drv = {
.name = "coresight-stm",
@@ -930,7 +943,12 @@ static struct amba_driver stm_driver = {
.suppress_bind_attrs = true,
},
.probe = stm_probe,
+ .remove = stm_remove,
.id_table = stm_ids,
};
-builtin_amba_driver(stm_driver);
+module_amba_driver(stm_driver);
+
+MODULE_AUTHOR("Pratik Patel <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight System Trace Macrocell driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
Allow to build coresight-dynamic-replicator as a module,
for ease of development.
- Kconfig becomes a tristate, to allow =m
- add a replicator_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 5 ++++-
.../coresight/coresight-dynamic-replicator.c | 19 ++++++++++++++++++-
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 8c5e5407b7dd..945b17ca2eed 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -71,13 +71,16 @@ config CORESIGHT_SOURCE_ETM4X
data tracing may also be available.
config CORESIGHT_DYNAMIC_REPLICATOR
- bool "CoreSight Programmable Replicator driver"
+ tristate "CoreSight Programmable Replicator driver"
depends on CORESIGHT_LINKS_AND_SINKS
help
This enables support for dynamic CoreSight replicator link driver.
The programmable ATB replicator allows independent filtering of the
trace data based on the traceid.
+ To compile this driver as a module, choose M here: the
+ module will be called coresight-dynamic-replicator.
+
config CORESIGHT_STM
depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
tristate "CoreSight System Trace Macrocell driver"
diff --git a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
index f6d0571ab9dd..b59055097436 100644
--- a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
@@ -159,6 +159,15 @@ static int replicator_probe(struct amba_device *adev, const struct amba_id *id)
return PTR_ERR_OR_ZERO(drvdata->csdev);
}
+static int __exit replicator_remove(struct amba_device *adev)
+{
+ struct replicator_state *drvdata = dev_get_drvdata(&adev->dev);
+
+ coresight_unregister(drvdata->csdev);
+
+ return 0;
+}
+
#ifdef CONFIG_PM
static int replicator_runtime_suspend(struct device *dev)
{
@@ -200,13 +209,21 @@ static const struct amba_id replicator_ids[] = {
{ 0, 0 },
};
+MODULE_DEVICE_TABLE(amba, replicator_ids);
+
static struct amba_driver replicator_driver = {
.drv = {
.name = "coresight-dynamic-replicator",
+ .owner = THIS_MODULE,
.pm = &replicator_dev_pm_ops,
.suppress_bind_attrs = true,
},
.probe = replicator_probe,
+ .remove = replicator_remove,
.id_table = replicator_ids,
};
-builtin_amba_driver(replicator_driver);
+module_amba_driver(replicator_driver);
+
+MODULE_AUTHOR("Pratik Patel <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight Dynamic Replicator Driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
Increment the refcnt for driver modules in current use by calling
module_get in coresight_build_path and module_put in release_path.
This prevents driver modules from being unloaded when they are in use,
either in sysfs or perf mode.
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/coresight.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 338f1719641c..1c941351f1d1 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -465,6 +465,12 @@ static int _coresight_build_path(struct coresight_device *csdev,
node->csdev = csdev;
list_add(&node->link, path);
+
+ if (!try_module_get(csdev->dev.parent->driver->owner)) {
+ dev_err(&csdev->dev, "could not get coresight driver module\n");
+ return -ENODEV;
+ }
+
pm_runtime_get_sync(csdev->dev.parent);
return 0;
@@ -510,6 +516,9 @@ void coresight_release_path(struct list_head *path)
csdev = nd->csdev;
pm_runtime_put_sync(csdev->dev.parent);
+
+ module_put(csdev->dev.parent->driver->owner);
+
list_del(&nd->link);
kfree(nd);
}
--
2.17.0
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 <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 5 +++-
drivers/hwtracing/coresight/Makefile | 3 ++-
...resight-etm3x.c => coresight-etm3x-core.c} | 24 ++++++++++++++++++-
3 files changed, 29 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 945b17ca2eed..56675e4f5a15 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -51,7 +51,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
@@ -60,6 +60,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 61db9dd0d571..c3281e37e542 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-objs := 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 7c74263c333d..d14c867e9e46 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -864,6 +864,20 @@ 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);
+
+ cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
+ cpuhp_remove_state_nocalls(hp_online);
+
+ coresight_unregister(drvdata->csdev);
+
+ return 0;
+}
+
#ifdef CONFIG_PM
static int etm_runtime_suspend(struct device *dev)
{
@@ -924,6 +938,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",
@@ -932,6 +948,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 <[email protected]>");
+MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
- provide the name of the module in the Kconfig help section
- define a MODULE_DEVICE_TABLE in order to be autoloaded on boot
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/Kconfig | 3 +++
drivers/hwtracing/coresight/coresight-cpu-debug.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index ef9cb3c164e1..d1209f5acf76 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -103,4 +103,7 @@ config CORESIGHT_CPU_DEBUG
properly, please refer Documentation/trace/coresight-cpu-debug.txt
for detailed description and the example for usage.
+ To compile this driver as a module, choose M here: the
+ module will be called coresight-cpu-debug.
+
endif
diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
index 45b2460f3166..1efe9626eb6c 100644
--- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -671,6 +671,8 @@ static const struct amba_id debug_ids[] = {
{ 0, 0 },
};
+MODULE_DEVICE_TABLE(amba, debug_ids);
+
static struct amba_driver debug_driver = {
.drv = {
.name = "coresight-cpu-debug",
--
2.17.0
Export these symbols for use by other coresight components,
so they can be built as modules.
coresight_timeout is used by the coresight stm, etm4x, tmc,
tpiu, and etb10 modules, and etm_perf_symlink is needed by
the etm3x/4x modules.
Cc: Mathieu Poirier <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/hwtracing/coresight/coresight-etm-perf.c | 1 +
drivers/hwtracing/coresight/coresight.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 677695635211..0fe7e43ea1c4 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -466,6 +466,7 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
return 0;
}
+EXPORT_SYMBOL_GPL(etm_perf_symlink);
static int __init etm_perf_init(void)
{
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 0cbc2948defc..338f1719641c 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -933,6 +933,7 @@ int coresight_timeout(void __iomem *addr, u32 offset, int position, int value)
return -EAGAIN;
}
+EXPORT_SYMBOL_GPL(coresight_timeout);
struct bus_type coresight_bustype = {
.name = "coresight",
--
2.17.0
On 06/05/2018 10:07 PM, Kim Phillips wrote:
> Increment the refcnt for driver modules in current use by calling
> module_get in coresight_build_path and module_put in release_path.
>
> This prevents driver modules from being unloaded when they are in use,
> either in sysfs or perf mode.
>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 338f1719641c..1c941351f1d1 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -465,6 +465,12 @@ static int _coresight_build_path(struct coresight_device *csdev,
>
> node->csdev = csdev;
> list_add(&node->link, path);
> +
> + if (!try_module_get(csdev->dev.parent->driver->owner)) {
> + dev_err(&csdev->dev, "could not get coresight driver module\n");
> + return -ENODEV;
> + }
> +
Kim,
It would be safer to do the check before we actually add the device to
the path above. That way, we don't add an unavailable device to the
path, which will be "released" in the release_path below.
Otherwise looks good to me.
> pm_runtime_get_sync(csdev->dev.parent);
>
> return 0;
> @@ -510,6 +516,9 @@ void coresight_release_path(struct list_head *path)
> csdev = nd->csdev;
>
> pm_runtime_put_sync(csdev->dev.parent);
> +
> + module_put(csdev->dev.parent->driver->owner);
> +
> list_del(&nd->link);
> kfree(nd);
> }
>
Suzuki
On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
> Increment the refcnt for driver modules in current use by calling
> module_get in coresight_build_path and module_put in release_path.
>
> This prevents driver modules from being unloaded when they are in use,
> either in sysfs or perf mode.
Why does it matter? Shouldn't you be allowed to remove any module at
any point in time, much like a networking driver?
>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 338f1719641c..1c941351f1d1 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -465,6 +465,12 @@ static int _coresight_build_path(struct coresight_device *csdev,
>
> node->csdev = csdev;
> list_add(&node->link, path);
> +
> + if (!try_module_get(csdev->dev.parent->driver->owner)) {
What is to keep parent->driver from going away right here? What keeps
parent around? This feels very fragile to me, I don't see any locking
anywhere around this code path to try to keep things in place.
thanks,
greg k-h
On Tue, Jun 05, 2018 at 04:06:57PM -0500, Kim Phillips wrote:
> - provide the name of the module in the Kconfig help section
> - define a MODULE_DEVICE_TABLE in order to be autoloaded on boot
>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/hwtracing/coresight/Kconfig | 3 +++
> drivers/hwtracing/coresight/coresight-cpu-debug.c | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index ef9cb3c164e1..d1209f5acf76 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -103,4 +103,7 @@ config CORESIGHT_CPU_DEBUG
> properly, please refer Documentation/trace/coresight-cpu-debug.txt
> for detailed description and the example for usage.
>
> + To compile this driver as a module, choose M here: the
> + module will be called coresight-cpu-debug.
> +
> endif
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> index 45b2460f3166..1efe9626eb6c 100644
> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -671,6 +671,8 @@ static const struct amba_id debug_ids[] = {
> { 0, 0 },
> };
>
> +MODULE_DEVICE_TABLE(amba, debug_ids);
> +
> static struct amba_driver debug_driver = {
> .drv = {
> .name = "coresight-cpu-debug",
This should be 2 patches as you are doing two different things here.
thanks,
greg k-h
On Tue, Jun 05, 2018 at 04:06:59PM -0500, Kim Phillips wrote:
> barrier_pkt[] is used in the etb and tmc-etf coresight
> components. Change barrier_pkt[] to a static definition,
> so as to allow them to be built as modules.
>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight-priv.h | 8 +++++++-
> drivers/hwtracing/coresight/coresight.c | 7 -------
> 2 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 158c720119dd..e76f19ca9e04 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -57,7 +57,13 @@ static DEVICE_ATTR_RO(name)
> #define coresight_simple_reg64(type, name, lo_off, hi_off) \
> __coresight_simple_func(type, NULL, name, lo_off, hi_off)
>
> -extern const u32 barrier_pkt[4];
> +/*
> + * When losing synchronisation a new barrier packet needs to be inserted at the
> + * beginning of the data collected in a buffer. That way the decoder knows that
> + * it needs to look for another sync sequence.
> + */
> +static const u32 barrier_pkt[4] = { 0x7fffffff, 0x7fffffff,
> + 0x7fffffff, 0x7fffffff };
Are you _sure_ this is doing what you think it is doing?
You now just created a bunch of different copies of this structure,
which might change the logic involved, right?
Putting a static variable in a .h file is generally considered a very
bad thing to do, I need a lot more "proof" this is ok before I can
accept this. Worse case, just put the variable in the individual places
where it is needed.
thanks,
greg k-h
On Tue, Jun 05, 2018 at 04:07:10PM -0500, Kim Phillips wrote:
> Allow to build coresight as a module. This enhances
> coresight developer efficiency by allowing the development to
> take place exclusively on the target, and without needing to
> reboot in between changes.
>
> - 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 core bus
> register/unregister function calls to the etm_perf init/exit
> functions, since coresight.c does not have etm_pmu defined.
> - add a MODULE_DEVICE_TABLE for autoloading on boot
>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/hwtracing/coresight/Kconfig | 5 ++++-
> drivers/hwtracing/coresight/Makefile | 7 +++++--
> .../coresight/{coresight.c => coresight-core.c} | 6 ------
> .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++++-
> 4 files changed, 25 insertions(+), 10 deletions(-)
> rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c} (99%)
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 181a44ea2d61..c05b265f7731 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -2,7 +2,7 @@
> # Coresight configuration
> #
> menuconfig CORESIGHT
> - bool "CoreSight Tracing Support"
> + tristate "CoreSight Tracing Support"
> select ARM_AMBA
> select PERF_EVENTS
> help
> @@ -12,6 +12,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 45d7a0f34170..ed2d4bcb017b 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -2,8 +2,11 @@
> #
> # Makefile for CoreSight drivers.
> #
> -obj-$(CONFIG_CORESIGHT) += coresight.o coresight-etm-perf.o
> -obj-$(CONFIG_OF) += of_coresight.o
> +obj-$(CONFIG_CORESIGHT) += coresight.o
> +coresight-objs := coresight-core.o coresight-etm-perf.o
Shouldn't this line be:
coresight-y := coresight-core.o coresight-etm-perf.o
> +ifeq ($(CONFIG_OF), y)
> +coresight-objs += of_coresight.o
> +endif
Those 3 lines should be written as 1 line:
coresight-$(CONFIG_OF) += of_coresight.o
thanks,
greg k-h
On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
>> Increment the refcnt for driver modules in current use by calling
>> module_get in coresight_build_path and module_put in release_path.
>>
>> This prevents driver modules from being unloaded when they are in use,
>> either in sysfs or perf mode.
>
> Why does it matter? Shouldn't you be allowed to remove any module at
> any point in time, much like a networking driver?
>
>
>>
>> Cc: Mathieu Poirier <[email protected]>
>> Cc: Leo Yan <[email protected]>
>> Cc: Alexander Shishkin <[email protected]>
>> Cc: Randy Dunlap <[email protected]>
>> Cc: Suzuki K Poulose <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Russell King <[email protected]>
>> Signed-off-by: Kim Phillips <[email protected]>
>> ---
>> drivers/hwtracing/coresight/coresight.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
>> index 338f1719641c..1c941351f1d1 100644
>> --- a/drivers/hwtracing/coresight/coresight.c
>> +++ b/drivers/hwtracing/coresight/coresight.c
>> @@ -465,6 +465,12 @@ static int _coresight_build_path(struct coresight_device *csdev,
>>
>> node->csdev = csdev;
>> list_add(&node->link, path);
>> +
>> + if (!try_module_get(csdev->dev.parent->driver->owner)) {
>
> What is to keep parent->driver from going away right here? What keeps
> parent around? This feels very fragile to me, I don't see any locking
> anywhere around this code path to try to keep things in place.
You're right. We do have coresight_mutex, which is held across the build
path and the csdev is removed when a device is unregistered. However, I
see that we don't hold the mutex while removing the connections from
coresight_unregister(). Holding the mutex should protect us from the
csdev being removed, while we build the path.
And while we are at this, I also realised that we hold references to the
parent devices for each connection (via bus_find_device() from
of_coresight_get_endpoint_device()), while parsing the platform data,
which is never released.
Thanks
Suzuki
>
> thanks,
>
> greg k-h
>
On 06/05/2018 10:07 PM, Kim Phillips wrote:
> Allow to build coresight as a module. This enhances
> coresight developer efficiency by allowing the development to
> take place exclusively on the target, and without needing to
> reboot in between changes.
>
> - 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 core bus
> register/unregister function calls to the etm_perf init/exit
> functions, since coresight.c does not have etm_pmu defined.
It feels a bit weird to move the coresight core specific steps to
etm_perf_init. Why not call etm_perf_init from coresight init routine
and keep everything core specific in the coresight-core.c ?
Suzuki
On Wed, 6 Jun 2018 10:46:36 +0100
Suzuki K Poulose <[email protected]> wrote:
> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
> > On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
> >> Increment the refcnt for driver modules in current use by calling
> >> module_get in coresight_build_path and module_put in release_path.
> >>
> >> This prevents driver modules from being unloaded when they are in use,
> >> either in sysfs or perf mode.
> >
> > Why does it matter? Shouldn't you be allowed to remove any module at
> > any point in time, much like a networking driver?
> >
> >
> >>
> >> Cc: Mathieu Poirier <[email protected]>
> >> Cc: Leo Yan <[email protected]>
> >> Cc: Alexander Shishkin <[email protected]>
> >> Cc: Randy Dunlap <[email protected]>
> >> Cc: Suzuki K Poulose <[email protected]>
> >> Cc: Greg Kroah-Hartman <[email protected]>
> >> Cc: Russell King <[email protected]>
> >> Signed-off-by: Kim Phillips <[email protected]>
> >> ---
> >> drivers/hwtracing/coresight/coresight.c | 9 +++++++++
> >> 1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> >> index 338f1719641c..1c941351f1d1 100644
> >> --- a/drivers/hwtracing/coresight/coresight.c
> >> +++ b/drivers/hwtracing/coresight/coresight.c
> >> @@ -465,6 +465,12 @@ static int _coresight_build_path(struct coresight_device *csdev,
> >>
> >> node->csdev = csdev;
> >> list_add(&node->link, path);
> >> +
> >> + if (!try_module_get(csdev->dev.parent->driver->owner)) {
> >
> > What is to keep parent->driver from going away right here? What keeps
> > parent around? This feels very fragile to me, I don't see any locking
> > anywhere around this code path to try to keep things in place.
>
> You're right. We do have coresight_mutex, which is held across the build
> path and the csdev is removed when a device is unregistered. However, I
> see that we don't hold the mutex while removing the connections from
> coresight_unregister(). Holding the mutex should protect us from the
> csdev being removed, while we build the path.
OK, I'll add this for the next version:
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index f96258de1e9b..da702507a55c 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1040,8 +1040,12 @@ EXPORT_SYMBOL_GPL(coresight_register);
void coresight_unregister(struct coresight_device *csdev)
{
+ mutex_lock(&coresight_mutex);
+
/* Remove references of that device in the topology */
coresight_remove_conns(csdev);
device_unregister(&csdev->dev);
+
+ mutex_unlock(&coresight_mutex);
}
EXPORT_SYMBOL_GPL(coresight_unregister);
> And while we are at this, I also realised that we hold references to the
> parent devices for each connection (via bus_find_device() from
> of_coresight_get_endpoint_device()), while parsing the platform data,
> which is never released.
Would this fix that?:
diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
index a33a92ebe74b..a43ab078c85e 100644
--- a/drivers/hwtracing/coresight/of_coresight.c
+++ b/drivers/hwtracing/coresight/of_coresight.c
@@ -181,6 +181,8 @@ of_get_coresight_platform_data(struct device *dev,
pdata->child_names[i] = dev_name(rdev);
pdata->child_ports[i] = rendpoint.id;
+ put_device(rdev);
+
i++;
} while (ep);
}
Thanks,
Kim
On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
> On Wed, 6 Jun 2018 10:46:36 +0100
> Suzuki K Poulose <[email protected]> wrote:
>
> > On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
> > >> Increment the refcnt for driver modules in current use by calling
> > >> module_get in coresight_build_path and module_put in release_path.
> > >>
> > >> This prevents driver modules from being unloaded when they are in use,
> > >> either in sysfs or perf mode.
> > >
> > > Why does it matter? Shouldn't you be allowed to remove any module at
> > > any point in time, much like a networking driver?
> > >
> > >
> > >>
> > >> Cc: Mathieu Poirier <[email protected]>
> > >> Cc: Leo Yan <[email protected]>
> > >> Cc: Alexander Shishkin <[email protected]>
> > >> Cc: Randy Dunlap <[email protected]>
> > >> Cc: Suzuki K Poulose <[email protected]>
> > >> Cc: Greg Kroah-Hartman <[email protected]>
> > >> Cc: Russell King <[email protected]>
> > >> Signed-off-by: Kim Phillips <[email protected]>
> > >> ---
> > >> drivers/hwtracing/coresight/coresight.c | 9 +++++++++
> > >> 1 file changed, 9 insertions(+)
> > >>
> > >> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> > >> index 338f1719641c..1c941351f1d1 100644
> > >> --- a/drivers/hwtracing/coresight/coresight.c
> > >> +++ b/drivers/hwtracing/coresight/coresight.c
> > >> @@ -465,6 +465,12 @@ static int _coresight_build_path(struct coresight_device *csdev,
> > >>
> > >> node->csdev = csdev;
> > >> list_add(&node->link, path);
> > >> +
> > >> + if (!try_module_get(csdev->dev.parent->driver->owner)) {
> > >
> > > What is to keep parent->driver from going away right here? What keeps
> > > parent around? This feels very fragile to me, I don't see any locking
> > > anywhere around this code path to try to keep things in place.
> >
> > You're right. We do have coresight_mutex, which is held across the build
> > path and the csdev is removed when a device is unregistered. However, I
> > see that we don't hold the mutex while removing the connections from
> > coresight_unregister(). Holding the mutex should protect us from the
> > csdev being removed, while we build the path.
>
> OK, I'll add this for the next version:
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index f96258de1e9b..da702507a55c 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1040,8 +1040,12 @@ EXPORT_SYMBOL_GPL(coresight_register);
>
> void coresight_unregister(struct coresight_device *csdev)
> {
> + mutex_lock(&coresight_mutex);
> +
Locks are to protect data, not code, be careful here please.
That's the big issue with the module reference counting, it "protects"
code, not data. If at all possible, never grab a module reference
count, as you should always be able to unload a module, unless you have
a file handle open, and if you have that, the kernel core will properly
protect you.
thanks,
greg k-h
Hi Greg,
On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote:
> On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
>> On Wed, 6 Jun 2018 10:46:36 +0100
>> Suzuki K Poulose <[email protected]> wrote:
>>
>>> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
>>>> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
>>>>> Increment the refcnt for driver modules in current use by calling
>>>>> module_get in coresight_build_path and module_put in release_path.
>>>>>
>>>>> This prevents driver modules from being unloaded when they are in use,
>>>>> either in sysfs or perf mode.
>>>>
>>>> Why does it matter? Shouldn't you be allowed to remove any module at
>>>> any point in time, much like a networking driver?
The user doesn't have an explicit refcount on the individual components
in a trace session. So, when a trace session is in progress, it is as
good as having a "file" open on each component that is part of the
active trace session. So, we don't want the driver to be removed when
the component is being used in the trace collection. This will be
released as soon as the session is ended. It is just like a PMU driver
where the module refcount is held to ensure the module stays until the
session is over. In this case, we have multiple components, each with
its own driver invisible to the PMU driver. Hence the coresight driver
must hold the reference.
>>>>
>>>>
>>>>>
>>>>> Cc: Mathieu Poirier <[email protected]>
>>>>> Cc: Leo Yan <[email protected]>
>>>>> Cc: Alexander Shishkin <[email protected]>
>>>>> Cc: Randy Dunlap <[email protected]>
>>>>> Cc: Suzuki K Poulose <[email protected]>
>>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>>> Cc: Russell King <[email protected]>
>>>>> Signed-off-by: Kim Phillips <[email protected]>
>>>>> ---
>>>>> drivers/hwtracing/coresight/coresight.c | 9 +++++++++
>>>>> 1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
>>>>> index 338f1719641c..1c941351f1d1 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight.c
>>>>> @@ -465,6 +465,12 @@ static int _coresight_build_path(struct coresight_device *csdev,
>>>>>
>>>>> node->csdev = csdev;
>>>>> list_add(&node->link, path);
>>>>> +
>>>>> + if (!try_module_get(csdev->dev.parent->driver->owner)) {
>>>>
>>>> What is to keep parent->driver from going away right here? What keeps
>>>> parent around? This feels very fragile to me, I don't see any locking
>>>> anywhere around this code path to try to keep things in place.
>>>
>>> You're right. We do have coresight_mutex, which is held across the build
>>> path and the csdev is removed when a device is unregistered. However, I
>>> see that we don't hold the mutex while removing the connections from
>>> coresight_unregister(). Holding the mutex should protect us from the
>>> csdev being removed, while we build the path.
>>
>> OK, I'll add this for the next version:
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
>> index f96258de1e9b..da702507a55c 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -1040,8 +1040,12 @@ EXPORT_SYMBOL_GPL(coresight_register);
>>
>> void coresight_unregister(struct coresight_device *csdev)
>> {
>> + mutex_lock(&coresight_mutex);
>> +
>
> Locks are to protect data, not code, be careful here please.
The mutex here is to protect updates to the device links. We
keep a list of connections from each device to form a trace path.
When we unregister a device, we must remove the references to the
device from all the other connected components to ensure they don't
end up accessing a device which is gone.
>
> That's the big issue with the module reference counting, it "protects"
> code, not data. If at all possible, never grab a module reference
> count, as you should always be able to unload a module, unless you have
> a file handle open, and if you have that, the kernel core will properly
> protect you.
So in a nutshell, we have user invisible components which cannot be
refcounted explicitly by the file handles, and thus the driver must
do it.
Now, one option we could explore is getting the refcount on the
devices itself, rather than the drivers for trace sessions. And each
device could potentially hold a refcount on the driver (which I assume
is already held), which can be dropped when the device is no longer
used and thus get rid of the reference on the module everywhere.
Thoughts ? Suggestions ?
Suzuki
>
> thanks,
>
> greg k-h
>
On 06/07/2018 10:13 AM, Greg Kroah-Hartman wrote:
> On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote:
>> Hi Greg,
>>
>> On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote:
>>> On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
>>>> On Wed, 6 Jun 2018 10:46:36 +0100
>>>> Suzuki K Poulose <[email protected]> wrote:
>>>>
>>>>> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
>>>>>> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
>>>>>>> Increment the refcnt for driver modules in current use by calling
>>>>>>> module_get in coresight_build_path and module_put in release_path.
>>>>>>>
>>>>>>> This prevents driver modules from being unloaded when they are in use,
>>>>>>> either in sysfs or perf mode.
>>>>>>
>>>>>> Why does it matter? Shouldn't you be allowed to remove any module at
>>>>>> any point in time, much like a networking driver?
>>
>> The user doesn't have an explicit refcount on the individual components
>> in a trace session. So, when a trace session is in progress, it is as
>> good as having a "file" open on each component that is part of the
>> active trace session. So, we don't want the driver to be removed when
>> the component is being used in the trace collection.
>
> Why not? What's wrong with that happening and then the trace collection
> starts failing with -ENODEV or something?
May be I am missing something here. Can we allow the driver to be
removed when one of its device is "turned ON" and we need the same
driver to "turn it OFF" when the session ends ? To make a better
comparison :
Can we unload a usb_mass_storage module when a USB disk(which uses the
module driver) is mounted and is being used ? I believe, the module
will eventually get unloaded when we unmount the disk, if someone did
a unload.
We have a similar situation here. The only difference is the driver is
referenced only when one of its device is in a trace session.
>
> Remember, removing a kernel module is something that only happens very
> rarely, and is an explicit choice by someone with root permissions. If
> you want to remove that module, it should be able to go, as you know
> what you are doing at that point in time.
Right, but when a device is "in use" can we do that ? I thought the user
will get a module is in use or busy, error.
>
> Don't try to "protect the user from themselves" here, they want to shoot
> their foot, make it hurt if they are aiming it there :)
>
The module_get/put added here are only triggered when we start a trace
session, where we build a path for the current session from the
configured "source" to the configured "sink" and the path is destroyed
at the end of the trace session. i.e, the path is not a permanent thing.
It is constructed per session. So it is perfectly possible to remove a
device in between trace sessions.
>> This will be
>> released as soon as the session is ended. It is just like a PMU driver
>> where the module refcount is held to ensure the module stays until the
>> session is over. In this case, we have multiple components, each with
>> its own driver invisible to the PMU driver. Hence the coresight driver
>> must hold the reference.
>
> Again, please think this through and don't add extra complexity to the
> normal path, and get it right if you do it (the existing patch is not
> right as I pointed out.) Personally, I feel the code should just be
> able to be unloaded whenever they want, user beware...
Sure, will explore more to refine the code. Thanks for the trigger.
Cheers
Suzuki
On 06/07/2018 10:32 AM, Suzuki K Poulose wrote:
> On 06/07/2018 10:13 AM, Greg Kroah-Hartman wrote:
>> On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote:
>>> Hi Greg,
>>>
>>> On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote:
>>>> On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
>>>>> On Wed, 6 Jun 2018 10:46:36 +0100
>>>>> Suzuki K Poulose <[email protected]> wrote:
>>>>>
>>>>>> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
>>>>>>> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
>>>>>>>> Increment the refcnt for driver modules in current use by calling
>>>>>>>> module_get in coresight_build_path and module_put in release_path.
>>>>>>>>
>>>>>>>> This prevents driver modules from being unloaded when they are
>>>>>>>> in use,
>>>>>>>> either in sysfs or perf mode.
>>>>>>>
>>>>>>> Why does it matter? Shouldn't you be allowed to remove any
>>>>>>> module at
>>>>>>> any point in time, much like a networking driver?
>>>
>>> The user doesn't have an explicit refcount on the individual components
>>> in a trace session. So, when a trace session is in progress, it is as
>>> good as having a "file" open on each component that is part of the
>>> active trace session. So, we don't want the driver to be removed when
>>> the component is being used in the trace collection.
>>
>> Why not? What's wrong with that happening and then the trace collection
>> starts failing with -ENODEV or something?
Forgot to add, this will indeed hit -ENODEV, if the device driver was
removed, as we fail to build the trace path before the session.
>
> May be I am missing something here. Can we allow the driver to be
> removed when one of its device is "turned ON" and we need the same
> driver to "turn it OFF" when the session ends ? To make a better
> comparison :
>
> Can we unload a usb_mass_storage module when a USB disk(which uses the
> module driver) is mounted and is being used ? I believe, the module
> will eventually get unloaded when we unmount the disk, if someone did
> a unload.
>
> We have a similar situation here. The only difference is the driver is
> referenced only when one of its device is in a trace session.
>
>>
>> Remember, removing a kernel module is something that only happens very
>> rarely, and is an explicit choice by someone with root permissions. If
>> you want to remove that module, it should be able to go, as you know
>> what you are doing at that point in time.
>
> Right, but when a device is "in use" can we do that ? I thought the user
> will get a module is in use or busy, error.
>
>
>>
>> Don't try to "protect the user from themselves" here, they want to shoot
>> their foot, make it hurt if they are aiming it there :)
>>
>
> The module_get/put added here are only triggered when we start a trace
> session, where we build a path for the current session from the
> configured "source" to the configured "sink" and the path is destroyed
> at the end of the trace session. i.e, the path is not a permanent thing.
> It is constructed per session. So it is perfectly possible to remove a
> device in between trace sessions.
>
>>> This will be
>>> released as soon as the session is ended. It is just like a PMU driver
>>> where the module refcount is held to ensure the module stays until the
>>> session is over. In this case, we have multiple components, each with
>>> its own driver invisible to the PMU driver. Hence the coresight driver
>>> must hold the reference.
>>
>> Again, please think this through and don't add extra complexity to the
>> normal path, and get it right if you do it (the existing patch is not
>> right as I pointed out.)Â Personally, I feel the code should just be
>> able to be unloaded whenever they want, user beware...
>
> Sure, will explore more to refine the code. Thanks for the trigger.
>
> Cheers
> Suzuki
Suzuki
On 06/06/2018 09:55 PM, Kim Phillips wrote:
> On Wed, 6 Jun 2018 10:46:36 +0100
> Suzuki K Poulose <[email protected]> wrote:
>
>> And while we are at this, I also realised that we hold references to the
>> parent devices for each connection (via bus_find_device() from
>> of_coresight_get_endpoint_device()), while parsing the platform data,
>> which is never released.
>
> Would this fix that?:
Not completely. We store the dev_name() as a reference, which itself can
be free'd, when the device is gone. I have a fix for this in my next
version of the DT clean up series [0], where I clean up most of the
platform parsing code.
[0]
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-June/582904.html
Cheers
Suzuki
>
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index a33a92ebe74b..a43ab078c85e 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -181,6 +181,8 @@ of_get_coresight_platform_data(struct device *dev,
> pdata->child_names[i] = dev_name(rdev);
> pdata->child_ports[i] = rendpoint.id;
>
> + put_device(rdev);
> +
> i++;
> } while (ep);
> }
On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote:
> Hi Greg,
>
> On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote:
> > On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
> > > On Wed, 6 Jun 2018 10:46:36 +0100
> > > Suzuki K Poulose <[email protected]> wrote:
> > >
> > > > On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
> > > > > > Increment the refcnt for driver modules in current use by calling
> > > > > > module_get in coresight_build_path and module_put in release_path.
> > > > > >
> > > > > > This prevents driver modules from being unloaded when they are in use,
> > > > > > either in sysfs or perf mode.
> > > > >
> > > > > Why does it matter? Shouldn't you be allowed to remove any module at
> > > > > any point in time, much like a networking driver?
>
> The user doesn't have an explicit refcount on the individual components
> in a trace session. So, when a trace session is in progress, it is as
> good as having a "file" open on each component that is part of the
> active trace session. So, we don't want the driver to be removed when
> the component is being used in the trace collection.
Why not? What's wrong with that happening and then the trace collection
starts failing with -ENODEV or something?
Remember, removing a kernel module is something that only happens very
rarely, and is an explicit choice by someone with root permissions. If
you want to remove that module, it should be able to go, as you know
what you are doing at that point in time.
Don't try to "protect the user from themselves" here, they want to shoot
their foot, make it hurt if they are aiming it there :)
> This will be
> released as soon as the session is ended. It is just like a PMU driver
> where the module refcount is held to ensure the module stays until the
> session is over. In this case, we have multiple components, each with
> its own driver invisible to the PMU driver. Hence the coresight driver
> must hold the reference.
Again, please think this through and don't add extra complexity to the
normal path, and get it right if you do it (the existing patch is not
right as I pointed out.) Personally, I feel the code should just be
able to be unloaded whenever they want, user beware...
thanks,
greg k-h
On Thu, Jun 07, 2018 at 10:32:21AM +0100, Suzuki K Poulose wrote:
> On 06/07/2018 10:13 AM, Greg Kroah-Hartman wrote:
> > On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote:
> > > Hi Greg,
> > >
> > > On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote:
> > > > On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
> > > > > On Wed, 6 Jun 2018 10:46:36 +0100
> > > > > Suzuki K Poulose <[email protected]> wrote:
> > > > >
> > > > > > On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
> > > > > > > On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
> > > > > > > > Increment the refcnt for driver modules in current use by calling
> > > > > > > > module_get in coresight_build_path and module_put in release_path.
> > > > > > > >
> > > > > > > > This prevents driver modules from being unloaded when they are in use,
> > > > > > > > either in sysfs or perf mode.
> > > > > > >
> > > > > > > Why does it matter? Shouldn't you be allowed to remove any module at
> > > > > > > any point in time, much like a networking driver?
> > >
> > > The user doesn't have an explicit refcount on the individual components
> > > in a trace session. So, when a trace session is in progress, it is as
> > > good as having a "file" open on each component that is part of the
> > > active trace session. So, we don't want the driver to be removed when
> > > the component is being used in the trace collection.
> >
> > Why not? What's wrong with that happening and then the trace collection
> > starts failing with -ENODEV or something?
>
> May be I am missing something here. Can we allow the driver to be removed
> when one of its device is "turned ON" and we need the same
> driver to "turn it OFF" when the session ends ? To make a better
> comparison :
>
> Can we unload a usb_mass_storage module when a USB disk(which uses the
> module driver) is mounted and is being used ? I believe, the module
> will eventually get unloaded when we unmount the disk, if someone did
> a unload.
No, mount causes the module count to be incrememted. Mount and
"open/close" are the old-school way of doing module reference counting.
Look at how network drivers work today, you can unload any network
driver even if there is a valid network connection "up and running"
attached to it. It just gets torn down when that request happens.
> We have a similar situation here. The only difference is the driver is
> referenced only when one of its device is in a trace session.
I understand, I'm saying that you have to be very careful when messing
around with module reference counts to get it correct and perhaps you
should just change your design to not care about module reference counts
at all, like networking did 15+ years ago.
Let's learn from the good examples in our past (like networking), and
not like the older bad examples (like mount/files).
> > Remember, removing a kernel module is something that only happens very
> > rarely, and is an explicit choice by someone with root permissions. If
> > you want to remove that module, it should be able to go, as you know
> > what you are doing at that point in time.
>
> Right, but when a device is "in use" can we do that ? I thought the user
> will get a module is in use or busy, error.
Try it on networking today :)
> > Don't try to "protect the user from themselves" here, they want to shoot
> > their foot, make it hurt if they are aiming it there :)
> >
>
> The module_get/put added here are only triggered when we start a trace
> session, where we build a path for the current session from the configured
> "source" to the configured "sink" and the path is destroyed
> at the end of the trace session. i.e, the path is not a permanent thing.
> It is constructed per session. So it is perfectly possible to remove a
> device in between trace sessions.
That's fine, but again, just be careful to get this correct. The patch
I reviewed did not seem to do that.
thanks,
greg k-h
On 06/07/2018 10:53 AM, Greg Kroah-Hartman wrote:
> On Thu, Jun 07, 2018 at 10:32:21AM +0100, Suzuki K Poulose wrote:
>> On 06/07/2018 10:13 AM, Greg Kroah-Hartman wrote:
>>> On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote:
>>>> Hi Greg,
>>>>
>>>> On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote:
>>>>> On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
>>>>>> On Wed, 6 Jun 2018 10:46:36 +0100
>>>>>> Suzuki K Poulose <[email protected]> wrote:
>>>>>>
>>>>>>> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
>>>>>>>> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
>>>>>>>>> Increment the refcnt for driver modules in current use by calling
>>>>>>>>> module_get in coresight_build_path and module_put in release_path.
>>>>>>>>>
>>>>>>>>> This prevents driver modules from being unloaded when they are in use,
>>>>>>>>> either in sysfs or perf mode.
>>>>>>>>
>>>>>>>> Why does it matter? Shouldn't you be allowed to remove any module at
>>>>>>>> any point in time, much like a networking driver?
>>>>
>>>> The user doesn't have an explicit refcount on the individual components
>>>> in a trace session. So, when a trace session is in progress, it is as
>>>> good as having a "file" open on each component that is part of the
>>>> active trace session. So, we don't want the driver to be removed when
>>>> the component is being used in the trace collection.
>>>
>>> Why not? What's wrong with that happening and then the trace collection
>>> starts failing with -ENODEV or something?
>>
>> May be I am missing something here. Can we allow the driver to be removed
>> when one of its device is "turned ON" and we need the same
>> driver to "turn it OFF" when the session ends ? To make a better
>> comparison :
>>
>> Can we unload a usb_mass_storage module when a USB disk(which uses the
>> module driver) is mounted and is being used ? I believe, the module
>> will eventually get unloaded when we unmount the disk, if someone did
>> a unload.
>
> No, mount causes the module count to be incrememted. Mount and
> "open/close" are the old-school way of doing module reference counting.
>
> Look at how network drivers work today, you can unload any network
> driver even if there is a valid network connection "up and running"
> attached to it. It just gets torn down when that request happens.
Ok, that makes more sense now. Thanks for the hints. However, it doesn't
look that easy from the coresight point due to the way the devices are
used in an interconnected manner which could be part of multiple trace
sessions.
e.g, a funnel could be part of two independent trace sessions with
different sets of sources/sinks. Tearing down the trace sessions is
going to be a difficult task unless we make drastic changes to the PMU
framework itself. But will see, what best we can do to make it modern
:-)
>
>> We have a similar situation here. The only difference is the driver is
>> referenced only when one of its device is in a trace session.
>
> I understand, I'm saying that you have to be very careful when messing
> around with module reference counts to get it correct and perhaps you
> should just change your design to not care about module reference counts
> at all, like networking did 15+ years ago.
>
> Let's learn from the good examples in our past (like networking), and
> not like the older bad examples (like mount/files).
>
>>> Remember, removing a kernel module is something that only happens very
>>> rarely, and is an explicit choice by someone with root permissions. If
>>> you want to remove that module, it should be able to go, as you know
>>> what you are doing at that point in time.
>>
>> Right, but when a device is "in use" can we do that ? I thought the user
>> will get a module is in use or busy, error.
>
> Try it on networking today :)
>
>>> Don't try to "protect the user from themselves" here, they want to shoot
>>> their foot, make it hurt if they are aiming it there :)
>>>
>>
>> The module_get/put added here are only triggered when we start a trace
>> session, where we build a path for the current session from the configured
>> "source" to the configured "sink" and the path is destroyed
>> at the end of the trace session. i.e, the path is not a permanent thing.
>> It is constructed per session. So it is perfectly possible to remove a
>> device in between trace sessions.
>
> That's fine, but again, just be careful to get this correct. The patch
> I reviewed did not seem to do that.
Thanks for the useful suggestions, we will explore this more.
Cheers
Suzuki
On Thu, 7 Jun 2018 11:07:15 +0100
Suzuki K Poulose <[email protected]> wrote:
> On 06/07/2018 10:53 AM, Greg Kroah-Hartman wrote:
> > On Thu, Jun 07, 2018 at 10:32:21AM +0100, Suzuki K Poulose wrote:
> >> On 06/07/2018 10:13 AM, Greg Kroah-Hartman wrote:
> >>> On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote:
> >>>> Hi Greg,
> >>>>
> >>>> On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote:
> >>>>> On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
> >>>>>> On Wed, 6 Jun 2018 10:46:36 +0100
> >>>>>> Suzuki K Poulose <[email protected]> wrote:
> >>>>>>
> >>>>>>> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
> >>>>>>>> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
> >>>>>>>>> Increment the refcnt for driver modules in current use by calling
> >>>>>>>>> module_get in coresight_build_path and module_put in release_path.
> >>>>>>>>>
> >>>>>>>>> This prevents driver modules from being unloaded when they are in use,
> >>>>>>>>> either in sysfs or perf mode.
> >>>>>>>>
> >>>>>>>> Why does it matter? Shouldn't you be allowed to remove any module at
> >>>>>>>> any point in time, much like a networking driver?
> >>>>
> >>>> The user doesn't have an explicit refcount on the individual components
> >>>> in a trace session. So, when a trace session is in progress, it is as
> >>>> good as having a "file" open on each component that is part of the
> >>>> active trace session. So, we don't want the driver to be removed when
> >>>> the component is being used in the trace collection.
> >>>
> >>> Why not? What's wrong with that happening and then the trace collection
> >>> starts failing with -ENODEV or something?
> >>
> >> May be I am missing something here. Can we allow the driver to be removed
> >> when one of its device is "turned ON" and we need the same
> >> driver to "turn it OFF" when the session ends ? To make a better
> >> comparison :
> >>
> >> Can we unload a usb_mass_storage module when a USB disk(which uses the
> >> module driver) is mounted and is being used ? I believe, the module
> >> will eventually get unloaded when we unmount the disk, if someone did
> >> a unload.
> >
> > No, mount causes the module count to be incrememted. Mount and
> > "open/close" are the old-school way of doing module reference counting.
> >
> > Look at how network drivers work today, you can unload any network
> > driver even if there is a valid network connection "up and running"
> > attached to it. It just gets torn down when that request happens.
>
> Ok, that makes more sense now. Thanks for the hints. However, it doesn't
> look that easy from the coresight point due to the way the devices are
> used in an interconnected manner which could be part of multiple trace
> sessions.
>
> e.g, a funnel could be part of two independent trace sessions with
> different sets of sources/sinks. Tearing down the trace sessions is
> going to be a difficult task unless we make drastic changes to the PMU
> framework itself. But will see, what best we can do to make it modern
> :-)
> >
> >> We have a similar situation here. The only difference is the driver is
> >> referenced only when one of its device is in a trace session.
> >
> > I understand, I'm saying that you have to be very careful when messing
> > around with module reference counts to get it correct and perhaps you
> > should just change your design to not care about module reference counts
> > at all, like networking did 15+ years ago.
> >
> > Let's learn from the good examples in our past (like networking), and
> > not like the older bad examples (like mount/files).
> >
> >>> Remember, removing a kernel module is something that only happens very
> >>> rarely, and is an explicit choice by someone with root permissions. If
> >>> you want to remove that module, it should be able to go, as you know
> >>> what you are doing at that point in time.
> >>
> >> Right, but when a device is "in use" can we do that ? I thought the user
> >> will get a module is in use or busy, error.
> >
> > Try it on networking today :)
> >
> >>> Don't try to "protect the user from themselves" here, they want to shoot
> >>> their foot, make it hurt if they are aiming it there :)
> >>>
> >>
> >> The module_get/put added here are only triggered when we start a trace
> >> session, where we build a path for the current session from the configured
> >> "source" to the configured "sink" and the path is destroyed
> >> at the end of the trace session. i.e, the path is not a permanent thing.
> >> It is constructed per session. So it is perfectly possible to remove a
> >> device in between trace sessions.
> >
> > That's fine, but again, just be careful to get this correct. The patch
> > I reviewed did not seem to do that.
>
> Thanks for the useful suggestions, we will explore this more.
I'm going to assume the series is still valid after this discussion,
since technically just this patch can get dropped, and the user is able
to shoot themselves in the foot. This series is for development
purposes, after all.
Let me know if I'm missing something.
Thanks,
Kim
On 06/07/2018 06:13 PM, Kim Phillips wrote:
> On Thu, 7 Jun 2018 11:07:15 +0100
> Suzuki K Poulose <[email protected]> wrote:
>
>> On 06/07/2018 10:53 AM, Greg Kroah-Hartman wrote:
>>> On Thu, Jun 07, 2018 at 10:32:21AM +0100, Suzuki K Poulose wrote:
>>>> On 06/07/2018 10:13 AM, Greg Kroah-Hartman wrote:
>>>>> On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote:
>>>>>>> On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
>>>>>>>> On Wed, 6 Jun 2018 10:46:36 +0100
>>>>>>>> Suzuki K Poulose <[email protected]> wrote:
>>>>>>>>
>>>>>>>>> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
>>>>>>>>>> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
>>>>>>>>>>> Increment the refcnt for driver modules in current use by calling
>>>>>>>>>>> module_get in coresight_build_path and module_put in release_path.
>>>>>>>>>>>
>>>>>>>>>>> This prevents driver modules from being unloaded when they are in use,
>>>>>>>>>>> either in sysfs or perf mode.
>>>>>>>>>>
>>>>>>>>>> Why does it matter? Shouldn't you be allowed to remove any module at
>>>>>>>>>> any point in time, much like a networking driver?
>>>>>>
>>>>>> The user doesn't have an explicit refcount on the individual components
>>>>>> in a trace session. So, when a trace session is in progress, it is as
>>>>>> good as having a "file" open on each component that is part of the
>>>>>> active trace session. So, we don't want the driver to be removed when
>>>>>> the component is being used in the trace collection.
>>>>>
>>>>> Why not? What's wrong with that happening and then the trace collection
>>>>> starts failing with -ENODEV or something?
>>>>
>>>> May be I am missing something here. Can we allow the driver to be removed
>>>> when one of its device is "turned ON" and we need the same
>>>> driver to "turn it OFF" when the session ends ? To make a better
>>>> comparison :
>>>>
>>>> Can we unload a usb_mass_storage module when a USB disk(which uses the
>>>> module driver) is mounted and is being used ? I believe, the module
>>>> will eventually get unloaded when we unmount the disk, if someone did
>>>> a unload.
>>>
>>> No, mount causes the module count to be incrememted. Mount and
>>> "open/close" are the old-school way of doing module reference counting.
>>>
>>> Look at how network drivers work today, you can unload any network
>>> driver even if there is a valid network connection "up and running"
>>> attached to it. It just gets torn down when that request happens.
>>
>> Ok, that makes more sense now. Thanks for the hints. However, it doesn't
>> look that easy from the coresight point due to the way the devices are
>> used in an interconnected manner which could be part of multiple trace
>> sessions.
>>
>> e.g, a funnel could be part of two independent trace sessions with
>> different sets of sources/sinks. Tearing down the trace sessions is
>> going to be a difficult task unless we make drastic changes to the PMU
>> framework itself. But will see, what best we can do to make it modern
>> :-)
>>>
>>>> We have a similar situation here. The only difference is the driver is
>>>> referenced only when one of its device is in a trace session.
>>>
>>> I understand, I'm saying that you have to be very careful when messing
>>> around with module reference counts to get it correct and perhaps you
>>> should just change your design to not care about module reference counts
>>> at all, like networking did 15+ years ago.
>>>
>>> Let's learn from the good examples in our past (like networking), and
>>> not like the older bad examples (like mount/files).
>>>
>>>>> Remember, removing a kernel module is something that only happens very
>>>>> rarely, and is an explicit choice by someone with root permissions. If
>>>>> you want to remove that module, it should be able to go, as you know
>>>>> what you are doing at that point in time.
>>>>
>>>> Right, but when a device is "in use" can we do that ? I thought the user
>>>> will get a module is in use or busy, error.
>>>
>>> Try it on networking today :)
>>>
>>>>> Don't try to "protect the user from themselves" here, they want to shoot
>>>>> their foot, make it hurt if they are aiming it there :)
>>>>>
>>>>
>>>> The module_get/put added here are only triggered when we start a trace
>>>> session, where we build a path for the current session from the configured
>>>> "source" to the configured "sink" and the path is destroyed
>>>> at the end of the trace session. i.e, the path is not a permanent thing.
>>>> It is constructed per session. So it is perfectly possible to remove a
>>>> device in between trace sessions.
>>>
>>> That's fine, but again, just be careful to get this correct. The patch
>>> I reviewed did not seem to do that.
>>
>> Thanks for the useful suggestions, we will explore this more.
Kim,
>
> I'm going to assume the series is still valid after this discussion,
> since technically just this patch can get dropped, and the user is able
> to shoot themselves in the foot.
That doesn't mean the kernel can panic() if the user decided to unload
the module while the trace session is in progress. It only means that
the trace session could be stopped in between in the worst case. But
nothing more harmful to the system.
> This series is for development purposes, after all.
Do you mean that this series is for internal development purposes and
not upstream ? Making the drivers modular are always helpful, especially
for something related to tracing, that allows the module to be unloaded
after use. So, it would be good to have this series in, but in a manner
which is usable and doesn't cause harm to the overall system usage.
I think the summary of the discussion is that we need more robust code
to handle the situation, which also allows unloading the modules without
any trouble.
Cheers
Suzuki
>
> Let me know if I'm missing something.
>
> Thanks,
>
> Kim
>
On 7 June 2018 at 15:10, Suzuki K Poulose <[email protected]> wrote:
> On 06/07/2018 06:13 PM, Kim Phillips wrote:
>>
>> On Thu, 7 Jun 2018 11:07:15 +0100
>> Suzuki K Poulose <[email protected]> wrote:
>>
>>> On 06/07/2018 10:53 AM, Greg Kroah-Hartman wrote:
>>>>
>>>> On Thu, Jun 07, 2018 at 10:32:21AM +0100, Suzuki K Poulose wrote:
>>>>>
>>>>> On 06/07/2018 10:13 AM, Greg Kroah-Hartman wrote:
>>>>>>
>>>>>> On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote:
>>>>>>>
>>>>>>> Hi Greg,
>>>>>>>
>>>>>>> On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote:
>>>>>>>>
>>>>>>>> On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote:
>>>>>>>>>
>>>>>>>>> On Wed, 6 Jun 2018 10:46:36 +0100
>>>>>>>>> Suzuki K Poulose <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>>> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Increment the refcnt for driver modules in current use by
>>>>>>>>>>>> calling
>>>>>>>>>>>> module_get in coresight_build_path and module_put in
>>>>>>>>>>>> release_path.
>>>>>>>>>>>>
>>>>>>>>>>>> This prevents driver modules from being unloaded when they are
>>>>>>>>>>>> in use,
>>>>>>>>>>>> either in sysfs or perf mode.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Why does it matter? Shouldn't you be allowed to remove any
>>>>>>>>>>> module at
>>>>>>>>>>> any point in time, much like a networking driver?
>>>>>>>
>>>>>>>
>>>>>>> The user doesn't have an explicit refcount on the individual
>>>>>>> components
>>>>>>> in a trace session. So, when a trace session is in progress, it is as
>>>>>>> good as having a "file" open on each component that is part of the
>>>>>>> active trace session. So, we don't want the driver to be removed when
>>>>>>> the component is being used in the trace collection.
>>>>>>
>>>>>>
>>>>>> Why not? What's wrong with that happening and then the trace
>>>>>> collection
>>>>>> starts failing with -ENODEV or something?
>>>>>
>>>>>
>>>>> May be I am missing something here. Can we allow the driver to be
>>>>> removed
>>>>> when one of its device is "turned ON" and we need the same
>>>>> driver to "turn it OFF" when the session ends ? To make a better
>>>>> comparison :
>>>>>
>>>>> Can we unload a usb_mass_storage module when a USB disk(which uses the
>>>>> module driver) is mounted and is being used ? I believe, the module
>>>>> will eventually get unloaded when we unmount the disk, if someone did
>>>>> a unload.
>>>>
>>>>
>>>> No, mount causes the module count to be incrememted. Mount and
>>>> "open/close" are the old-school way of doing module reference counting.
>>>>
>>>> Look at how network drivers work today, you can unload any network
>>>> driver even if there is a valid network connection "up and running"
>>>> attached to it. It just gets torn down when that request happens.
>>>
>>>
>>> Ok, that makes more sense now. Thanks for the hints. However, it doesn't
>>> look that easy from the coresight point due to the way the devices are
>>> used in an interconnected manner which could be part of multiple trace
>>> sessions.
>>>
>>> e.g, a funnel could be part of two independent trace sessions with
>>> different sets of sources/sinks. Tearing down the trace sessions is
>>> going to be a difficult task unless we make drastic changes to the PMU
>>> framework itself. But will see, what best we can do to make it modern
>>> :-)
>>>>
>>>>
>>>>> We have a similar situation here. The only difference is the driver is
>>>>> referenced only when one of its device is in a trace session.
>>>>
>>>>
>>>> I understand, I'm saying that you have to be very careful when messing
>>>> around with module reference counts to get it correct and perhaps you
>>>> should just change your design to not care about module reference counts
>>>> at all, like networking did 15+ years ago.
>>>>
>>>> Let's learn from the good examples in our past (like networking), and
>>>> not like the older bad examples (like mount/files).
>>>>
>>>>>> Remember, removing a kernel module is something that only happens very
>>>>>> rarely, and is an explicit choice by someone with root permissions.
>>>>>> If
>>>>>> you want to remove that module, it should be able to go, as you know
>>>>>> what you are doing at that point in time.
>>>>>
>>>>>
>>>>> Right, but when a device is "in use" can we do that ? I thought the
>>>>> user
>>>>> will get a module is in use or busy, error.
>>>>
>>>>
>>>> Try it on networking today :)
>>>>
>>>>>> Don't try to "protect the user from themselves" here, they want to
>>>>>> shoot
>>>>>> their foot, make it hurt if they are aiming it there :)
>>>>>>
>>>>>
>>>>> The module_get/put added here are only triggered when we start a trace
>>>>> session, where we build a path for the current session from the
>>>>> configured
>>>>> "source" to the configured "sink" and the path is destroyed
>>>>> at the end of the trace session. i.e, the path is not a permanent
>>>>> thing.
>>>>> It is constructed per session. So it is perfectly possible to remove a
>>>>> device in between trace sessions.
>>>>
>>>>
>>>> That's fine, but again, just be careful to get this correct. The patch
>>>> I reviewed did not seem to do that.
>>>
>>>
>>> Thanks for the useful suggestions, we will explore this more.
>
>
> Kim,
>
>>
>> I'm going to assume the series is still valid after this discussion,
>> since technically just this patch can get dropped, and the user is able
>> to shoot themselves in the foot.
>
>
> That doesn't mean the kernel can panic() if the user decided to unload the
> module while the trace session is in progress. It only means that
> the trace session could be stopped in between in the worst case. But
> nothing more harmful to the system.
>
>> This series is for development purposes, after all.
>
>
> Do you mean that this series is for internal development purposes and not
> upstream ? Making the drivers modular are always helpful, especially for
> something related to tracing, that allows the module to be unloaded after
> use. So, it would be good to have this series in, but in a manner which is
> usable and doesn't cause harm to the overall system usage.
Correct, we can't have a patchset that generates a kernel panic.
>
> I think the summary of the discussion is that we need more robust code
> to handle the situation, which also allows unloading the modules without
> any trouble.
The tricky part is the "unloading without any trouble". The first
thing to so is if the driver is being used, the _remove() functions
need to go through the same process as it would under normal
condition. That will allow to reinsert the module and have a fairly
good level of assurance that things will work properly.
Looking at things a little closer all the interconnection dependencies
in the core are done using a csdev and a lot of the current code is
already checking for a NULL condition (more checks may be needed with
the introduction of this set). The real problem is with the "path"
used to keep track of the devices taking part in active sessions.
Those can be accessed when a process is swapped in and out, mandating
something fast and efficient. One thing we could do is in a path,
keep track of a reference on csdev rather than make a copy of their
addresses. That way the _remove() functions could simply set those to
NULL, making it easy to deal with.
>
> Cheers
>
> Suzuki
>
>>
>> Let me know if I'm missing something.
>>
>> Thanks,
>>
>> Kim
>>
>
On Thu, 7 Jun 2018 22:10:07 +0100
Suzuki K Poulose <[email protected]> wrote:
> On 06/07/2018 06:13 PM, Kim Phillips wrote:
> > I'm going to assume the series is still valid after this discussion,
> > since technically just this patch can get dropped, and the user is able
> > to shoot themselves in the foot.
>
> That doesn't mean the kernel can panic() if the user decided to unload
> the module while the trace session is in progress. It only means that
> the trace session could be stopped in between in the worst case. But
> nothing more harmful to the system.
FWIW, I didn't see the kernel panic in my basic tests; just some bad
accesses: the new remove functions take care of cleaning up most items,
and most drivers still depend on the links and sinks (funnel,
replicator) drivers, so they can't be upset too bad.
> > This series is for development purposes, after all.
>
> Do you mean that this series is for internal development purposes and
> not upstream ? Making the drivers modular are always helpful, especially
no, I'm posting them for upstream review because I'd like them upstream.
> for something related to tracing, that allows the module to be unloaded
> after use. So, it would be good to have this series in, but in a manner
> which is usable and doesn't cause harm to the overall system usage.
>
> I think the summary of the discussion is that we need more robust code
> to handle the situation, which also allows unloading the modules without
> any trouble.
Trouble's relative. My point was since the series is going to be used
mainly by developers testing their code, they already prepare for, and
expect badness to occur anyway. Greg's point isn't lost here, and in
my interpretation, his review of this patch was that it was in the
wrong direction of safety: it made things unnecessarily too safe, up
front, and that items relative to the perf core should strive to adhere
to the higher standards set in place by the networking subsystem. So,
this patch doesn't get his ack.
I compiled a new v5 series that omits this patch, and overwrote the v4
series here:
git://linux-arm.org/linux-kp.git, coresight-modules branch
but I'll hold of submitting a v5 for now.
I don't know how the perf core handles AUXTRACE drivers hanging up on
it. I see intel-pt record support can't be built as a module. I'm
guessing more testing for actual panics when using perf or sysfs is
what's being sought here?
Kim
On 5 June 2018 at 15:07, Kim Phillips <[email protected]> wrote:
> Allow to build coresight as a module. This enhances
> coresight developer efficiency by allowing the development to
> take place exclusively on the target, and without needing to
> reboot in between changes.
>
> - 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 core bus
> register/unregister function calls to the etm_perf init/exit
> functions, since coresight.c does not have etm_pmu defined.
> - add a MODULE_DEVICE_TABLE for autoloading on boot
>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/hwtracing/coresight/Kconfig | 5 ++++-
> drivers/hwtracing/coresight/Makefile | 7 +++++--
> .../coresight/{coresight.c => coresight-core.c} | 6 ------
> .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++++-
> 4 files changed, 25 insertions(+), 10 deletions(-)
> rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c} (99%)
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 181a44ea2d61..c05b265f7731 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -2,7 +2,7 @@
> # Coresight configuration
> #
> menuconfig CORESIGHT
> - bool "CoreSight Tracing Support"
> + tristate "CoreSight Tracing Support"
> select ARM_AMBA
> select PERF_EVENTS
> help
> @@ -12,6 +12,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 45d7a0f34170..ed2d4bcb017b 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -2,8 +2,11 @@
> #
> # Makefile for CoreSight drivers.
> #
> -obj-$(CONFIG_CORESIGHT) += coresight.o coresight-etm-perf.o
> -obj-$(CONFIG_OF) += of_coresight.o
> +obj-$(CONFIG_CORESIGHT) += coresight.o
> +coresight-objs := coresight-core.o coresight-etm-perf.o
> +ifeq ($(CONFIG_OF), y)
> +coresight-objs += of_coresight.o
> +endif
> obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
> coresight-tmc-objs := 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 99%
> rename from drivers/hwtracing/coresight/coresight.c
> rename to drivers/hwtracing/coresight/coresight-core.c
> index 1c941351f1d1..f96258de1e9b 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -948,12 +948,6 @@ struct bus_type coresight_bustype = {
> .name = "coresight",
> };
>
> -static int __init coresight_init(void)
> -{
> - return bus_register(&coresight_bustype);
> -}
> -postcore_initcall(coresight_init);
> -
> struct coresight_device *coresight_register(struct coresight_desc *desc)
> {
> int i;
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 0fe7e43ea1c4..ceac9aee4a82 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -472,6 +472,10 @@ static int __init etm_perf_init(void)
> {
> int ret;
>
> + ret = bus_register(&coresight_bustype);
> + if (ret)
> + return ret;
> +
> etm_pmu.capabilities = PERF_PMU_CAP_EXCLUSIVE;
>
> etm_pmu.attr_groups = etm_pmu_attr_groups;
> @@ -494,4 +498,15 @@ static int __init etm_perf_init(void)
>
> return ret;
> }
> -device_initcall(etm_perf_init);
> +postcore_initcall(etm_perf_init);
> +
> +static void __exit etm_perf_exit(void)
> +{
> + perf_pmu_unregister(&etm_pmu);
> + bus_unregister(&coresight_bustype);
> +}
> +module_exit(etm_perf_exit);
I see the perf functionality as an accessory to the core rather than
the other way around. Initialisation in the core code should be
driving the PMU registration.
> +
> +MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
> +MODULE_DESCRIPTION("Arm CoreSight tracer driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.0
>
On 7 June 2018 at 15:47, Kim Phillips <[email protected]> wrote:
> On Thu, 7 Jun 2018 22:10:07 +0100
> Suzuki K Poulose <[email protected]> wrote:
>
>> On 06/07/2018 06:13 PM, Kim Phillips wrote:
>> > I'm going to assume the series is still valid after this discussion,
>> > since technically just this patch can get dropped, and the user is able
>> > to shoot themselves in the foot.
>>
>> That doesn't mean the kernel can panic() if the user decided to unload
>> the module while the trace session is in progress. It only means that
>> the trace session could be stopped in between in the worst case. But
>> nothing more harmful to the system.
>
> FWIW, I didn't see the kernel panic in my basic tests; just some bad
> accesses: the new remove functions take care of cleaning up most items,
> and most drivers still depend on the links and sinks (funnel,
> replicator) drivers, so they can't be upset too bad.
>
>> > This series is for development purposes, after all.
>>
>> Do you mean that this series is for internal development purposes and
>> not upstream ? Making the drivers modular are always helpful, especially
>
> no, I'm posting them for upstream review because I'd like them upstream.
>
>> for something related to tracing, that allows the module to be unloaded
>> after use. So, it would be good to have this series in, but in a manner
>> which is usable and doesn't cause harm to the overall system usage.
>>
>> I think the summary of the discussion is that we need more robust code
>> to handle the situation, which also allows unloading the modules without
>> any trouble.
>
> Trouble's relative. My point was since the series is going to be used
> mainly by developers testing their code, they already prepare for, and
> expect badness to occur anyway. Greg's point isn't lost here, and in
> my interpretation, his review of this patch was that it was in the
> wrong direction of safety: it made things unnecessarily too safe, up
> front, and that items relative to the perf core should strive to adhere
> to the higher standards set in place by the networking subsystem. So,
> this patch doesn't get his ack.
Greg's point was that it's OK to let users harm themselves (which I
totally support), but if you're going to prevent it, make sure to do
it right.
>
> I compiled a new v5 series that omits this patch, and overwrote the v4
> series here:
>
> git://linux-arm.org/linux-kp.git, coresight-modules branch
>
> but I'll hold of submitting a v5 for now.
>
> I don't know how the perf core handles AUXTRACE drivers hanging up on
> it. I see intel-pt record support can't be built as a module. I'm
> guessing more testing for actual panics when using perf or sysfs is
> what's being sought here?
There are two ways to approach the problem:
1) Kill active trace sessions (either sysFS or perf) if a driver that
is being used is removed.
2) Deal with the removal in the coresight core, making sure we don't
access operations provided by removed drivers.
The end result in both cases will be the same: failure to properly
terminate the trace session because of user action.
I'm personally in favour of the second option, simply because it keeps
problems resolution with the CS subsystem.
>
> Kim
On 06/07/2018 10:47 PM, Kim Phillips wrote:
> On Thu, 7 Jun 2018 22:10:07 +0100
> Suzuki K Poulose <[email protected]> wrote:
>
>> On 06/07/2018 06:13 PM, Kim Phillips wrote:
>>> I'm going to assume the series is still valid after this discussion,
>>> since technically just this patch can get dropped, and the user is able
>>> to shoot themselves in the foot.
>>
>> That doesn't mean the kernel can panic() if the user decided to unload
>> the module while the trace session is in progress. It only means that
>> the trace session could be stopped in between in the worst case. But
>> nothing more harmful to the system.
Kim,
>
> FWIW, I didn't see the kernel panic in my basic tests; just some bad
> accesses: the new remove functions take care of cleaning up most items,
> and most drivers still depend on the links and sinks (funnel,
> replicator) drivers, so they can't be upset too bad.
Bad accesses are still bad. The bad access could trigger an Oops for
e.g, or could even corrupt the other parts of the kernel if we try
to access a memory that is free'd (and reallocated to somebody else).
So, the point is there are issues with the series which we know for
sure from code analysis. It may take different forms to show up at
runtime.
>
>>> This series is for development purposes, after all.
>>
>> Do you mean that this series is for internal development purposes and
>> not upstream ? Making the drivers modular are always helpful, especially
>
> no, I'm posting them for upstream review because I'd like them upstream.
>
>> for something related to tracing, that allows the module to be unloaded
>> after use. So, it would be good to have this series in, but in a manner
>> which is usable and doesn't cause harm to the overall system usage.
>>
>> I think the summary of the discussion is that we need more robust code
>> to handle the situation, which also allows unloading the modules without
>> any trouble.
>
> Trouble's relative. My point was since the series is going to be used
> mainly by developers testing their code, they already prepare for, and
> expect badness to occur anyway. Greg's point isn't lost here, and in
Making something modular is not really just for the use of developers.
There are and will be other users for a device driver as a module and
it is a fundamental feature people expect (especially in the enterprise
world, where there is one kernel which builds most of the stuff as
module to let the users pick the individual drivers as they need).
So, at the kernel driver you can't really be sure, if the user is
actually aware of the "developer" only mode and he knows that we can
crash the kernel.
> my interpretation, his review of this patch was that it was in the
> wrong direction of safety: it made things unnecessarily too safe, up
> front, and that items relative to the perf core should strive to adhere
One of the areas of improvement towards the "modern" behavior is failing
the activation of the trace schedule, when a component in the path has
been removed when we go through coresight_enable_path(). Right now, we
create a path and then we do enable_path() and disable_path() around the
trace schedules and the path is destroyed only at pmu->free_aux(). With
the current patch, we hold the reference to the device/driver throughout
the duration of the life time of the tracing, even when the tracing
may be disabled in between.
I think, if we get to that point, we should be at the best we can reach
towards the expected behavior. But having said that, it is indeed tricky
to get that. May be we could play a little bit with the refcounting on
csdev and check if the refcount is only held by the number of paths this
component is part of (needs more thought).
Suzuki