| Message ID | 20191120012859.23300-23-oohall@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [Very,RFC,01/46] powerpc/eeh: Don't attempt to restore VF config space after reset | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (784eee1cc44801366d4f197e0ade7739ee8e1e83) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (0695f8bca93ea0c57f0e8e21b4b4db70183b3d1c) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (c74386d50fbaf4a54fd3fe560f1abc709c0cff4b) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (7d6475051fb3d9339c5c760ed9883bc0a9048b21) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (5d1131b4d61e52e5702e0fa4bcbec81ac7d6ef52) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On 20/11/2019 12:28, Oliver O'Halloran wrote: > 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? > 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"? I am pretty sure we do not have to print anything if alloc failed as alloc prints an error anyway. Thanks, > + return NULL; > + } > + > /* Initialize eeh device */ > + edev->bdfn = config_addr; > + edev->controller = phb->hose; > + > edev->class_code = pdev->class; > edev->pcix_cap = pci_find_capability(pdev, PCI_CAP_ID_PCIX); > edev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP); > edev->af_cap = pci_find_capability(pdev, PCI_CAP_ID_AF); > edev->aer_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR); > + > + /* TODO: stash the vf_index in here? */ > + if (pdev->is_virtfn) > + edev->physfn = pdev->physfn; > + > if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_PCI) { > edev->mode |= EEH_DEV_BRIDGE; > if (edev->pcie_cap) { >
On Mon, Nov 25, 2019 at 2:27 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > > On 20/11/2019 12:28, Oliver O'Halloran wrote: > > 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. It's bad. > > 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 I am pretty sure we do not have to print anything if alloc failed > as alloc prints an error anyway. Thanks, It does? Neat.
On 25/11/2019 15:26, Oliver O'Halloran wrote: > On Mon, Nov 25, 2019 at 2:27 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> >> >> >> On 20/11/2019 12:28, Oliver O'Halloran wrote: >>> 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 :) > >>> 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 >> 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.
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__); + return NULL; + } + /* Initialize eeh device */ + edev->bdfn = config_addr; + edev->controller = phb->hose; + edev->class_code = pdev->class; edev->pcix_cap = pci_find_capability(pdev, PCI_CAP_ID_PCIX); edev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP); edev->af_cap = pci_find_capability(pdev, PCI_CAP_ID_AF); edev->aer_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR); + + /* TODO: stash the vf_index in here? */ + if (pdev->is_virtfn) + edev->physfn = pdev->physfn; + if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_PCI) { edev->mode |= EEH_DEV_BRIDGE; if (edev->pcie_cap) {
Have the PowerNV EEH backend allocate the eeh_dev if needed rather than using the one attached to the pci_dn. 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(-)