On 25/11/2019 15:26, Oliver O'Halloran wrote:

On Mon, Nov 25, 2019 at 2:27 PM Alexey Kardashevskiy [off-list ref] wrote:

quoted

On 20/11/2019 12:28, Oliver O'Halloran wrote:

quoted

Have the PowerNV EEH backend allocate the eeh_dev if needed rather than using
the one attached to the pci_dn.

So that pci_dn attached one is leaked then?

Sorta, the eeh_dev attached to the pci_dn is supposed to have the same
lifetime as the pci_dn it's attached to. Whatever frees the pci_dn
should also be freeing the eeh_dev, but I'm pretty sure the only
situation where that actually happens is when removing the pci_dn for
VFs.

Oh, that's lovely. add_sriov_vf_pdns() calls eeh_dev_init() to allocate
@edev but remove_sriov_vf_pdns() does kfree(edev) by itself.

It's bad.

No sh*t :)

quoted

quoted

This gets us most of the way towards decoupling
pci_dn from the PowerNV EEH code.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
We should probably be free()ing the eeh_dev somewhere. The pci_dev release
function is the right place for it.
---
 arch/powerpc/platforms/powernv/eeh-powernv.c | 22 ++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index 1cd80b399995..7aba18e08996 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c

@@ -366,10 +366,9 @@ static int pnv_eeh_write_config(struct eeh_dev *edev,
  */
 static struct eeh_dev *pnv_eeh_probe_pdev(struct pci_dev *pdev)
 {
-     struct pci_dn *pdn = pci_get_pdn(pdev);
-     struct pci_controller *hose = pdn->phb;
-     struct pnv_phb *phb = hose->private_data;
-     struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
+     struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
+     struct pci_controller *hose = phb->hose;
+     struct eeh_dev *edev;
      uint32_t pcie_flags;
      int ret;
      int config_addr = (pdev->bus->number << 8) | (pdev->devfn);

@@ -415,12 +414,27 @@ static struct eeh_dev *pnv_eeh_probe_pdev(struct pci_dev *pdev)
      if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA)
              return NULL;

+     /* otherwise allocate and initialise a new eeh_dev */
+     edev = kzalloc(sizeof(*edev), GFP_KERNEL);
+     if (!edev) {
+             pr_err("%s: out of memory lol\n", __func__);

"lol"?

yeah lol

"unprofessional" is the word for this ;)

I am pretty sure we do not have to print anything if alloc failed

quoted

as alloc prints an error anyway. Thanks,

It does? Neat.

Well, it is this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n878

===
These generic allocation functions all emit a stack dump on failure when
used
without __GFP_NOWARN so there is no use in emitting an additional failure
message when NULL is returned.
===

More than a printk. A small detail though.


-- 
Alexey