Re: [PATCH v1 5/9]powerpc/powernv: nest pmu feature detection support

[Daniel Axtens <dja@axtens.net>]

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

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]