| Message ID | 20241213070241.3334854-1-gthiagarajan@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] PCI : Fix pcie_flag_reg in set_pcie_port_type | expand |
[+cc Mika, Andy] On Fri, Dec 13, 2024 at 12:32:41PM +0530, Gowthami Thiagarajan wrote: > When an invalid PCIe topology is detected, the set_pcie_port_type function > does not set the port type correctly. This issue can occur in > configurations such as: > > Root Port ---> Downstream Port ---> Root Port > > In such cases, the topology is identified as invalid and due to the > incorrect port type setting, the extended configuration space of the > child device becomes inaccessible. From reading the code, it looks like the underlying problem is components that advertise the wrong PCIe Device/Port Type. set_pcie_port_type() already detects that incorrect Port Type and tries to correct it, but it puts the corrected type in bits [3:0] instead of [7:4] where it belongs. This looks like a bug from ca78410403dd ("PCI: Get rid of dev->has_secondary_link flag"). If so, we should add a Fixes: tag for that and possibly a stable tag. This looks like a clear bug. What system tripped over this? It's useful to have bread crumbs like that in case we see other issues caused by hardware/firmware defects like this. > Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com> > --- > v1->v2: > Updated commit description > > drivers/pci/probe.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 4f68414c3086..263ec21451d9 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1596,7 +1596,7 @@ void set_pcie_port_type(struct pci_dev *pdev) > if (pcie_downstream_port(parent)) { > pci_info(pdev, "claims to be downstream port but is acting as upstream port, correcting type\n"); > pdev->pcie_flags_reg &= ~PCI_EXP_FLAGS_TYPE; > - pdev->pcie_flags_reg |= PCI_EXP_TYPE_UPSTREAM; > + pdev->pcie_flags_reg |= PCI_EXP_TYPE_UPSTREAM << 4; > } > } else if (type == PCI_EXP_TYPE_UPSTREAM) { > /* > @@ -1607,7 +1607,7 @@ void set_pcie_port_type(struct pci_dev *pdev) > if (pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM) { > pci_info(pdev, "claims to be upstream port but is acting as downstream port, correcting type\n"); > pdev->pcie_flags_reg &= ~PCI_EXP_FLAGS_TYPE; > - pdev->pcie_flags_reg |= PCI_EXP_TYPE_DOWNSTREAM; > + pdev->pcie_flags_reg |= PCI_EXP_TYPE_DOWNSTREAM << 4; > } > } > } > -- > 2.25.1 >
On Fri, Dec 13, 2024 at 12:47:00PM -0600, Bjorn Helgaas wrote: > [+cc Mika, Andy] > > On Fri, Dec 13, 2024 at 12:32:41PM +0530, Gowthami Thiagarajan wrote: > > When an invalid PCIe topology is detected, the set_pcie_port_type function > > does not set the port type correctly. This issue can occur in > > configurations such as: > > > > Root Port ---> Downstream Port ---> Root Port > > > > In such cases, the topology is identified as invalid and due to the > > incorrect port type setting, the extended configuration space of the > > child device becomes inaccessible. > > >From reading the code, it looks like the underlying problem is > components that advertise the wrong PCIe Device/Port Type. > > set_pcie_port_type() already detects that incorrect Port Type and > tries to correct it, but it puts the corrected type in bits [3:0] > instead of [7:4] where it belongs. > > This looks like a bug from ca78410403dd ("PCI: Get rid of > dev->has_secondary_link flag"). If so, we should add a Fixes: tag for > that and possibly a stable tag. Yeah agree, this is definitely a bug in that commit. > This looks like a clear bug. What system tripped over this? It's > useful to have bread crumbs like that in case we see other issues > caused by hardware/firmware defects like this. > > > Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com> > > --- > > v1->v2: > > Updated commit description > > > > drivers/pci/probe.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 4f68414c3086..263ec21451d9 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -1596,7 +1596,7 @@ void set_pcie_port_type(struct pci_dev *pdev) > > if (pcie_downstream_port(parent)) { > > pci_info(pdev, "claims to be downstream port but is acting as upstream port, correcting type\n"); > > pdev->pcie_flags_reg &= ~PCI_EXP_FLAGS_TYPE; > > - pdev->pcie_flags_reg |= PCI_EXP_TYPE_UPSTREAM; > > + pdev->pcie_flags_reg |= PCI_EXP_TYPE_UPSTREAM << 4; > > } > > } else if (type == PCI_EXP_TYPE_UPSTREAM) { > > /* > > @@ -1607,7 +1607,7 @@ void set_pcie_port_type(struct pci_dev *pdev) > > if (pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM) { > > pci_info(pdev, "claims to be upstream port but is acting as downstream port, correcting type\n"); > > pdev->pcie_flags_reg &= ~PCI_EXP_FLAGS_TYPE; > > - pdev->pcie_flags_reg |= PCI_EXP_TYPE_DOWNSTREAM; > > + pdev->pcie_flags_reg |= PCI_EXP_TYPE_DOWNSTREAM << 4; > > } > > } > > } > > -- > > 2.25.1 > >
On Fri, 13 Dec 2024, Gowthami Thiagarajan wrote: > When an invalid PCIe topology is detected, the set_pcie_port_type function > does not set the port type correctly. This issue can occur in > configurations such as: > > Root Port ---> Downstream Port ---> Root Port > > In such cases, the topology is identified as invalid and due to the incorrect > port type setting, the extended configuration space of the child device becomes > inaccessible. > > Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com> > --- > v1->v2: > Updated commit description > > drivers/pci/probe.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 4f68414c3086..263ec21451d9 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1596,7 +1596,7 @@ void set_pcie_port_type(struct pci_dev *pdev) > if (pcie_downstream_port(parent)) { > pci_info(pdev, "claims to be downstream port but is acting as upstream port, correcting type\n"); > pdev->pcie_flags_reg &= ~PCI_EXP_FLAGS_TYPE; > - pdev->pcie_flags_reg |= PCI_EXP_TYPE_UPSTREAM; > + pdev->pcie_flags_reg |= PCI_EXP_TYPE_UPSTREAM << 4; Use FIELD_PREP() please.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 4f68414c3086..263ec21451d9 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1596,7 +1596,7 @@ void set_pcie_port_type(struct pci_dev *pdev) if (pcie_downstream_port(parent)) { pci_info(pdev, "claims to be downstream port but is acting as upstream port, correcting type\n"); pdev->pcie_flags_reg &= ~PCI_EXP_FLAGS_TYPE; - pdev->pcie_flags_reg |= PCI_EXP_TYPE_UPSTREAM; + pdev->pcie_flags_reg |= PCI_EXP_TYPE_UPSTREAM << 4; } } else if (type == PCI_EXP_TYPE_UPSTREAM) { /* @@ -1607,7 +1607,7 @@ void set_pcie_port_type(struct pci_dev *pdev) if (pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM) { pci_info(pdev, "claims to be upstream port but is acting as downstream port, correcting type\n"); pdev->pcie_flags_reg &= ~PCI_EXP_FLAGS_TYPE; - pdev->pcie_flags_reg |= PCI_EXP_TYPE_DOWNSTREAM; + pdev->pcie_flags_reg |= PCI_EXP_TYPE_DOWNSTREAM << 4; } } }
When an invalid PCIe topology is detected, the set_pcie_port_type function does not set the port type correctly. This issue can occur in configurations such as: Root Port ---> Downstream Port ---> Root Port In such cases, the topology is identified as invalid and due to the incorrect port type setting, the extended configuration space of the child device becomes inaccessible. Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com> --- v1->v2: Updated commit description drivers/pci/probe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)