On 3/5/19 11:04 AM, Oliver wrote:
> On Sat, Mar 2, 2019 at 3:04 AM Sergey Miroshnichenko
> <s.miroshniche...@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.miroshniche...@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
>>
signature.asc
Description: OpenPGP digital signature
