Re: [PATCH RFC v4 3/9] powerpc/pci: Create pci_dn on demand

[Sergey Miroshnichenko <s.miroshnichenko@yadro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7784 bytes --]

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
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]