IB/ehca: In case of lost interrupts, trigger EOI to reenable interrupts
PATCH 1/1 IB/ehca: In case of lost interrupts, trigger EOI to reenable interrupts
During corner case testing, we noticed that some versions of ehca do not properly transition to interrupt done in special load situations. This can be resolved by periodically triggering EOI through H_EOI, if eqes are pending.
---
This patch replaces my previous patch-set.
As Paul suggested, this version of the patch calls H_EOI directly and doesn't need any ibmebus changes.
drivers/infiniband/hw/ehca/ehca_main.c | 11 +++++++++-- drivers/infiniband/hw/ehca/hcp_if.c | 11 +++++++++++ drivers/infiniband/hw/ehca/hcp_if.h | 1 + 3 files changed, 21 insertions(+), 2 deletions(-)
drivers/infiniband/hw/ehca/ehca_main.c +9 -2
@@ -937,6 +937,7 @@ static struct of_platform_driver ehca_driver = {void ehca_poll_eqs (unsigned long data) {struct ehca_shca *shca; +u64 ret;spin_lock (&shca_list_lock);list_for_each_entry (shca, &shca_list, shca_list) {
@@ -955,8 +956,14 @@ void ehca_poll_eqs(unsigned long data)spin_unlock_irqrestore (&eq->spinlock, flags); max--; }while (q_ofs == q_ofs2 && max >0 ); -if (q_ofs == q_ofs2) -ehca_process_eq (shca,0 ); +if (q_ofs == q_ofs2) { + ret =hipz_h_eoi (eq->ist); +if (ret !=H_SUCCESS ) +ehca_err (&shca->ib_device, +"bad return code EOI -" +"rc = %ld\n" , ret); +tasklet_hi_schedule (&shca->eq.interrupt_task); + } } }mod_timer (&poll_eqs_timer,round_jiffies (jiffies +HZ ));
drivers/infiniband/hw/ehca/hcp_if.c +11 -0
@@ -933,3 +933,14 @@ u64 hipz_h_error_data(const struct ipz_adapter_handle adapter_handle, r_cb, 0, 0, 0, 0); } + +u64 hipz_h_eoi (int irq) +{ +int value; +unsigned long xirr; + +iosync (); + value = (0xff <<24 ) | irq; + xirr = value &0xffffffff ; +return plpar_hcall_norets (H_EOI , xirr); +}
drivers/infiniband/hw/ehca/hcp_if.h +1 -0
@@ -260,5 +260,6 @@ u64 hipz_h_error_data(const struct ipz_adapter_handle adapter_handle,const u64 ressource_handle, void *rblock, unsigned long *byte_count); +u64 hipz_h_eoi (int irq);#endif /* __HCP_IF_H__ */
Signature · 1 line
1.5.5
Quoted 4 lines
During corner case testing, we noticed that some versions of ehca do not properly transition to interrupt done in special load situations. This can be resolved by periodically triggering EOI through
H_EOI, if eqes are pending.
So just to be clear: this is a workaround for a hardware/firmware bug?
- R.
Quoted 9 lines · On Tuesday 10 June 2008 16:52:57 Roland Dreier wrote:
Quoted 4 lines
During corner case testing, we noticed that some versions of ehca do not properly transition to interrupt done in special load situations. This can be resolved by periodically triggering EOI through
H_EOI, if eqes are pending.So just to be clear: this is a workaround for a hardware/firmware bug?
- R.
Yes it is.
regards Stefan
Quoted 3 lines
Quoted 1 line
So just to be clear: this is a workaround for a hardware/firmware bug?
Yes it is.
OK, so paulus et al... does it seem like a good approach to call H_EOI from driver code (given that this driver makes tons of other hcalls)?
How critical is this? Since you said "corner case testing" I suspect we can defer this to 2.6.27 and maybe get it into -stable later?
Also, out of curiousity:
drivers/infiniband/hw/ehca/hcp_if.c u64 hipz_h_error_data(const struct ipz_adapter_handle adapter_handle, · line 942
Quoted 6 lines
drivers/infiniband/hw/ehca/hcp_if.c +6
-0+u64 hipz_h_eoi (int irq) +{ +int value; +unsigned long xirr; + +iosync ();
what is the iosync() required for here?
drivers/infiniband/hw/ehca/hcp_if.c u64 hipz_h_error_data(const struct ipz_adapter_handle adapter_handle, · line 944
Quoted 2 lines
drivers/infiniband/hw/ehca/hcp_if.c +2
-0+ value = (0xff <<24 ) | irq; + xirr = value &0xffffffff ;
given that irq and value are ints, is there any possible way value could have bits outside of the low 32 set? If you're worried about sign extension isn't it simpler to just make value unsigned?
drivers/infiniband/hw/ehca/hcp_if.c u64 hipz_h_error_data(const struct ipz_adapter_handle adapter_handle, · line 946
Quoted 2 lines
drivers/infiniband/hw/ehca/hcp_if.c +2
-0+return plpar_hcall_norets (H_EOI , xirr); +}
ie why not:
u64 hipz_h_eoi(int irq)
{
unsigned xirr;
iosync();
xirr = (0xff << 24) | irq;
return plpar_hcall_norets(H_EOI, xirr);
}Quoted 6 lines · Roland Dreier
Quoted 3 lines
Quoted 1 line
So just to be clear: this is a workaround for a hardware/firmware bug?
Yes it is.
OK, so paulus et al... does it seem like a good approach to call
H_EOIfrom driver code (given that this driver makes tons of other hcalls)?
If this workaround is approved by the relevant hardware & firmware folks, and demonstrably fixes the bug, then I am happy with it.
Paul.
Hi Roland,
Quoted 9 lines · On Tuesday 10 June 2008 18:18:50 Roland Dreier wrote:
Quoted 3 lines
Quoted 1 line
So just to be clear: this is a workaround for a hardware/firmware bug?
Yes it is.
OK, so paulus et al... does it seem like a good approach to call
H_EOIfrom driver code (given that this driver makes tons of other hcalls)?How critical is this? Since you said "corner case testing" I suspect we can defer this to 2.6.27 and maybe get it into -stable later?
No, it's ok with me if you pick this for 2.6.27.
Quoted 11 lines
Also, out of curiousity:
drivers/infiniband/hw/ehca/hcp_if.c u64 hipz_h_error_data(const struct ipz_adapter_handle adapter_handle, · line 942
Quoted 6 lines
drivers/infiniband/hw/ehca/hcp_if.c +6
-0+u64 hipz_h_eoi (int irq) +{ +int value; +unsigned long xirr; + +iosync ();what is the
iosync()required for here?
It's the same sequence as the interrupt handler for powerpc is implemented.
Quoted 13 lines
drivers/infiniband/hw/ehca/hcp_if.c u64 hipz_h_error_data(const struct ipz_adapter_handle adapter_handle, · line 944
Quoted 3 lines
drivers/infiniband/hw/ehca/hcp_if.c +2
-0+ value = (0xff <<24 ) | irq; + xirr = value &0xffffffff ;given that irq and value are ints, is there any possible way value could have bits outside of the low 32 set? If you're worried about sign extension isn't it simpler to just make value unsigned?
drivers/infiniband/hw/ehca/hcp_if.c u64 hipz_h_error_data(const struct ipz_adapter_handle adapter_handle, · line 946
Quoted 2 lines
drivers/infiniband/hw/ehca/hcp_if.c +2
-0+return plpar_hcall_norets (H_EOI , xirr); +}ie why not:
Quoted 9 lines
u64
hipz_h_eoi(int irq){ unsigned xirr; iosync(); xirr = (0xff << 24) | irq; return plpar_hcall_norets(H_EOI, xirr); }
Yeah, you are rigth I will change that with the final patch. I will send the final patch soon.
regards Stefan
Quoted 6 lines · Stefan Roscher
During corner case testing, we noticed that some versions of ehca do not properly transition to interrupt done in special load situations. This can be resolved by periodically triggering EOI through
H_EOI, if eqes are pending.Signed-off-by Stefan Roscher <[email protected]>
This patch is fine with me as long as the FW/HW people can confirm that calling spurrious EOI's like that will not affect other interrupts.
The side effect of writing 0xff to the xirr should be irrelevant as long as this is not done from within a HW interrupt handler (timer interrupts or softirqs are fine).
Due to the already incestuous relationship between HCA and the hypervisor, I don't mind having the H call directly in the driver.
So as long as the FW/HW people are ok with that workaround, then it has my ack as well.
---
Quoted 6 lines · Stefan Roscher
During corner case testing, we noticed that some versions of ehca do not properly transition to interrupt done in special load situations. This can be resolved by periodically triggering EOI through
H_EOI, if eqes are pending.Signed-off-by Stefan Roscher <[email protected]>
If this is OKed by the hypervisor team, then you can add:
Paul.