| Message ID | 20211125124605.25915-4-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:53PM +0100, Pali Rohár wrote: > Driver cannot handle PCI bridges at non-zero function address. So add > appropriate check. Currently all in-tree kernel DTS files set PCI bridge > function to zero. Why can the driver not handle bridges at non-zero function addresses? The PCI spec allows that, doesn't it? Is this a hardware limitation? > Signed-off-by: Pali Rohár <pali@kernel.org> > Cc: stable@vger.kernel.org > --- > drivers/pci/controller/pci-mvebu.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > index 6197f7e7c317..08274132cdfb 100644 > --- a/drivers/pci/controller/pci-mvebu.c > +++ b/drivers/pci/controller/pci-mvebu.c > @@ -864,6 +864,11 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, > port->devfn = of_pci_get_devfn(child); > if (port->devfn < 0) > goto skip; > + if (PCI_FUNC(port->devfn) != 0) { > + dev_err(dev, "%s: invalid function number, must be zero\n", > + port->name); > + goto skip; > + } > > ret = mvebu_get_tgt_attr(dev->of_node, port->devfn, IORESOURCE_MEM, > &port->mem_target, &port->mem_attr); > -- > 2.20.1 >
On Friday 07 January 2022 12:15:12 Bjorn Helgaas wrote: > On Thu, Nov 25, 2021 at 01:45:53PM +0100, Pali Rohár wrote: > > Driver cannot handle PCI bridges at non-zero function address. So add > > appropriate check. Currently all in-tree kernel DTS files set PCI bridge > > function to zero. > > Why can the driver not handle bridges at non-zero function addresses? > The PCI spec allows that, doesn't it? Is this a hardware limitation? It is software / kernel limitation. Because this bridge is virtual, emulated by pci-bridge-emul.c driver and this driver can emulate only single function PCI-to-PCI bridge device. > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Cc: stable@vger.kernel.org > > --- > > drivers/pci/controller/pci-mvebu.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > > index 6197f7e7c317..08274132cdfb 100644 > > --- a/drivers/pci/controller/pci-mvebu.c > > +++ b/drivers/pci/controller/pci-mvebu.c > > @@ -864,6 +864,11 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, > > port->devfn = of_pci_get_devfn(child); > > if (port->devfn < 0) > > goto skip; > > + if (PCI_FUNC(port->devfn) != 0) { > > + dev_err(dev, "%s: invalid function number, must be zero\n", > > + port->name); > > + goto skip; > > + } > > > > ret = mvebu_get_tgt_attr(dev->of_node, port->devfn, IORESOURCE_MEM, > > &port->mem_target, &port->mem_attr); > > -- > > 2.20.1 > >
On Fri, Jan 07, 2022 at 07:18:19PM +0100, Pali Rohár wrote: > On Friday 07 January 2022 12:15:12 Bjorn Helgaas wrote: > > On Thu, Nov 25, 2021 at 01:45:53PM +0100, Pali Rohár wrote: > > > Driver cannot handle PCI bridges at non-zero function address. So add > > > appropriate check. Currently all in-tree kernel DTS files set PCI bridge > > > function to zero. > > > > Why can the driver not handle bridges at non-zero function addresses? > > The PCI spec allows that, doesn't it? Is this a hardware limitation? > > It is software / kernel limitation. > > Because this bridge is virtual, emulated by pci-bridge-emul.c driver and > this driver can emulate only single function PCI-to-PCI bridge device. That's weird. Why does pci-bridge-emul.c care about the function number? Or maybe you're saying that pci-mvebu.c isn't smart enough to build an emulated bridge at a non-zero function? Or, since this is emulated, maybe there's just no *reason* to ever use a non-zero function? It would really be nice to have the commit log and maybe even a comment allude to what's going on here . Otherwise this check sort of dangles without having an obvious reason for existence. Does this issue also affect pci-aardvark.c (which looks like the only other user of pci_bridge_emul_init())? > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > Cc: stable@vger.kernel.org > > > --- > > > drivers/pci/controller/pci-mvebu.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > > > index 6197f7e7c317..08274132cdfb 100644 > > > --- a/drivers/pci/controller/pci-mvebu.c > > > +++ b/drivers/pci/controller/pci-mvebu.c > > > @@ -864,6 +864,11 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, > > > port->devfn = of_pci_get_devfn(child); > > > if (port->devfn < 0) > > > goto skip; > > > + if (PCI_FUNC(port->devfn) != 0) { > > > + dev_err(dev, "%s: invalid function number, must be zero\n", > > > + port->name); > > > + goto skip; > > > + } > > > > > > ret = mvebu_get_tgt_attr(dev->of_node, port->devfn, IORESOURCE_MEM, > > > &port->mem_target, &port->mem_attr); > > > -- > > > 2.20.1 > > >
On Friday 07 January 2022 15:09:02 Bjorn Helgaas wrote: > On Fri, Jan 07, 2022 at 07:18:19PM +0100, Pali Rohár wrote: > > On Friday 07 January 2022 12:15:12 Bjorn Helgaas wrote: > > > On Thu, Nov 25, 2021 at 01:45:53PM +0100, Pali Rohár wrote: > > > > Driver cannot handle PCI bridges at non-zero function address. So add > > > > appropriate check. Currently all in-tree kernel DTS files set PCI bridge > > > > function to zero. > > > > > > Why can the driver not handle bridges at non-zero function addresses? > > > The PCI spec allows that, doesn't it? Is this a hardware limitation? > > > > It is software / kernel limitation. > > > > Because this bridge is virtual, emulated by pci-bridge-emul.c driver and > > this driver can emulate only single function PCI-to-PCI bridge device. > > That's weird. Why does pci-bridge-emul.c care about the function > number? pci-bridge-emul.c emulates whole PCI config space and multifunction PCI device needs to have Multi-Function Device bit set. Which pci-bridge-emul.c does not do as it "emulates" only singe-function device. Also some extended PCIe registers needs to be aligned for multifunction device. And for simplification nothing from this is implemented in that pci-bridge-emul.c driver. Basically single function device is easily to emulate than multi function device. And for simplicity of driver it is just better to do not implement more stuff if it is not required. > Or maybe you're saying that pci-mvebu.c isn't smart enough to > build an emulated bridge at a non-zero function? Or, since this is > emulated, maybe there's just no *reason* to ever use a non-zero > function? These PCIe root ports are basically on different PCI domains, every root port with its subtree has its own configuration, including own access to config space of subdevices. And I do not think that there is a reason to try putting root port (as emulated pci-to-pci bridge) on non-zero function and putting separate root ports into "one" multifunction device. Technically it could be possible to implement it and also properly, as it is just emulation of PCI device. But why? Just big complication without any benefit. At least I do not see benefit. > It would really be nice to have the commit log and maybe even a > comment allude to what's going on here . Otherwise this check sort of > dangles without having an obvious reason for existence. > > Does this issue also affect pci-aardvark.c (which looks like the only > other user of pci_bridge_emul_init())? Theoretically yes. But pci-aardvark is single-root-port hardware and therefore it is single-function device. And emulated device is registered by pci-aardvark driver, not by DT. Because it is single-root-port HW there is no DT node for root port like for any other single-root-port PCIe controllers. So practically no. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > Cc: stable@vger.kernel.org > > > > --- > > > > drivers/pci/controller/pci-mvebu.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > > > > index 6197f7e7c317..08274132cdfb 100644 > > > > --- a/drivers/pci/controller/pci-mvebu.c > > > > +++ b/drivers/pci/controller/pci-mvebu.c > > > > @@ -864,6 +864,11 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, > > > > port->devfn = of_pci_get_devfn(child); > > > > if (port->devfn < 0) > > > > goto skip; > > > > + if (PCI_FUNC(port->devfn) != 0) { > > > > + dev_err(dev, "%s: invalid function number, must be zero\n", > > > > + port->name); > > > > + goto skip; > > > > + } > > > > > > > > ret = mvebu_get_tgt_attr(dev->of_node, port->devfn, IORESOURCE_MEM, > > > > &port->mem_target, &port->mem_attr); > > > > -- > > > > 2.20.1 > > > >
diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c index 6197f7e7c317..08274132cdfb 100644 --- a/drivers/pci/controller/pci-mvebu.c +++ b/drivers/pci/controller/pci-mvebu.c @@ -864,6 +864,11 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, port->devfn = of_pci_get_devfn(child); if (port->devfn < 0) goto skip; + if (PCI_FUNC(port->devfn) != 0) { + dev_err(dev, "%s: invalid function number, must be zero\n", + port->name); + goto skip; + } ret = mvebu_get_tgt_attr(dev->of_node, port->devfn, IORESOURCE_MEM, &port->mem_target, &port->mem_attr);
Driver cannot handle PCI bridges at non-zero function address. So add appropriate check. Currently all in-tree kernel DTS files set PCI bridge function to zero. Signed-off-by: Pali Rohár <pali@kernel.org> Cc: stable@vger.kernel.org --- drivers/pci/controller/pci-mvebu.c | 5 +++++ 1 file changed, 5 insertions(+)