On 2014/11/6 7:48, Thomas Gleixner wrote:

On Tue, 4 Nov 2014, Jiang Liu wrote:

quoted

 /* Number of irqs reserved for a legacy isa controller */
 #define NUM_ISA_INTERRUPTS	16

@@ -64,6 +66,16 @@ struct irq_domain_ops {
 	int (*xlate)(struct irq_domain *d, struct device_node *node,
 		     const u32 *intspec, unsigned int intsize,
 		     unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	/* extended V2 interfaces to support hierarchy irq_domains */
+	int (*alloc)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs, void *arg);
+	void (*free)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs);
+	int (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+	int (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);

Why do we have a return value here? Especially the deactivate one
makes no sense at all.

quoted

+extern int irq_domain_activate_irq(struct irq_data *irq_data);
+extern int irq_domain_deactivate_irq(struct irq_data *irq_data);

And here.

quoted

@@ -178,6 +179,7 @@ int irq_startup(struct irq_desc *desc, bool resend)
 	irq_state_clr_disabled(desc);
 	desc->depth = 0;
 
+	irq_domain_activate_irq(&desc->irq_data);

We do not check it and we cannot do here AFAICT.

quoted

 	if (desc->irq_data.chip->irq_startup) {
 		ret = desc->irq_data.chip->irq_startup(&desc->irq_data);
 		irq_state_clr_masked(desc);

@@ -199,6 +201,7 @@ void irq_shutdown(struct irq_desc *desc)
 		desc->irq_data.chip->irq_disable(&desc->irq_data);
 	else
 		desc->irq_data.chip->irq_mask(&desc->irq_data);
+	irq_domain_deactivate_irq(&desc->irq_data);

Ditto.

So the return value for irq_domain_deactivate_irq() is silly to begin
with, but also the return value for irq_domain_activate_irq() does not
really make sense. We've allocated the resources for the interrupt
already down the hierarchy chain. So there is no reason why the actual
activation should fail.

Hi Thomas,
	Fair enough, I have changed them to return void, which also
simplify the implementation. But add one or two BUG_ON()s:)
Regards!
Gerry

Thanks,

	tglx