> > 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:
> +u64 hipz_h_eoi(int irq)
> +{
> + int value;
> + unsigned long xirr;
> +
> + iosync();
what is the iosync() required for here?
> + 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?
> + 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);
}
Roland Dreier writes:
> > 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)?
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,
On Tuesday 10 June 2008 18:18:50 Roland Dreier wrote:
> > 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?
No, it's ok with me if you pick this for 2.6.27.
Also, out of curiousity:
> +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.
> + 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?
> + 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);
}
Yeah, you are rigth I will change that with the final patch.
I will send the final patch soon.
regards Stefan