b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
of legacy interrupts when actually nr_legacy_irqs() returns 0 after
probe_8259A(). Use NR_IRQS_LEGACY instead.
Signed-off-by: Stefano Stabellini <[email protected]>
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index beac4df..349b8ce 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -491,8 +491,11 @@ int __init pci_xen_initial_domain(void)
#endif
__acpi_register_gsi = acpi_register_gsi_xen;
__acpi_unregister_gsi = NULL;
- /* Pre-allocate legacy irqs */
- for (irq = 0; irq < nr_legacy_irqs(); irq++) {
+ /*
+ * Pre-allocate the legacy IRQs. Use NR_LEGACY_IRQS here
+ * because we don't have a PIC and thus nr_legacy_irqs() is zero.
+ */
+ for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
int trigger, polarity;
if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
On 20/04/16 15:15, Stefano Stabellini wrote:
> b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
> of legacy interrupts when actually nr_legacy_irqs() returns 0 after
> probe_8259A(). Use NR_IRQS_LEGACY instead.
Would you mind describing the resulting problem? With this commit
message I'm absolutely not capable to decide whether e.g. the other
use of nr_legacy_irqs() in pci_xen_initial_domain() is correct or
not.
Juergen
>
> Signed-off-by: Stefano Stabellini <[email protected]>
>
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index beac4df..349b8ce 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -491,8 +491,11 @@ int __init pci_xen_initial_domain(void)
> #endif
> __acpi_register_gsi = acpi_register_gsi_xen;
> __acpi_unregister_gsi = NULL;
> - /* Pre-allocate legacy irqs */
> - for (irq = 0; irq < nr_legacy_irqs(); irq++) {
> + /*
> + * Pre-allocate the legacy IRQs. Use NR_LEGACY_IRQS here
> + * because we don't have a PIC and thus nr_legacy_irqs() is zero.
> + */
> + for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
> int trigger, polarity;
>
> if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
>
On Thu, 21 Apr 2016, Juergen Gross wrote:
> On 20/04/16 15:15, Stefano Stabellini wrote:
> > b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
> > of legacy interrupts when actually nr_legacy_irqs() returns 0 after
> > probe_8259A(). Use NR_IRQS_LEGACY instead.
>
> Would you mind describing the resulting problem?
This is a good question. The symptom is:
ata_piix: probe of 0000:00:01.1 failed with error -22
> With this commit message I'm absolutely not capable to decide whether
> e.g. the other use of nr_legacy_irqs() in pci_xen_initial_domain() is
> correct or not.
I looked at it but I couldn't really test that code because if I try to
change the number of ioapics in the system using the "noapic" command
line option (which actually changes the number if ioapics, not lapics),
I get an error from Linux saying that noapic is not supported when
running on Xen.
In my opinion having nr_legacy_irqs() calls in Xen code, which returns
0, is like playing with fire. I think it would be safer/saner to replace
them all with NR_IRQS_LEGACY, simply because reading the code one would
not expect that all those loops don't actually have any iterations.
However I didn't make the change because I couldn't test it properly.
> > Signed-off-by: Stefano Stabellini <[email protected]>
> >
> > diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> > index beac4df..349b8ce 100644
> > --- a/arch/x86/pci/xen.c
> > +++ b/arch/x86/pci/xen.c
> > @@ -491,8 +491,11 @@ int __init pci_xen_initial_domain(void)
> > #endif
> > __acpi_register_gsi = acpi_register_gsi_xen;
> > __acpi_unregister_gsi = NULL;
> > - /* Pre-allocate legacy irqs */
> > - for (irq = 0; irq < nr_legacy_irqs(); irq++) {
> > + /*
> > + * Pre-allocate the legacy IRQs. Use NR_LEGACY_IRQS here
> > + * because we don't have a PIC and thus nr_legacy_irqs() is zero.
> > + */
> > + for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
> > int trigger, polarity;
> >
> > if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
> >
>
On Wed, Apr 20, Stefano Stabellini wrote:
> b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
> of legacy interrupts when actually nr_legacy_irqs() returns 0 after
> probe_8259A(). Use NR_IRQS_LEGACY instead.
>
> Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Olaf Hering <[email protected]>
Fixes my M9400 laptop (and should go to stable as well):
http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg02624.html
Olaf
On 21/04/16 11:30, Stefano Stabellini wrote:
> On Thu, 21 Apr 2016, Juergen Gross wrote:
>> On 20/04/16 15:15, Stefano Stabellini wrote:
>>> b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
>>> of legacy interrupts when actually nr_legacy_irqs() returns 0 after
>>> probe_8259A(). Use NR_IRQS_LEGACY instead.
>>
>> Would you mind describing the resulting problem?
>
> This is a good question. The symptom is:
>
> ata_piix: probe of 0000:00:01.1 failed with error -22
>
>
>> With this commit message I'm absolutely not capable to decide whether
>> e.g. the other use of nr_legacy_irqs() in pci_xen_initial_domain() is
>> correct or not.
>
> I looked at it but I couldn't really test that code because if I try to
> change the number of ioapics in the system using the "noapic" command
> line option (which actually changes the number if ioapics, not lapics),
> I get an error from Linux saying that noapic is not supported when
> running on Xen.
>
> In my opinion having nr_legacy_irqs() calls in Xen code, which returns
> 0, is like playing with fire. I think it would be safer/saner to replace
> them all with NR_IRQS_LEGACY, simply because reading the code one would
> not expect that all those loops don't actually have any iterations.
I'm quite sure you should change both uses of nr_legacy_irqs() in
pci_xen_initial_domain().
Looking at xen_pcifront_enable_irq() I'm not really sure what is the
correct thing to do.
Adding Konrad as he might have a better insight.
Juergen
>
> However I didn't make the change because I couldn't test it properly.
>
>
>>> Signed-off-by: Stefano Stabellini <[email protected]>
>>>
>>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>>> index beac4df..349b8ce 100644
>>> --- a/arch/x86/pci/xen.c
>>> +++ b/arch/x86/pci/xen.c
>>> @@ -491,8 +491,11 @@ int __init pci_xen_initial_domain(void)
>>> #endif
>>> __acpi_register_gsi = acpi_register_gsi_xen;
>>> __acpi_unregister_gsi = NULL;
>>> - /* Pre-allocate legacy irqs */
>>> - for (irq = 0; irq < nr_legacy_irqs(); irq++) {
>>> + /*
>>> + * Pre-allocate the legacy IRQs. Use NR_LEGACY_IRQS here
>>> + * because we don't have a PIC and thus nr_legacy_irqs() is zero.
>>> + */
>>> + for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
>>> int trigger, polarity;
>>>
>>> if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
>>>
>>
>
>
On 27/04/16 06:02, Juergen Gross wrote:
> On 21/04/16 11:30, Stefano Stabellini wrote:
>> On Thu, 21 Apr 2016, Juergen Gross wrote:
>>> On 20/04/16 15:15, Stefano Stabellini wrote:
>>>> b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
>>>> of legacy interrupts when actually nr_legacy_irqs() returns 0 after
>>>> probe_8259A(). Use NR_IRQS_LEGACY instead.
>>>
>>> Would you mind describing the resulting problem?
>>
>> This is a good question. The symptom is:
>>
>> ata_piix: probe of 0000:00:01.1 failed with error -22
>>
>>
>>> With this commit message I'm absolutely not capable to decide whether
>>> e.g. the other use of nr_legacy_irqs() in pci_xen_initial_domain() is
>>> correct or not.
>>
>> I looked at it but I couldn't really test that code because if I try to
>> change the number of ioapics in the system using the "noapic" command
>> line option (which actually changes the number if ioapics, not lapics),
>> I get an error from Linux saying that noapic is not supported when
>> running on Xen.
>>
>> In my opinion having nr_legacy_irqs() calls in Xen code, which returns
>> 0, is like playing with fire. I think it would be safer/saner to replace
>> them all with NR_IRQS_LEGACY, simply because reading the code one would
>> not expect that all those loops don't actually have any iterations.
>
> I'm quite sure you should change both uses of nr_legacy_irqs() in
> pci_xen_initial_domain().
>
> Looking at xen_pcifront_enable_irq() I'm not really sure what is the
> correct thing to do.
>
> Adding Konrad as he might have a better insight.
I wonder if it would be helpful to have a xen-specific #define like
XEN_NR_LEGACY_PIRQS or something, and document carefully what this means
and why it is != nr_legacy_irqs().
David
On 04/27/2016 05:35 AM, David Vrabel wrote:
> On 27/04/16 06:02, Juergen Gross wrote:
>> On 21/04/16 11:30, Stefano Stabellini wrote:
>>> On Thu, 21 Apr 2016, Juergen Gross wrote:
>>>> On 20/04/16 15:15, Stefano Stabellini wrote:
>>>>> b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
>>>>> of legacy interrupts when actually nr_legacy_irqs() returns 0 after
>>>>> probe_8259A(). Use NR_IRQS_LEGACY instead.
>>>> Would you mind describing the resulting problem?
>>> This is a good question. The symptom is:
>>>
>>> ata_piix: probe of 0000:00:01.1 failed with error -22
>>>
>>>
>>>> With this commit message I'm absolutely not capable to decide whether
>>>> e.g. the other use of nr_legacy_irqs() in pci_xen_initial_domain() is
>>>> correct or not.
>>> I looked at it but I couldn't really test that code because if I try to
>>> change the number of ioapics in the system using the "noapic" command
>>> line option (which actually changes the number if ioapics, not lapics),
>>> I get an error from Linux saying that noapic is not supported when
>>> running on Xen.
>>>
>>> In my opinion having nr_legacy_irqs() calls in Xen code, which returns
>>> 0, is like playing with fire. I think it would be safer/saner to replace
>>> them all with NR_IRQS_LEGACY, simply because reading the code one would
>>> not expect that all those loops don't actually have any iterations.
>> I'm quite sure you should change both uses of nr_legacy_irqs() in
>> pci_xen_initial_domain().
>>
>> Looking at xen_pcifront_enable_irq() I'm not really sure what is the
>> correct thing to do.
>>
>> Adding Konrad as he might have a better insight.
> I wonder if it would be helpful to have a xen-specific #define like
> XEN_NR_LEGACY_PIRQS or something, and document carefully what this means
> and why it is != nr_legacy_irqs().
int xen_nr_legacy_irqs()
{
if (xen_hvm_domain())
return nr_legacy_irqs();
if (xen_initial_domain())
return NR_IRQS_LEGACY;
return 0;
}
?
-boris
On 27/04/16 14:38, Boris Ostrovsky wrote:
>
> int xen_nr_legacy_irqs()
> {
> if (xen_hvm_domain())
> return nr_legacy_irqs();
> if (xen_initial_domain())
> return NR_IRQS_LEGACY;
> return 0;
> }
Yeah, if that does the right thing...
David
On 04/27/2016 09:40 AM, David Vrabel wrote:
> On 27/04/16 14:38, Boris Ostrovsky wrote:
>> int xen_nr_legacy_irqs()
>> {
>> if (xen_hvm_domain())
>> return nr_legacy_irqs();
>> if (xen_initial_domain())
>> return NR_IRQS_LEGACY;
>> return 0;
>> }
> Yeah, if that does the right thing...
I think it will break xen_allocate_irq_gsi() again, unless we check for
HVM domain explicitly. Which would be ugly.
-boris