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 <ooh...@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);
>
--
Alexey
