* [PATCH] x86: ioapic needs check attr when programmed more than once
@ 2013-08-22 8:46 Liu Ping Fan
2013-08-23 7:30 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Liu Ping Fan @ 2013-08-22 8:46 UTC (permalink / raw)
To: x86; +Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Kevin Hao
When programming ioapic pinX more than once, current code
does not check whether the later attr (trigger&polarity) is the
same as the former or not. This causes a broken semantic.
Fix it by reporting -EBUSY, when attr is different.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
arch/x86/kernel/acpi/boot.c | 5 ++++-
arch/x86/kernel/apic/io_apic.c | 7 ++++++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 230c8ea..9ef2ce9 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1113,6 +1113,7 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
int ioapic;
int ioapic_pin;
struct io_apic_irq_attr irq_attr;
+ int ret;
if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC)
return gsi;
@@ -1142,7 +1143,9 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
set_io_apic_irq_attr(&irq_attr, ioapic, ioapic_pin,
trigger == ACPI_EDGE_SENSITIVE ? 0 : 1,
polarity == ACPI_ACTIVE_HIGH ? 0 : 1);
- io_apic_set_pci_routing(dev, gsi_to_irq(gsi), &irq_attr);
+ ret = io_apic_set_pci_routing(dev, gsi_to_irq(gsi), &irq_attr);
+ if (ret < 0)
+ gsi = INT_MIN;
return gsi;
}
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 9ed796c..ce31d902 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3375,12 +3375,17 @@ int io_apic_setup_irq_pin_once(unsigned int irq, int node,
{
unsigned int ioapic_idx = attr->ioapic, pin = attr->ioapic_pin;
int ret;
+ struct IO_APIC_route_entry orig;
/* Avoid redundant programming */
if (test_bit(pin, ioapics[ioapic_idx].pin_programmed)) {
pr_debug("Pin %d-%d already programmed\n",
mpc_ioapic_id(ioapic_idx), pin);
- return 0;
+ orig = ioapic_read_entry(attr->ioapic, pin);
+ if (attr->trigger == orig.trigger &&
+ attr->polarity == orig.polarity)
+ return 0;
+ return -EBUSY;
}
ret = io_apic_setup_irq_pin(irq, node, attr);
if (!ret)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] x86: ioapic needs check attr when programmed more than once
2013-08-22 8:46 [PATCH] x86: ioapic needs check attr when programmed more than once Liu Ping Fan
@ 2013-08-23 7:30 ` Ingo Molnar
2013-08-23 8:04 ` Liu ping fan
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2013-08-23 7:30 UTC (permalink / raw)
To: Liu Ping Fan
Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Kevin Hao
* Liu Ping Fan <kernelfans@gmail.com> wrote:
> When programming ioapic pinX more than once, current code
> does not check whether the later attr (trigger&polarity) is the
> same as the former or not. This causes a broken semantic.
>
> Fix it by reporting -EBUSY, when attr is different.
Was this observed in real life somehow, and if yes, what is
the before/after behavior?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: ioapic needs check attr when programmed more than once
2013-08-23 7:30 ` Ingo Molnar
@ 2013-08-23 8:04 ` Liu ping fan
2013-08-23 8:09 ` Liu ping fan
0 siblings, 1 reply; 6+ messages in thread
From: Liu ping fan @ 2013-08-23 8:04 UTC (permalink / raw)
To: Ingo Molnar
Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Kevin Hao
On Fri, Aug 23, 2013 at 3:30 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Liu Ping Fan <kernelfans@gmail.com> wrote:
>
>> When programming ioapic pinX more than once, current code
>> does not check whether the later attr (trigger&polarity) is the
>> same as the former or not. This causes a broken semantic.
>>
>> Fix it by reporting -EBUSY, when attr is different.
>
> Was this observed in real life somehow, and if yes, what is
> the before/after behavior?
>
Using qemu q35 machine, I found the ioapic's ioredtbl[] will never has
low-active, even the hpet driver registered it. After tracing, I found
it shared a high-level active IRQ line with other device. So in fact,
the acpi_register_gsi(, ACPI_ACTIVE_LOW) in hpet driver fail, but it
did not detect it.
The effect in qemu: when hpet-dev assert low-level, the kernel can not respond.
Thanks,
Pingfan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: ioapic needs check attr when programmed more than once
2013-08-23 8:04 ` Liu ping fan
@ 2013-08-23 8:09 ` Liu ping fan
2013-08-23 8:23 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Liu ping fan @ 2013-08-23 8:09 UTC (permalink / raw)
To: Ingo Molnar
Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Kevin Hao
On Fri, Aug 23, 2013 at 4:04 PM, Liu ping fan <kernelfans@gmail.com> wrote:
> On Fri, Aug 23, 2013 at 3:30 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Liu Ping Fan <kernelfans@gmail.com> wrote:
>>
>>> When programming ioapic pinX more than once, current code
>>> does not check whether the later attr (trigger&polarity) is the
>>> same as the former or not. This causes a broken semantic.
>>>
>>> Fix it by reporting -EBUSY, when attr is different.
>>
>> Was this observed in real life somehow, and if yes, what is
>> the before/after behavior?
>>
> Using qemu q35 machine, I found the ioapic's ioredtbl[] will never has
> low-active, even the hpet driver registered it. After tracing, I found
> it shared a high-level active IRQ line with other device. So in fact,
> the acpi_register_gsi(, ACPI_ACTIVE_LOW) in hpet driver fail, but it
> did not detect it.
> The effect in qemu: when hpet-dev assert low-level, the kernel can not respond.
>
After changing, I can observe the low-active be set in ioredtbl[x],
and with some bug fix in qemu's hpet, the kernel can work.
>
> Thanks,
> Pingfan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: ioapic needs check attr when programmed more than once
2013-08-23 8:09 ` Liu ping fan
@ 2013-08-23 8:23 ` Ingo Molnar
0 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2013-08-23 8:23 UTC (permalink / raw)
To: Liu ping fan
Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Kevin Hao
* Liu ping fan <kernelfans@gmail.com> wrote:
> On Fri, Aug 23, 2013 at 4:04 PM, Liu ping fan <kernelfans@gmail.com> wrote:
> > On Fri, Aug 23, 2013 at 3:30 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >>
> >> * Liu Ping Fan <kernelfans@gmail.com> wrote:
> >>
> >>> When programming ioapic pinX more than once, current code
> >>> does not check whether the later attr (trigger&polarity) is the
> >>> same as the former or not. This causes a broken semantic.
> >>>
> >>> Fix it by reporting -EBUSY, when attr is different.
> >>
> >> Was this observed in real life somehow, and if yes, what is
> >> the before/after behavior?
> >>
> > Using qemu q35 machine, I found the ioapic's ioredtbl[] will never has
> > low-active, even the hpet driver registered it. After tracing, I found
> > it shared a high-level active IRQ line with other device. So in fact,
> > the acpi_register_gsi(, ACPI_ACTIVE_LOW) in hpet driver fail, but it
> > did not detect it.
> > The effect in qemu: when hpet-dev assert low-level, the kernel can not respond.
> >
> After changing, I can observe the low-active be set in ioredtbl[x],
> and with some bug fix in qemu's hpet, the kernel can work.
Okay, so because in practice this kind of information is
much more important to users than anything else in the
changelog please put all this into the changelog and
re-send.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] x86: ioapic needs check attr when programmed more than once
@ 2013-08-23 8:58 Liu Ping Fan
0 siblings, 0 replies; 6+ messages in thread
From: Liu Ping Fan @ 2013-08-23 8:58 UTC (permalink / raw)
To: x86; +Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Kevin Hao
When programming ioapic pinX more than once, current code
does not check whether the later attr (trigger&polarity) is the
same as the former or not. This causes a broken semantic.
This can be observed in qemu q35 machine, where ioapic's ioredtbl[x]
can never be set as low-active, even if the hpet driver registered it.
And hpet driver may share a high-level active IRQ line with other
device. So in qemu, when hpet-dev asserts low-level as kernel expects,
the kernel has no response. With this patch, we can observe an
ioredtbl[x] set as low-active for hpet.
Fix it by reporting -EBUSY to the caller, when attr is different.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
arch/x86/kernel/acpi/boot.c | 5 ++++-
arch/x86/kernel/apic/io_apic.c | 7 ++++++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 230c8ea..9ef2ce9 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1113,6 +1113,7 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
int ioapic;
int ioapic_pin;
struct io_apic_irq_attr irq_attr;
+ int ret;
if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC)
return gsi;
@@ -1142,7 +1143,9 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
set_io_apic_irq_attr(&irq_attr, ioapic, ioapic_pin,
trigger == ACPI_EDGE_SENSITIVE ? 0 : 1,
polarity == ACPI_ACTIVE_HIGH ? 0 : 1);
- io_apic_set_pci_routing(dev, gsi_to_irq(gsi), &irq_attr);
+ ret = io_apic_set_pci_routing(dev, gsi_to_irq(gsi), &irq_attr);
+ if (ret < 0)
+ gsi = INT_MIN;
return gsi;
}
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 9ed796c..ce31d902 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3375,12 +3375,17 @@ int io_apic_setup_irq_pin_once(unsigned int irq, int node,
{
unsigned int ioapic_idx = attr->ioapic, pin = attr->ioapic_pin;
int ret;
+ struct IO_APIC_route_entry orig;
/* Avoid redundant programming */
if (test_bit(pin, ioapics[ioapic_idx].pin_programmed)) {
pr_debug("Pin %d-%d already programmed\n",
mpc_ioapic_id(ioapic_idx), pin);
- return 0;
+ orig = ioapic_read_entry(attr->ioapic, pin);
+ if (attr->trigger == orig.trigger &&
+ attr->polarity == orig.polarity)
+ return 0;
+ return -EBUSY;
}
ret = io_apic_setup_irq_pin(irq, node, attr);
if (!ret)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-23 8:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-22 8:46 [PATCH] x86: ioapic needs check attr when programmed more than once Liu Ping Fan
2013-08-23 7:30 ` Ingo Molnar
2013-08-23 8:04 ` Liu ping fan
2013-08-23 8:09 ` Liu ping fan
2013-08-23 8:23 ` Ingo Molnar
-- strict thread matches above, loose matches on Subject: below --
2013-08-23 8:58 Liu Ping Fan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox