| Message ID | 1436447951-22357-5-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 9 Jul 2015, Marc Zyngier wrote: > When used as a primary interrupt controller, the GIC driver uses > irq_data->chip_data to extract controller-specific information. > > When used as a secondary interrupt controller, it uses handler_data > instead. As this difference is relatively pointless and only creates > confusion, change the secondary path to match what the rest of the > driver does. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/irqchip/irq-gic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index e264675..3c7f3a4 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) > > static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) > { > - struct gic_chip_data *chip_data = irq_get_handler_data(irq); > + struct gic_chip_data *chip_data = irq_get_chip_data(irq); > struct irq_chip *chip = irq_get_chip(irq); You should make that chip_data = irq_desc_get_chip_data(desc); chip = irq_desc_get_chip(desc); That avoids two pointless lookups of irqdesc .... Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/07/15 22:33, Thomas Gleixner wrote: > On Thu, 9 Jul 2015, Marc Zyngier wrote: > >> When used as a primary interrupt controller, the GIC driver uses >> irq_data->chip_data to extract controller-specific information. >> >> When used as a secondary interrupt controller, it uses handler_data >> instead. As this difference is relatively pointless and only creates >> confusion, change the secondary path to match what the rest of the >> driver does. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> drivers/irqchip/irq-gic.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >> index e264675..3c7f3a4 100644 >> --- a/drivers/irqchip/irq-gic.c >> +++ b/drivers/irqchip/irq-gic.c >> @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) >> >> static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) >> { >> - struct gic_chip_data *chip_data = irq_get_handler_data(irq); >> + struct gic_chip_data *chip_data = irq_get_chip_data(irq); >> struct irq_chip *chip = irq_get_chip(irq); > > You should make that > > chip_data = irq_desc_get_chip_data(desc); > chip = irq_desc_get_chip(desc); > > That avoids two pointless lookups of irqdesc .... Ah, very good point. And it turns out that these constructs (use of irq_get_* when the irq_desc is readily available) is actually fairly common in a number of irqchip implementations used as secondary interrupt controllers. Time for another cleanup series, I believe... ;-) M.
On 2015/7/10 15:52, Marc Zyngier wrote: > On 09/07/15 22:33, Thomas Gleixner wrote: >> On Thu, 9 Jul 2015, Marc Zyngier wrote: >> >>> When used as a primary interrupt controller, the GIC driver uses >>> irq_data->chip_data to extract controller-specific information. >>> >>> When used as a secondary interrupt controller, it uses handler_data >>> instead. As this difference is relatively pointless and only creates >>> confusion, change the secondary path to match what the rest of the >>> driver does. >>> >>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>> --- >>> drivers/irqchip/irq-gic.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>> index e264675..3c7f3a4 100644 >>> --- a/drivers/irqchip/irq-gic.c >>> +++ b/drivers/irqchip/irq-gic.c >>> @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) >>> >>> static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) >>> { >>> - struct gic_chip_data *chip_data = irq_get_handler_data(irq); >>> + struct gic_chip_data *chip_data = irq_get_chip_data(irq); >>> struct irq_chip *chip = irq_get_chip(irq); >> >> You should make that >> >> chip_data = irq_desc_get_chip_data(desc); >> chip = irq_desc_get_chip(desc); >> >> That avoids two pointless lookups of irqdesc .... > > Ah, very good point. > > And it turns out that these constructs (use of irq_get_* when the > irq_desc is readily available) is actually fairly common in a number of > irqchip implementations used as secondary interrupt controllers. > > Time for another cleanup series, I believe... ;-) Hi Marc, I'm ahead of your request and expect an reviewed-by from you then:). Please refer to http://lkml.org/lkml/2015/6/4/743 for what we have done to clear up this interfaces. Thanks! Gerry -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/07/15 09:17, Jiang Liu wrote: > On 2015/7/10 15:52, Marc Zyngier wrote: >> On 09/07/15 22:33, Thomas Gleixner wrote: >>> On Thu, 9 Jul 2015, Marc Zyngier wrote: >>> >>>> When used as a primary interrupt controller, the GIC driver uses >>>> irq_data->chip_data to extract controller-specific information. >>>> >>>> When used as a secondary interrupt controller, it uses handler_data >>>> instead. As this difference is relatively pointless and only creates >>>> confusion, change the secondary path to match what the rest of the >>>> driver does. >>>> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> drivers/irqchip/irq-gic.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index e264675..3c7f3a4 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) >>>> >>>> static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) >>>> { >>>> - struct gic_chip_data *chip_data = irq_get_handler_data(irq); >>>> + struct gic_chip_data *chip_data = irq_get_chip_data(irq); >>>> struct irq_chip *chip = irq_get_chip(irq); >>> >>> You should make that >>> >>> chip_data = irq_desc_get_chip_data(desc); >>> chip = irq_desc_get_chip(desc); >>> >>> That avoids two pointless lookups of irqdesc .... >> >> Ah, very good point. >> >> And it turns out that these constructs (use of irq_get_* when the >> irq_desc is readily available) is actually fairly common in a number of >> irqchip implementations used as secondary interrupt controllers. >> >> Time for another cleanup series, I believe... ;-) > Hi Marc, > I'm ahead of your request and expect an reviewed-by from > you then:). Please refer to http://lkml.org/lkml/2015/6/4/743 for > what we have done to clear up this interfaces. Ah, let me dig this now (yeah, I've been slacking off on reviewing lately...). Looks awesome! :-) Thanks! M.
Hi Marc, On 07/09/2015 03:19 PM, Marc Zyngier wrote: > When used as a primary interrupt controller, the GIC driver uses > irq_data->chip_data to extract controller-specific information. > > When used as a secondary interrupt controller, it uses handler_data > instead. As this difference is relatively pointless and only creates > confusion, change the secondary path to match what the rest of the > driver does. not really related to this series' topic, is it? Eric > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/irqchip/irq-gic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index e264675..3c7f3a4 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) > > static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) > { > - struct gic_chip_data *chip_data = irq_get_handler_data(irq); > + struct gic_chip_data *chip_data = irq_get_chip_data(irq); > struct irq_chip *chip = irq_get_chip(irq); > unsigned int cascade_irq, gic_irq; > unsigned long status; > @@ -341,7 +341,7 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq) > { > if (gic_nr >= MAX_GIC_NR) > BUG(); > - if (irq_set_handler_data(irq, &gic_data[gic_nr]) != 0) > + if (irq_set_chip_data(irq, &gic_data[gic_nr]) != 0) > BUG(); > irq_set_chained_handler(irq, gic_handle_cascade_irq); > } > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/08/15 10:45, Eric Auger wrote: > Hi Marc, > On 07/09/2015 03:19 PM, Marc Zyngier wrote: >> When used as a primary interrupt controller, the GIC driver uses >> irq_data->chip_data to extract controller-specific information. >> >> When used as a secondary interrupt controller, it uses handler_data >> instead. As this difference is relatively pointless and only creates >> confusion, change the secondary path to match what the rest of the >> driver does. > not really related to this series' topic, is it? It is. Or was. We use the handler_data field to hold the vcpu information, but that's also used by the chained interrupt handler to find out which secondary it is. But this patch is wrong anyway, as it entirely breaks secondary GICs (I found out when I merged this with a patch from Jon Hunter fixing some other related stuff). Took me hours to get an EB up and running! I have a working version now, which makes this patch irrelevant. Thanks, M.
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index e264675..3c7f3a4 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) { - struct gic_chip_data *chip_data = irq_get_handler_data(irq); + struct gic_chip_data *chip_data = irq_get_chip_data(irq); struct irq_chip *chip = irq_get_chip(irq); unsigned int cascade_irq, gic_irq; unsigned long status; @@ -341,7 +341,7 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq) { if (gic_nr >= MAX_GIC_NR) BUG(); - if (irq_set_handler_data(irq, &gic_data[gic_nr]) != 0) + if (irq_set_chip_data(irq, &gic_data[gic_nr]) != 0) BUG(); irq_set_chained_handler(irq, gic_handle_cascade_irq); }
When used as a primary interrupt controller, the GIC driver uses irq_data->chip_data to extract controller-specific information. When used as a secondary interrupt controller, it uses handler_data instead. As this difference is relatively pointless and only creates confusion, change the secondary path to match what the rest of the driver does. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- drivers/irqchip/irq-gic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)