[RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Kenji Kaneshige]

Hi Jes-san, Russ-san, Christoph-san and SN platform developers,

I glanced over the SN code, and I found SN platform doesn't have SN
specific irq_to_vector() function, though it has SN specific
local_vector_to_irq() function. I guess it was OK because maybe SN
platform depends on irq = vector and old generic irq_to_vector() was
just returning the same value as irq.

The irq_to_vector() function, however, has been changed by vector
domain support, and currently it can return the different value from
irq, even when VECTOR_DOMAIN_PERCPU is not enabled. This can happen
when an interrupt thorough I/O SAPIC is unregistered and another
interupt is registered. So if my guess (SN platform depends on irq =
vector) is true, something problem might happen on SN platform when
PCI hotplugging or driver reloading.

If it is true that SN platform depends on irq = vector, we need to
implement SN specific irq_to_vector() that returns the same value as
irq, I think. I made sample patches to implement platform specific
irq_to_vector() as follows. Note that [PATCH 2/2] is not tested yet
because I don't have any SN platform unfortunately.

    [PATCH 1/2] Add base support for platform_irq_to_vector()
    [PATCH 2/2] Implement platform_irq_to_vector() for SN

Could you give me comments?

Thanks,
Kenji Kaneshige

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Jes Sorensen]

Hi Kenji-san

I believe we need this property, but I am not 100% sure, so I am
going to forward your patch to our team who maintain the IRQ code
for Altix.

Mike, Aaron or John, would one of you have time to take a look at
this?

Thanks,
Jes


Kenji Kaneshige wrote:
> Hi Jes-san, Russ-san, Christoph-san and SN platform developers,
> 
> I glanced over the SN code, and I found SN platform doesn't have SN
> specific irq_to_vector() function, though it has SN specific
> local_vector_to_irq() function. I guess it was OK because maybe SN
> platform depends on irq = vector and old generic irq_to_vector() was
> just returning the same value as irq.
> 
> The irq_to_vector() function, however, has been changed by vector
> domain support, and currently it can return the different value from
> irq, even when VECTOR_DOMAIN_PERCPU is not enabled. This can happen
> when an interrupt thorough I/O SAPIC is unregistered and another
> interupt is registered. So if my guess (SN platform depends on irq =
> vector) is true, something problem might happen on SN platform when
> PCI hotplugging or driver reloading.
> 
> If it is true that SN platform depends on irq = vector, we need to
> implement SN specific irq_to_vector() that returns the same value as
> irq, I think. I made sample patches to implement platform specific
> irq_to_vector() as follows. Note that [PATCH 2/2] is not tested yet
> because I don't have any SN platform unfortunately.
> 
>     [PATCH 1/2] Add base support for platform_irq_to_vector()
>     [PATCH 2/2] Implement platform_irq_to_vector() for SN
> 
> Could you give me comments?
> 
> Thanks,
> Kenji Kaneshige
> 

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[John Keller]

Yes, we'll take a look at it.

John


> 
> Hi Kenji-san
> 
> I believe we need this property, but I am not 100% sure, so I am
> going to forward your patch to our team who maintain the IRQ code
> for Altix.
> 
> Mike, Aaron or John, would one of you have time to take a look at
> this?
> 
> Thanks,
> Jes
> 
> 
> Kenji Kaneshige wrote:
> > Hi Jes-san, Russ-san, Christoph-san and SN platform developers,
> > 
> > I glanced over the SN code, and I found SN platform doesn't have SN
> > specific irq_to_vector() function, though it has SN specific
> > local_vector_to_irq() function. I guess it was OK because maybe SN
> > platform depends on irq = vector and old generic irq_to_vector() was
> > just returning the same value as irq.
> > 
> > The irq_to_vector() function, however, has been changed by vector
> > domain support, and currently it can return the different value from
> > irq, even when VECTOR_DOMAIN_PERCPU is not enabled. This can happen
> > when an interrupt thorough I/O SAPIC is unregistered and another
> > interupt is registered. So if my guess (SN platform depends on irq =
> > vector) is true, something problem might happen on SN platform when
> > PCI hotplugging or driver reloading.
> > 
> > If it is true that SN platform depends on irq = vector, we need to
> > implement SN specific irq_to_vector() that returns the same value as
> > irq, I think. I made sample patches to implement platform specific
> > irq_to_vector() as follows. Note that [PATCH 2/2] is not tested yet
> > because I don't have any SN platform unfortunately.
> > 
> >     [PATCH 1/2] Add base support for platform_irq_to_vector()
> >     [PATCH 2/2] Implement platform_irq_to_vector() for SN
> > 
> > Could you give me comments?
> > 
> > Thanks,
> > Kenji Kaneshige
> > 
> 
> 

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Kenji Kaneshige]

Hi,

This is FYI.

If VECTOR_DOMAIN_PERCPU is NOT enabled:

The "irq = vector" is always true in the following cases:
    - reserve_irq_vector(), which is used only by SN
    - create_irq() used on the system that has NO I/O SAPIC
    - assign_irq_vector() with AUTO_ASSIGN specified
    - Vectors outside FIRST_DEVICE_VECTOR to LAST_DEVICE_VECTOR

The "irq = vector" is not always true in the folliwng cases:
    - create_irq() used on the system that has I/O SAPICs.
    - assign_irq_vector() with the value other than AUTO_ASSIGN
      specified on the system has I/O SAPICs.

And it is not ensured that those are true even in the future.

Yasuaki, could you check if it is correct?

Thanks,
Kenji Kaneshige



2007-08-02 (木) の 08:26 -0500 に John Keller さんは書きました:
> Yes, we'll take a look at it.
> 
> John
> 
> 
> > 
> > Hi Kenji-san
> > 
> > I believe we need this property, but I am not 100% sure, so I am
> > going to forward your patch to our team who maintain the IRQ code
> > for Altix.
> > 
> > Mike, Aaron or John, would one of you have time to take a look at
> > this?
> > 
> > Thanks,
> > Jes
> > 
> > 
> > Kenji Kaneshige wrote:
> > > Hi Jes-san, Russ-san, Christoph-san and SN platform developers,
> > > 
> > > I glanced over the SN code, and I found SN platform doesn't have SN
> > > specific irq_to_vector() function, though it has SN specific
> > > local_vector_to_irq() function. I guess it was OK because maybe SN
> > > platform depends on irq = vector and old generic irq_to_vector() was
> > > just returning the same value as irq.
> > > 
> > > The irq_to_vector() function, however, has been changed by vector
> > > domain support, and currently it can return the different value from
> > > irq, even when VECTOR_DOMAIN_PERCPU is not enabled. This can happen
> > > when an interrupt thorough I/O SAPIC is unregistered and another
> > > interupt is registered. So if my guess (SN platform depends on irq =
> > > vector) is true, something problem might happen on SN platform when
> > > PCI hotplugging or driver reloading.
> > > 
> > > If it is true that SN platform depends on irq = vector, we need to
> > > implement SN specific irq_to_vector() that returns the same value as
> > > irq, I think. I made sample patches to implement platform specific
> > > irq_to_vector() as follows. Note that [PATCH 2/2] is not tested yet
> > > because I don't have any SN platform unfortunately.
> > > 
> > >     [PATCH 1/2] Add base support for platform_irq_to_vector()
> > >     [PATCH 2/2] Implement platform_irq_to_vector() for SN
> > > 
> > > Could you give me comments?
> > > 
> > > Thanks,
> > > Kenji Kaneshige
> > > 
> > 
> > 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Russ Anderson]

Kenji Kaneshige wrote:
> 
> Hi Jes-san, Russ-san, Christoph-san and SN platform developers,
> 
> I glanced over the SN code, and I found SN platform doesn't have SN
> specific irq_to_vector() function, though it has SN specific
> local_vector_to_irq() function. I guess it was OK because maybe SN
> platform depends on irq = vector and old generic irq_to_vector() was
> just returning the same value as irq.
> 
> The irq_to_vector() function, however, has been changed by vector
> domain support, and currently it can return the different value from
> irq, even when VECTOR_DOMAIN_PERCPU is not enabled. This can happen
> when an interrupt thorough I/O SAPIC is unregistered and another
> interupt is registered. So if my guess (SN platform depends on irq =
> vector) is true, something problem might happen on SN platform when
> PCI hotplugging or driver reloading.
> 
> If it is true that SN platform depends on irq = vector, we need to
> implement SN specific irq_to_vector() that returns the same value as
> irq, I think. I made sample patches to implement platform specific
> irq_to_vector() as follows. Note that [PATCH 2/2] is not tested yet
> because I don't have any SN platform unfortunately.

There seems to be an issue with this for CPEs.  The net result is
when there is a correctable error, the interrupt does not get handled
resulting in this message:

-----------------------------------------------------------------
irq 30, desc: a000000100c37000, depth: 1, count: 0, unhandled: 0
->handle_irq():  a0000001009a6a50, __end_rodata+0x5010/0x225c0
->chip(): a000000100dfee80, irq_type_sn+0x0/0x80
->action(): 0000000000000000
  IRQ_DISABLED set
Unexpected irq vector 0x1e on CPU 0!
-----------------------------------------------------------------

The CPE handler should be on irq 0x1e (30).  See IA64_CPE_VECTOR .
In ia64_mca_late_init() the line

           if (irq_to_vector(irq) = cpe_vector) {

is never true, so setup_irq(irq, &mca_cpe_irqaction); never gets called.


>     [PATCH 1/2] Add base support for platform_irq_to_vector()
>     [PATCH 2/2] Implement platform_irq_to_vector() for SN
> 
> Could you give me comments?
> 
> Thanks,
> Kenji Kaneshige
> 
> 


-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Keith Rich]

-----------------------------------------------------------------
irq 30, desc: a000000100c37000, depth: 1, count: 0, unhandled: 0
->handle_irq():  a0000001009a6a50, __end_rodata+0x5010/0x225c0
->chip(): a000000100dfee80, irq_type_sn+0x0/0x80
->action(): 0000000000000000
  IRQ_DISABLED set
Unexpected irq vector 0x1e on CPU 0!
-----------------------------------------------------------------

Russ,  I see this on dorado, but not on A4700.
I don't understand why there's a difference.

Keith

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[John Keller]

> 
> Hi,
> 
> This is FYI.
> 
> If VECTOR_DOMAIN_PERCPU is NOT enabled:
> 
> The "irq = vector" is always true in the following cases:
>     - reserve_irq_vector(), which is used only by SN
>     - create_irq() used on the system that has NO I/O SAPIC

Could you say a bit more about this case. I see create_irq()
calling find_unassigned_vector() and find_unassigned_irq().
How is "irq = vector" guaranteed to be always true in this case?

John



>     - assign_irq_vector() with AUTO_ASSIGN specified
>     - Vectors outside FIRST_DEVICE_VECTOR to LAST_DEVICE_VECTOR
> 
> The "irq = vector" is not always true in the folliwng cases:
>     - create_irq() used on the system that has I/O SAPICs.
>     - assign_irq_vector() with the value other than AUTO_ASSIGN
>       specified on the system has I/O SAPICs.
> 
> And it is not ensured that those are true even in the future.
> 
> Yasuaki, could you check if it is correct?
> 
> Thanks,
> Kenji Kaneshige
> 
> 
> 
> 2007-08-02 (木) の 08:26 -0500 に John Keller さんは書きました:
> > Yes, we'll take a look at it.
> > 
> > John
> > 
> > 
> > > 
> > > Hi Kenji-san
> > > 
> > > I believe we need this property, but I am not 100% sure, so I am
> > > going to forward your patch to our team who maintain the IRQ code
> > > for Altix.
> > > 
> > > Mike, Aaron or John, would one of you have time to take a look at
> > > this?
> > > 
> > > Thanks,
> > > Jes
> > > 
> > > 
> > > Kenji Kaneshige wrote:
> > > > Hi Jes-san, Russ-san, Christoph-san and SN platform developers,
> > > > 
> > > > I glanced over the SN code, and I found SN platform doesn't have SN
> > > > specific irq_to_vector() function, though it has SN specific
> > > > local_vector_to_irq() function. I guess it was OK because maybe SN
> > > > platform depends on irq = vector and old generic irq_to_vector() was
> > > > just returning the same value as irq.
> > > > 
> > > > The irq_to_vector() function, however, has been changed by vector
> > > > domain support, and currently it can return the different value from
> > > > irq, even when VECTOR_DOMAIN_PERCPU is not enabled. This can happen
> > > > when an interrupt thorough I/O SAPIC is unregistered and another
> > > > interupt is registered. So if my guess (SN platform depends on irq =
> > > > vector) is true, something problem might happen on SN platform when
> > > > PCI hotplugging or driver reloading.
> > > > 
> > > > If it is true that SN platform depends on irq = vector, we need to
> > > > implement SN specific irq_to_vector() that returns the same value as
> > > > irq, I think. I made sample patches to implement platform specific
> > > > irq_to_vector() as follows. Note that [PATCH 2/2] is not tested yet
> > > > because I don't have any SN platform unfortunately.
> > > > 
> > > >     [PATCH 1/2] Add base support for platform_irq_to_vector()
> > > >     [PATCH 2/2] Implement platform_irq_to_vector() for SN
> > > > 
> > > > Could you give me comments?
> > > > 
> > > > Thanks,
> > > > Kenji Kaneshige
> > > > 
> > > 
> > > 
> > 
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Kenji Kaneshige]

2007-08-02 (木) の 09:50 -0500 に Russ Anderson さんは書きました: 
> There seems to be an issue with this for CPEs.  The net result is
> when there is a correctable error, the interrupt does not get handled
> resulting in this message:
> 
> -----------------------------------------------------------------
> irq 30, desc: a000000100c37000, depth: 1, count: 0, unhandled: 0
> ->handle_irq():  a0000001009a6a50, __end_rodata+0x5010/0x225c0
> ->chip(): a000000100dfee80, irq_type_sn+0x0/0x80
> ->action(): 0000000000000000
>   IRQ_DISABLED set
> Unexpected irq vector 0x1e on CPU 0!
> -----------------------------------------------------------------
> 
> The CPE handler should be on irq 0x1e (30).  See IA64_CPE_VECTOR .
> In ia64_mca_late_init() the line
> 
>            if (irq_to_vector(irq) = cpe_vector) {
> 
> is never true, so setup_irq(irq, &mca_cpe_irqaction); never gets called.
> 

Could you please check if [PATCH 2/2] is applied on your kernel again?

With [PATCH 2/2], irq_to_vector(irq) just returns the same value as
vector. So the line

	for (irq = 0; irq < NR_IRQS; ++irq)
		if (irq_to_vector(irq) = cpe_vector) {

should be true if cpe_vector is within 0 to NR_IRQ-1.

I'm sorry but the important thing was missing in my "FYI".
I said like folows:

> The "irq = vector" is always true in the following cases:
>     ...
>     - Vectors outside FIRST_DEVICE_VECTOR to LAST_DEVICE_VECTOR

But if the irq is not initialized by __bind_irq_vector() through
bind_irq_vector() or register_percpu_irq(), irq_to_vector(irq) returns 
IRQ_VECTOR_UNASSIGNED, which is zero.

So if [PATCH 2/2] was not applied, irq_to_vector(IA64_CPE_VECTOR)
may returns zero and "irq_to_vector(irq) = cpe_vector" would never
be true.

BTW, as you pointed out in the other mail, sn_irq_to_vector() will
truncate any IRQ > 255. Because of this, "if (irq_to_vector(irq) = 
cpe_vector)" check will true several times. But I don't know if
this is the cause of CPE problem, becasue CPE is still fine on my
dig64 kernel even when the following debug patch applied.

Thanks,
Kenji Kaneshige


---
 arch/ia64/kernel/irq.c    |    6 ++++--
 include/asm-ia64/hw_irq.h |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

Index: linux-2.6.23-rc1/arch/ia64/kernel/irq.c
=================================--- linux-2.6.23-rc1.orig/arch/ia64/kernel/irq.c
+++ linux-2.6.23-rc1/arch/ia64/kernel/irq.c
@@ -35,12 +35,14 @@ void ack_bad_irq(unsigned int irq)
 #ifdef CONFIG_IA64_GENERIC
 ia64_vector __ia64_irq_to_vector(int irq)
 {
-	return irq_cfg[irq].vector;
+	return (ia64_vector)irq;
+	//return irq_cfg[irq].vector;
 }
 
 unsigned int __ia64_local_vector_to_irq (ia64_vector vec)
 {
-	return __get_cpu_var(vector_irq)[vec];
+	return (unsigned int)vec;
+	//return __get_cpu_var(vector_irq)[vec];
 }
 #endif
 
Index: linux-2.6.23-rc1/include/asm-ia64/hw_irq.h
=================================--- linux-2.6.23-rc1.orig/include/asm-ia64/hw_irq.h
+++ linux-2.6.23-rc1/include/asm-ia64/hw_irq.h
@@ -126,13 +126,15 @@ extern irq_desc_t irq_desc[NR_IRQS];
 #ifndef CONFIG_IA64_GENERIC
 static inline ia64_vector __ia64_irq_to_vector(int irq)
 {
-	return irq_cfg[irq].vector;
+	return (ia64_vector)irq;
+	//return irq_cfg[irq].vector;
 }
 
 static inline unsigned int
 __ia64_local_vector_to_irq (ia64_vector vec)
 {
-	return __get_cpu_var(vector_irq)[vec];
+	return (unsigned int)vec;
+	//return __get_cpu_var(vector_irq)[vec];
 }
 #endif
 

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Yasuaki Ishimatsu]

Hi John,

John Keller wrote:
>> Hi,
>>
>> This is FYI.
>>
>> If VECTOR_DOMAIN_PERCPU is NOT enabled:
>>
>> The "irq = vector" is always true in the following cases:
>>     - reserve_irq_vector(), which is used only by SN
>>     - create_irq() used on the system that has NO I/O SAPIC
> 
> Could you say a bit more about this case. I see create_irq()
> calling find_unassigned_vector() and find_unassigned_irq().
> How is "irq = vector" guaranteed to be always true in this case?

It's guaranteed as follows:

- When find_unassigned_vector()/find_unassigned_irq() search
   available vector and irq, it always start to search from
   IA64_FIRST_DEVICE_VECTOR.

- clear_irq_vector() releases both vector and irq.

Thanks,
Yasuaki

> John
> 
> 
> 
>>     - assign_irq_vector() with AUTO_ASSIGN specified
>>     - Vectors outside FIRST_DEVICE_VECTOR to LAST_DEVICE_VECTOR
>>
>> The "irq = vector" is not always true in the folliwng cases:
>>     - create_irq() used on the system that has I/O SAPICs.
>>     - assign_irq_vector() with the value other than AUTO_ASSIGN
>>       specified on the system has I/O SAPICs.
>>
>> And it is not ensured that those are true even in the future.
>>
>> Yasuaki, could you check if it is correct?
>>
>> Thanks,
>> Kenji Kaneshige
>>
>>
>>
>> 2007-08-02 (木) の 08:26 -0500 に John Keller さんは書きました:
>>> Yes, we'll take a look at it.
>>>
>>> John
>>>
>>>
>>>> Hi Kenji-san
>>>>
>>>> I believe we need this property, but I am not 100% sure, so I am
>>>> going to forward your patch to our team who maintain the IRQ code
>>>> for Altix.
>>>>
>>>> Mike, Aaron or John, would one of you have time to take a look at
>>>> this?
>>>>
>>>> Thanks,
>>>> Jes
>>>>
>>>>
>>>> Kenji Kaneshige wrote:
>>>>> Hi Jes-san, Russ-san, Christoph-san and SN platform developers,
>>>>>
>>>>> I glanced over the SN code, and I found SN platform doesn't have SN
>>>>> specific irq_to_vector() function, though it has SN specific
>>>>> local_vector_to_irq() function. I guess it was OK because maybe SN
>>>>> platform depends on irq = vector and old generic irq_to_vector() was
>>>>> just returning the same value as irq.
>>>>>
>>>>> The irq_to_vector() function, however, has been changed by vector
>>>>> domain support, and currently it can return the different value from
>>>>> irq, even when VECTOR_DOMAIN_PERCPU is not enabled. This can happen
>>>>> when an interrupt thorough I/O SAPIC is unregistered and another
>>>>> interupt is registered. So if my guess (SN platform depends on irq =
>>>>> vector) is true, something problem might happen on SN platform when
>>>>> PCI hotplugging or driver reloading.
>>>>>
>>>>> If it is true that SN platform depends on irq = vector, we need to
>>>>> implement SN specific irq_to_vector() that returns the same value as
>>>>> irq, I think. I made sample patches to implement platform specific
>>>>> irq_to_vector() as follows. Note that [PATCH 2/2] is not tested yet
>>>>> because I don't have any SN platform unfortunately.
>>>>>
>>>>>     [PATCH 1/2] Add base support for platform_irq_to_vector()
>>>>>     [PATCH 2/2] Implement platform_irq_to_vector() for SN
>>>>>
>>>>> Could you give me comments?
>>>>>
>>>>> Thanks,
>>>>> Kenji Kaneshige
>>>>>
>>>>
>>> -
>>> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Kenji Kaneshige]

2007-08-02 (木) の 14:32 -0500 に John Keller さんは書きました:
> > 
> > Hi,
> > 
> > This is FYI.
> > 
> > If VECTOR_DOMAIN_PERCPU is NOT enabled:
> > 
> > The "irq = vector" is always true in the following cases:
> >     - reserve_irq_vector(), which is used only by SN
> >     - create_irq() used on the system that has NO I/O SAPIC
> 
> Could you say a bit more about this case. I see create_irq()
> calling find_unassigned_vector() and find_unassigned_irq().
> How is "irq = vector" guaranteed to be always true in this case?
> 

Both find_unassigned_vector() and find_unassigned_irq() searchs the
number from IA64_FIRST_DEVICE_VECTOR. And all irq and vector numbers
from IA64_FIRST_DEVICE_VECTOR to IA64_LAST_DEVICE_VECTOR are not used.

The create_irq() acquires both irq number and vector number using
find_unassigned_irq() and find_unassigned_vector() respectively and
bind them, with vector_lock held.

The clear_irq_vector(), which is called through destroy_irq() and so
on, frees both irq and vector with vector_lock held.

Since oth irq and vector are acquired/freed at a time, "irq = vector"
is guaranteed.

But there is a exception destroy_and_reserve_irq(), which is called
by IOSAPIC code. It frees both irq and vector, and reserve irq after
that. That is, it only frees vector. This is required for fixed
mapping between gsi and irq.

Thanks,
Kenji Kaneshige


> John
> 
> 
> 
> >     - assign_irq_vector() with AUTO_ASSIGN specified
> >     - Vectors outside FIRST_DEVICE_VECTOR to LAST_DEVICE_VECTOR
> > 
> > The "irq = vector" is not always true in the folliwng cases:
> >     - create_irq() used on the system that has I/O SAPICs.
> >     - assign_irq_vector() with the value other than AUTO_ASSIGN
> >       specified on the system has I/O SAPICs.
> > 
> > And it is not ensured that those are true even in the future.
> > 
> > Yasuaki, could you check if it is correct?
> > 
> > Thanks,
> > Kenji Kaneshige
> > 
> > 
> > 
> > 2007-08-02 (木) の 08:26 -0500 に John Keller さんは書きました:
> > > Yes, we'll take a look at it.
> > > 
> > > John
> > > 
> > > 
> > > > 
> > > > Hi Kenji-san
> > > > 
> > > > I believe we need this property, but I am not 100% sure, so I am
> > > > going to forward your patch to our team who maintain the IRQ code
> > > > for Altix.
> > > > 
> > > > Mike, Aaron or John, would one of you have time to take a look at
> > > > this?
> > > > 
> > > > Thanks,
> > > > Jes
> > > > 
> > > > 
> > > > Kenji Kaneshige wrote:
> > > > > Hi Jes-san, Russ-san, Christoph-san and SN platform developers,
> > > > > 
> > > > > I glanced over the SN code, and I found SN platform doesn't have SN
> > > > > specific irq_to_vector() function, though it has SN specific
> > > > > local_vector_to_irq() function. I guess it was OK because maybe SN
> > > > > platform depends on irq = vector and old generic irq_to_vector() was
> > > > > just returning the same value as irq.
> > > > > 
> > > > > The irq_to_vector() function, however, has been changed by vector
> > > > > domain support, and currently it can return the different value from
> > > > > irq, even when VECTOR_DOMAIN_PERCPU is not enabled. This can happen
> > > > > when an interrupt thorough I/O SAPIC is unregistered and another
> > > > > interupt is registered. So if my guess (SN platform depends on irq =
> > > > > vector) is true, something problem might happen on SN platform when
> > > > > PCI hotplugging or driver reloading.
> > > > > 
> > > > > If it is true that SN platform depends on irq = vector, we need to
> > > > > implement SN specific irq_to_vector() that returns the same value as
> > > > > irq, I think. I made sample patches to implement platform specific
> > > > > irq_to_vector() as follows. Note that [PATCH 2/2] is not tested yet
> > > > > because I don't have any SN platform unfortunately.
> > > > > 
> > > > >     [PATCH 1/2] Add base support for platform_irq_to_vector()
> > > > >     [PATCH 2/2] Implement platform_irq_to_vector() for SN
> > > > > 
> > > > > Could you give me comments?
> > > > > 
> > > > > Thanks,
> > > > > Kenji Kaneshige
> > > > > 
> > > > 
> > > > 
> > > 
> > > -
> > > To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Russ Anderson]

Kenji Kaneshige wrote:
> > There seems to be an issue with this for CPEs.  The net result is
> > when there is a correctable error, the interrupt does not get handled
> > resulting in this message:
> > 
> > -----------------------------------------------------------------
> > irq 30, desc: a000000100c37000, depth: 1, count: 0, unhandled: 0
> > ->handle_irq():  a0000001009a6a50, __end_rodata+0x5010/0x225c0
> > ->chip(): a000000100dfee80, irq_type_sn+0x0/0x80
> > ->action(): 0000000000000000
> >   IRQ_DISABLED set
> > Unexpected irq vector 0x1e on CPU 0!
> > -----------------------------------------------------------------
> > 
> > The CPE handler should be on irq 0x1e (30).  See IA64_CPE_VECTOR .
> > In ia64_mca_late_init() the line
> > 
> >            if (irq_to_vector(irq) = cpe_vector) {
> > 
> > is never true, so setup_irq(irq, &mca_cpe_irqaction); never gets called.
> > 
> 
> Could you please check if [PATCH 2/2] is applied on your kernel again?

Your patch [PATCH 2/2] fixes the problem.

> With [PATCH 2/2], irq_to_vector(irq) just returns the same value as
> vector. So the line
> 
> 	for (irq = 0; irq < NR_IRQS; ++irq)
> 		if (irq_to_vector(irq) = cpe_vector) {
> 
> should be true if cpe_vector is within 0 to NR_IRQ-1.
> 
> I'm sorry but the important thing was missing in my "FYI".
> I said like folows:
> 
> > The "irq = vector" is always true in the following cases:
> >     ...
> >     - Vectors outside FIRST_DEVICE_VECTOR to LAST_DEVICE_VECTOR
> 
> But if the irq is not initialized by __bind_irq_vector() through
> bind_irq_vector() or register_percpu_irq(), irq_to_vector(irq) returns 
> IRQ_VECTOR_UNASSIGNED, which is zero.
> 
> So if [PATCH 2/2] was not applied, irq_to_vector(IA64_CPE_VECTOR)
> may returns zero and "irq_to_vector(irq) = cpe_vector" would never
> be true.

Correct.

> BTW, as you pointed out in the other mail, sn_irq_to_vector() will
> truncate any IRQ > 255. Because of this, "if (irq_to_vector(irq) = 
> cpe_vector)" check will true several times. But I don't know if
> this is the cause of CPE problem, becasue CPE is still fine on my
> dig64 kernel even when the following debug patch applied.

That caused a different problem of the CPE handler getting 
registered on multiple irqs.

From /proc/interrupts:
 30:          1          0          0          0          SN hub  cpe_hndlr
286:          0          0          0          0          SN hub  cpe_hndlr
542:          0          0          0          0          SN hub  cpe_hndlr
798:          0          0          0          0          SN hub  cpe_hndlr

I have a patch in the works to clean up ia64_mca_late_init()
so the handler only gets registered for irq 30.  I will post
it shortly.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[John Keller]

> 
> 2007-08-02 (木) の 14:32 -0500 に John Keller さんは書きました:
> > > 
> > > Hi,
> > > 
> > > This is FYI.
> > > 
> > > If VECTOR_DOMAIN_PERCPU is NOT enabled:
> > > 
> > > The "irq = vector" is always true in the following cases:
> > >     - reserve_irq_vector(), which is used only by SN
> > >     - create_irq() used on the system that has NO I/O SAPIC
> > 
> > Could you say a bit more about this case. I see create_irq()
> > calling find_unassigned_vector() and find_unassigned_irq().
> > How is "irq = vector" guaranteed to be always true in this case?
> > 
> 
> Both find_unassigned_vector() and find_unassigned_irq() searchs the
> number from IA64_FIRST_DEVICE_VECTOR. And all irq and vector numbers
> from IA64_FIRST_DEVICE_VECTOR to IA64_LAST_DEVICE_VECTOR are not used.
> 
> The create_irq() acquires both irq number and vector number using
> find_unassigned_irq() and find_unassigned_vector() respectively and
> bind them, with vector_lock held.
> 
> The clear_irq_vector(), which is called through destroy_irq() and so
> on, frees both irq and vector with vector_lock held.
> 
> Since oth irq and vector are acquired/freed at a time, "irq = vector"
> is guaranteed.
> 
> But there is a exception destroy_and_reserve_irq(), which is called
> by IOSAPIC code. It frees both irq and vector, and reserve irq after
> that. That is, it only frees vector. This is required for fixed
> mapping between gsi and irq.
> 
> Thanks,
> Kenji Kaneshige


Thanks for the info.
Your patch looks good to me, along with the other change to
sn_irq_to_vector().

> What about this:
> 
> ia64_vector sn_irq_to_vector(int irq)
> {
>        if (irq >= IA64_NUM_VECTORS)
>                return 0;
>        return (ia64_vector)irq;
> }
> 
> 

John Keller



> 
> 
> > John
> > 
> > 
> > 
> > >     - assign_irq_vector() with AUTO_ASSIGN specified
> > >     - Vectors outside FIRST_DEVICE_VECTOR to LAST_DEVICE_VECTOR
> > > 
> > > The "irq = vector" is not always true in the folliwng cases:
> > >     - create_irq() used on the system that has I/O SAPICs.
> > >     - assign_irq_vector() with the value other than AUTO_ASSIGN
> > >       specified on the system has I/O SAPICs.
> > > 
> > > And it is not ensured that those are true even in the future.
> > > 
> > > Yasuaki, could you check if it is correct?
> > > 
> > > Thanks,
> > > Kenji Kaneshige
> > > 
> > > 
> > > 
> > > 2007-08-02 (木) の 08:26 -0500 に John Keller さんは書きました:
> > > > Yes, we'll take a look at it.
> > > > 
> > > > John
> > > > 
> > > > 
> > > > > 
> > > > > Hi Kenji-san
> > > > > 
> > > > > I believe we need this property, but I am not 100% sure, so I am
> > > > > going to forward your patch to our team who maintain the IRQ code
> > > > > for Altix.
> > > > > 
> > > > > Mike, Aaron or John, would one of you have time to take a look at
> > > > > this?
> > > > > 
> > > > > Thanks,
> > > > > Jes
> > > > > 
> > > > > 
> > > > > Kenji Kaneshige wrote:
> > > > > > Hi Jes-san, Russ-san, Christoph-san and SN platform developers,
> > > > > > 
> > > > > > I glanced over the SN code, and I found SN platform doesn't have SN
> > > > > > specific irq_to_vector() function, though it has SN specific
> > > > > > local_vector_to_irq() function. I guess it was OK because maybe SN
> > > > > > platform depends on irq = vector and old generic irq_to_vector() was
> > > > > > just returning the same value as irq.
> > > > > > 
> > > > > > The irq_to_vector() function, however, has been changed by vector
> > > > > > domain support, and currently it can return the different value from
> > > > > > irq, even when VECTOR_DOMAIN_PERCPU is not enabled. This can happen
> > > > > > when an interrupt thorough I/O SAPIC is unregistered and another
> > > > > > interupt is registered. So if my guess (SN platform depends on irq =
> > > > > > vector) is true, something problem might happen on SN platform when
> > > > > > PCI hotplugging or driver reloading.
> > > > > > 
> > > > > > If it is true that SN platform depends on irq = vector, we need to
> > > > > > implement SN specific irq_to_vector() that returns the same value as
> > > > > > irq, I think. I made sample patches to implement platform specific
> > > > > > irq_to_vector() as follows. Note that [PATCH 2/2] is not tested yet
> > > > > > because I don't have any SN platform unfortunately.
> > > > > > 
> > > > > >     [PATCH 1/2] Add base support for platform_irq_to_vector()
> > > > > >     [PATCH 2/2] Implement platform_irq_to_vector() for SN
> > > > > > 
> > > > > > Could you give me comments?
> > > > > > 
> > > > > > Thanks,
> > > > > > Kenji Kaneshige
> > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > -
> > > > To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > > 
> > 
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

RE: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[Luck, Tony]

> Thanks for the info.
> Your patch looks good to me, along with the other change to
> sn_irq_to_vector().

Sounds like an Ack ... can one of you fold the changes into a new
patch please.

-Tony

Re: [RFC][PATCH 0/2] SN platform needs platform_irq_to_vector?

[John Keller]

> 
> > Thanks for the info.
> > Your patch looks good to me, along with the other change to
> > sn_irq_to_vector().
> 
> Sounds like an Ack ... can one of you fold the changes into a new
> patch please.
> 
> -Tony

This is the patch as I was running it.



Add base support for implementing platform_irq_to_vector().

Acked-by: John Keller <jpk@sgi.com>

---
 arch/ia64/kernel/irq.c          |    5 +++++
 arch/ia64/sn/kernel/irq.c       |    7 +++++++
 include/asm-ia64/hw_irq.h       |    7 ++++++-
 include/asm-ia64/machvec.h      |    7 +++++++
 include/asm-ia64/machvec_init.h |    1 +
 include/asm-ia64/machvec_sn2.h  |    2 ++
 6 files changed, 28 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/ia64/kernel/irq.c
=================================--- linux-2.6.orig/arch/ia64/kernel/irq.c	2007-08-04 18:45:15.705972596 -0500
+++ linux-2.6/arch/ia64/kernel/irq.c	2007-08-04 18:45:43.197343168 -0500
@@ -33,6 +33,11 @@ void ack_bad_irq(unsigned int irq)
 }
 
 #ifdef CONFIG_IA64_GENERIC
+ia64_vector __ia64_irq_to_vector(int irq)
+{
+	return irq_cfg[irq].vector;
+}
+
 unsigned int __ia64_local_vector_to_irq (ia64_vector vec)
 {
 	return __get_cpu_var(vector_irq)[vec];
Index: linux-2.6/include/asm-ia64/hw_irq.h
=================================--- linux-2.6.orig/include/asm-ia64/hw_irq.h	2007-08-04 18:45:15.705972596 -0500
+++ linux-2.6/include/asm-ia64/hw_irq.h	2007-08-04 18:45:43.217345621 -0500
@@ -124,6 +124,11 @@ static inline void ia64_resend_irq(unsig
 extern irq_desc_t irq_desc[NR_IRQS];
 
 #ifndef CONFIG_IA64_GENERIC
+static inline ia64_vector __ia64_irq_to_vector(int irq)
+{
+	return irq_cfg[irq].vector;
+}
+
 static inline unsigned int
 __ia64_local_vector_to_irq (ia64_vector vec)
 {
@@ -145,7 +150,7 @@ __ia64_local_vector_to_irq (ia64_vector 
 static inline ia64_vector
 irq_to_vector (int irq)
 {
-	return irq_cfg[irq].vector;
+	return platform_irq_to_vector(irq);
 }
 
 /*
Index: linux-2.6/include/asm-ia64/machvec.h
=================================--- linux-2.6.orig/include/asm-ia64/machvec.h	2007-08-04 18:45:15.709973086 -0500
+++ linux-2.6/include/asm-ia64/machvec.h	2007-08-04 18:45:43.237348073 -0500
@@ -30,6 +30,7 @@ typedef void ia64_mv_send_ipi_t (int, in
 typedef void ia64_mv_timer_interrupt_t (int, void *);
 typedef void ia64_mv_global_tlb_purge_t (struct mm_struct *, unsigned long, unsigned long, unsigned long);
 typedef void ia64_mv_tlb_migrate_finish_t (struct mm_struct *);
+typedef u8 ia64_mv_irq_to_vector (int);
 typedef unsigned int ia64_mv_local_vector_to_irq (u8);
 typedef char *ia64_mv_pci_get_legacy_mem_t (struct pci_bus *);
 typedef int ia64_mv_pci_legacy_read_t (struct pci_bus *, u16 port, u32 *val,
@@ -145,6 +146,7 @@ extern void machvec_tlb_migrate_finish (
 #  define platform_dma_sync_sg_for_device ia64_mv.dma_sync_sg_for_device
 #  define platform_dma_mapping_error		ia64_mv.dma_mapping_error
 #  define platform_dma_supported	ia64_mv.dma_supported
+#  define platform_irq_to_vector	ia64_mv.irq_to_vector
 #  define platform_local_vector_to_irq	ia64_mv.local_vector_to_irq
 #  define platform_pci_get_legacy_mem	ia64_mv.pci_get_legacy_mem
 #  define platform_pci_legacy_read	ia64_mv.pci_legacy_read
@@ -198,6 +200,7 @@ struct ia64_machine_vector {
 	ia64_mv_dma_sync_sg_for_device *dma_sync_sg_for_device;
 	ia64_mv_dma_mapping_error *dma_mapping_error;
 	ia64_mv_dma_supported *dma_supported;
+	ia64_mv_irq_to_vector *irq_to_vector;
 	ia64_mv_local_vector_to_irq *local_vector_to_irq;
 	ia64_mv_pci_get_legacy_mem_t *pci_get_legacy_mem;
 	ia64_mv_pci_legacy_read_t *pci_legacy_read;
@@ -247,6 +250,7 @@ struct ia64_machine_vector {
 	platform_dma_sync_sg_for_device,	\
 	platform_dma_mapping_error,			\
 	platform_dma_supported,			\
+	platform_irq_to_vector,			\
 	platform_local_vector_to_irq,		\
 	platform_pci_get_legacy_mem,		\
 	platform_pci_legacy_read,		\
@@ -366,6 +370,9 @@ extern ia64_mv_dma_supported		swiotlb_dm
 #ifndef platform_dma_supported
 # define  platform_dma_supported	swiotlb_dma_supported
 #endif
+#ifndef platform_irq_to_vector
+# define platform_irq_to_vector		__ia64_irq_to_vector
+#endif
 #ifndef platform_local_vector_to_irq
 # define platform_local_vector_to_irq	__ia64_local_vector_to_irq
 #endif
Index: linux-2.6/include/asm-ia64/machvec_init.h
=================================--- linux-2.6.orig/include/asm-ia64/machvec_init.h	2007-08-04 18:45:15.709973086 -0500
+++ linux-2.6/include/asm-ia64/machvec_init.h	2007-08-04 18:45:43.253350035 -0500
@@ -2,6 +2,7 @@
 
 extern ia64_mv_send_ipi_t ia64_send_ipi;
 extern ia64_mv_global_tlb_purge_t ia64_global_tlb_purge;
+extern ia64_mv_irq_to_vector __ia64_irq_to_vector;
 extern ia64_mv_local_vector_to_irq __ia64_local_vector_to_irq;
 extern ia64_mv_pci_get_legacy_mem_t ia64_pci_get_legacy_mem;
 extern ia64_mv_pci_legacy_read_t ia64_pci_legacy_read;
Index: linux-2.6/arch/ia64/sn/kernel/irq.c
=================================--- linux-2.6.orig/arch/ia64/sn/kernel/irq.c	2007-08-04 18:45:15.705972596 -0500
+++ linux-2.6/arch/ia64/sn/kernel/irq.c	2007-08-04 18:45:43.273352487 -0500
@@ -256,6 +256,13 @@ struct irq_chip irq_type_sn = {
 	.set_affinity	= sn_set_affinity_irq
 };
 
+ia64_vector sn_irq_to_vector(int irq)
+{
+	if (irq >= IA64_NUM_VECTORS)
+		return 0;
+	return (ia64_vector)irq;
+}
+
 unsigned int sn_local_vector_to_irq(u8 vector)
 {
 	return (CPU_VECTOR_TO_IRQ(smp_processor_id(), vector));
Index: linux-2.6/include/asm-ia64/machvec_sn2.h
=================================--- linux-2.6.orig/include/asm-ia64/machvec_sn2.h	2007-08-04 18:45:15.709973086 -0500
+++ linux-2.6/include/asm-ia64/machvec_sn2.h	2007-08-04 18:45:43.289354449 -0500
@@ -35,6 +35,7 @@ extern ia64_mv_send_ipi_t sn2_send_IPI;
 extern ia64_mv_timer_interrupt_t sn_timer_interrupt;
 extern ia64_mv_global_tlb_purge_t sn2_global_tlb_purge;
 extern ia64_mv_tlb_migrate_finish_t	sn_tlb_migrate_finish;
+extern ia64_mv_irq_to_vector sn_irq_to_vector;
 extern ia64_mv_local_vector_to_irq sn_local_vector_to_irq;
 extern ia64_mv_pci_get_legacy_mem_t sn_pci_get_legacy_mem;
 extern ia64_mv_pci_legacy_read_t sn_pci_legacy_read;
@@ -104,6 +105,7 @@ extern ia64_mv_pci_fixup_bus_t		sn_pci_f
 #define platform_readw_relaxed		__sn_readw_relaxed
 #define platform_readl_relaxed		__sn_readl_relaxed
 #define platform_readq_relaxed		__sn_readq_relaxed
+#define platform_irq_to_vector		sn_irq_to_vector
 #define platform_local_vector_to_irq	sn_local_vector_to_irq
 #define platform_pci_get_legacy_mem	sn_pci_get_legacy_mem
 #define platform_pci_legacy_read	sn_pci_legacy_read