| Message ID | 20190301160440.26262-4-s.miroshnichenko@yadro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/powernv/pci: Make hotplug self-sufficient, independent of FW and DT | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 137 lines checked |
On Sat, Mar 2, 2019 at 3:04 AM Sergey Miroshnichenko <s.miroshnichenko@yadro.com> wrote: > > If a struct pci_dn hasn't yet been created for the PCIe device (there was > no DT node for it), allocate this structure and fill with info read from > the device directly. > > Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> > --- > arch/powerpc/kernel/pci_dn.c | 79 ++++++++++++++++++++++++++++++++---- > 1 file changed, 72 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c > index 341ed71250f1..67ccd7be8344 100644 > --- a/arch/powerpc/kernel/pci_dn.c > +++ b/arch/powerpc/kernel/pci_dn.c > @@ -33,6 +33,8 @@ > #include <asm/firmware.h> > #include <asm/eeh.h> > > +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent); > + > /* > * The function is used to find the firmware data of one > * specific PCI device, which is attached to the indicated > @@ -65,6 +67,9 @@ struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus) > dn = pci_bus_to_OF_node(pbus); > pdn = dn ? PCI_DN(dn) : NULL; > > + if (!pdn && pbus->self) > + pdn = pbus->self->dev.archdata.pci_data; > + > return pdn; > } > > @@ -74,10 +79,13 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus, > struct device_node *dn = NULL; > struct pci_dn *parent, *pdn; > struct pci_dev *pdev = NULL; > + bool pdev_found = false; > > /* Fast path: fetch from PCI device */ > list_for_each_entry(pdev, &bus->devices, bus_list) { > if (pdev->devfn == devfn) { > + pdev_found = true; > + > if (pdev->dev.archdata.pci_data) > return pdev->dev.archdata.pci_data; > > @@ -86,6 +94,9 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus, > } > } > > + if (!pdev_found) > + pdev = NULL; > + > /* Fast path: fetch from device node */ > pdn = dn ? PCI_DN(dn) : NULL; > if (pdn) > @@ -98,9 +109,12 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus, > > list_for_each_entry(pdn, &parent->child_list, list) { > if (pdn->busno == bus->number && > - pdn->devfn == devfn) > - return pdn; > - } > + pdn->devfn == devfn) { > + if (pdev) > + pdev->dev.archdata.pci_data = pdn; > + return pdn; > + } > + } > > return NULL; > } > @@ -130,14 +144,15 @@ struct pci_dn *pci_get_pdn(struct pci_dev *pdev) > > list_for_each_entry(pdn, &parent->child_list, list) { > if (pdn->busno == pdev->bus->number && > - pdn->devfn == pdev->devfn) > + pdn->devfn == pdev->devfn) { > + pdev->dev.archdata.pci_data = pdn; > return pdn; > + } > } > > - return NULL; > + return create_pdn(pdev, parent); > } Eh... If it works I guess it's fine? Killing pdn entirely (or at the very least greatly restricting it's role) is something we should work towards though since it doesn't make much sense on PNV. > -#ifdef CONFIG_PCI_IOV > static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent, > int vf_index, > int busno, int devfn) > @@ -156,7 +171,9 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent, > pdn->parent = parent; > pdn->busno = busno; > pdn->devfn = devfn; > + #ifdef CONFIG_PCI_IOV > pdn->vf_index = vf_index; > + #endif /* CONFIG_PCI_IOV */ This function is currently only used when adding VFs, so I'd move the VF specific params from this and put them in the call site at add_dev_pci_data(). Then you can drop the #ifdef hacks. It might be worth renaming add_one_dev_pci_data() and add_one_dev_pci_data() since their actual usage is limited to the VF init path, but their name seem generic. > pdn->pe_number = IODA_INVALID_PE; > INIT_LIST_HEAD(&pdn->child_list); > INIT_LIST_HEAD(&pdn->list); > @@ -164,7 +181,55 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent, > > return pdn; > } > -#endif > + > +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent) > +{ > + struct pci_dn *pdn = NULL; > + > + if (!parent) > + return NULL; > + > + pdn = add_one_dev_pci_data(parent, 0, pdev->bus->busn_res.start, pdev->devfn); > + dev_info(&pdev->dev, "Create a new pdn for devfn %2x\n", pdev->devfn / 8); > + > + if (pdn) { Check for !pdn and exit early. Stacking indentation levels just gets kind of ugly. > + #ifdef CONFIG_EEH > + struct eeh_dev *edev; > + #endif /* CONFIG_EEH */ You don't use the variable for anything. You can either switch this to a void pointer or just check the return value of eeh_dev_init() directly and drop this. > + u32 class_code; > + u16 device_id; > + u16 vendor_id; > + > + #ifdef CONFIG_EEH > + edev = eeh_dev_init(pdn); > + if (!edev) { > + kfree(pdn); > + dev_err(&pdev->dev, "%s: Failed to allocate edev\n", __func__); > + return NULL; > + } > + #endif /* CONFIG_EEH */ > + > + pci_bus_read_config_word(pdev->bus, pdev->devfn, > + PCI_VENDOR_ID, &vendor_id); > + pdn->vendor_id = vendor_id; > + > + pci_bus_read_config_word(pdev->bus, pdev->devfn, > + PCI_DEVICE_ID, &device_id); > + pdn->device_id = device_id; > + > + pci_bus_read_config_dword(pdev->bus, pdev->devfn, > + PCI_CLASS_REVISION, &class_code); > + class_code >>= 8; > + pdn->class_code = class_code; > + > + pdn->pci_ext_config_space = 0; Does this need to be explicitly initialised or can we rely on it being allocated with kzalloc() or similar? > + pdev->dev.archdata.pci_data = pdn; > + } else { > + dev_err(&pdev->dev, "%s: Failed to allocate pdn\n", __func__); > + } > + > + return pdn; > +} > > struct pci_dn *add_dev_pci_data(struct pci_dev *pdev) > { > -- > 2.20.1 >
On 3/5/19 11:04 AM, Oliver wrote: > On Sat, Mar 2, 2019 at 3:04 AM Sergey Miroshnichenko > <s.miroshnichenko@yadro.com> wrote: >> >> If a struct pci_dn hasn't yet been created for the PCIe device (there was >> no DT node for it), allocate this structure and fill with info read from >> the device directly. >> >> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> >> --- >> arch/powerpc/kernel/pci_dn.c | 79 ++++++++++++++++++++++++++++++++---- >> 1 file changed, 72 insertions(+), 7 deletions(-) >> >> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c >> index 341ed71250f1..67ccd7be8344 100644 >> --- a/arch/powerpc/kernel/pci_dn.c >> +++ b/arch/powerpc/kernel/pci_dn.c >> @@ -33,6 +33,8 @@ >> #include <asm/firmware.h> >> #include <asm/eeh.h> >> >> +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent); >> + >> /* >> * The function is used to find the firmware data of one >> * specific PCI device, which is attached to the indicated >> @@ -65,6 +67,9 @@ struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus) >> dn = pci_bus_to_OF_node(pbus); >> pdn = dn ? PCI_DN(dn) : NULL; >> >> + if (!pdn && pbus->self) >> + pdn = pbus->self->dev.archdata.pci_data; >> + >> return pdn; >> } >> >> @@ -74,10 +79,13 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus, >> struct device_node *dn = NULL; >> struct pci_dn *parent, *pdn; >> struct pci_dev *pdev = NULL; >> + bool pdev_found = false; >> >> /* Fast path: fetch from PCI device */ >> list_for_each_entry(pdev, &bus->devices, bus_list) { >> if (pdev->devfn == devfn) { >> + pdev_found = true; >> + >> if (pdev->dev.archdata.pci_data) >> return pdev->dev.archdata.pci_data; >> >> @@ -86,6 +94,9 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus, >> } >> } >> >> + if (!pdev_found) >> + pdev = NULL; >> + >> /* Fast path: fetch from device node */ >> pdn = dn ? PCI_DN(dn) : NULL; >> if (pdn) >> @@ -98,9 +109,12 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus, >> >> list_for_each_entry(pdn, &parent->child_list, list) { >> if (pdn->busno == bus->number && >> - pdn->devfn == devfn) >> - return pdn; >> - } >> + pdn->devfn == devfn) { >> + if (pdev) >> + pdev->dev.archdata.pci_data = pdn; >> + return pdn; >> + } >> + } >> >> return NULL; >> } >> @@ -130,14 +144,15 @@ struct pci_dn *pci_get_pdn(struct pci_dev *pdev) >> >> list_for_each_entry(pdn, &parent->child_list, list) { >> if (pdn->busno == pdev->bus->number && >> - pdn->devfn == pdev->devfn) >> + pdn->devfn == pdev->devfn) { >> + pdev->dev.archdata.pci_data = pdn; >> return pdn; >> + } >> } >> >> - return NULL; >> + return create_pdn(pdev, parent); >> } > > Eh... If it works I guess it's fine? Killing pdn entirely (or at the > very least greatly restricting it's role) is something we should work > towards though since it doesn't make much sense on PNV. > As I can see, pdns are mostly used on PNV for the following: - storing a pe_number; - storing struct resource holes - to reserve memory for SR-IOV; - IOMMU table_group; - storing m64_map; - storing struct eeh_dev *edev; Other fields (busno, devfn, etc.) are available in struct pci_dev. The pe_numbers are probably can be somehow moved to struct pnv_phb, and they can be fetched from there by a bdfn. The same for other used fields. Or there could some new structure - shorter version of struct pci_dn, without .child_list etc. Thanks for the review, all the issues you've pointed below will be fixed in v5! Best regards, Serge >> -#ifdef CONFIG_PCI_IOV >> static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent, >> int vf_index, >> int busno, int devfn) >> @@ -156,7 +171,9 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent, >> pdn->parent = parent; >> pdn->busno = busno; >> pdn->devfn = devfn; >> + #ifdef CONFIG_PCI_IOV >> pdn->vf_index = vf_index; >> + #endif /* CONFIG_PCI_IOV */ > > This function is currently only used when adding VFs, so I'd move the > VF specific params from this and put them in the call site at > add_dev_pci_data(). Then you can drop the #ifdef hacks. It might be > worth renaming add_one_dev_pci_data() and add_one_dev_pci_data() since > their actual usage is limited to the VF init path, but their name seem > generic. > >> pdn->pe_number = IODA_INVALID_PE; >> INIT_LIST_HEAD(&pdn->child_list); >> INIT_LIST_HEAD(&pdn->list); >> @@ -164,7 +181,55 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent, >> >> return pdn; >> } >> -#endif >> + >> +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent) >> +{ >> + struct pci_dn *pdn = NULL; >> + >> + if (!parent) >> + return NULL; >> + >> + pdn = add_one_dev_pci_data(parent, 0, pdev->bus->busn_res.start, pdev->devfn); >> + dev_info(&pdev->dev, "Create a new pdn for devfn %2x\n", pdev->devfn / 8); >> + > >> + if (pdn) { > > Check for !pdn and exit early. Stacking indentation levels just gets > kind of ugly. > >> + #ifdef CONFIG_EEH >> + struct eeh_dev *edev; >> + #endif /* CONFIG_EEH */ > You don't use the variable for anything. You can either switch this to > a void pointer or just check the return value of eeh_dev_init() > directly and drop this. > >> + u32 class_code; >> + u16 device_id; >> + u16 vendor_id; >> + > >> + #ifdef CONFIG_EEH >> + edev = eeh_dev_init(pdn); >> + if (!edev) { >> + kfree(pdn); >> + dev_err(&pdev->dev, "%s: Failed to allocate edev\n", __func__); >> + return NULL; >> + } >> + #endif /* CONFIG_EEH */ >> + >> + pci_bus_read_config_word(pdev->bus, pdev->devfn, >> + PCI_VENDOR_ID, &vendor_id); >> + pdn->vendor_id = vendor_id; >> + >> + pci_bus_read_config_word(pdev->bus, pdev->devfn, >> + PCI_DEVICE_ID, &device_id); >> + pdn->device_id = device_id; >> + >> + pci_bus_read_config_dword(pdev->bus, pdev->devfn, >> + PCI_CLASS_REVISION, &class_code); >> + class_code >>= 8; >> + pdn->class_code = class_code; >> + >> + pdn->pci_ext_config_space = 0; > Does this need to be explicitly initialised or can we rely on it being > allocated with kzalloc() or similar? > >> + pdev->dev.archdata.pci_data = pdn; >> + } else { >> + dev_err(&pdev->dev, "%s: Failed to allocate pdn\n", __func__); >> + } >> + >> + return pdn; >> +} >> >> struct pci_dn *add_dev_pci_data(struct pci_dev *pdev) >> { >> -- >> 2.20.1 >>
diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c index 341ed71250f1..67ccd7be8344 100644 --- a/arch/powerpc/kernel/pci_dn.c +++ b/arch/powerpc/kernel/pci_dn.c @@ -33,6 +33,8 @@ #include <asm/firmware.h> #include <asm/eeh.h> +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent); + /* * The function is used to find the firmware data of one * specific PCI device, which is attached to the indicated @@ -65,6 +67,9 @@ struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus) dn = pci_bus_to_OF_node(pbus); pdn = dn ? PCI_DN(dn) : NULL; + if (!pdn && pbus->self) + pdn = pbus->self->dev.archdata.pci_data; + return pdn; } @@ -74,10 +79,13 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus, struct device_node *dn = NULL; struct pci_dn *parent, *pdn; struct pci_dev *pdev = NULL; + bool pdev_found = false; /* Fast path: fetch from PCI device */ list_for_each_entry(pdev, &bus->devices, bus_list) { if (pdev->devfn == devfn) { + pdev_found = true; + if (pdev->dev.archdata.pci_data) return pdev->dev.archdata.pci_data; @@ -86,6 +94,9 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus, } } + if (!pdev_found) + pdev = NULL; + /* Fast path: fetch from device node */ pdn = dn ? PCI_DN(dn) : NULL; if (pdn) @@ -98,9 +109,12 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus, list_for_each_entry(pdn, &parent->child_list, list) { if (pdn->busno == bus->number && - pdn->devfn == devfn) - return pdn; - } + pdn->devfn == devfn) { + if (pdev) + pdev->dev.archdata.pci_data = pdn; + return pdn; + } + } return NULL; } @@ -130,14 +144,15 @@ struct pci_dn *pci_get_pdn(struct pci_dev *pdev) list_for_each_entry(pdn, &parent->child_list, list) { if (pdn->busno == pdev->bus->number && - pdn->devfn == pdev->devfn) + pdn->devfn == pdev->devfn) { + pdev->dev.archdata.pci_data = pdn; return pdn; + } } - return NULL; + return create_pdn(pdev, parent); } -#ifdef CONFIG_PCI_IOV static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent, int vf_index, int busno, int devfn) @@ -156,7 +171,9 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent, pdn->parent = parent; pdn->busno = busno; pdn->devfn = devfn; + #ifdef CONFIG_PCI_IOV pdn->vf_index = vf_index; + #endif /* CONFIG_PCI_IOV */ pdn->pe_number = IODA_INVALID_PE; INIT_LIST_HEAD(&pdn->child_list); INIT_LIST_HEAD(&pdn->list); @@ -164,7 +181,55 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent, return pdn; } -#endif + +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent) +{ + struct pci_dn *pdn = NULL; + + if (!parent) + return NULL; + + pdn = add_one_dev_pci_data(parent, 0, pdev->bus->busn_res.start, pdev->devfn); + dev_info(&pdev->dev, "Create a new pdn for devfn %2x\n", pdev->devfn / 8); + + if (pdn) { + #ifdef CONFIG_EEH + struct eeh_dev *edev; + #endif /* CONFIG_EEH */ + u32 class_code; + u16 device_id; + u16 vendor_id; + + #ifdef CONFIG_EEH + edev = eeh_dev_init(pdn); + if (!edev) { + kfree(pdn); + dev_err(&pdev->dev, "%s: Failed to allocate edev\n", __func__); + return NULL; + } + #endif /* CONFIG_EEH */ + + pci_bus_read_config_word(pdev->bus, pdev->devfn, + PCI_VENDOR_ID, &vendor_id); + pdn->vendor_id = vendor_id; + + pci_bus_read_config_word(pdev->bus, pdev->devfn, + PCI_DEVICE_ID, &device_id); + pdn->device_id = device_id; + + pci_bus_read_config_dword(pdev->bus, pdev->devfn, + PCI_CLASS_REVISION, &class_code); + class_code >>= 8; + pdn->class_code = class_code; + + pdn->pci_ext_config_space = 0; + pdev->dev.archdata.pci_data = pdn; + } else { + dev_err(&pdev->dev, "%s: Failed to allocate pdn\n", __func__); + } + + return pdn; +} struct pci_dn *add_dev_pci_data(struct pci_dev *pdev) {
If a struct pci_dn hasn't yet been created for the PCIe device (there was no DT node for it), allocate this structure and fill with info read from the device directly. Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> --- arch/powerpc/kernel/pci_dn.c | 79 ++++++++++++++++++++++++++++++++---- 1 file changed, 72 insertions(+), 7 deletions(-)