| Message ID | 20211125124605.25915-6-pali@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | pci: mvebu: Various fixes | expand |
On Thu, Nov 25, 2021 at 01:45:55PM +0100, Pali Rohár wrote: > Interrupt support on mvebu emulated bridges is not implemented yet. Is this mvebu-specific, or is aardvar also affected? > So properly indicate return value to callers that they cannot request > interrupts from emulated bridge. Pet peeve: descriptions that say "do this *properly*". As though the previous authors were just ignorant or intentionally did something *improperly* :) > Signed-off-by: Pali Rohár <pali@kernel.org> > Cc: stable@vger.kernel.org > --- > drivers/pci/controller/pci-mvebu.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > index 19c6ee298442..a3df352d440e 100644 > --- a/drivers/pci/controller/pci-mvebu.c > +++ b/drivers/pci/controller/pci-mvebu.c > @@ -705,6 +705,15 @@ static struct pci_ops mvebu_pcie_ops = { > .write = mvebu_pcie_wr_conf, > }; > > +static int mvebu_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin) > +{ > + /* Interrupt support on mvebu emulated bridges is not implemented yet */ > + if (dev->bus->number == 0) > + return 0; /* Proper return code 0 == NO_IRQ */ > + > + return of_irq_parse_and_map_pci(dev, slot, pin); Is this something that could be done with a .read_base() op, e.g., make PCI_INTERRUPT_PIN contain zero (PCI_INTERRUPT_UNKNOWN)? > +} > + > static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > const struct resource *res, > resource_size_t start, > @@ -1119,6 +1128,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > bridge->sysdata = pcie; > bridge->ops = &mvebu_pcie_ops; > bridge->align_resource = mvebu_pcie_align_resource; > + bridge->map_irq = mvebu_pcie_map_irq; I assume this means INTx doesn't work for some devices? Which ones? I guess anything on the root bus? But INTx for devices *below* these emulated Root Ports *does* work? > return pci_host_probe(bridge); > } > -- > 2.20.1 >
On Friday 07 January 2022 15:32:16 Bjorn Helgaas wrote: > On Thu, Nov 25, 2021 at 01:45:55PM +0100, Pali Rohár wrote: > > Interrupt support on mvebu emulated bridges is not implemented yet. > > Is this mvebu-specific, or is aardvar also affected? This is pci-mvebu.c driver specific, it does not implement emulation of neither INTx, nor MSI interrupts for emulated pci bridge (root port). As we know this HW does not have compliant pci root port, it needs to be emulated in driver, and emulation for interrupts is missing. (it means that also AER interrupt is missing). And pci-aardvark.c driver has same issue and similar patch is required for pci-aardvark.c too. Marek should take care of it. But for pci-aardvark we already have implementation which emulates INTx interrupts and it is waiting for review on the list: https://lore.kernel.org/linux-pci/20211208061851.31867-1-kabel@kernel.org/ > > So properly indicate return value to callers that they cannot request > > interrupts from emulated bridge. > > Pet peeve: descriptions that say "do this *properly*". As though the > previous authors were just ignorant or intentionally did something > *improperly* :) > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Cc: stable@vger.kernel.org > > --- > > drivers/pci/controller/pci-mvebu.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > > index 19c6ee298442..a3df352d440e 100644 > > --- a/drivers/pci/controller/pci-mvebu.c > > +++ b/drivers/pci/controller/pci-mvebu.c > > @@ -705,6 +705,15 @@ static struct pci_ops mvebu_pcie_ops = { > > .write = mvebu_pcie_wr_conf, > > }; > > > > +static int mvebu_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin) > > +{ > > + /* Interrupt support on mvebu emulated bridges is not implemented yet */ > > + if (dev->bus->number == 0) > > + return 0; /* Proper return code 0 == NO_IRQ */ > > + > > + return of_irq_parse_and_map_pci(dev, slot, pin); > > Is this something that could be done with a .read_base() op, e.g., > make PCI_INTERRUPT_PIN contain zero (PCI_INTERRUPT_UNKNOWN)? I'm not sure... maybe. I choose this style as after I implement emulation of INTx interrupts it allows me just to replace "return 0;" by "return my_mapping_function_for_root_port(...);". Similarly like it is in pending pci-aardvark.c patch: https://lore.kernel.org/linux-pci/20211208061851.31867-15-kabel@kernel.org/ > > +} > > + > > static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > > const struct resource *res, > > resource_size_t start, > > @@ -1119,6 +1128,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > > bridge->sysdata = pcie; > > bridge->ops = &mvebu_pcie_ops; > > bridge->align_resource = mvebu_pcie_align_resource; > > + bridge->map_irq = mvebu_pcie_map_irq; > > I assume this means INTx doesn't work for some devices? Which ones? > I guess anything on the root bus? But INTx for devices *below* these > emulated Root Ports *does* work? Exactly. All devices except emulated root ports (which are on bus 0) have working MSI, MSI-X and INTx interrupts. > > return pci_host_probe(bridge); > > } > > -- > > 2.20.1 > >
On Fri, Jan 07, 2022 at 11:13:48PM +0100, Pali Rohár wrote: > On Friday 07 January 2022 15:32:16 Bjorn Helgaas wrote: > > On Thu, Nov 25, 2021 at 01:45:55PM +0100, Pali Rohár wrote: > > > Interrupt support on mvebu emulated bridges is not implemented yet. > > > > Is this mvebu-specific, or is aardvar also affected? > > This is pci-mvebu.c driver specific, it does not implement emulation of > neither INTx, nor MSI interrupts for emulated pci bridge (root port). As > we know this HW does not have compliant pci root port, it needs to be > emulated in driver, and emulation for interrupts is missing. (it means > that also AER interrupt is missing). > > And pci-aardvark.c driver has same issue and similar patch is required > for pci-aardvark.c too. Marek should take care of it. But for > pci-aardvark we already have implementation which emulates INTx > interrupts and it is waiting for review on the list: > https://lore.kernel.org/linux-pci/20211208061851.31867-1-kabel@kernel.org/ > > > > So properly indicate return value to callers that they cannot request > > > interrupts from emulated bridge. > > > > Pet peeve: descriptions that say "do this *properly*". As though the > > previous authors were just ignorant or intentionally did something > > *improperly* :) > > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > Cc: stable@vger.kernel.org > > > --- > > > drivers/pci/controller/pci-mvebu.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > > > index 19c6ee298442..a3df352d440e 100644 > > > --- a/drivers/pci/controller/pci-mvebu.c > > > +++ b/drivers/pci/controller/pci-mvebu.c > > > @@ -705,6 +705,15 @@ static struct pci_ops mvebu_pcie_ops = { > > > .write = mvebu_pcie_wr_conf, > > > }; > > > > > > +static int mvebu_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin) > > > +{ > > > + /* Interrupt support on mvebu emulated bridges is not implemented yet */ > > > + if (dev->bus->number == 0) > > > + return 0; /* Proper return code 0 == NO_IRQ */ > > > + > > > + return of_irq_parse_and_map_pci(dev, slot, pin); > > > > Is this something that could be done with a .read_base() op, e.g., > > make PCI_INTERRUPT_PIN contain zero (PCI_INTERRUPT_UNKNOWN)? > > I'm not sure... maybe. I choose this style as after I implement > emulation of INTx interrupts it allows me just to replace "return 0;" by > "return my_mapping_function_for_root_port(...);". OK, so even after you implement INTx for the emulated Root Ports, the default of_irq_parse_and_map_pci() is insufficient, and you will require an mvebu .map_irq() function. That's reasonable. "PCI_INTERRUPT_PIN == 0" is the way software learns that a device doesn't use INTx, of course, and I suppose PCI_INTERRUPT_PIN already reads as zero, since mvebu_pci_bridge_emul_init() doesn't set bridge->conf.intpin, and I assume the default value would be zero? Bjorn
On Friday 07 January 2022 17:01:55 Bjorn Helgaas wrote: > On Fri, Jan 07, 2022 at 11:13:48PM +0100, Pali Rohár wrote: > > On Friday 07 January 2022 15:32:16 Bjorn Helgaas wrote: > > > On Thu, Nov 25, 2021 at 01:45:55PM +0100, Pali Rohár wrote: > > > > Interrupt support on mvebu emulated bridges is not implemented yet. > > > > > > Is this mvebu-specific, or is aardvar also affected? > > > > This is pci-mvebu.c driver specific, it does not implement emulation of > > neither INTx, nor MSI interrupts for emulated pci bridge (root port). As > > we know this HW does not have compliant pci root port, it needs to be > > emulated in driver, and emulation for interrupts is missing. (it means > > that also AER interrupt is missing). > > > > And pci-aardvark.c driver has same issue and similar patch is required > > for pci-aardvark.c too. Marek should take care of it. But for > > pci-aardvark we already have implementation which emulates INTx > > interrupts and it is waiting for review on the list: > > https://lore.kernel.org/linux-pci/20211208061851.31867-1-kabel@kernel.org/ > > > > > > So properly indicate return value to callers that they cannot request > > > > interrupts from emulated bridge. > > > > > > Pet peeve: descriptions that say "do this *properly*". As though the > > > previous authors were just ignorant or intentionally did something > > > *improperly* :) > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > Cc: stable@vger.kernel.org > > > > --- > > > > drivers/pci/controller/pci-mvebu.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > > > > index 19c6ee298442..a3df352d440e 100644 > > > > --- a/drivers/pci/controller/pci-mvebu.c > > > > +++ b/drivers/pci/controller/pci-mvebu.c > > > > @@ -705,6 +705,15 @@ static struct pci_ops mvebu_pcie_ops = { > > > > .write = mvebu_pcie_wr_conf, > > > > }; > > > > > > > > +static int mvebu_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin) > > > > +{ > > > > + /* Interrupt support on mvebu emulated bridges is not implemented yet */ > > > > + if (dev->bus->number == 0) > > > > + return 0; /* Proper return code 0 == NO_IRQ */ > > > > + > > > > + return of_irq_parse_and_map_pci(dev, slot, pin); > > > > > > Is this something that could be done with a .read_base() op, e.g., > > > make PCI_INTERRUPT_PIN contain zero (PCI_INTERRUPT_UNKNOWN)? > > > > I'm not sure... maybe. I choose this style as after I implement > > emulation of INTx interrupts it allows me just to replace "return 0;" by > > "return my_mapping_function_for_root_port(...);". > > OK, so even after you implement INTx for the emulated Root Ports, the > default of_irq_parse_and_map_pci() is insufficient, and you will > require an mvebu .map_irq() function. That's reasonable. > > "PCI_INTERRUPT_PIN == 0" is the way software learns that a device > doesn't use INTx, of course, and I suppose PCI_INTERRUPT_PIN already > reads as zero, since mvebu_pci_bridge_emul_init() doesn't set > bridge->conf.intpin, and I assume the default value would be zero? > > Bjorn Yes, looks like that zeros are in emulated config space for fields not explicitly initialized. Which is the pci-mvebu.c case. But now I'm looking at pci-aardvark.c driver and it sets PCI_INTERRUPT_PIN register to A: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-aardvark.c?h=v5.16-rc8#n953 And that comment "/* Support interrupt A for MSI feature */" must be total nonsense as INTA for sure is not required for MSI... Plus we know that pci-aardvark.c driver does not implement for pci bridge neither INTx nor MSI... Ach... seems that this code is here since beginning and needs to be fixed...
diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c index 19c6ee298442..a3df352d440e 100644 --- a/drivers/pci/controller/pci-mvebu.c +++ b/drivers/pci/controller/pci-mvebu.c @@ -705,6 +705,15 @@ static struct pci_ops mvebu_pcie_ops = { .write = mvebu_pcie_wr_conf, }; +static int mvebu_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin) +{ + /* Interrupt support on mvebu emulated bridges is not implemented yet */ + if (dev->bus->number == 0) + return 0; /* Proper return code 0 == NO_IRQ */ + + return of_irq_parse_and_map_pci(dev, slot, pin); +} + static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, const struct resource *res, resource_size_t start, @@ -1119,6 +1128,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) bridge->sysdata = pcie; bridge->ops = &mvebu_pcie_ops; bridge->align_resource = mvebu_pcie_align_resource; + bridge->map_irq = mvebu_pcie_map_irq; return pci_host_probe(bridge); }
Interrupt support on mvebu emulated bridges is not implemented yet. So properly indicate return value to callers that they cannot request interrupts from emulated bridge. Signed-off-by: Pali Rohár <pali@kernel.org> Cc: stable@vger.kernel.org --- drivers/pci/controller/pci-mvebu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)