Going through hpet_broadcast_init(), I see that hpet_setup_msi_irq() gets called during resume, thus causing setup_irq() to be called. I''m failing to spot the corresponding release_irq(), and hence I can''t see how this whole code path is supposed to work during resume (other than always falling back to using legacy_hpet_event). Instead I''m wondering whether in the resume case only msi_compose_msg()/ hpet_msi_write() should be called for each IRQ used rather than the whole hpet_broadcast_init(). Thanks for any hints or pointers, Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi Jan, PRC are in Chinese New Year holiday, we will work on it after that. Thanks! -Xin> -----Original Message----- > From: xen-devel-bounces@lists.xensource.com > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Jan Beulich > Sent: Friday, January 28, 2011 5:24 PM > To: xen-devel@lists.xensource.com > Subject: [Xen-devel] x86 HPET MSI IRQs vs resume from S3 > > Going through hpet_broadcast_init(), I see that hpet_setup_msi_irq() > gets called during resume, thus causing setup_irq() to be called. I''m > failing to spot the corresponding release_irq(), and hence I can''t see > how this whole code path is supposed to work during resume (other > than always falling back to using legacy_hpet_event). Instead I''m > wondering whether in the resume case only msi_compose_msg()/ > hpet_msi_write() should be called for each IRQ used rather than the > whole hpet_broadcast_init(). > > Thanks for any hints or pointers, > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich wrote on 2011-01-28:> Going through hpet_broadcast_init(), I see that hpet_setup_msi_irq() > gets called during resume, thus causing setup_irq() to be called. I''m > failing to spot the corresponding release_irq(), and hence I can''t see > how this whole code path is supposed to work during resume (other than > always falling back to using legacy_hpet_event). Instead I''m wondering > whether in the resume case only msi_compose_msg()/ > hpet_msi_write() should be called for each IRQ used rather than the > whole hpet_broadcast_init().I do think below patch could resolve this issue well. Didn''t create a new path for hpet broadcast init while resume because there exists many condition checks in existing path. diff -r 67f2fed57034 xen/arch/x86/hpet.c --- a/xen/arch/x86/hpet.c Fri Feb 11 18:22:37 2011 +0000 +++ b/xen/arch/x86/hpet.c Tue Feb 15 14:48:54 2011 +0800 @@ -367,12 +367,20 @@ static int hpet_setup_msi_irq(unsigned i int ret; struct msi_msg msg; struct hpet_event_channel *ch = &hpet_events[irq_to_channel(irq)]; - - irq_desc[irq].handler = &hpet_msi_type; - ret = request_irq(irq, hpet_interrupt_handler, - 0, "HPET", ch); - if ( ret < 0 ) - return ret; + irq_desc_t *desc = irq_to_desc(irq); + + if ( desc->handler == &no_irq_type ) + { + desc->handler = &hpet_msi_type; + ret = request_irq(irq, hpet_interrupt_handler, + 0, "HPET", ch); + if ( ret < 0 ) + return ret; + } + else if ( desc->handler != &hpet_msi_type ) + { + return -EINVAL; + } msi_compose_msg(NULL, irq, &msg); hpet_msi_write(irq, &msg); Jimmy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 14.02.11 at 02:00, "Wei, Gang" <gang.wei@intel.com> wrote: > Jan Beulich wrote on 2011-01-28: >> Going through hpet_broadcast_init(), I see that hpet_setup_msi_irq() >> gets called during resume, thus causing setup_irq() to be called. I''m >> failing to spot the corresponding release_irq(), and hence I can''t see >> how this whole code path is supposed to work during resume (other than >> always falling back to using legacy_hpet_event). Instead I''m wondering >> whether in the resume case only msi_compose_msg()/ >> hpet_msi_write() should be called for each IRQ used rather than the >> whole hpet_broadcast_init(). > > I do think below patch could resolve this issue well. Didn''t create a new > path for hpet broadcast init while resume because there exists many condition > checks in existing path.Yes, for 4.1 this seems a reasonable fix. Post-4.1 I''d prefer to split the code paths so that failure during resume (which shouldn''t happen anyway) would lead to e.g. calling destroy_irq(), and also to get more stuff __init-annotated (with your change here, hpet_assign_irq() in the resume path wouldn''t call create_irq() and request_irq() [and shouldn''t call destroy_irq() as said above], and the full hpet_fsb_cap_lookup() doesn''t look to be necessary in the resume path either). Jan> --- a/xen/arch/x86/hpet.c Fri Feb 11 18:22:37 2011 +0000 > +++ b/xen/arch/x86/hpet.c Tue Feb 15 14:48:54 2011 +0800 > @@ -367,12 +367,20 @@ static int hpet_setup_msi_irq(unsigned i > int ret; > struct msi_msg msg; > struct hpet_event_channel *ch = &hpet_events[irq_to_channel(irq)]; > - > - irq_desc[irq].handler = &hpet_msi_type; > - ret = request_irq(irq, hpet_interrupt_handler, > - 0, "HPET", ch); > - if ( ret < 0 ) > - return ret; > + irq_desc_t *desc = irq_to_desc(irq); > + > + if ( desc->handler == &no_irq_type ) > + { > + desc->handler = &hpet_msi_type; > + ret = request_irq(irq, hpet_interrupt_handler, > + 0, "HPET", ch); > + if ( ret < 0 ) > + return ret; > + } > + else if ( desc->handler != &hpet_msi_type ) > + { > + return -EINVAL; > + } > > msi_compose_msg(NULL, irq, &msg); > hpet_msi_write(irq, &msg); > > Jimmy_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich wrote on 2011-02-14:>>>> On 14.02.11 at 02:00, "Wei, Gang" <gang.wei@intel.com> wrote: >> Jan Beulich wrote on 2011-01-28: >>> Going through hpet_broadcast_init(), I see that >>> hpet_setup_msi_irq() gets called during resume, thus causing >>> setup_irq() to be called. I''m failing to spot the corresponding >>> release_irq(), and hence I can''t see how this whole code path is >>> supposed to work during resume (other than always falling back to >>> using legacy_hpet_event). Instead I''m wondering whether in the >>> resume case only msi_compose_msg()/ >>> hpet_msi_write() should be called for each IRQ used rather than the >>> whole hpet_broadcast_init(). >> >> I do think below patch could resolve this issue well. Didn''t create >> a new path for hpet broadcast init while resume because there exists >> many condition checks in existing path. > > Yes, for 4.1 this seems a reasonable fix. Post-4.1 I''d prefer to split > the code paths so that failure during resume (which shouldn''t happen > anyway) would lead to e.g. calling destroy_irq(), and also to get more > stuff __init-annotated (with your change here, > hpet_assign_irq() in the resume path wouldn''t call create_irq() and > request_irq() [and shouldn''t call destroy_irq() as said above], and > the full > hpet_fsb_cap_lookup() doesn''t look to be necessary in the resume path either).I agree. I will send a 4.1 fix patch first and a little bit later a Post-4.1 clean up patch just as you suggested above. Thanks for finding and raising this issue. Jimmy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei, Gang
2011-Feb-14 09:30 UTC
[PATCH] x86: Fix S3 resume for HPET MSI IRQ case (RE: [Xen-devel] x86 HPET MSI IRQs vs resume from S3)
x86: Fix S3 resume for HPET MSI IRQ case
Jan Beulich found that for S3 resume on platforms without ARAT feature but with
MSI capable HPET, request_irq() will be called in hpet_setup_msi_irq() for irq
already setup(no release_irq() called during S3 suspend), so that always falling
back to using legacy_hpet_event.
Fix it by conditional calling request_irq() for 4.1. Planned to split the S3
resume path from booting path post 4.1, as Jan suggested.
Signed-off-by: Wei Gang <gang.wei@intel.com>
diff -r 67f2fed57034 xen/arch/x86/hpet.c
--- a/xen/arch/x86/hpet.c Fri Feb 11 18:22:37 2011 +0000
+++ b/xen/arch/x86/hpet.c Tue Feb 15 14:48:54 2011 +0800
@@ -367,12 +367,20 @@ static int hpet_setup_msi_irq(unsigned i
int ret;
struct msi_msg msg;
struct hpet_event_channel *ch = &hpet_events[irq_to_channel(irq)];
-
- irq_desc[irq].handler = &hpet_msi_type;
- ret = request_irq(irq, hpet_interrupt_handler,
- 0, "HPET", ch);
- if ( ret < 0 )
- return ret;
+ irq_desc_t *desc = irq_to_desc(irq);
+
+ if ( desc->handler == &no_irq_type )
+ {
+ desc->handler = &hpet_msi_type;
+ ret = request_irq(irq, hpet_interrupt_handler,
+ 0, "HPET", ch);
+ if ( ret < 0 )
+ return ret;
+ }
+ else if ( desc->handler != &hpet_msi_type )
+ {
+ return -EINVAL;
+ }
msi_compose_msg(NULL, irq, &msg);
hpet_msi_write(irq, &msg);
Jimmy
Jan Beulich wrote on 2011-01-28:> Going through hpet_broadcast_init(), I see that hpet_setup_msi_irq()
> gets called during resume, thus causing setup_irq() to be called.
I''m
> failing to spot the corresponding release_irq(), and hence I can''t
see
> how this whole code path is supposed to work during resume (other than
> always falling back to using legacy_hpet_event). Instead I''m
wondering
> whether in the resume case only msi_compose_msg()/
> hpet_msi_write() should be called for each IRQ used rather than the
> whole hpet_broadcast_init().
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jan Beulich
2011-Feb-14 09:59 UTC
[PATCH] x86: Fix S3 resume for HPET MSI IRQ case (RE: [Xen-devel] x86 HPET MSI IRQs vs resume from S3)
>>> On 14.02.11 at 10:30, "Wei, Gang" <gang.wei@intel.com> wrote: > x86: Fix S3 resume for HPET MSI IRQ case > > Jan Beulich found that for S3 resume on platforms without ARAT feature but > with MSI capable HPET, request_irq() will be called in hpet_setup_msi_irq() > for irq already setup(no release_irq() called during S3 suspend), so that > always falling back to using legacy_hpet_event. > > Fix it by conditional calling request_irq() for 4.1. Planned to split the S3 > resume path from booting path post 4.1, as Jan suggested. > > Signed-off-by: Wei Gang <gang.wei@intel.com>Acked-by: Jan Beulich <jbeulich@novell.com>> diff -r 67f2fed57034 xen/arch/x86/hpet.c > --- a/xen/arch/x86/hpet.c Fri Feb 11 18:22:37 2011 +0000 > +++ b/xen/arch/x86/hpet.c Tue Feb 15 14:48:54 2011 +0800 > @@ -367,12 +367,20 @@ static int hpet_setup_msi_irq(unsigned i > int ret; > struct msi_msg msg; > struct hpet_event_channel *ch = &hpet_events[irq_to_channel(irq)]; > - > - irq_desc[irq].handler = &hpet_msi_type; > - ret = request_irq(irq, hpet_interrupt_handler, > - 0, "HPET", ch); > - if ( ret < 0 ) > - return ret; > + irq_desc_t *desc = irq_to_desc(irq); > + > + if ( desc->handler == &no_irq_type ) > + { > + desc->handler = &hpet_msi_type; > + ret = request_irq(irq, hpet_interrupt_handler, > + 0, "HPET", ch); > + if ( ret < 0 ) > + return ret; > + } > + else if ( desc->handler != &hpet_msi_type ) > + { > + return -EINVAL; > + } > > msi_compose_msg(NULL, irq, &msg); > hpet_msi_write(irq, &msg); > > Jimmy > > Jan Beulich wrote on 2011-01-28: >> Going through hpet_broadcast_init(), I see that hpet_setup_msi_irq() >> gets called during resume, thus causing setup_irq() to be called. I''m >> failing to spot the corresponding release_irq(), and hence I can''t see >> how this whole code path is supposed to work during resume (other than >> always falling back to using legacy_hpet_event). Instead I''m wondering >> whether in the resume case only msi_compose_msg()/ >> hpet_msi_write() should be called for each IRQ used rather than the >> whole hpet_broadcast_init()._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel