lkml.org 
[lkml]   [2015]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 5/9]powerpc/powernv: nest pmu feature detection support
From
Date
On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote:
> Patch adds a device tree function to detect the nest pmu
> support. Function will look for specific dt property "ibm,ima-chip"
> as a detection mechanism for the nest pmu.
>
> For Nest pmu, device tree will have two set of information.
> 1) Per-chip Homer address region for nest pmu counter collection area.
> 2) Supported Nest PMUs and events
What's HOMER?

>
> +static int nest_ima_detect_parse(void)
> +{
> + const __be32 *gcid;
> + const __be64 *chip_ima_reg;
> + const __be64 *chip_ima_size;
> + struct device_node *dev;
> + int rc = -EINVAL, idx;
> +
> + for_each_node_with_property(dev, "ibm,ima-chip") {
> + gcid = of_get_property(dev, "ibm,chip-id", NULL);
> + chip_ima_reg = of_get_property(dev, "reg", NULL);
> + chip_ima_size = of_get_property(dev, "size", NULL);
> + if ((!gcid) || (!chip_ima_reg) || (!chip_ima_size)) {
> + pr_err("%s: device %s missing property \n",
> + __func__, dev->full_name);
This is not a particularly informative error message. It'd be good if it
mentioned that it was for PMU.
> + return rc;
> + }
> +
> + idx = (uint32_t)be32_to_cpup(gcid);
> + p8_perchip_nest_info[idx].pbase = be64_to_cpup(chip_ima_reg);
> + p8_perchip_nest_info[idx].size = be64_to_cpup(chip_ima_size);
> + p8_perchip_nest_info[idx].vbase = (uint64_t)
> + phys_to_virt(p8_perchip_nest_info[idx].pbase);
> +
> + rc = 0;
> + }
> +
> + return rc;

I'm not sure your rc handling is correct. As I understand it:
- Start with rc = -EINVAL.
- If your first node is missing a property, return -EINVAL.
- Once your first node succeeds, set rc = 0
- If any subsequent node is missing a property, return 0.
- Return 0 if any node is successfully processed, otherwise return
-EINVAL.

If that's what you intended (especially with regards to returning 0 when
a subsequent node is missing a property), a comment explaining it would
be great.

Also, why bail out if a property is missing on any node? Why not try all
of them and see if any succeed?

> +}
> +
> static int __init nest_pmu_init(void)
> {
> int ret = 0;
> @@ -256,6 +287,12 @@ static int __init nest_pmu_init(void)
>
> cpumask_chip();
>
> + /*
> + * Detect the Nest PMU feature
> + */
> + if (nest_ima_detect_parse())
> + return 0;
> +
> return 0;
> }
Zero is returned regardless of the output of nest_ima_detect_parse. Is
that intentional? If so, do you need the 'if'?

> device_initcall(nest_pmu_init);

Regards,
Daniel Axtens

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2015-06-03 02:41    [W:0.198 / U:0.568 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site