Re: [PATCH 11/12] irqchip/mips-gic: Report that effective affinity is a single target

[Paul Burton <paul.burton@imgtec.com>]

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

Hi Marc,

On Tuesday, 15 August 2017 09:44:24 PDT Marc Zyngier wrote:
> On 15/08/17 17:23, Paul Burton wrote:
> > Hi Marc,
> > 
> > On Tuesday, 15 August 2017 04:34:46 PDT Marc Zyngier wrote:
> >> The MIPS GIC driver only targets a single CPU at a time, even if
> >> the notional affinity is wider. Let's inform the core code
> >> about this.
> >> 
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >> 
> >>  drivers/irqchip/Kconfig        | 1 +
> >>  drivers/irqchip/irq-mips-gic.c | 2 ++
> >>  2 files changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> >> index 39bfa5b25b54..bca9a88012f0 100644
> >> --- a/drivers/irqchip/Kconfig
> >> +++ b/drivers/irqchip/Kconfig
> >> @@ -141,6 +141,7 @@ config IRQ_MIPS_CPU
> >> 
> >>  	select GENERIC_IRQ_IPI if SYS_SUPPORTS_MULTITHREADING
> >>  	select IRQ_DOMAIN
> >>  	select IRQ_DOMAIN_HIERARCHY if GENERIC_IRQ_IPI
> >> 
> >> +	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
> >> 
> >>  config CLPS711X_IRQCHIP
> >>  
> >>  	bool
> >> 
> >> diff --git a/drivers/irqchip/irq-mips-gic.c
> >> b/drivers/irqchip/irq-mips-gic.c index 6ab1d3afec02..e075cb25fad6 100644
> >> --- a/drivers/irqchip/irq-mips-gic.c
> >> +++ b/drivers/irqchip/irq-mips-gic.c
> >> @@ -463,6 +463,7 @@ static int gic_set_affinity(struct irq_data *d, const
> >> struct cpumask *cpumask, set_bit(irq,
> >> pcpu_masks[cpumask_first(&tmp)].pcpu_mask);
> >> 
> >>  	cpumask_copy(irq_data_get_affinity_mask(d), cpumask);
> >> 
> >> +	irq_data_update_effective_affinity(d, cpumask);
> > 
> > This doesn't seem right - it's just setting the effective affinity to the
> > same as affinity, not taking into account CPU restrictions at all. I
> > think this> 
> > should be:
> >   irq_data_update_effective_affinity(d, cpumask_of(cpumask_first(&tmp)));
> > 
> > (or something cleaner but to that effect)
> 
> Gah, you're absolutely right (/me plugs brain back in). How about something
> like this:
> 
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index e075cb25fad6..6461380ff1a4 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -445,25 +445,27 @@ static int gic_set_affinity(struct irq_data *d, const
> struct cpumask *cpumask, unsigned int irq = GIC_HWIRQ_TO_SHARED(d->hwirq);
>  	cpumask_t	tmp = CPU_MASK_NONE;
>  	unsigned long	flags;
> -	int		i;
> +	int		i, cpu;
> 
>  	cpumask_and(&tmp, cpumask, cpu_online_mask);
>  	if (cpumask_empty(&tmp))
>  		return -EINVAL;
> 
> +	cpu = cpumask_first(&tmp);
> +
>  	/* Assumption : cpumask refers to a single CPU */
>  	spin_lock_irqsave(&gic_lock, flags);
> 
>  	/* Re-route this IRQ */
> -	gic_map_to_vpe(irq, mips_cm_vp_id(cpumask_first(&tmp)));
> +	gic_map_to_vpe(irq, mips_cm_vp_id(cpu));
> 
>  	/* Update the pcpu_masks */
>  	for (i = 0; i < min(gic_vpes, NR_CPUS); i++)
>  		clear_bit(irq, pcpu_masks[i].pcpu_mask);
> -	set_bit(irq, pcpu_masks[cpumask_first(&tmp)].pcpu_mask);
> +	set_bit(irq, pcpu_masks[cpu].pcpu_mask);
> 
>  	cpumask_copy(irq_data_get_affinity_mask(d), cpumask);
> -	irq_data_update_effective_affinity(d, cpumask);
> +	irq_data_update_effective_affinity(d, cpumask_of(cpu));
>  	spin_unlock_irqrestore(&gic_lock, flags);
> 
>  	return IRQ_SET_MASK_OK_NOCOPY;
> 
> Thanks,
> 
> 	M.

That looks better (though patch 37 of my "irqchip: mips-gic: Cleanup & 
optimisation" series would get you that cpu variable already ;) ).

Thanks,
    Paul

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