| Message ID | 1476915664-27231-2-git-send-email-okaya@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Oct 20, 2016 at 12:21 AM, Sinan Kaya <okaya@codeaurora.org> wrote: > The penalty determination of ISA IRQ goes through 4 paths. > 1. assign PCI_USING during power up via acpi_irq_penalty_init. > 2. update the penalty with acpi_penalize_isa_irq function based on the > active parameter. > 3. kernel command line penalty update via acpi_irq_penalty_update function. > 4. increment the penalty as USING right after the IRQ is assign to PCI. > > acpi_penalize_isa_irq and acpi_irq_penalty_update functions get called > before the ACPI subsystem is started. > > These API need to bypass the acpi_irq_get_penalty function. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/acpi/pci_link.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > index c983bf7..4f37938 100644 > --- a/drivers/acpi/pci_link.c > +++ b/drivers/acpi/pci_link.c > @@ -849,7 +849,7 @@ static int __init acpi_irq_penalty_update(char *str, int used) > continue; > > if (used) > - new_penalty = acpi_irq_get_penalty(irq) + > + new_penalty = acpi_isa_irq_penalty[irq] + > PIRQ_PENALTY_ISA_USED; > else > new_penalty = 0; > @@ -871,7 +871,7 @@ static int __init acpi_irq_penalty_update(char *str, int used) > void acpi_penalize_isa_irq(int irq, int active) > { > if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_isa_irq_penalty))) > - acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) + > + acpi_isa_irq_penalty[irq] = acpi_isa_irq_penalty[irq] + This looks slightly odd. What about + acpi_isa_irq_penalty[irq] += > (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING); > } > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 19, 2016 at 06:21:02PM -0400, Sinan Kaya wrote: > The penalty determination of ISA IRQ goes through 4 paths. > 1. assign PCI_USING during power up via acpi_irq_penalty_init. > 2. update the penalty with acpi_penalize_isa_irq function based on the > active parameter. > 3. kernel command line penalty update via acpi_irq_penalty_update function. > 4. increment the penalty as USING right after the IRQ is assign to PCI. > > acpi_penalize_isa_irq and acpi_irq_penalty_update functions get called > before the ACPI subsystem is started. > > These API need to bypass the acpi_irq_get_penalty function. I don't mind this patch, but the changelog doesn't tell me what's broken and why we need this fix. Apparently acpi_irq_get_penalty() doesn't work before ACPI is initialized, but I don't see *why* it wouldn't work. However, I see one bug it *does* fix: we do not store the SCI penalty in the acpi_isa_irq_penalty[] table because acpi_isa_irq_penalty[] only holds ISA IRQ penalties, and there's no guarantee that the SCI is an ISA IRQ. But prior to this patch, we added in the SCI penalty to the acpi_isa_irq_penalty[] entry when the SCI was an ISA IRQ, which makes acpi_irq_get_penalty() return the wrong thing. Consider: Initially acpi_isa_irq_penalty[9] = 0. Assume sci_interrupt = 9. Then acpi_irq_get_penalty(9) returns X. If we call acpi_penalize_isa_irq(9, 1), it sets acpi_isa_irq_penalty[9] = X, and now acpi_irq_get_penalty(9) returns X + X. I'd propose a changelog like this: We do not want to store the SCI penalty in the acpi_isa_irq_penalty[] table because acpi_isa_irq_penalty[] only holds ISA IRQ penalties and there's no guarantee that the SCI is an ISA IRQ. We add in the SCI penalty as a special case in acpi_irq_get_penalty(). But if we called acpi_penalize_isa_irq() or acpi_irq_penalty_update() for an SCI that happened to be an ISA IRQ, they stored the SCI penalty (part of the acpi_irq_get_penalty() return value) in acpi_isa_irq_penalty[]. Subsequent calls to acpi_irq_get_penalty() returned a penalty that included *two* SCI penalties. If this actually fixes a worse problem related to ACPI initialization, of course you should detail that. Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/acpi/pci_link.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > index c983bf7..4f37938 100644 > --- a/drivers/acpi/pci_link.c > +++ b/drivers/acpi/pci_link.c > @@ -849,7 +849,7 @@ static int __init acpi_irq_penalty_update(char *str, int used) > continue; > > if (used) > - new_penalty = acpi_irq_get_penalty(irq) + > + new_penalty = acpi_isa_irq_penalty[irq] + > PIRQ_PENALTY_ISA_USED; > else > new_penalty = 0; > @@ -871,7 +871,7 @@ static int __init acpi_irq_penalty_update(char *str, int used) > void acpi_penalize_isa_irq(int irq, int active) > { > if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_isa_irq_penalty))) > - acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) + > + acpi_isa_irq_penalty[irq] = acpi_isa_irq_penalty[irq] + > (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING); > } > > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 20, 2016 at 08:39:30PM -0500, Bjorn Helgaas wrote: > On Wed, Oct 19, 2016 at 06:21:02PM -0400, Sinan Kaya wrote: > > The penalty determination of ISA IRQ goes through 4 paths. > > 1. assign PCI_USING during power up via acpi_irq_penalty_init. > > 2. update the penalty with acpi_penalize_isa_irq function based on the > > active parameter. > > 3. kernel command line penalty update via acpi_irq_penalty_update function. > > 4. increment the penalty as USING right after the IRQ is assign to PCI. > > > > acpi_penalize_isa_irq and acpi_irq_penalty_update functions get called > > before the ACPI subsystem is started. > > > > These API need to bypass the acpi_irq_get_penalty function. > > I don't mind this patch, but the changelog doesn't tell me what's > broken and why we need this fix. Apparently acpi_irq_get_penalty() > doesn't work before ACPI is initialized, but I don't see *why* it > wouldn't work. > > However, I see one bug it *does* fix: we do not store the SCI penalty > in the acpi_isa_irq_penalty[] table because acpi_isa_irq_penalty[] > only holds ISA IRQ penalties, and there's no guarantee that the SCI is > an ISA IRQ. But prior to this patch, we added in the SCI penalty to > the acpi_isa_irq_penalty[] entry when the SCI was an ISA IRQ, which > makes acpi_irq_get_penalty() return the wrong thing. Consider: > > Initially acpi_isa_irq_penalty[9] = 0. > Assume sci_interrupt = 9. > Then acpi_irq_get_penalty(9) returns X. > If we call acpi_penalize_isa_irq(9, 1), > it sets acpi_isa_irq_penalty[9] = X, > and now acpi_irq_get_penalty(9) returns X + X. Oops, I forgot the penalty we *intended* to add with acpi_penalize_isa_irq(). It's really like this, where X is the SCI penalty and Y is the part added by acpi_penalize_isa_irq(): Initially acpi_isa_irq_penalty[9] = 0. Assume sci_interrupt = 9. Then acpi_irq_get_penalty(9) returns X. If we call acpi_penalize_isa_irq(9, 1), it sets acpi_isa_irq_penalty[9] = X + Y, and now acpi_irq_get_penalty(9) returns X + X + Y. At the end, acpi_irq_get_penalty(9) *should* return X + Y, but instead it returns X + X + Y, i.e., the SCI penalty is included twice. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20 October 2016 at 09:21, Sinan Kaya <okaya@codeaurora.org> wrote: > The penalty determination of ISA IRQ goes through 4 paths. > 1. assign PCI_USING during power up via acpi_irq_penalty_init. > 2. update the penalty with acpi_penalize_isa_irq function based on the > active parameter. > 3. kernel command line penalty update via acpi_irq_penalty_update function. > 4. increment the penalty as USING right after the IRQ is assign to PCI. > > acpi_penalize_isa_irq and acpi_irq_penalty_update functions get called > before the ACPI subsystem is started. > > These API need to bypass the acpi_irq_get_penalty function. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/acpi/pci_link.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > index c983bf7..4f37938 100644 > --- a/drivers/acpi/pci_link.c > +++ b/drivers/acpi/pci_link.c > @@ -849,7 +849,7 @@ static int __init acpi_irq_penalty_update(char *str, int used) > continue; > > if (used) > - new_penalty = acpi_irq_get_penalty(irq) + > + new_penalty = acpi_isa_irq_penalty[irq] + > PIRQ_PENALTY_ISA_USED; > else > new_penalty = 0; > @@ -871,7 +871,7 @@ static int __init acpi_irq_penalty_update(char *str, int used) > void acpi_penalize_isa_irq(int irq, int active) > { > if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_isa_irq_penalty))) > - acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) + > + acpi_isa_irq_penalty[irq] = acpi_isa_irq_penalty[irq] + > (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING); > } > This series fixes one or more network adapters not working in Linux 32-bit x86 guest running inside VirtualBox if I have 4 network adapters enabled. The following message no longer appears in the kernel log: ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or acpi=off Tested-by: Jonathan Liu <net147@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/20/2016 9:39 PM, Bjorn Helgaas wrote: >> These API need to bypass the acpi_irq_get_penalty function. > I don't mind this patch, but the changelog doesn't tell me what's > broken and why we need this fix. Apparently acpi_irq_get_penalty() > doesn't work before ACPI is initialized, but I don't see *why* it > wouldn't work. I'll update the commit message as you suggested. The code doesn't work if we apply PATCH V4 2/3 + PATCH V4 3/3 without PATCH V4 1/3 since the caller is going to end up calling get_penalty function while ACPI is not initialized. This happened during the debug of this regression.
On 10/20/2016 5:39 PM, Rafael J. Wysocki wrote: >> @@ -871,7 +871,7 @@ static int __init acpi_irq_penalty_update(char *str, int used) >> > void acpi_penalize_isa_irq(int irq, int active) >> > { >> > if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_isa_irq_penalty))) >> > - acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) + >> > + acpi_isa_irq_penalty[irq] = acpi_isa_irq_penalty[irq] + > This looks slightly odd. What about > > + acpi_isa_irq_penalty[irq] += > >> > (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING); >> > } Makes sense.
Thanks, On 10/22/2016 11:48 PM, Jonathan Liu wrote: > This series fixes one or more network adapters not working in Linux > 32-bit x86 guest running inside VirtualBox if I have 4 network > adapters enabled. The following message no longer appears in the > kernel log: > ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or acpi=off > > Tested-by: Jonathan Liu <net147@gmail.com> I'm hoping that you can retest V5 so that Rafael can pull in your tested-by into the commit message.
On 24 October 2016 at 15:17, Sinan Kaya <okaya@codeaurora.org> wrote: > Thanks, > > On 10/22/2016 11:48 PM, Jonathan Liu wrote: >> This series fixes one or more network adapters not working in Linux >> 32-bit x86 guest running inside VirtualBox if I have 4 network >> adapters enabled. The following message no longer appears in the >> kernel log: >> ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or acpi=off >> >> Tested-by: Jonathan Liu <net147@gmail.com> > > I'm hoping that you can retest V5 so that Rafael can pull in your tested-by into > the commit message. > > -- > Sinan Kaya > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. > Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. Sure. Please CC me when you submit V5. Regards, Jonathan -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c index c983bf7..4f37938 100644 --- a/drivers/acpi/pci_link.c +++ b/drivers/acpi/pci_link.c @@ -849,7 +849,7 @@ static int __init acpi_irq_penalty_update(char *str, int used) continue; if (used) - new_penalty = acpi_irq_get_penalty(irq) + + new_penalty = acpi_isa_irq_penalty[irq] + PIRQ_PENALTY_ISA_USED; else new_penalty = 0; @@ -871,7 +871,7 @@ static int __init acpi_irq_penalty_update(char *str, int used) void acpi_penalize_isa_irq(int irq, int active) { if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_isa_irq_penalty))) - acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) + + acpi_isa_irq_penalty[irq] = acpi_isa_irq_penalty[irq] + (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING); }
The penalty determination of ISA IRQ goes through 4 paths. 1. assign PCI_USING during power up via acpi_irq_penalty_init. 2. update the penalty with acpi_penalize_isa_irq function based on the active parameter. 3. kernel command line penalty update via acpi_irq_penalty_update function. 4. increment the penalty as USING right after the IRQ is assign to PCI. acpi_penalize_isa_irq and acpi_irq_penalty_update functions get called before the ACPI subsystem is started. These API need to bypass the acpi_irq_get_penalty function. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/acpi/pci_link.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)