Re: [PATCH -v2 2/2] genericirq: change ack/mask in irq_chip to take irq_desc instead of irq

[ebiederm@xmission.com (Eric W. Biederman)]

Yinghai Lu <yinghai@kernel.org> writes:

> will have
> void            (*ack)(struct irq_desc *desc);
> void            (*mask)(struct irq_desc *desc);
> void            (*mask_ack)(struct irq_desc *desc);
> void            (*unmask)(struct irq_desc *desc);
> void            (*eoi)(struct irq_desc *desc);
>
> so for sparseirq with raidix tree, we don't call extra irq_to_desc, and could use desc directly

Overall this looks pretty decent.  This look pretty complete.
How many platforms did you manage to compile test this on?

I have found a couple of issues (see below).

A few times you change a bit more than is necessary which is a bit
spooky in a patch this far ranging.

Reading through this patch to review it took an uncomfortably long
time.


> -v2: change all member of irq_chip to use desc only.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

> Index: linux-2.6/arch/alpha/kernel/sys_rx164.c
> ===================================================================
> --- linux-2.6.orig/arch/alpha/kernel/sys_rx164.c
> +++ linux-2.6/arch/alpha/kernel/sys_rx164.c
>  static void
> -rx164_end_irq(unsigned int irq)
> +rx164_end_irq(struct irq_desc *desc)
>  {
> -	if (!(irq_desc[irq].status & (IRQ_DISABLED|IRQ_INPROGRESS)))
> -		rx164_enable_irq(irq);
> +	if (!(desc_.status & (IRQ_DISABLED|IRQ_INPROGRESS)))
> +		rx164_enable_irq(desc);
>  }

You have mispelled desc-> here.

> Index: linux-2.6/arch/arm/mach-s3c2410/bast-irq.c
> ===================================================================
> --- linux-2.6.orig/arch/arm/mach-s3c2410/bast-irq.c
> +++ linux-2.6/arch/arm/mach-s3c2410/bast-irq.c
> @@ -75,31 +75,31 @@ static unsigned char bast_pc104_irqmasks
>  
>  static void
> -bast_pc104_maskack(unsigned int irqno)
> +bast_pc104_maskack(struct irq_desc *desc)
>  {
> -	struct irq_desc *desc = irq_desc + IRQ_ISA;
> -
> -	bast_pc104_mask(irqno);
> -	desc->chip->ack(IRQ_ISA);
> +	bast_pc104_mask(desc);
> +	desc->chip->ack(desc);
>  }

This is not a trivial 1 to 1 conversion.  IRQ_ISA != irqno.
I don't know if the original code is broken here or
just very peculiar.

>  static struct irq_chip s3c_irq_cam = {
> Index: linux-2.6/arch/arm/plat-s3c64xx/irq-eint.c
> ===================================================================
> --- linux-2.6.orig/arch/arm/plat-s3c64xx/irq-eint.c
> +++ linux-2.6/arch/arm/plat-s3c64xx/irq-eint.c

> -static void s3c_irq_eint_maskack(unsigned int irq)
> +static void s3c_irq_eint_maskack(struct irq_desc *desc)
>  {
> +	unsigned int irq = desc->irq;
>  	/* compiler should in-line these */
> -	s3c_irq_eint_mask(irq);
> -	s3c_irq_eint_ack(irq);
> +	s3c_irq_eint_mask(irq, desc);
> +	s3c_irq_eint_ack(irq, desc);
>  }

It appears this function has the wrong conversion.

> Index: linux-2.6/arch/blackfin/mach-common/ints-priority.c
> ===================================================================
> --- linux-2.6.orig/arch/blackfin/mach-common/ints-priority.c
> +++ linux-2.6/arch/blackfin/mach-common/ints-priority.c
> @@ -925,7 +938,7 @@ static void bfin_demux_gpio_irq(unsigned
>  static struct irq_chip bfin_gpio_irqchip = {
>  	.name = "GPIO",
>  	.ack = bfin_gpio_ack_irq,
> -	.mask = bfin_gpio_mask_irq,
> +	.mask = bfin_gpio_mask_irq_desc,

You have not introduced bfin_gpio_mask_irq_desc ...

>  	.mask_ack = bfin_gpio_mask_ack_irq,
>  	.unmask = bfin_gpio_unmask_irq,
>  	.disable = bfin_gpio_mask_irq,

> Index: linux-2.6/arch/ia64/kernel/iosapic.c
> ===================================================================
> --- linux-2.6.orig/arch/ia64/kernel/iosapic.c
> +++ linux-2.6/arch/ia64/kernel/iosapic.c
> @@ -267,6 +267,10 @@ static void
>  nop (unsigned int irq)
>  {
>  	/* do nothing... */
> +static void
> +nop_desc(struct irq_desc *desc)
> +{
> +	/* do nothing... */
>  }

Two things here.  
- nop has no closing brace.
- You should just be able to convert nop here, and not
  need to introduce nop_desc.

>  #define iosapic_shutdown_level_irq	mask_irq
>  #define iosapic_enable_level_irq	unmask_irq
>  #define iosapic_disable_level_irq	mask_irq
> -#define iosapic_ack_level_irq		nop
> +#define iosapic_ack_level_irq		nop_desc

Making this change unnecessary.

>  #define iosapic_enable_edge_irq		unmask_irq
>  #define iosapic_disable_edge_irq	nop
> -#define iosapic_end_edge_irq		nop
> +#define iosapic_end_edge_irq		nop_desc

and this change unnecessary.

>  
>  static struct irq_chip irq_type_iosapic_edge = {
>  	.name =		"IO-SAPIC-edge",

> Index: linux-2.6/arch/m68knommu/platform/coldfire/intc-simr.c
> ===================================================================
> --- linux-2.6.orig/arch/m68knommu/platform/coldfire/intc-simr.c
> +++ linux-2.6/arch/m68knommu/platform/coldfire/intc-simr.c
> @@ -40,6 +42,7 @@ static void intc_irq_unmask(unsigned int
>  
>  static int intc_irq_set_type(unsigned int irq, unsigned int type)
>  {
> +	unsigned int irq = desc->irq;

This function does not take a desc argument.

>  	if (irq >= MCFINT_VECBASE) {
>  		if (irq < MCFINT_VECBASE + 64)
>  			__raw_writeb(5, MCFINTC0_ICR0 + irq - MCFINT_VECBASE);

> Index: linux-2.6/arch/mips/alchemy/common/irq.c
> ===================================================================
> --- linux-2.6.orig/arch/mips/alchemy/common/irq.c
> +++ linux-2.6/arch/mips/alchemy/common/irq.c
> @@ -288,17 +288,19 @@ void restore_au1xxx_intctl(void)
>  #endif /* CONFIG_PM */
>  
>  
> -static void au1x_ic0_unmask(unsigned int irq_nr)
> +static void au1x_ic0_unmask(struct irq_desc *desc)
>  {
> -	unsigned int bit = irq_nr - AU1000_INTC0_INT_BASE;
> +	unsigned int irq = desc->irq;
> +	unsigned int bit = irq_ - AU1000_INTC0_INT_BASE;

You misspelled irq there.

>  	au_writel(1 << bit, IC0_MASKSET);
>  	au_writel(1 << bit, IC0_WAKESET);
>  	au_sync();
>  }
>  

> Index: linux-2.6/arch/mips/bcm63xx/irq.c
> ===================================================================
> --- linux-2.6.orig/arch/mips/bcm63xx/irq.c
> +++ linux-2.6/arch/mips/bcm63xx/irq.c
> @@ -75,8 +75,9 @@ asmlinkage void plat_irq_dispatch(void)
>   * internal IRQs operations: only mask/unmask on PERF irq mask
>   * register.
>   */
> -static inline void bcm63xx_internal_irq_mask(unsigned int irq)
> +static inline void bcm63xx_internal_irq_mask(struct irq_desc *desc)
>  {
> +	unsigned int irq = desc->irq;
>  	u32 mask;
>  
>  	irq -= IRQ_INTERNAL_BASE;
> @@ -84,9 +85,15 @@ static inline void bcm63xx_internal_irq_
>  	mask &= ~(1 << irq);
>  	bcm_perf_writel(mask, PERF_IRQMASK_REG);
>  }
> +static void
> +bcm63xx_internal_irq_mask_desc(struct irq_desc *desc)
> +{
> +	bcm63xx_internal_irq_mask(desc);
> +}

You have converted bcm63xx_internal_irq_mask making bcm63xx_internal_irq_mask_desc
unnecessary.

> @@ -213,8 +225,8 @@ static struct irq_chip bcm63xx_internal_
>  	.startup	= bcm63xx_internal_irq_startup,
>  	.shutdown	= bcm63xx_internal_irq_mask,
>  
> -	.mask		= bcm63xx_internal_irq_mask,
> -	.mask_ack	= bcm63xx_internal_irq_mask,
> +	.mask		= bcm63xx_internal_irq_mask_desc,
> +	.mask_ack	= bcm63xx_internal_irq_mask_desc,
>  	.unmask		= bcm63xx_internal_irq_unmask,
>  };

These changes are also unnecessary.

> Index: linux-2.6/arch/mips/sni/pcimt.c
> ===================================================================
> --- linux-2.6.orig/arch/mips/sni/pcimt.c
> +++ linux-2.6/arch/mips/sni/pcimt.c

> -static void end_pcimt_irq(unsigned int irq)
> -{
> -	if (!(irq_desc[irq].status & (IRQ_DISABLED|IRQ_INPROGRESS)))
> -		enable_pcimt_irq(irq);
> +static void end_pcimt_irq(struct irq_desc *desc)
> +
> +	if (!(desc->status & (IRQ_DISABLED|IRQ_INPROGRESS)))
> +		enable_pcimt_irq(desc);
>  }

You have deleted the starting brace of the function....

> Index: linux-2.6/arch/powerpc/platforms/cell/axon_msi.c
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/platforms/cell/axon_msi.c
> +++ linux-2.6/arch/powerpc/platforms/cell/axon_msi.c
> @@ -308,10 +308,15 @@ static void axon_msi_teardown_msi_irqs(s
>  	}
>  }
>  
> +static void axon_unmask_msi_irq(struct irq_desc *desc)
> +{
> +	unmask_msi_irq(desc);
> +}
> +
>  static struct irq_chip msic_irq_chip = {
>  	.mask		= mask_msi_irq,
>  	.unmask		= unmask_msi_irq,
> -	.shutdown	= unmask_msi_irq,
> +	.shutdown	= axon_unmask_msi_irq,
>  	.name		= "AXON-MSI",
>  };

This pair of changes is unnecessary.


> Index: linux-2.6/arch/powerpc/sysdev/mpic_u3msi.c
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/sysdev/mpic_u3msi.c
> +++ linux-2.6/arch/powerpc/sysdev/mpic_u3msi.c
> @@ -23,20 +23,25 @@
>  /* A bit ugly, can we get this from the pci_dev somehow? */
>  static struct mpic *msi_mpic;
>  
> -static void mpic_u3msi_mask_irq(unsigned int irq)
> +static void mpic_u3msi_mask_irq(struct irq_desc *desc)
>  {
> -	mask_msi_irq(irq);
> -	mpic_mask_irq(irq);
> +	mask_msi_irq(desc);
> +	mpic_mask_irq(desc);
>  }
>  
> -static void mpic_u3msi_unmask_irq(unsigned int irq)
> +static void mpic_u3msi_shutdown_irq(struct irq_desc *desc)
>  {
> -	mpic_unmask_irq(irq);
> -	unmask_msi_irq(irq);
> +	mpic_u3msi_mask_irq(desc);
> +}
> +
> +static void mpic_u3msi_unmask_irq(struct irq_desc *desc)
> +{
> +	mpic_unmask_irq(desc);
> +	unmask_msi_irq(desc);
>  }
>  
>  static struct irq_chip mpic_u3msi_chip = {
> -	.shutdown	= mpic_u3msi_mask_irq,
> +	.shutdown	= mpic_u3msi_shutdown_irq,
>  	.mask		= mpic_u3msi_mask_irq,
>  	.unmask		= mpic_u3msi_unmask_irq,
>  	.eoi		= mpic_end_irq,

This change to add a new mpic_u3msi_shutdow_irq appears harmless, but
it is confusing and unnecessary.


> Index: linux-2.6/arch/sparc/kernel/pci_msi.c
> ===================================================================
> --- linux-2.6.orig/arch/sparc/kernel/pci_msi.c
> +++ linux-2.6/arch/sparc/kernel/pci_msi.c
> @@ -111,12 +111,21 @@ static void free_msi(struct pci_pbm_info
>  	clear_bit(msi_num, pbm->msi_bitmap);
>  }
>  
> +static void sparc64_enable_msi_irq(struct irq_desc *desc)
> +{
> +	unmask_msi_irq(desc);
> +}
> +static void sparc64_diable_msi_irq(struct irq_desc *desc)
> +{
> +	mask_msi_irq(desc);
> +}
> +
>  static struct irq_chip msi_irq = {
>  	.name		= "PCI-MSI",
>  	.mask		= mask_msi_irq,
>  	.unmask		= unmask_msi_irq,
> -	.enable		= unmask_msi_irq,
> -	.disable	= mask_msi_irq,
> +	.enable		= sparc64_enable_msi_irq,
> +	.disable	= sparc64_disable_msi_irq,
>  	/* XXX affinity XXX */
>  };

Ouch! this hunk is unnecessary.

> Index: linux-2.6/drivers/xen/events.c
> ===================================================================
> --- linux-2.6.orig/drivers/xen/events.c
> +++ linux-2.6/drivers/xen/events.c
> @@ -748,35 +748,46 @@ int resend_irq_on_evtchn(unsigned int ir
>  	return 1;
>  }
>  
> -static void enable_dynirq(unsigned int irq)
> +static void enable_dynirq(struct irq_desc *desc)
>  {
> -	int evtchn = evtchn_from_irq(irq);
> +	int evtchn = evtchn_from_irq(desc->irq);
>  
>  	if (VALID_EVTCHN(evtchn))
>  		unmask_evtchn(evtchn);
>  }
>  
> -static void disable_dynirq(unsigned int irq)
> +static void unmask_dynirq(struct irq_desc *desc)
>  {
> -	int evtchn = evtchn_from_irq(irq);
> +	enable_dynirq(desc);
> +}
> +

Introducing disable_dynirq, and  mask_dynirq for xen is unnecessary.

> +static void disable_dynirq(struct irq_desc *desc)
> +{
> +	int evtchn = evtchn_from_irq(desc->irq);
>  
>  	if (VALID_EVTCHN(evtchn))
>  		mask_evtchn(evtchn);
>  }
>  
> -static void ack_dynirq(unsigned int irq)
> +static void mask_dynirq(struct irq_desc *desc)
>  {
> -	int evtchn = evtchn_from_irq(irq);
> +	disable_dynirq(desc);
> +}
> +
> +static void ack_dynirq(struct irq_desc *desc)
> +{
> +	int evtchn = evtchn_from_irq(desc->irq);
>  
> -	move_native_irq(irq);
> +	move_native_irq(desc);
>  
>  	if (VALID_EVTCHN(evtchn))
>  		clear_evtchn(evtchn);
>  }
>  
> -static int retrigger_dynirq(unsigned int irq)
> +static int retrigger_dynirq(struct irq_desc *desc)
>  {
> -	int evtchn = evtchn_from_irq(irq);
> +	int evtchn = evtchn_from_irq(desc->irq);
>  	struct shared_info *sh = HYPERVISOR_shared_info;
>  	int ret = 0;
>  
> @@ -924,10 +935,11 @@ static struct irq_chip xen_dynamic_chip
>  	.name		= "xen-dyn",
>  
>  	.disable	= disable_dynirq,
> -	.mask		= disable_dynirq,
> -	.unmask		= enable_dynirq,
>  
> +	.mask		= mask_dynirq,
> +	.unmask		= unmask_dynirq,
>  	.ack		= ack_dynirq,
> +
>  	.set_affinity	= set_affinity_irq,
>  	.retrigger	= retrigger_dynirq,
>  };


> Index: linux-2.6/include/linux/htirq.h
> ===================================================================
> --- linux-2.6.orig/include/linux/htirq.h
> +++ linux-2.6/include/linux/htirq.h
> @@ -7,16 +7,17 @@ struct ht_irq_msg {
>  };
>  
>  /* Helper functions.. */
> -void fetch_ht_irq_msg(unsigned int irq, struct ht_irq_msg *msg);
> -void write_ht_irq_msg(unsigned int irq, struct ht_irq_msg *msg);
> -void mask_ht_irq(unsigned int irq);
> -void unmask_ht_irq(unsigned int irq);
> +struct irq_desc;
> +void fetch_ht_irq_msg(struct irq_desc *desc, struct ht_irq_msg *msg);
> +void write_ht_irq_msg(struct irq_desc *desc, struct ht_irq_msg *msg);
> +void mask_ht_irq(struct irq_desc *);
> +void unmask_ht_irq(struct irq_desc *);
>  
>  /* The arch hook for getting things started */
>  int arch_setup_ht_irq(unsigned int irq, struct pci_dev *dev);
>  
>  /* For drivers of buggy hardware */
> -typedef void (ht_irq_update_t)(struct pci_dev *dev, int irq,
> +typedef void (ht_irq_update_t)(struct pci_dev *dev, struct irq_desc *desc,
>  			       struct ht_irq_msg *msg);

The corresponding update to drivers/infiniband/hw/ipath/ipath_iba6110.c
is missing from your patch.

>  int __ht_create_irq(struct pci_dev *dev, int idx, ht_irq_update_t *update);

> Index: linux-2.6/kernel/irq/chip.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq/chip.c
> +++ linux-2.6/kernel/irq/chip.c
> @@ -131,7 +131,7 @@ int set_irq_chip(unsigned int irq, struc
>  	unsigned long flags;
>  
>  	if (!desc) {
> -		WARN(1, KERN_ERR "Trying to install chip for IRQ%d\n", irq);
> +		WARN(1, KERN_ERR "Trying to install chip for IRQ\n");
>  		return -EINVAL;

We still have an irq value here so this change is wrong.

> @@ -657,7 +651,7 @@ __set_irq_handler(unsigned int irq, irq_
>  
>  	if (!desc) {
>  		printk(KERN_ERR
> -		       "Trying to install type control for IRQ%d\n", irq);
> +		       "Trying to install type control for IRQ\n");

Again we still have an irq value here, so we should not remove it from
the printk.