From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S933412AbcJMQfI (ORCPT <rfc822;w@1wt.eu>);
        Thu, 13 Oct 2016 12:35:08 -0400
Received: from foss.arm.com ([217.140.101.70]:52564 "EHLO foss.arm.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1755258AbcJMQZA (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 13 Oct 2016 12:25:00 -0400
Date: Thu, 13 Oct 2016 16:31:40 +0100
From: Marc Zyngier <marc.zyngier@arm.com>
To: Cheng Chao <cs.os.kernel@gmail.com>
Cc: tglx@linutronix.de, jason@lakedaemon.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] irqchip/gic: Enable gic_set_affinity set more than one
 cpu
Message-ID: <20161013163140.5f23abce@arm.com>
In-Reply-To: <1476356234-7570-1-git-send-email-cs.os.kernel@gmail.com>
References: <1476356234-7570-1-git-send-email-cs.os.kernel@gmail.com>
Organization: ARM Ltd
X-Mailer: Claws Mail 3.14.0 (GTK+ 2.24.30; arm-unknown-linux-gnueabihf)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, 13 Oct 2016 18:57:14 +0800
Cheng Chao <cs.os.kernel@gmail.com> wrote:

> GIC can distribute an interrupt to more than one cpu,
> but now, gic_set_affinity sets only one cpu to handle interrupt.

What makes you think this is a good idea? What purpose does it serves?
I can only see drawbacks to this: You're waking up more than one CPU,
wasting power, adding jitter and clobbering the cache.

I assume you see a benefit to that approach, so can you please spell it
out?

> 
> Signed-off-by: Cheng Chao <cs.os.kernel@gmail.com>
> ---
>  drivers/irqchip/irq-gic.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 58e5b4e..198d33f 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -328,18 +328,38 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>  	unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
>  	u32 val, mask, bit;
>  	unsigned long flags;
> +	u32 valid_mask;
>  
> -	if (!force)
> -		cpu = cpumask_any_and(mask_val, cpu_online_mask);
> -	else
> +	if (!force) {
> +		valid_mask = cpumask_bits(mask_val)[0];
> +		valid_mask &= cpumask_bits(cpu_online_mask)[0];
> +
> +		cpu = cpumask_any((struct cpumask *)&valid_mask);

What is wrong with with cpumask_any_and?

> +	} else {
>  		cpu = cpumask_first(mask_val);
> +	}
>  
>  	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
>  		return -EINVAL;
>  
>  	gic_lock_irqsave(flags);
>  	mask = 0xff << shift;
> -	bit = gic_cpu_map[cpu] << shift;
> +
> +	if (!force) {
> +		bit = 0;
> +
> +		for_each_cpu(cpu, (struct cpumask *)&valid_mask) {
> +			if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
> +				break;

Shouldn't that be an error?

> +
> +			bit |= gic_cpu_map[cpu];
> +		}
> +
> +		bit = bit << shift;
> +	} else {
> +		bit = gic_cpu_map[cpu] << shift;
> +	}
> +
>  	val = readl_relaxed(reg) & ~mask;
>  	writel_relaxed(val | bit, reg);
>  	gic_unlock_irqrestore(flags);

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.