Re: [Xen-devel] [PATCH RFC 3/3] x86/hvm/rtc: Always deassert the IRQ line when clearing REG_C.IRQF.

[Tim Deegan <tim@xxxxxxx>]

At 13:46 +0000 on 14 Feb (1392381999), Jan Beulich wrote:
> >>> On 13.02.14 at 18:32, Tim Deegan <tim@xxxxxxx> wrote:
> > Even in no-ack mode, there's no reason to leave the line asserted
> > after an explicit ack of the interrupt.
> > 
> > Signed-off-by: Tim Deegan <tim@xxxxxxx>
> > ---
> >  xen/arch/x86/hvm/rtc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> > index 18a4fe8..b592547 100644
> > --- a/xen/arch/x86/hvm/rtc.c
> > +++ b/xen/arch/x86/hvm/rtc.c
> > @@ -674,7 +674,7 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t 
> > addr)
> >          check_for_pf_ticks(s);
> >          ret = s->hw.cmos_data[s->hw.cmos_index];
> >          s->hw.cmos_data[RTC_REG_C] = 0x00;
> > -        if ( (ret & RTC_IRQF) && !rtc_mode_is(s, no_ack) )
> > +        if ( ret & RTC_IRQF )
> >              hvm_isa_irq_deassert(d, RTC_IRQ);
> >          rtc_update_irq(s);
> >          check_update_timer(s);
> 
> Wait - does one of the earlier patches remove the other de-assert?
> Looking... No, they don't. Doing it in exactly one of the two places
> should be sufficient, shouldn't it?

Yes; it just seemed odd to leave it asserted in some cases when the
hardware wouldn't.  Given that we know the line is never shared and
that it should always be edge-sensitive in the IOAPIC, it doesn't
matter very much. 

> The more that the other de-assert
> is in rtc_update_irq(), which is being called right afterwards. 
> 
> But then again - that call seems pointless (I think I mentioned this in
> response to Andrew's first attempt): Since REG_C is now clear, the
> first conditional return path in that function will never be taken, and
> the second one always will be.
>
> So I think together with removing that call, and considering the
> intended level-ness of the IRQ, I think I agree with you after all,

Right; this patch could just drop that call too.  I think the original
idea was that having all the irq frobbing in one place was a good
idea.  So maybe the better choice is to make sure that
rtc_update_irq() DTRT and just drop the deassert from here altogether.

> provided two de-asserts with no assert in between are not a
> problem.

They shouldn't be -- the _isa_irq_[de]assert operations are idempotent.

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel