| Message ID | 20191120012859.23300-22-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: > Use the pnv_eeh_find_edev() helper to look up the eeh_dev for a device > rather than doing it via the pci_dn. This is not what the patch does. I struggle to see what is that thing really. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > --- > arch/powerpc/platforms/powernv/eeh-powernv.c | 44 ++++++++++++++------ > 1 file changed, 31 insertions(+), 13 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c > index 6ba74836a9f8..1cd80b399995 100644 > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > @@ -374,20 +374,40 @@ static struct eeh_dev *pnv_eeh_probe_pdev(struct pci_dev *pdev) > int ret; > int config_addr = (pdev->bus->number << 8) | (pdev->devfn); > > + pci_dbg(pdev, "%s: probing\n", __func__); > + > /* > - * When probing the root bridge, which doesn't have any > - * subordinate PCI devices. We don't have OF node for > - * the root bridge. So it's not reasonable to continue > - * the probing. > + * EEH keeps the eeh_dev alive over a recovery pass even when the > + * corresponding pci_dev has been torn down. In that case we need > + * to find the existing eeh_dev and re-bind the two. > */ > - if (!edev || edev->pe) > - return NULL; > + edev = pnv_eeh_find_edev(phb, config_addr); What was @edev before this line? > + if (edev) { > + eeh_edev_dbg(edev, "Found existing edev!\n"); > + > + /* > + * XXX: eeh_remove_device() clears pdev so we shouldn't hit this > + * normally. I've found that screwing around with the pci probe > + * path can result in eeh_probe_pdev() being called twice. This > + * is harmless at the moment, but it's pretty strange so emit a > + * warning to be on the safe side. > + */ > + if (WARN_ON(edev->pdev)) > + eeh_edev_dbg(edev, "%s: already bound to a pdev!\n", __func__); > + > + edev->pdev = pdev; > + > + /* should we be doing something with REMOVED too? */ > + edev->mode &= EEH_DEV_DISCONNECTED; > + > + /* update the primary bus if we need to */ > + // XXX: why do we need to do this? is the pci_bus going away? what cleared the flag? From just reading this patch alone: if you do not know why we need it, then why did you add it here (it is not cut-n-paste)? Thanks, > + if (!(edev->pe->state & EEH_PE_PRI_BUS)) { > + edev->pe->bus = pdev->bus; > + if (edev->pe->bus) > + edev->pe->state |= EEH_PE_PRI_BUS; > + } > > - /* already configured? */ > - if (edev->pdev) { > - pr_debug("%s: found existing edev for %04x:%02x:%02x.%01x\n", > - __func__, hose->global_number, config_addr >> 8, > - PCI_SLOT(config_addr), PCI_FUNC(config_addr)); > return edev; > } > > @@ -395,8 +415,6 @@ static struct eeh_dev *pnv_eeh_probe_pdev(struct pci_dev *pdev) > if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) > return NULL; > > - eeh_edev_dbg(edev, "Probing device\n"); > - > /* Initialize eeh device */ > edev->class_code = pdev->class; > edev->pcix_cap = pci_find_capability(pdev, PCI_CAP_ID_PCIX); >
On Mon, Nov 25, 2019 at 2:20 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > > On 20/11/2019 12:28, Oliver O'Halloran wrote: > > Use the pnv_eeh_find_edev() helper to look up the eeh_dev for a device > > rather than doing it via the pci_dn. > > This is not what the patch does. I struggle to see what is that thing > really. Hmm, looks like a rebase screw up. This patch and the following one (22/46) used to be one patch, but I thought it was getting a bit large and split them. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > > --- > > arch/powerpc/platforms/powernv/eeh-powernv.c | 44 ++++++++++++++------ > > 1 file changed, 31 insertions(+), 13 deletions(-) > > > > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c > > index 6ba74836a9f8..1cd80b399995 100644 > > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > > @@ -374,20 +374,40 @@ static struct eeh_dev *pnv_eeh_probe_pdev(struct pci_dev *pdev) > > int ret; > > int config_addr = (pdev->bus->number << 8) | (pdev->devfn); > > > > + pci_dbg(pdev, "%s: probing\n", __func__); > > + > > /* > > - * When probing the root bridge, which doesn't have any > > - * subordinate PCI devices. We don't have OF node for > > - * the root bridge. So it's not reasonable to continue > > - * the probing. > > + * EEH keeps the eeh_dev alive over a recovery pass even when the > > + * corresponding pci_dev has been torn down. In that case we need > > + * to find the existing eeh_dev and re-bind the two. > > */ > > - if (!edev || edev->pe) > > - return NULL; > > + edev = pnv_eeh_find_edev(phb, config_addr); > > > What was @edev before this line? 22/46 has the following hunk which should probably be in this patch: diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c index 1cd80b3..7aba18e 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; > > + if (edev) { > > + eeh_edev_dbg(edev, "Found existing edev!\n"); > > + > > + /* > > + * XXX: eeh_remove_device() clears pdev so we shouldn't hit this > > + * normally. I've found that screwing around with the pci probe > > + * path can result in eeh_probe_pdev() being called twice. This > > + * is harmless at the moment, but it's pretty strange so emit a > > + * warning to be on the safe side. > > + */ > > + if (WARN_ON(edev->pdev)) > > + eeh_edev_dbg(edev, "%s: already bound to a pdev!\n", __func__); > > + > > + edev->pdev = pdev; > > + > > + /* should we be doing something with REMOVED too? */ > > + edev->mode &= EEH_DEV_DISCONNECTED; > > + > > + /* update the primary bus if we need to */ > > + // XXX: why do we need to do this? is the pci_bus going away? what cleared the flag? > > From just reading this patch alone: if you do not know why we need it, There's a few comments in here that are essentially notes to myself that I thought other people might be able to shed light on. The series is tagged "Very RFC" for a reason ;) > then why did you add it here (it is not cut-n-paste)? Thanks, dunno lol
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c index 6ba74836a9f8..1cd80b399995 100644 --- a/arch/powerpc/platforms/powernv/eeh-powernv.c +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c @@ -374,20 +374,40 @@ static struct eeh_dev *pnv_eeh_probe_pdev(struct pci_dev *pdev) int ret; int config_addr = (pdev->bus->number << 8) | (pdev->devfn); + pci_dbg(pdev, "%s: probing\n", __func__); + /* - * When probing the root bridge, which doesn't have any - * subordinate PCI devices. We don't have OF node for - * the root bridge. So it's not reasonable to continue - * the probing. + * EEH keeps the eeh_dev alive over a recovery pass even when the + * corresponding pci_dev has been torn down. In that case we need + * to find the existing eeh_dev and re-bind the two. */ - if (!edev || edev->pe) - return NULL; + edev = pnv_eeh_find_edev(phb, config_addr); + if (edev) { + eeh_edev_dbg(edev, "Found existing edev!\n"); + + /* + * XXX: eeh_remove_device() clears pdev so we shouldn't hit this + * normally. I've found that screwing around with the pci probe + * path can result in eeh_probe_pdev() being called twice. This + * is harmless at the moment, but it's pretty strange so emit a + * warning to be on the safe side. + */ + if (WARN_ON(edev->pdev)) + eeh_edev_dbg(edev, "%s: already bound to a pdev!\n", __func__); + + edev->pdev = pdev; + + /* should we be doing something with REMOVED too? */ + edev->mode &= EEH_DEV_DISCONNECTED; + + /* update the primary bus if we need to */ + // XXX: why do we need to do this? is the pci_bus going away? what cleared the flag? + if (!(edev->pe->state & EEH_PE_PRI_BUS)) { + edev->pe->bus = pdev->bus; + if (edev->pe->bus) + edev->pe->state |= EEH_PE_PRI_BUS; + } - /* already configured? */ - if (edev->pdev) { - pr_debug("%s: found existing edev for %04x:%02x:%02x.%01x\n", - __func__, hose->global_number, config_addr >> 8, - PCI_SLOT(config_addr), PCI_FUNC(config_addr)); return edev; } @@ -395,8 +415,6 @@ static struct eeh_dev *pnv_eeh_probe_pdev(struct pci_dev *pdev) if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) return NULL; - eeh_edev_dbg(edev, "Probing device\n"); - /* Initialize eeh device */ edev->class_code = pdev->class; edev->pcix_cap = pci_find_capability(pdev, PCI_CAP_ID_PCIX);
Use the pnv_eeh_find_edev() helper to look up the eeh_dev for a device rather than doing it via the pci_dn. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- arch/powerpc/platforms/powernv/eeh-powernv.c | 44 ++++++++++++++------ 1 file changed, 31 insertions(+), 13 deletions(-)