[...]
> > In other words: what if PCI express capability is lower in the address
> > space (config) than the subsystem ID vendor capability ?
>
> Current code expects that if host controller driver sets both pcie_start
> and ssid_start, those values are correct, non-overlapping and can be
> handled correctly.
>
> And if offset to PCI express capability is lower than offset to SSID
> capability then there should not be any issue. First capability is
> correctly set into capabilities_pointer (via min function) and then
> pci_bridge_emul_conf_read() should handle it.
I don't understand how the pointer to the SSID cap is set in this
specific case, I don't see any code doing that but that's most certainly
because I don't know in details the emul bridge internals.
IIUC, in pci_bridge_emul_read_ssid(), we set the next cap pointer if
the PCI express capability is at an address higher (current case) than
the SSID capability otherwise we set it to 0 (end of the list).
The other way around, I don't see the PCI express next cap pointer
being set anywhere (where, IIUC, it should be set to point to the
SSID cap) - I am not sure you are handling it.
That's the only question I have on this patch.
Lorenzo
> The whole my idea is to construct capabilities linked list structure
> correctly based on input requirements (e.g. fixed location of some
> capability, etc).
>
> > I am just trying to understand the patch, so forgive me if the question
> > is already addressed in the code.
> >
> > Thanks,
> > Lorenzo
> >
> > > + else
> > > + bridge->pcie_start = bridge->ssid_start + PCI_CAP_SSID_SIZEOF;
> > > + }
> > > +
> > > + bridge->conf.capabilities_pointer = min(bridge->ssid_start, bridge->pcie_start);
> > >
> > > if (bridge->conf.capabilities_pointer)
> > > bridge->conf.status |= cpu_to_le16(PCI_STATUS_CAP_LIST);
> > > @@ -459,15 +468,17 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
> > > read_op = bridge->ops->read_base;
> > > cfgspace = (__le32 *) &bridge->conf;
> > > behavior = bridge->pci_regs_behavior;
> > > - } else if (reg >= PCI_CAP_SSID_START && reg < PCI_CAP_SSID_END && bridge->subsystem_vendor_id) {
> > > + } else if (reg >= bridge->ssid_start && reg < bridge->ssid_start + PCI_CAP_SSID_SIZEOF &&
> > > + bridge->subsystem_vendor_id) {
> > > /* Emulated PCI Bridge Subsystem Vendor ID capability */
> > > - reg -= PCI_CAP_SSID_START;
> > > + reg -= bridge->ssid_start;
> > > read_op = pci_bridge_emul_read_ssid;
> > > cfgspace = NULL;
> > > behavior = NULL;
> > > - } else if (reg >= PCI_CAP_PCIE_START && reg < PCI_CAP_PCIE_END && bridge->has_pcie) {
> > > + } else if (reg >= bridge->pcie_start && reg < bridge->pcie_start + PCI_CAP_PCIE_SIZEOF &&
> > > + bridge->has_pcie) {
> > > /* Our emulated PCIe capability */
> > > - reg -= PCI_CAP_PCIE_START;
> > > + reg -= bridge->pcie_start;
> > > read_op = bridge->ops->read_pcie;
> > > cfgspace = (__le32 *) &bridge->pcie_conf;
> > > behavior = bridge->pcie_cap_regs_behavior;
> > > @@ -538,9 +549,10 @@ int pci_bridge_emul_conf_write(struct pci_bridge_emul *bridge, int where,
> > > write_op = bridge->ops->write_base;
> > > cfgspace = (__le32 *) &bridge->conf;
> > > behavior = bridge->pci_regs_behavior;
> > > - } else if (reg >= PCI_CAP_PCIE_START && reg < PCI_CAP_PCIE_END && bridge->has_pcie) {
> > > + } else if (reg >= bridge->pcie_start && reg < bridge->pcie_start + PCI_CAP_PCIE_SIZEOF &&
> > > + bridge->has_pcie) {
> > > /* Our emulated PCIe capability */
> > > - reg -= PCI_CAP_PCIE_START;
> > > + reg -= bridge->pcie_start;
> > > write_op = bridge->ops->write_pcie;
> > > cfgspace = (__le32 *) &bridge->pcie_conf;
> > > behavior = bridge->pcie_cap_regs_behavior;
> > > diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h
> > > index 71392b67471d..2a0e59c7f0d9 100644
> > > --- a/drivers/pci/pci-bridge-emul.h
> > > +++ b/drivers/pci/pci-bridge-emul.h
> > > @@ -131,6 +131,8 @@ struct pci_bridge_emul {
> > > struct pci_bridge_reg_behavior *pci_regs_behavior;
> > > struct pci_bridge_reg_behavior *pcie_cap_regs_behavior;
> > > void *data;
> > > + u8 pcie_start;
> > > + u8 ssid_start;
> > > bool has_pcie;
> > > u16 subsystem_vendor_id;
> > > u16 subsystem_id;
> > > --
> > > 2.20.1
> > >
