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

[Alexey Kardashevskiy]

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 at 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