Re: [PATCH 08/10] powerpc/xive: take into account '/ibm,plat-res-int-priorities'

[David Gibson <david@gibson.dropbear.id.au>]

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

On Wed, Aug 09, 2017 at 09:14:49AM +0200, Cédric Le Goater wrote:
> On 08/09/2017 06:02 AM, David Gibson wrote:
> > On Tue, Aug 08, 2017 at 10:56:18AM +0200, Cédric Le Goater wrote:
> >> '/ibm,plat-res-int-priorities' contains a list of priorities that the
> >> hypervisor has reserved for its own use. Scan these ranges to choose
> >> the lowest unused priority for the xive spapr backend.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  arch/powerpc/sysdev/xive/spapr.c | 62 +++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 61 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
> >> index 7fc40047c23d..220331986bd8 100644
> >> --- a/arch/powerpc/sysdev/xive/spapr.c
> >> +++ b/arch/powerpc/sysdev/xive/spapr.c
> >> @@ -532,13 +532,70 @@ static const struct xive_ops xive_spapr_ops = {
> >>  	.name			= "spapr",
> >>  };
> >>  
> >> +/*
> >> + * get max priority from "/ibm,plat-res-int-priorities"
> >> + */
> >> +static bool xive_get_max_prio(u8 *max_prio)
> >> +{
> >> +	struct device_node *rootdn;
> >> +	const __be32 *reg;
> >> +	u32 len;
> >> +	int prio, found;
> >> +
> >> +	rootdn = of_find_node_by_path("/");
> >> +	if (!rootdn) {
> >> +		pr_err("not root node found !\n");
> >> +		return false;
> >> +	}
> >> +
> >> +	reg = of_get_property(rootdn, "ibm,plat-res-int-priorities", &len);
> >> +	if (!reg) {
> >> +		pr_err("Failed to read 'ibm,plat-res-int-priorities' property\n");
> >> +		return false;
> >> +	}
> >> +
> >> +	if (len % (2 * sizeof(u32)) != 0) {
> >> +		pr_err("invalid 'ibm,plat-res-int-priorities' property\n");
> >> +		return false;
> >> +	}
> >> +
> >> +	/* HW supports priorities in the range [0-7] and 0xFF is a
> >> +	 * wildcard priority used to mask. We scan the ranges reserved
> >> +	 * by the hypervisor to find the lowest priority we can use.
> >> +	 */
> >> +	found = 0xFF;
> >> +	for (prio = 0; prio < 8; prio++) {
> >> +		int reserved = 0;
> >> +		int i;
> >> +
> >> +		for (i = 0; i < len / (2 * sizeof(u32)); i++) {
> >> +			int base  = be32_to_cpu(reg[2 * i]);
> >> +			int range = be32_to_cpu(reg[2 * i + 1]);
> >> +
> >> +			if (prio >= base && prio < base + range)
> >> +				reserved++;
> >> +		}
> >> +
> >> +		if (!reserved)
> >> +			found = prio;
> > 
> > So you continue the loop here, rather than using break.  Which means
> > found will be the highest valued priority that's not reserved.  Is
> > that what you intended?  The commit message says you find the lowest
> > unused, but do lower numbers mean higher priorities or the other way around?
> 
> yes. I should probably add a statement on how the priorities are 
> ordered : the most privileged is the lowest value.

Ok.  My inclination would be to reverse the order of the loop, and
break; on the first (==lowest priority) unused entry.  But you could
fairly argue that's premature optimization.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]