RE: [PATCH v5] perf: arm_cspmu: Separate Arm and vendor module

Hi Will,

Please see my reply inline.

-----Original Message-----
From: Will Deacon <will@kernel.org>
Sent: Friday, July 28, 2023 8:22 AM
To: Besar Wicaksono <redacted>; suzuki.poulose@arm.com
Cc: robin.murphy@arm.com; ilkka@os.amperecomputing.com;
catalin.marinas@arm.com; mark.rutland@arm.com; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
tegra@vger.kernel.org; Thierry Reding [off-list ref]; Jonathan
Hunter [off-list ref]; Vikram Sethi [off-list ref]; Richard
Wiley [off-list ref]; Eric Funsten [off-list ref]
Subject: Re: [PATCH v5] perf: arm_cspmu: Separate Arm and vendor module

External email: Use caution opening links or attachments


On Wed, Jul 05, 2023 at 05:47:45AM -0500, Besar Wicaksono wrote:

quoted

Arm Coresight PMU driver consists of main standard code and
vendor backend code. Both are currently built as a single module.
This patch adds vendor registration API to separate the two to
keep things modular. The main driver requests each known backend
module during initialization and defer device binding process.
The backend module then registers an init callback to the main
driver and continue the device driver binding process.

Signed-off-by: Besar Wicaksono <redacted>
---

Changes from v4:
 * Fix warning reported by kernel test robot
v4: https://lore.kernel.org/linux-arm-kernel/20230620041438.32514-1-

bwicaksono@nvidia.com/T/#u

One minor comment below, but this mostly looks good to me. I'd like Suzuki's
Ack before I queue it, though.

quoted

+     /* Load implementer module and initialize the callbacks. */
+     if (match) {
+             mutex_lock(&arm_cspmu_lock);
+
+             if (match->impl_init_ops) {
+                     if (try_module_get(match->module)) {
+                             cspmu->impl.match = match;
+                             ret = match->impl_init_ops(cspmu);
+                             module_put(match->module);

Why is it safe to drop the module reference here? If I'm understanding the
flow correctly, ->impl_init_ops() will populate more function pointers
in the cspmu->impl.ops structure, and we don't appear to take a module
reference when calling those.

What happens if the backend module is unloaded while the core module
is executed those functions?

We also update the call to perf_pmu_register and provide the backend module handle.
The core perf kernel will acquire the reference on the backend module prior to calling the
functions in cspmu->imp.ops. Please see the change below

+static inline struct module *arm_cspmu_get_module(struct arm_cspmu *cspmu)
+{
+	return (cspmu->impl.match) ? cspmu->impl.match->module : THIS_MODULE;
+}
+
 static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
 {
 	int ret, capabilities;

@@ -1149,7 +1173,7 @@ static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
 
 	cspmu->pmu = (struct pmu){
 		.task_ctx_nr	= perf_invalid_context,
-		.module		= THIS_MODULE,
+		.module		= arm_cspmu_get_module(cspmu),
 		.pmu_enable	= arm_cspmu_enable,
 		.pmu_disable	= arm_cspmu_disable,
 		.event_init	= arm_cspmu_event_init,

Regards,
Besar

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