Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller <miltonm@bga.com>]

On Apr 26, 2007, at 4:24 AM, Mohan Kumar M wrote:
> On Fri, Apr 20, 2007 at 12:45:15AM -0500, Milton Miller wrote:
[snip]
> Milton, I hope this patch meets all your requirements.

Closer, much better.
> Cc: Milton Miller <miltonm@bga.com>,
>     Michael Ellerman <michael@ellerman.id.au>
> Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/xics.c |   39 
> ++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
>
> Index: linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
> ===================================================================
> --- linux-2.6.21-rc4.orig/arch/powerpc/platforms/pseries/xics.c
> +++ linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
> @@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
[snipping]
> +	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>
> +		server = first_cpu(tmp);
> +
> +		if (server < NR_CPUS)
> +			return get_hard_smp_processor_id(server);
> +		else {
> +			if(strict_check)
> +				return (-1);

No parens around the return value.  That is, use return -1;

> +			else
> +				return default_distrib_server;
> +		}
...

> @@ -415,7 +421,10 @@ static void xics_set_affinity(unsigned i
>
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		newmask = default_distrib_server;
> +		if (cpus_equal(cpu_online_map, cpu_present_map))
> +			newmask = default_distrib_server;
> +		else
> +			newmask = default_server;
>  	} else {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>  		if (cpus_empty(tmp))
>
>
> ===================
>> ...
>>> @@ -415,7 +419,10 @@ static void xics_set_affinity(unsigned i
>> ...
>>> 	/* For the moment only implement delivery to all cpus or one cpu */
>>> 	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
>>> -		newmask = default_distrib_server;
>>> +		if (cpus_equal(cpu_online_map, cpu_present_map))
>>>
>>
>> this was supposed to be the call with strict = 1
>
> Do you mean to use 'strict_check' argument in xics_set_affinity?
> set_affinity call is declared in linux/irq.h, so if modifying
> xics_set_affinity will affect other arch's set_affinity also.

Yes.   The whole point of
> -static int get_irq_server(unsigned int virq)
> +static int get_irq_server(unsigned int virq, unsigned int 
> strict_check)
was to factor out the common code in this function.

I wasn't trying to change the prototype of xics_set_affinity.

Looking at the code a bit, I think part of the confusion is that newmask
is horribly misnamed.   Please rename it to server or irqserver.
Obtain its value by calling get_irq_server.  If the server returned
is -1 (in strict mode), don't call rtas (just return like today).
I guess a printk could be in order since the function is void, and
only root can request the change.

milton