Re: [Very RFC 21/46] powernv/eeh: Rework finding an existing edev in probe_pdev()

["Oliver O'Halloran" <oohall@gmail.com>]

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