Hi Will, Please see my reply inline. > -----Original Message----- > From: Will Deacon <will@xxxxxxxxxx> > Sent: Friday, July 28, 2023 8:22 AM > To: Besar Wicaksono <bwicaksono@xxxxxxxxxx>; suzuki.poulose@xxxxxxx > Cc: robin.murphy@xxxxxxx; ilkka@xxxxxxxxxxxxxxxxxxxxxx; > catalin.marinas@xxxxxxx; mark.rutland@xxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > tegra@xxxxxxxxxxxxxxx; Thierry Reding <treding@xxxxxxxxxx>; Jonathan > Hunter <jonathanh@xxxxxxxxxx>; Vikram Sethi <vsethi@xxxxxxxxxx>; Richard > Wiley <rwiley@xxxxxxxxxx>; Eric Funsten <efunsten@xxxxxxxxxx> > 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: > > 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 <bwicaksono@xxxxxxxxxx> > > --- > > > > Changes from v4: > > * Fix warning reported by kernel test robot > > v4: https://lore.kernel.org/linux-arm-kernel/20230620041438.32514-1- > bwicaksono@xxxxxxxxxx/T/#u > > One minor comment below, but this mostly looks good to me. I'd like Suzuki's > Ack before I queue it, though. > > > + /* 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
