Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller <miltonm@bga.com>]

On Jun 4, 2007, at 5:54 AM, Mohan Kumar M wrote:

> Milton,
>
> How about this patch?

I'd make a few minor changes (see below), but I'll ack it.

I've checked it compiles and boots but not the server selection.

> =============================================================
> In some of the PPC970 based systems, interrupt would be distributed to
> offline cpus also even when booted with "maxcpus=1". So check whether
> cpu online map and cpu present map are equal or not. If they are equal
> default_distrib_server is used as interrupt server otherwise boot cpu
> (default_server) used as interrupt server.
>
> In addition to this, if an interrupt is assigned to a specific cpu (ie
> smp affinity) and if that cpu is not online, the earlier code used to
> return the default_distrib_server as interrupt server. This patch
> introduces an additional paramter to the get_irq function ie
> strict_check, based on this parameter, if the cpu is not online either
> default_distrib_server or -1 is returned.
>
> Cc: Milton Miller <miltonm@bga.com>,
>     Michael Ellerman <michael@ellerman.id.au>
> Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>

Acked-By: Milton Miller <miltonm@bga.com>

> @@ -166,22 +166,24 @@ static int get_irq_server(unsigned int v
>  	if (!distribute_irqs)
>  		return default_server;
>
> -	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		server = default_distrib_server;
> -	} else {
> +	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>
> -		if (cpus_empty(tmp))
> -			server = default_distrib_server;
> -		else
> -			server = get_hard_smp_processor_id(first_cpu(tmp));
> -	}
> +		server = first_cpu(tmp);
>
> -	return server;
> +		if (server < NR_CPUS)
> +			return get_hard_smp_processor_id(server);
> +		else if (strict_check)
> +				return -1;

The indent on the return is 1 tab too many.
See next comment.

> +	}
>
> +	if (cpus_equal(cpu_online_map, cpu_present_map))
> +		return default_distrib_server;
> +	else
> +		return default_server;

I'd actually remove both occurrences of "else" bringing this and the 
previous return back a tabstop.  The else is unnecessary because the if 
statement is return .  (Seperate the new if from the previous with a 
blank line in both cases).

>  }
>  #else
> -static int get_irq_server(unsigned int virq)
> +static int get_irq_server(unsigned int virq, unsigned int 
> strict_check)
>  {
>  	return default_server;
>  }