Re: [PATCH v3] platforms/powernv: Avoid re-registration of imc debugfs directory

From: "Nageswara R Sastry" <nasastry@in.ibm.com>
To: "Anju T Sudhakar" <anju@linux.vnet.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v3] platforms/powernv: Avoid re-registration of imc debugfs directory
Date: Thu, 28 Nov 2019 15:50:36 +0530	[thread overview]
Message-ID: <OF14C9B4C3.B03A7C39-ON652584C0.0037AA8C-652584C0.0038D198@notes.na.collabserv.com> (raw)
In-Reply-To: <20191127072035.4283-1-anju@linux.vnet.ibm.com>

[-- Attachment #1: Type: text/plain, Size: 6399 bytes --]

"Anju T Sudhakar" <anju@linux.vnet.ibm.com> wrote on 27/11/2019 12:50:35
PM:

> From: "Anju T Sudhakar" <anju@linux.vnet.ibm.com>
> To: mpe@ellerman.id.au
> Cc: linuxppc-dev@lists.ozlabs.org, anju@linux.vnet.ibm.com,
> Nageswara R Sastry/India/IBM@IBMIN
> Date: 27/11/2019 12:50 PM
> Subject: [PATCH v3] platforms/powernv: Avoid re-registration of imc
> debugfs directory
>
> export_imc_mode_and_cmd() function which creates the debugfs interface
for
> imc-mode and imc-command, is invoked when each nest pmu units is
> registered.
> When the first nest pmu unit is registered, export_imc_mode_and_cmd()
> creates 'imc' directory under `/debug/powerpc/`. In the subsequent
> invocations debugfs_create_dir() function returns, since the directory
> already exists.
>
> The recent commit <c33d442328f55> (debugfs: make error message a bit more
> verbose), throws a warning if we try to invoke `debugfs_create_dir()`
> with an already existing directory name.
>
> Address this warning by making the debugfs directory registration
> in the opal_imc_counters_probe() function, i.e invoke
> export_imc_mode_and_cmd() function from the probe function.
>
> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> ---
> Changes from v2 -> v3:
>
> * Invoke export_imc_mode_and_cmd(), which does the imc debugfs
>   directory registration and deletion, from the probe fucntion.
> * Change the return type of imc_pmu_create() to get the
>   control block address for nest units in the probe
>   function
> * Remove unnecessary comments

Tested-by: Nageswara R Sastry <nasastry@in.ibm.com>

Not seeing any Kernel OOPs/warnings in dmesg
# cat /sys/kernel/debug/powerpc/imc/*
0x0000000000000000
0x0000000000000000
0x0000000000000000
0x0000000000000000


> ---
>  arch/powerpc/platforms/powernv/opal-imc.c | 39 ++++++++++++
> +------------------
>  1 file changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/opal-imc.c b/arch/
> powerpc/platforms/powernv/opal-imc.c
> index e04b206..3b4518f 100644
> --- a/arch/powerpc/platforms/powernv/opal-imc.c
> +++ b/arch/powerpc/platforms/powernv/opal-imc.c
> @@ -59,10 +59,6 @@ static void export_imc_mode_and_cmd(struct
> device_node *node,
>
>     imc_debugfs_parent = debugfs_create_dir("imc", powerpc_debugfs_root);
>
> -   /*
> -    * Return here, either because 'imc' directory already exists,
> -    * Or failed to create a new one.
> -    */
>     if (!imc_debugfs_parent)
>        return;
>
> @@ -135,7 +131,6 @@ static int imc_get_mem_addr_nest(struct device_node
*node,
>     }
>
>     pmu_ptr->imc_counter_mmaped = true;
> -   export_imc_mode_and_cmd(node, pmu_ptr);
>     kfree(base_addr_arr);
>     kfree(chipid_arr);
>     return 0;
> @@ -151,7 +146,7 @@ static int imc_get_mem_addr_nest(struct device_node
*node,
>   *          and domain as the inputs.
>   * Allocates memory for the struct imc_pmu, sets up its domain,
> size and offsets
>   */
> -static int imc_pmu_create(struct device_node *parent, int
> pmu_index, int domain)
> +static struct imc_pmu *imc_pmu_create(struct device_node *parent,
> int pmu_index, int domain)
>  {
>     int ret = 0;
>     struct imc_pmu *pmu_ptr;
> @@ -159,27 +154,23 @@ static int imc_pmu_create(struct device_node
> *parent, int pmu_index, int domain)
>
>     /* Return for unknown domain */
>     if (domain < 0)
> -      return -EINVAL;
> +      return NULL;
>
>     /* memory for pmu */
>     pmu_ptr = kzalloc(sizeof(*pmu_ptr), GFP_KERNEL);
>     if (!pmu_ptr)
> -      return -ENOMEM;
> +      return NULL;
>
>     /* Set the domain */
>     pmu_ptr->domain = domain;
>
>     ret = of_property_read_u32(parent, "size", &pmu_ptr->
counter_mem_size);
> -   if (ret) {
> -      ret = -EINVAL;
> +   if (ret)
>        goto free_pmu;
> -   }
>
>     if (!of_property_read_u32(parent, "offset", &offset)) {
> -      if (imc_get_mem_addr_nest(parent, pmu_ptr, offset)) {
> -         ret = -EINVAL;
> +      if (imc_get_mem_addr_nest(parent, pmu_ptr, offset))
>           goto free_pmu;
> -      }
>     }
>
>     /* Function to register IMC pmu */
> @@ -190,14 +181,14 @@ static int imc_pmu_create(struct device_node
> *parent, int pmu_index, int domain)
>        if (pmu_ptr->domain == IMC_DOMAIN_NEST)
>           kfree(pmu_ptr->mem_info);
>        kfree(pmu_ptr);
> -      return ret;
> +      return NULL;
>     }
>
> -   return 0;
> +   return pmu_ptr;
>
>  free_pmu:
>     kfree(pmu_ptr);
> -   return ret;
> +   return NULL;
>  }
>
>  static void disable_nest_pmu_counters(void)
> @@ -254,6 +245,7 @@ int get_max_nest_dev(void)
>  static int opal_imc_counters_probe(struct platform_device *pdev)
>  {
>     struct device_node *imc_dev = pdev->dev.of_node;
> +   struct imc_pmu *pmu;
>     int pmu_count = 0, domain;
>     bool core_imc_reg = false, thread_imc_reg = false;
>     u32 type;
> @@ -269,6 +261,7 @@ static int opal_imc_counters_probe(struct
> platform_device *pdev)
>     }
>
>     for_each_compatible_node(imc_dev, NULL, IMC_DTB_UNIT_COMPAT) {
> +      pmu = NULL;
>        if (of_property_read_u32(imc_dev, "type", &type)) {
>           pr_warn("IMC Device without type property\n");
>           continue;
> @@ -293,9 +286,13 @@ static int opal_imc_counters_probe(struct
> platform_device *pdev)
>           break;
>        }
>
> -      if (!imc_pmu_create(imc_dev, pmu_count, domain)) {
> -         if (domain == IMC_DOMAIN_NEST)
> +      pmu = imc_pmu_create(imc_dev, pmu_count, domain);
> +      if (pmu != NULL) {
> +         if (domain == IMC_DOMAIN_NEST) {
> +            if (!imc_debugfs_parent)
> +               export_imc_mode_and_cmd(imc_dev, pmu);
>              pmu_count++;
> +         }
>           if (domain == IMC_DOMAIN_CORE)
>              core_imc_reg = true;
>           if (domain == IMC_DOMAIN_THREAD)
> @@ -303,10 +300,6 @@ static int opal_imc_counters_probe(struct
> platform_device *pdev)
>        }
>     }
>
> -   /* If none of the nest units are registered, remove debugfs interface
*/
> -   if (pmu_count == 0)
> -      debugfs_remove_recursive(imc_debugfs_parent);
> -
>     /* If core imc is not registered, unregister thread-imc */
>     if (!core_imc_reg && thread_imc_reg)
>        unregister_thread_imc();
> --
> 1.8.3.1
>

[-- Attachment #2: Type: text/html, Size: 9399 bytes --]