* x86 HPET MSI IRQs vs resume from S3
@ 2011-01-28 9:23 Jan Beulich
2011-02-01 2:36 ` Li, Xin
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jan Beulich @ 2011-01-28 9:23 UTC (permalink / raw)
To: xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: x86 HPET MSI IRQs vs resume from S3
2011-01-28 9:23 x86 HPET MSI IRQs vs resume from S3 Jan Beulich
@ 2011-02-01 2:36 ` Li, Xin
2011-02-14 1:00 ` Wei, Gang
2011-02-14 9:30 ` [PATCH] x86: Fix S3 resume for HPET MSI IRQ case (RE: x86 HPET MSI IRQs vs resume from S3) Wei, Gang
2 siblings, 0 replies; 7+ messages in thread
From: Li, Xin @ 2011-02-01 2:36 UTC (permalink / raw)
To: Jan Beulich, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: x86 HPET MSI IRQs vs resume from S3
2011-01-28 9:23 x86 HPET MSI IRQs vs resume from S3 Jan Beulich
2011-02-01 2:36 ` Li, Xin
@ 2011-02-14 1:00 ` Wei, Gang
2011-02-14 8:35 ` Jan Beulich
2011-02-14 9:30 ` [PATCH] x86: Fix S3 resume for HPET MSI IRQ case (RE: x86 HPET MSI IRQs vs resume from S3) Wei, Gang
2 siblings, 1 reply; 7+ messages in thread
From: Wei, Gang @ 2011-02-14 1:00 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei, Gang
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: x86 HPET MSI IRQs vs resume from S3
2011-02-14 1:00 ` Wei, Gang
@ 2011-02-14 8:35 ` Jan Beulich
2011-02-14 8:48 ` Wei, Gang
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2011-02-14 8:35 UTC (permalink / raw)
To: Gang Wei; +Cc: 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: x86 HPET MSI IRQs vs resume from S3
2011-02-14 8:35 ` Jan Beulich
@ 2011-02-14 8:48 ` Wei, Gang
0 siblings, 0 replies; 7+ messages in thread
From: Wei, Gang @ 2011-02-14 8:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Wei, Gang
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] x86: Fix S3 resume for HPET MSI IRQ case (RE: x86 HPET MSI IRQs vs resume from S3)
2011-01-28 9:23 x86 HPET MSI IRQs vs resume from S3 Jan Beulich
2011-02-01 2:36 ` Li, Xin
2011-02-14 1:00 ` Wei, Gang
@ 2011-02-14 9:30 ` Wei, Gang
2011-02-14 9:59 ` Jan Beulich
2 siblings, 1 reply; 7+ messages in thread
From: Wei, Gang @ 2011-02-14 9:30 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser, Wei, Gang
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().
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] x86: Fix S3 resume for HPET MSI IRQ case (RE: x86 HPET MSI IRQs vs resume from S3)
2011-02-14 9:30 ` [PATCH] x86: Fix S3 resume for HPET MSI IRQ case (RE: x86 HPET MSI IRQs vs resume from S3) Wei, Gang
@ 2011-02-14 9:59 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2011-02-14 9:59 UTC (permalink / raw)
To: Gang Wei; +Cc: xen-devel, Keir Fraser
>>> 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().
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-02-14 9:59 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-28 9:23 x86 HPET MSI IRQs vs resume from S3 Jan Beulich
2011-02-01 2:36 ` Li, Xin
2011-02-14 1:00 ` Wei, Gang
2011-02-14 8:35 ` Jan Beulich
2011-02-14 8:48 ` Wei, Gang
2011-02-14 9:30 ` [PATCH] x86: Fix S3 resume for HPET MSI IRQ case (RE: x86 HPET MSI IRQs vs resume from S3) Wei, Gang
2011-02-14 9:59 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.