[4/6] irqchip: GIC: Use chip_data instead of handler_data for cascaded interrupts
diff mbox

Message ID 1436447951-22357-5-git-send-email-marc.zyngier@arm.com
State New
Headers show

Commit Message

Marc Zyngier July 9, 2015, 1:19 p.m. UTC
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(-)

Comments

Thomas Gleixner July 9, 2015, 9:33 p.m. UTC | #1
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
Marc Zyngier July 10, 2015, 7:52 a.m. UTC | #2
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.
Jiang Liu July 10, 2015, 8:17 a.m. UTC | #3
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
Marc Zyngier July 10, 2015, 8:21 a.m. UTC | #4
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.
Eric Auger Aug. 11, 2015, 9:45 a.m. UTC | #5
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
Marc Zyngier Aug. 12, 2015, 1:41 p.m. UTC | #6
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.

Patch
diff mbox

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);
 }