Hi Robin, > -----Original Message----- > From: Robin Murphy <robin.murphy@xxxxxxx> > Sent: Friday, May 5, 2023 12:24 PM > To: Besar Wicaksono <bwicaksono@xxxxxxxxxx>; suzuki.poulose@xxxxxxx; > catalin.marinas@xxxxxxx; will@xxxxxxxxxx; mark.rutland@xxxxxxx > Cc: 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 v3] perf: arm_cspmu: Separate Arm and vendor module > > External email: Use caution opening links or attachments > > > On 2023-05-05 01:59, 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. Main driver maintains a list of backend module > > and will request it when probing the device. > > > > Signed-off-by: Besar Wicaksono <bwicaksono@xxxxxxxxxx> > > --- > > > > Changes from v2: > > * Move sysfs_event/format_show definition to arm_cspmu.h and move > impl > > registration API definition to a separate module so main driver and vendor > > module are independent. > > * The registration API now just sets the impl_init_ops callback, no reprobe. > > * Add PMIIDR table that maps to the vendor module name. During device > probe, > > main driver requests the vendor module if PMIIDR is matching. > > * Keeping the name of the main driver module as arm_cspmu_module. > > Thanks to Robin and Suzuki for the feedback. > > v2: https://lore.kernel.org/linux-arm-kernel/20230418062030.45620-1- > bwicaksono@xxxxxxxxxx/T/#u > > > > Changes from v1: > > * Added separate Kconfig entry for nvidia backend > > * Added lock to protect accesses to the lists > > * Added support for matching subset devices from a vendor > > * Added state tracking to avoid reprobe when a device is in use > > Thanks to Suzuki for the feedback. > > v1: https://lore.kernel.org/linux-arm-kernel/20230403163905.20354-1- > bwicaksono@xxxxxxxxxx/T/#u > > > > --- > > drivers/perf/arm_cspmu/Kconfig | 9 +- > > drivers/perf/arm_cspmu/Makefile | 7 +- > > drivers/perf/arm_cspmu/arm_cspmu.c | 86 ++++++------------ > > drivers/perf/arm_cspmu/arm_cspmu.h | 55 ++++++++++-- > > drivers/perf/arm_cspmu/arm_cspmu_impl.c | 114 > ++++++++++++++++++++++++ > > drivers/perf/arm_cspmu/nvidia_cspmu.c | 35 +++++++- > > drivers/perf/arm_cspmu/nvidia_cspmu.h | 17 ---- > > 7 files changed, 235 insertions(+), 88 deletions(-) > > create mode 100644 drivers/perf/arm_cspmu/arm_cspmu_impl.c > > delete mode 100644 drivers/perf/arm_cspmu/nvidia_cspmu.h > > > > diff --git a/drivers/perf/arm_cspmu/Kconfig > b/drivers/perf/arm_cspmu/Kconfig > > index 0b316fe69a45..8ce7b45a0075 100644 > > --- a/drivers/perf/arm_cspmu/Kconfig > > +++ b/drivers/perf/arm_cspmu/Kconfig > > @@ -1,6 +1,6 @@ > > # SPDX-License-Identifier: GPL-2.0 > > # > > -# Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > +# Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > > > config ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU > > tristate "ARM Coresight Architecture PMU" > > @@ -11,3 +11,10 @@ config ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU > > based on ARM CoreSight PMU architecture. Note that this PMU > > architecture does not have relationship with the ARM CoreSight > > Self-Hosted Tracing. > > + > > +config NVIDIA_CORESIGHT_PMU_ARCH_SYSTEM_PMU > > + tristate "NVIDIA Coresight Architecture PMU" > > + depends on ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU > > + help > > + Provides NVIDIA specific attributes for performance monitoring unit > > + (PMU) devices based on ARM CoreSight PMU architecture. > > diff --git a/drivers/perf/arm_cspmu/Makefile > b/drivers/perf/arm_cspmu/Makefile > > index fedb17df982d..602ecb99dc57 100644 > > --- a/drivers/perf/arm_cspmu/Makefile > > +++ b/drivers/perf/arm_cspmu/Makefile > > @@ -1,6 +1,9 @@ > > -# Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > +# Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > # > > # SPDX-License-Identifier: GPL-2.0 > > > > +obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += > arm_cspmu_impl.o > > Not sure what's up with this... I have no complaint with keeping the > impl infrastucture together in its own source file, but it still wants > to end up as part of arm_cspmu_module. Doing otherwise just adds > unnecessary overhead at many levels and invites more problems. My intention is to separate arm_cspmu_impl, arm_cspmu, and vendor backend into different modules. Here is the dependency I have in mind: arm_cspmu_impl ____|____ | | arm_cspmu nvidia_cspmu This helps during device probe that the call to request_module can be made as a blocking call and the backend init_impl_ops will always be ready to use after request_module returns. The code seems simpler this way. Could you please elaborate the potential issue that might arise with this approach? After reading your other comments on built-in kernel, can we use late_initcall for arm_cspmu module to assume that the main driver will always be init'ed after backend module ? > > > obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += > arm_cspmu_module.o > > -arm_cspmu_module-y := arm_cspmu.o nvidia_cspmu.o > > +arm_cspmu_module-y := arm_cspmu.o > > + > > +obj-$(CONFIG_NVIDIA_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += > nvidia_cspmu.o > > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c > b/drivers/perf/arm_cspmu/arm_cspmu.c > > index a3f1c410b417..04c318744f55 100644 > > --- a/drivers/perf/arm_cspmu/arm_cspmu.c > > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c > > @@ -16,7 +16,7 @@ > > * The user should refer to the vendor technical documentation to get > details > > * about the supported events. > > * > > - * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > + * Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > * > > */ > > > > @@ -31,7 +31,6 @@ > > #include <acpi/processor.h> > > > > #include "arm_cspmu.h" > > -#include "nvidia_cspmu.h" > > > > #define PMUNAME "arm_cspmu" > > #define DRVNAME "arm-cs-arch-pmu" > > @@ -117,9 +116,6 @@ > > */ > > #define HILOHI_MAX_POLL 1000 > > > > -/* JEDEC-assigned JEP106 identification code */ > > -#define ARM_CSPMU_IMPL_ID_NVIDIA 0x36B > > - > > static unsigned long arm_cspmu_cpuhp_state; > > > > /* > > @@ -186,16 +182,6 @@ static inline bool use_64b_counter_reg(const > struct arm_cspmu *cspmu) > > return (counter_size(cspmu) > 32); > > } > > > > -ssize_t arm_cspmu_sysfs_event_show(struct device *dev, > > - struct device_attribute *attr, char *buf) > > -{ > > - struct dev_ext_attribute *eattr = > > - container_of(attr, struct dev_ext_attribute, attr); > > - return sysfs_emit(buf, "event=0x%llx\n", > > - (unsigned long long)eattr->var); > > -} > > -EXPORT_SYMBOL_GPL(arm_cspmu_sysfs_event_show); > > - > > /* Default event list. */ > > static struct attribute *arm_cspmu_event_attrs[] = { > > ARM_CSPMU_EVENT_ATTR(cycles, > ARM_CSPMU_EVT_CYCLES_DEFAULT), > > @@ -231,16 +217,6 @@ arm_cspmu_event_attr_is_visible(struct kobject > *kobj, > > return attr->mode; > > } > > > > -ssize_t arm_cspmu_sysfs_format_show(struct device *dev, > > - struct device_attribute *attr, > > - char *buf) > > -{ > > - struct dev_ext_attribute *eattr = > > - container_of(attr, struct dev_ext_attribute, attr); > > - return sysfs_emit(buf, "%s\n", (char *)eattr->var); > > -} > > -EXPORT_SYMBOL_GPL(arm_cspmu_sysfs_format_show); > > - > > Is there a reason for moving these (other than bodging around issues > caused by the Makefile mishap above)? > The main reason is to remove backend module (nvidia_cspmu) dependency to main driver. > (also, I'm now wondering why they're exported in the first place, since > a backend module is hardly going to need to override the default > implementations with the default implementations...) My intention is to make the event and format attribute macro on the header file to be reusable for the backend module. The event/format attribute on the other PMUs is pretty generic, so I thought it would be harmless. > > > static struct attribute *arm_cspmu_format_attrs[] = { > > ARM_CSPMU_FORMAT_EVENT_ATTR, > > ARM_CSPMU_FORMAT_FILTER_ATTR, > > @@ -379,27 +355,12 @@ static struct attribute_group > arm_cspmu_cpumask_attr_group = { > > .attrs = arm_cspmu_cpumask_attrs, > > }; > > > > -struct impl_match { > > - u32 pmiidr; > > - u32 mask; > > - int (*impl_init_ops)(struct arm_cspmu *cspmu); > > -}; > > - > > -static const struct impl_match impl_match[] = { > > - { > > - .pmiidr = ARM_CSPMU_IMPL_ID_NVIDIA, > > - .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER, > > - .impl_init_ops = nv_cspmu_init_ops > > - }, > > - {} > > -}; > > - > > static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu) > > { > > - int ret; > > + int ret = 0; > > struct acpi_apmt_node *apmt_node = cspmu->apmt_node; > > struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops; > > - const struct impl_match *match = impl_match; > > + const struct arm_cspmu_impl_module *match; > > > > /* > > * Get PMU implementer and product id from APMT node. > > @@ -411,18 +372,21 @@ static int arm_cspmu_init_impl_ops(struct > arm_cspmu *cspmu) > > readl(cspmu->base0 + PMIIDR); > > > > /* Find implementer specific attribute ops. */ > > - for (; match->pmiidr; match++) { > > - const u32 mask = match->mask; > > - > > - if ((match->pmiidr & mask) == (cspmu->impl.pmiidr & mask)) { > > - ret = match->impl_init_ops(cspmu); > > - if (ret) > > - return ret; > > - > > - break; > > + match = arm_cspmu_impl_match_module(cspmu->impl.pmiidr); > > + if (match) { > > + request_module(match->name); > > Are we confident this can't deadlock when it's already in the middle of > loading the main module? > The backend module does not depend on the main driver module anymore (please see my top comment). The blocking call to request_module should be able to return. > I'm mostly just paranoid thanks to my plentiful experience of > inadvertently making driver probe routines crash, leaving some lock held > in the module loader which typically ends up in having to hard-reset the > machine :( > > > + > > + if (match->param.module && match->param.impl_init_ops) { > > The module handles can be NULL if everything is built-in, so > impl_init_ops is our only useful indicator here. > Thanks, I will update it on next version. > > + if (try_module_get(match->param.module)) { > > + cspmu->impl.module = match->param.module; > > + ret = match->param.impl_init_ops(cspmu); > > + } > > As mentioned previously, I think it might be appropriate to warn that > we're falling back to the generic implementation when we've failed to > find the ops for a match we *do* know about. > Sure, will do. > > } > > } > > > > + if (ret) > > + return ret; > > + > > /* Use default callbacks if implementer doesn't provide one. */ > > CHECK_DEFAULT_IMPL_OPS(impl_ops, get_event_attrs); > > CHECK_DEFAULT_IMPL_OPS(impl_ops, get_format_attrs); > > @@ -484,11 +448,6 @@ arm_cspmu_alloc_attr_group(struct arm_cspmu > *cspmu) > > struct attribute_group **attr_groups = NULL; > > struct device *dev = cspmu->dev; > > const struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops; > > - int ret; > > - > > - ret = arm_cspmu_init_impl_ops(cspmu); > > - if (ret) > > - return NULL; > > > > cspmu->identifier = impl_ops->get_identifier(cspmu); > > cspmu->name = impl_ops->get_name(cspmu); > > @@ -1203,11 +1162,21 @@ static int arm_cspmu_device_probe(struct > platform_device *pdev) > > if (ret) > > return ret; > > > > + ret = arm_cspmu_init_impl_ops(cspmu); > > + if (ret) > > + goto module_put; > > + > > ret = arm_cspmu_register_pmu(cspmu); > > if (ret) > > - return ret; > > + goto module_put; > > > > return 0; > > + > > +module_put: > > + if (cspmu->impl.module) > > No need to check, module_put(NULL) is valid. > Got it, will make the change on next version. > > + module_put(cspmu->impl.module); > > + > > + return ret; > > } > > > > static int arm_cspmu_device_remove(struct platform_device *pdev) > > @@ -1217,6 +1186,9 @@ static int arm_cspmu_device_remove(struct > platform_device *pdev) > > perf_pmu_unregister(&cspmu->pmu); > > cpuhp_state_remove_instance(arm_cspmu_cpuhp_state, &cspmu- > >cpuhp_node); > > > > + if (cspmu->impl.module) > > Ditto. > Got it. > > + module_put(cspmu->impl.module); > > + > > return 0; > > } > > > > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h > b/drivers/perf/arm_cspmu/arm_cspmu.h > > index 51323b175a4a..0b60165f770a 100644 > > --- a/drivers/perf/arm_cspmu/arm_cspmu.h > > +++ b/drivers/perf/arm_cspmu/arm_cspmu.h > > @@ -1,7 +1,7 @@ > > /* SPDX-License-Identifier: GPL-2.0 > > * > > * ARM CoreSight Architecture PMU driver. > > - * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > + * Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > * > > */ > > > > @@ -68,8 +68,13 @@ > > > > /* PMIIDR register field */ > > #define ARM_CSPMU_PMIIDR_IMPLEMENTER GENMASK(11, 0) > > +#define ARM_CSPMU_PMIIDR_REVISION GENMASK(15, 12) > > +#define ARM_CSPMU_PMIIDR_VARIANT GENMASK(19, 16) > > #define ARM_CSPMU_PMIIDR_PRODUCTID GENMASK(31, 20) > > > > +/* JEDEC-assigned JEP106 identification code */ > > +#define ARM_CSPMU_IMPL_ID_NVIDIA 0x36B > > + > > struct arm_cspmu; > > > > /* This tracks the events assigned to each counter in the PMU. */ > > @@ -107,10 +112,28 @@ struct arm_cspmu_impl_ops { > > struct attribute *attr, int unused); > > }; > > > > +/* Vendor/implementer registration parameter. */ > > +struct arm_cspmu_impl_param { > > + /* Backend module. */ > > + struct module *module; > > + /* PMIIDR value/mask. */ > > + u32 pmiidr_val; > > + u32 pmiidr_mask; > > + /* Callback to vendor backend to init arm_cspmu_impl::ops. */ > > + int (*impl_init_ops)(struct arm_cspmu *cspmu); > > +}; > > + > > +/* Vendor/implementer module. */ > > +struct arm_cspmu_impl_module { > > + const char *name; > > + struct arm_cspmu_impl_param param; > > +}; > > Nit: FWIW I'd just have a single flat structure and call it something > like arm_cspmu_impl_match. > Sure. > > + > > /* Vendor/implementer descriptor. */ > > struct arm_cspmu_impl { > > u32 pmiidr; > > struct arm_cspmu_impl_ops ops; > > + struct module *module; > > void *ctx; > > }; > > > > @@ -139,13 +162,31 @@ struct arm_cspmu { > > }; > > > > /* Default function to show event attribute in sysfs. */ > > -ssize_t arm_cspmu_sysfs_event_show(struct device *dev, > > - struct device_attribute *attr, > > - char *buf); > > +static inline ssize_t arm_cspmu_sysfs_event_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct dev_ext_attribute *eattr = > > + container_of(attr, struct dev_ext_attribute, attr); > > + return sysfs_emit(buf, "event=0x%llx\n", > > + (unsigned long long)eattr->var); > > +} > > > > /* Default function to show format attribute in sysfs. */ > > -ssize_t arm_cspmu_sysfs_format_show(struct device *dev, > > - struct device_attribute *attr, > > - char *buf); > > +static inline ssize_t arm_cspmu_sysfs_format_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct dev_ext_attribute *eattr = > > + container_of(attr, struct dev_ext_attribute, attr); > > + return sysfs_emit(buf, "%s\n", (char *)eattr->var); > > +} > > + > > +/* Register vendor backend. */ > > +int arm_cspmu_impl_register(const struct arm_cspmu_impl_param > *impl_param); > > + > > +/* Unregister vendor backend. */ > > +void arm_cspmu_impl_unregister(const struct arm_cspmu_impl_param > *impl_param); > > + > > +/* Get matching vendor module compatible with /p pmiidr. */ > > +const struct arm_cspmu_impl_module > *arm_cspmu_impl_match_module(u32 pmiidr); > > > > #endif /* __ARM_CSPMU_H__ */ > > diff --git a/drivers/perf/arm_cspmu/arm_cspmu_impl.c > b/drivers/perf/arm_cspmu/arm_cspmu_impl.c > > new file mode 100644 > > index 000000000000..35e0f4c2410a > > --- /dev/null > > +++ b/drivers/perf/arm_cspmu/arm_cspmu_impl.c > > @@ -0,0 +1,114 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Implementation specific backend registration. > > + * > > + * Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > + * > > + */ > > + > > +#include <linux/mutex.h> > > + > > +#include "arm_cspmu.h" > > + > > +static DEFINE_MUTEX(arm_cspmu_lock); > > + > > +static struct arm_cspmu_impl_module module_list[] = { > > + { > > + .name = "nvidia_cspmu", > > + .param = { > > + .pmiidr_val = ARM_CSPMU_IMPL_ID_NVIDIA, > > + .pmiidr_mask = ARM_CSPMU_PMIIDR_IMPLEMENTER, > > + }, > > + }, > > + {} > > +}; > > + > > +static struct arm_cspmu_impl_module *arm_cspmu_impl_find_module( > > + const struct arm_cspmu_impl_param *impl_param) > > +{ > > + struct arm_cspmu_impl_module *module; > > + > > + for (module = module_list; module->name; module++) { > > + if (!strcmp(module->name, impl_param->module->name) && > > + module->param.pmiidr_val == impl_param->pmiidr_val && > > + module->param.pmiidr_mask == impl_param->pmiidr_mask) > > + return module; > > + } > > + > > + return NULL; > > +} > > I don't think we want different logic for this - probe has to be able to > get an unambiguous match from a PMIIDR value alone, therefore it must be > sufficient for backend registration to do the same, e.g. > arm_cspmu_impl_match_module(ARM_CSPMU_IMPL_ID_NVIDIA). Haveing > different > match conditions makes it all to easy to let subtle bugs in. > Ok, it's reasonable I think. One reason I also did the name check is to make sure that it is the "correct" module. But I guess I can add a WARN_ON in case another module register an entry that was already registered. > > + > > +const struct arm_cspmu_impl_module > *arm_cspmu_impl_match_module(u32 pmiidr) > > +{ > > + struct arm_cspmu_impl_module *module; > > Nit: just initialise to NULL... Sure. > > > + mutex_lock(&arm_cspmu_lock); > > + > > + for (module = module_list; module->name; module++) { > > + u32 mask = module->param.pmiidr_mask; > > + > > + if ((module->param.pmiidr_val & mask) == (pmiidr & mask)) { > > ...and break here. Sure. > > > + mutex_unlock(&arm_cspmu_lock); > > + return module; > > + } > > + } > > + > > + mutex_unlock(&arm_cspmu_lock); > > However, this locking doesn't work anyway - module_list and the > pmiidr_{mask,val} values are static, so there is no need to protect the > match lookup itself. The potential race which does exist is between > arm_cspmu_impl_register() writing to module/impl_init_ops and > arm_cspmu_init_impl_ops() reading them, which is not covered. > Ah yes, you are right, thanks for catching this. > > + > > + return NULL; > > +} > > +EXPORT_SYMBOL_GPL(arm_cspmu_impl_match_module); > > + > > +int arm_cspmu_impl_register(const struct arm_cspmu_impl_param > *impl_param) > > Ah, I see, this explains the structure split. I reckon it would be fine > to either just pass the PMIIDR value, module handle and impl_init_ops as > explicit arguments here, or a whole dummy match template with only those > fields filled in. > Got it. > > +{ > > + struct arm_cspmu_impl_module *module; > > + int ret = 0; > > + > > + if (!impl_param->module || !impl_param->impl_init_ops) { > > Again, this module check prevents it from working with the whole driver > + backend built-in. > > > + pr_err("arm_cspmu reg failed, invalid module or init_ops\n"); > > If anything, probably just WARN_ON() impl_init_ops being NULL here, > since that would mean the backend module is fundamentally broken. > However it would effectvely already be covered if we had a more general > user warning based on not finding the ops for a known match at probe, as > suggested earlier. The corner case of loading a broken module and *not* > trying to use it doesn't seem worth bothering about. > Sure, I will add the WARN_ON and warning on falling back to generic path. > > + return -EINVAL; > > + } > > + > > + mutex_lock(&arm_cspmu_lock); > > + > > + module = arm_cspmu_impl_find_module(impl_param); > > + if (module) { > > + module->param.module = impl_param->module; > > + module->param.impl_init_ops = impl_param->impl_init_ops; > > + } else { > > I do however think it might be worth also checking if a match is found > but the ops are already set. That would mean our match table is broken > and contains a non-unique PMIIDR match, which isn't going to be easily > caught anywhere else, and the implications might be subtle and only show > up for other users later. > Agree, this condition needs to be check. > > + pr_err("arm_cspmu reg failed, unable to find pmiidr: 0x%x, mask: > 0x%x, module: %s\n", > > + impl_param->pmiidr_val, > > + impl_param->pmiidr_mask, > > + impl_param->module->name); > > + > > + ret = -EINVAL; > > + } > > + > > + > > + mutex_unlock(&arm_cspmu_lock); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(arm_cspmu_impl_register); > > + > > +void arm_cspmu_impl_unregister(const struct arm_cspmu_impl_param > *impl_param) > > +{ > > + struct arm_cspmu_impl_module *module; > > + > > + mutex_lock(&arm_cspmu_lock); > > + > > + module = arm_cspmu_impl_find_module(impl_param); > > + if (module) { > > I think it's reasonable to have a usage model where unregister should > only be called if register succeeded, and thus we can assume this lookup > never fails. That certainly fits if the expectation is that > register/unregister are tied to module_init/module_exit. > Yup, that is the expectation. It is still good to validate the module pointer right ? Or do you think it will hide a bug, if any ? > Thanks, > Robin. > > > + module->param.module = NULL; > > + module->param.impl_init_ops = NULL; > > + } else > > + pr_err("arm_cspmu unreg failed, unable to find pmiidr: 0x%x, mask: > 0x%x, module: %s\n", > > + impl_param->pmiidr_val, > > + impl_param->pmiidr_mask, > > + impl_param->module->name); > > + > > + mutex_unlock(&arm_cspmu_lock); > > +} > > +EXPORT_SYMBOL_GPL(arm_cspmu_impl_unregister); > > + > > +MODULE_LICENSE("GPL v2"); > > diff --git a/drivers/perf/arm_cspmu/nvidia_cspmu.c > b/drivers/perf/arm_cspmu/nvidia_cspmu.c > > index 72ef80caa3c8..7ac8f17de116 100644 > > --- a/drivers/perf/arm_cspmu/nvidia_cspmu.c > > +++ b/drivers/perf/arm_cspmu/nvidia_cspmu.c > > @@ -1,14 +1,15 @@ > > // SPDX-License-Identifier: GPL-2.0 > > /* > > - * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > + * Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > * > > */ > > > > /* Support for NVIDIA specific attributes. */ > > > > +#include <linux/module.h> > > #include <linux/topology.h> > > > > -#include "nvidia_cspmu.h" > > +#include "arm_cspmu.h" > > > > #define NV_PCIE_PORT_COUNT 10ULL > > #define NV_PCIE_FILTER_ID_MASK > GENMASK_ULL(NV_PCIE_PORT_COUNT - 1, 0) > > @@ -351,7 +352,7 @@ static char *nv_cspmu_format_name(const struct > arm_cspmu *cspmu, > > return name; > > } > > > > -int nv_cspmu_init_ops(struct arm_cspmu *cspmu) > > +static int nv_cspmu_init_ops(struct arm_cspmu *cspmu) > > { > > u32 prodid; > > struct nv_cspmu_ctx *ctx; > > @@ -395,6 +396,32 @@ int nv_cspmu_init_ops(struct arm_cspmu > *cspmu) > > > > return 0; > > } > > -EXPORT_SYMBOL_GPL(nv_cspmu_init_ops); > > + > > +/* Match all NVIDIA Coresight PMU devices */ > > +static const struct arm_cspmu_impl_param nv_cspmu_param = { > > + .module = THIS_MODULE, > > + .pmiidr_val = ARM_CSPMU_IMPL_ID_NVIDIA, > > + .pmiidr_mask = ARM_CSPMU_PMIIDR_IMPLEMENTER, > > + .impl_init_ops = nv_cspmu_init_ops > > +}; > > + > > +static int __init nvidia_cspmu_init(void) > > +{ > > + int ret; > > + > > + ret = arm_cspmu_impl_register(&nv_cspmu_param); > > + if (ret) > > + pr_err("nvidia_cspmu backend registration error: %d\n", ret); > > + > > + return ret; > > +} > > + > > +static void __exit nvidia_cspmu_exit(void) > > +{ > > + arm_cspmu_impl_unregister(&nv_cspmu_param); > > +} > > + > > +module_init(nvidia_cspmu_init); > > +module_exit(nvidia_cspmu_exit); > > > > MODULE_LICENSE("GPL v2"); > > diff --git a/drivers/perf/arm_cspmu/nvidia_cspmu.h > b/drivers/perf/arm_cspmu/nvidia_cspmu.h > > deleted file mode 100644 > > index 71e18f0dc50b..000000000000 > > --- a/drivers/perf/arm_cspmu/nvidia_cspmu.h > > +++ /dev/null > > @@ -1,17 +0,0 @@ > > -/* SPDX-License-Identifier: GPL-2.0 > > - * > > - * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights > reserved. > > - * > > - */ > > - > > -/* Support for NVIDIA specific attributes. */ > > - > > -#ifndef __NVIDIA_CSPMU_H__ > > -#define __NVIDIA_CSPMU_H__ > > - > > -#include "arm_cspmu.h" > > - > > -/* Allocate NVIDIA descriptor. */ > > -int nv_cspmu_init_ops(struct arm_cspmu *cspmu); > > - > > -#endif /* __NVIDIA_CSPMU_H__ */ > > > > base-commit: 145e5cddfe8b4bf607510b2dcf630d95f4db420f
