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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Suzuki,


> -----Original Message-----
> From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Sent: Friday, August 11, 2023 6:29 AM
> To: Besar Wicaksono <bwicaksono@xxxxxxxxxx>; robin.murphy@xxxxxxx;
> ilkka@xxxxxxxxxxxxxxxxxxxxxx; 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 v5] perf: arm_cspmu: Separate Arm and vendor module
> 
> External email: Use caution opening links or attachments
> 
> 
> On 05/07/2023 11:47, 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>
> 
> Apologies for the delay.
> 
> > ---
> >
> > 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
> >
> > Changes from v3:
> >   * Move impl registration module back to main driver module.
> >   * Rebase from will (for-next/perf)
> > Thanks to Robin for the feedback.
> > v3: https://lore.kernel.org/linux-arm-kernel/20230505005956.22837-1-
> bwicaksono@xxxxxxxxxx/T/#u
> >
> > 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       |   6 +-
> >   drivers/perf/arm_cspmu/arm_cspmu.c    | 166
> +++++++++++++++++++++-----
> >   drivers/perf/arm_cspmu/arm_cspmu.h    |  24 +++-
> >   drivers/perf/arm_cspmu/nvidia_cspmu.c |  34 +++++-
> >   drivers/perf/arm_cspmu/nvidia_cspmu.h |  17 ---
> >   6 files changed, 199 insertions(+), 57 deletions(-)
> >   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 25d25ded0983..d5f787d22234 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"
> > @@ -10,3 +10,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..0309d2ff264a 100644
> > --- a/drivers/perf/arm_cspmu/Makefile
> > +++ b/drivers/perf/arm_cspmu/Makefile
> > @@ -1,6 +1,8 @@
> > -# 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_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 e2b7827c4563..04be94b4aa48 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.
> >    *
> >    */
> >
> > @@ -26,11 +26,11 @@
> >   #include <linux/interrupt.h>
> >   #include <linux/io-64-nonatomic-lo-hi.h>
> >   #include <linux/module.h>
> > +#include <linux/mutex.h>
> >   #include <linux/perf_event.h>
> >   #include <linux/platform_device.h>
> >
> >   #include "arm_cspmu.h"
> > -#include "nvidia_cspmu.h"
> >
> >   #define PMUNAME "arm_cspmu"
> >   #define DRVNAME "arm-cs-arch-pmu"
> > @@ -112,11 +112,10 @@
> >    */
> >   #define HILOHI_MAX_POLL     1000
> >
> > -/* JEDEC-assigned JEP106 identification code */
> > -#define ARM_CSPMU_IMPL_ID_NVIDIA             0x36B
> > -
> >   static unsigned long arm_cspmu_cpuhp_state;
> >
> > +static DEFINE_MUTEX(arm_cspmu_lock);
> > +
> >   static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
> >   {
> >       return *(struct acpi_apmt_node **)dev_get_platdata(dev);
> > @@ -373,27 +372,37 @@ 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[] = {
> > +static struct arm_cspmu_impl_match impl_match[] = {
> >       {
> > -       .pmiidr = ARM_CSPMU_IMPL_ID_NVIDIA,
> > -       .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
> > -       .impl_init_ops = nv_cspmu_init_ops
> > +             .module_name    = "nvidia_cspmu",
> > +             .pmiidr_val     = ARM_CSPMU_IMPL_ID_NVIDIA,
> > +             .pmiidr_mask    = ARM_CSPMU_PMIIDR_IMPLEMENTER,
> > +             .module         = NULL,
> > +             .impl_init_ops  = NULL,
> >       },
> > -     {}
> > +     {0}
> >   };
> >
> > +static struct arm_cspmu_impl_match *arm_cspmu_impl_match_get(u32
> pmiidr)
> > +{
> > +     struct arm_cspmu_impl_match *match = impl_match;
> > +
> > +     for (; match->pmiidr_val; match++) {
> > +             u32 mask = match->pmiidr_mask;
> > +
> > +             if ((match->pmiidr_val & mask) == (pmiidr & mask))
> > +                     break;
> > +     }
> > +
> > +     return match;
> > +}
> > +
> >   static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
> >   {
> > -     int ret;
> > +     int ret = 0;
> >       struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
> >       struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(cspmu-
> >dev);
> > -     const struct impl_match *match = impl_match;
> > +     struct arm_cspmu_impl_match *match;
> >
> >       /*
> >        * Get PMU implementer and product id from APMT node.
> > @@ -405,16 +414,31 @@ 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;
> > +     match = arm_cspmu_impl_match_get(cspmu->impl.pmiidr);
> > +
> > +     /* 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);
> > +                     } else {
> > +                             WARN(1, "arm_cspmu failed to get module: %s\n",
> > +                                     match->module_name);
> > +                             ret = -EINVAL;
> > +                     }
> > +             } else {
> > +                     request_module_nowait(match->module_name);
> > +                     ret = -EPROBE_DEFER;
> > +             }
> >
> > -             if ((match->pmiidr & mask) == (cspmu->impl.pmiidr & mask)) {
> > -                     ret = match->impl_init_ops(cspmu);
> > -                     if (ret)
> > -                             return ret;
> > +             mutex_unlock(&arm_cspmu_lock);
> >
> > -                     break;
> > -             }
> > +             if (ret)
> > +                     return ret;
> >       }
> >
> >       /* Use default callbacks if implementer doesn't provide one. */
> > @@ -478,11 +502,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);
> > @@ -1129,6 +1148,11 @@ static int arm_cspmu_get_cpus(struct
> arm_cspmu *cspmu)
> >       return arm_cspmu_acpi_get_cpus(cspmu);
> >   }
> >
> > +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),
> 
> We are accessing the module field without the mutex and the
> impl.match->module could be reset ?


Good catch, it will be fixed on v6.


> 
> >               .pmu_enable     = arm_cspmu_enable,
> >               .pmu_disable    = arm_cspmu_disable,
> >               .event_init     = arm_cspmu_event_init,
> > @@ -1196,6 +1220,10 @@ static int arm_cspmu_device_probe(struct
> platform_device *pdev)
> >       if (ret)
> >               return ret;
> >
> > +     ret = arm_cspmu_init_impl_ops(cspmu);
> > +     if (ret)
> > +             return ret;
> > +
> >       ret = arm_cspmu_register_pmu(cspmu);
> >       if (ret)
> >               return ret;
> > @@ -1300,6 +1328,80 @@ static void __exit arm_cspmu_exit(void)
> >       cpuhp_remove_multi_state(arm_cspmu_cpuhp_state);
> >   }
> >
> > +int arm_cspmu_impl_register(const struct arm_cspmu_impl_match
> *impl_match)
> > +{
> > +     struct arm_cspmu_impl_match *match;
> > +     int ret = 0;
> > +
> > +     match = arm_cspmu_impl_match_get(impl_match->pmiidr_val);
> > +
> > +     if (match) {
> > +             mutex_lock(&arm_cspmu_lock);
> > +
> > +             if (!match->impl_init_ops) {
> > +                     match->module = impl_match->module;
> > +                     match->impl_init_ops = impl_match->impl_init_ops;
> > +             } else {
> > +                     /* Broken match table may contain non-unique entries */
> > +                     WARN(1, "arm_cspmu backend already registered for module:
> %s, pmiidr: 0x%x, mask: 0x%x\n",
> > +                             match->module_name,
> > +                             match->pmiidr_val,
> > +                             match->pmiidr_mask);
> > +
> > +                     ret = -EINVAL;
> > +             }
> > +
> > +             mutex_unlock(&arm_cspmu_lock);
> > +
> > +             if (!ret)
> > +                     ret = driver_attach(&arm_cspmu_driver.driver);
> > +     } else {
> > +             pr_err("arm_cspmu reg failed, unable to find a match for pmiidr:
> 0x%x\n",
> > +                     impl_match->pmiidr_val);
> > +
> > +             ret = -EINVAL;
> > +     }
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(arm_cspmu_impl_register);
> > +
> > +static int arm_cspmu_match_device(struct device *dev, const void *match)
> > +{
> > +     struct arm_cspmu *cspmu =
> platform_get_drvdata(to_platform_device(dev));
> > +
> > +     return (cspmu && cspmu->impl.match == match) ? 1 : 0;
> > +}
> > +
> > +void arm_cspmu_impl_unregister(const struct arm_cspmu_impl_match
> *impl_match)
> > +{
> > +     struct device *dev;
> > +     struct arm_cspmu_impl_match *match;
> > +
> > +     match = arm_cspmu_impl_match_get(impl_match->pmiidr_val);
> > +
> > +     WARN_ON(!match);
> > +
> > +     if (match) {
> > +             /* Unbind the driver from all matching backend devices. */
> > +dev_release:
> > +             dev = driver_find_device(&arm_cspmu_driver.driver, NULL,
> > +                     match, arm_cspmu_match_device);
> > +             if (dev) {
> > +                     device_release_driver(dev);
> > +                     goto dev_release;
> > +             }
> 
> minor nit: We could simply do :
> 
> static int arm_cspmu_release_driver(struct device *dev, void *data)
> {
>         struct arm_cspmu *cspmu =
> platform_get_drvdata(to_platform_device(dev));
> 
>         if (cspmu && cspmu->impl.match == match)
>                 device_release_driver(dev);
>         return 0;
> }
> 
>                 ret = driver_for_each_device(&driver, NULL, match,
> arm_csmpu_release_driver);
> 

It doesn’t seem to work for me.
Is it safe to release while iterating via driver_for_each_device ?

Regards,
Besar




[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux