On Tue, Jan 3, 2012 at 1:07 AM, Petr Vandrovec <petr@xxxxxxxxxx> wrote:
> ...
> There are two more problems I've discovered during testing:
>
> 1. I had to restrict I/O base to 0x500-0xBF8 (from original
> 0x100-0xFFFF), as otherwise kernel happilly enables one of serial ports
> at 0x170, although IDE already occupied that port - it seems that resources
> occupied by IDE are reported only after libata loads, and as libata loads
> after serial port driver, pnpacpi does not seem to notice that 0x170 is
> in use, rather than available for allocation, enables device there,
> and then system gets very confused.
I think this is a consequence of the fact that the PNP & ACPI cores
don't reserve resources used by devices. The only reservation happens
when a driver claims the device. If a driver is loaded late or not at
all, problems like this can occur. This is different from PCI, where
the core *does* reserve device resources independent of drivers. I
think this is a PNP/ACPI bug, but it's not completely trivial to fix.
> 2. pnpacpi lacks support for both hot-add and hot-remove. So serial
> ports cannot be neither added nor removed while system runs. I guess
> that's the next task.
Right. I'd be thrilled if you fixed this :)
> Thanks,
> Petr Vandrovec
>
>
>
> From: Petr Vandrovec <petr@xxxxxxxxxx>
>
> Convert interrupt numbers from GSI to IRQ for _PRS
>
> pnpacpi calls acpi_register_gsi only when interrupt resource is read
> via _CRS method - either when device is discovered (if it was already
> enabled when pnpacpi initializes), or when someone calls 'get' method
> on the device. What is missing from this list is invoking acpi_register_gsi
> when someone issues _SRS method to enable device.
That's exactly the problem.
> And if nobody
> issues acpi_register_gsi, then device may not work if it is only device
> using GSI selected by the kernel.
>
> As whole kernel thinks in terms of IRQs, rather than GSIs, it seems that
> correct place where to call acpi_register_gsi is when possible resources
> are retrieved from _PRS, rather than just before _SRS invocation, so
> that is what I did.
I don't think this is the correct solution though.
acpi_register_gsi() programs IOAPIC entries, and we should only do
that for an IRQ that we're actually *using*, not one that is merely a
possible choice.
Maybe somewhere in the _SRS path would work. Currently, I think Linux
only calls _SRS when it *changes* something, but the ACPI spec (v4.0a,
sec 6.2) has a hint that maybe we should *always* call it before using
a device:
When OSPM enumerates a device, it calls _PRS to determine the resource
requirements of the device. It may also call _CRS to find the
current resource
settings for the device. Using this information, the Plug and Play system
determines what resources the device should consume and sets those
resources by calling the device’s _SRS control method.
It'd be cool if we could figure out whether Windows does that. Maybe
somebody with good virtualization and ACPI expertise :)
There's also a bunch of ugliness around the fact that we never
*unregister* GSIs for PNP devices, so I expect some things are broken
there, too.
Bjorn
> I've enabled checking to make sure we are not selecting mismatching
> level/edge configuration (with dev_warn, same way _CRS warns). But it
> may be too noisy, as on almost all systems I could check we get warnings
> that IRQ 9 is used by ACPI as level/low, while resource descriptors
> list it as edge/high (together with other IRQs in 3-15 range).
>
> Signed-off-by: Petr Vandrovec <petr@xxxxxxxxxx>
>
> diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
> index 5be4a39..3243bf4 100644
> --- a/drivers/pnp/pnpacpi/rsparser.c
> +++ b/drivers/pnp/pnpacpi/rsparser.c
> @@ -518,6 +518,47 @@ static __init void pnpacpi_parse_dma_option(struct pnp_dev *dev,
> pnp_register_dma_resource(dev, option_flags, map, flags);
> }
>
> +static __init void pnpacpi_gsi_option_to_irq(struct pnp_dev *dev,
> + pnp_irq_mask_t *map,
> + u32 gsi,
> + int triggering,
> + int polarity)
> +{
> + int p, t;
> + int irq;
> +
> + /* GSI 0 is ignored. */
> + if (!gsi)
> + return;
> +
> + /*
> + * In IO-APIC mode, accept interrupt only if its trigger/polarity
> + * matches with one already selected.
> + */
> + if (!acpi_get_override_irq(gsi, &t, &p)) {
> + t = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
> + p = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> + if (triggering != t || polarity != p) {
> + dev_warn(&dev->dev, "IRQ %d status %s, %s does not match override %s, %s\n",
> + gsi, triggering ? "edge" : "level",
> + polarity ? "low" : "high",
> + t ? "edge":"level", p ? "low":"high");
> + return;
> + }
> + }
> + irq = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
> + if (irq < 0)
> + dev_err(&dev->dev, "ignoring IRQ %d option "
> + "(cannot be mapped to platform IRQ)\n",
> + gsi);
> + else if (irq < PNP_IRQ_NR)
> + __set_bit(irq, map->bits);
> + else
> + dev_err(&dev->dev, "ignoring IRQ %d (GSI %d) option "
> + "(too large for %d entry bitmap)\n",
> + irq, gsi, PNP_IRQ_NR);
> +}
> +
> static __init void pnpacpi_parse_irq_option(struct pnp_dev *dev,
> unsigned int option_flags,
> struct acpi_resource_irq *p)
> @@ -528,8 +569,8 @@ static __init void pnpacpi_parse_irq_option(struct pnp_dev *dev,
>
> bitmap_zero(map.bits, PNP_IRQ_NR);
> for (i = 0; i < p->interrupt_count; i++)
> - if (p->interrupts[i])
> - __set_bit(p->interrupts[i], map.bits);
> + pnpacpi_gsi_option_to_irq(dev, &map, p->interrupts[i],
> + p->triggering, p->polarity);
>
> flags = irq_flags(p->triggering, p->polarity, p->sharable);
> pnp_register_irq_resource(dev, option_flags, &map, flags);
> @@ -544,16 +585,9 @@ static __init void pnpacpi_parse_ext_irq_option(struct pnp_dev *dev,
> unsigned char flags;
>
> bitmap_zero(map.bits, PNP_IRQ_NR);
> - for (i = 0; i < p->interrupt_count; i++) {
> - if (p->interrupts[i]) {
> - if (p->interrupts[i] < PNP_IRQ_NR)
> - __set_bit(p->interrupts[i], map.bits);
> - else
> - dev_err(&dev->dev, "ignoring IRQ %d option "
> - "(too large for %d entry bitmap)\n",
> - p->interrupts[i], PNP_IRQ_NR);
> - }
> - }
> + for (i = 0; i < p->interrupt_count; i++)
> + pnpacpi_gsi_option_to_irq(dev, &map, p->interrupts[i],
> + p->triggering, p->polarity);
>
> flags = irq_flags(p->triggering, p->polarity, p->sharable);
> pnp_register_irq_resource(dev, option_flags, &map, flags);
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
