[v2,3/6] powerpc/eeh: Improve debug messages around device addition

Also remove useless comment.

Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 arch/powerpc/kernel/eeh.c                    |  2 +-
 arch/powerpc/platforms/powernv/eeh-powernv.c | 14 ++++++++----
 arch/powerpc/platforms/pseries/eeh_pseries.c | 23 +++++++++++++++-----
 3 files changed, 28 insertions(+), 11 deletions(-)

On 07/05/2019 14:30, Sam Bobroff wrote:
> Also remove useless comment.
> 
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  arch/powerpc/kernel/eeh.c                    |  2 +-
>  arch/powerpc/platforms/powernv/eeh-powernv.c | 14 ++++++++----
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 23 +++++++++++++++-----
>  3 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 8d3c36a1f194..b14d89547895 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1291,7 +1291,7 @@ void eeh_add_device_late(struct pci_dev *dev)
>  	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
>  	edev = pdn_to_eeh_dev(pdn);
>  	if (edev->pdev == dev) {
> -		pr_debug("EEH: Already referenced !\n");
> +		pr_debug("EEH: Device %s already referenced!\n", pci_name(dev));
>  		return;
>  	}
>  
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index 6fc1a463b796..0e374cdba961 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -50,10 +50,7 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
>  	if (!pdev->is_virtfn)
>  		return;
>  
> -	/*
> -	 * The following operations will fail if VF's sysfs files
> -	 * aren't created or its resources aren't finalized.
> -	 */
> +	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));


dev_dbg() seems more appropriate.


>  	eeh_add_device_early(pdn);
>  	eeh_add_device_late(pdev);
>  	eeh_sysfs_add_device(pdev);
> @@ -397,6 +394,10 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
>  	int ret;
>  	int config_addr = (pdn->busno << 8) | (pdn->devfn);
>  
> +	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
> +		__func__, hose->global_number, pdn->busno,
> +		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
> +
>  	/*
>  	 * When probing the root bridge, which doesn't have any
>  	 * subordinate PCI devices. We don't have OF node for
> @@ -491,6 +492,11 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
>  	/* Save memory bars */
>  	eeh_save_bars(edev);
>  
> +	pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
> +		__func__, pdn->busno, PCI_SLOT(pdn->devfn),
> +		PCI_FUNC(pdn->devfn), edev->pe->phb->global_number,
> +		edev->pe->addr);
> +
>  	return NULL;
>  }
>  
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 7aa50258dd42..ae06878fbdea 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -65,6 +65,8 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
>  	if (!pdev->is_virtfn)
>  		return;
>  
> +	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));
> +
>  	pdn->device_id  =  pdev->device;
>  	pdn->vendor_id  =  pdev->vendor;
>  	pdn->class_code =  pdev->class;
> @@ -251,6 +253,10 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>  	int enable = 0;
>  	int ret;
>  
> +	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
> +		__func__, pdn->phb->global_number, pdn->busno,
> +		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
> +
>  	/* Retrieve OF node and eeh device */
>  	edev = pdn_to_eeh_dev(pdn);
>  	if (!edev || edev->pe)
> @@ -294,7 +300,12 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>  
>  	/* Enable EEH on the device */
>  	ret = eeh_ops->set_option(&pe, EEH_OPT_ENABLE);
> -	if (!ret) {
> +	if (ret) {
> +		pr_debug("%s: EEH failed to enable on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
> +			__func__, pdn->busno, PCI_SLOT(pdn->devfn),
> +			PCI_FUNC(pdn->devfn), pe.phb->global_number,
> +			pe.addr, ret);
> +	} else {


edev!=NULL here so you could do dev_dbg(&edev->pdev->dev,...) and skip
PCI_SLOT/PCI_FUNC. Or is (edev!=NULL && edev->pdev==NULL) possible (it
could be, just asking)?


>  		/* Retrieve PE address */
>  		edev->pe_config_addr = eeh_ops->get_pe_addr(&pe);
>  		pe.addr = edev->pe_config_addr;
> @@ -310,11 +321,6 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>  		if (enable) {
>  			eeh_add_flag(EEH_ENABLED);
>  			eeh_add_to_parent_pe(edev);
> -
> -			pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
> -				__func__, pdn->busno, PCI_SLOT(pdn->devfn),
> -				PCI_FUNC(pdn->devfn), pe.phb->global_number,
> -				pe.addr);
>  		} else if (pdn->parent && pdn_to_eeh_dev(pdn->parent) &&
>  			   (pdn_to_eeh_dev(pdn->parent))->pe) {
>  			/* This device doesn't support EEH, but it may have an
> @@ -323,6 +329,11 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>  			edev->pe_config_addr = pdn_to_eeh_dev(pdn->parent)->pe_config_addr;
>  			eeh_add_to_parent_pe(edev);
>  		}
> +		pr_debug("%s: EEH %s on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
> +			__func__, (enable ? "enabled" : "unsupported"),
> +			pdn->busno, PCI_SLOT(pdn->devfn),
> +			PCI_FUNC(pdn->devfn), pe.phb->global_number,
> +			pe.addr, ret);

Same here. I understand though this one is a cut-n-paste :)


>  	}
>  
>  	/* Save memory bars */
>

On Tue, Jun 11, 2019 at 03:47:58PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 07/05/2019 14:30, Sam Bobroff wrote:
> > Also remove useless comment.
> > 
> > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> >  arch/powerpc/kernel/eeh.c                    |  2 +-
> >  arch/powerpc/platforms/powernv/eeh-powernv.c | 14 ++++++++----
> >  arch/powerpc/platforms/pseries/eeh_pseries.c | 23 +++++++++++++++-----
> >  3 files changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> > index 8d3c36a1f194..b14d89547895 100644
> > --- a/arch/powerpc/kernel/eeh.c
> > +++ b/arch/powerpc/kernel/eeh.c
> > @@ -1291,7 +1291,7 @@ void eeh_add_device_late(struct pci_dev *dev)
> >  	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
> >  	edev = pdn_to_eeh_dev(pdn);
> >  	if (edev->pdev == dev) {
> > -		pr_debug("EEH: Already referenced !\n");
> > +		pr_debug("EEH: Device %s already referenced!\n", pci_name(dev));
> >  		return;
> >  	}
> >  
> > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> > index 6fc1a463b796..0e374cdba961 100644
> > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> > @@ -50,10 +50,7 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
> >  	if (!pdev->is_virtfn)
> >  		return;
> >  
> > -	/*
> > -	 * The following operations will fail if VF's sysfs files
> > -	 * aren't created or its resources aren't finalized.
> > -	 */
> > +	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));
> 
> 
> dev_dbg() seems more appropriate.

Oh! It does, or even pci_debug() :-)

I'll change it if I need to do another version, otherwise I'll clean it
up later.

> >  	eeh_add_device_early(pdn);
> >  	eeh_add_device_late(pdev);
> >  	eeh_sysfs_add_device(pdev);
> > @@ -397,6 +394,10 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
> >  	int ret;
> >  	int config_addr = (pdn->busno << 8) | (pdn->devfn);
> >  
> > +	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
> > +		__func__, hose->global_number, pdn->busno,
> > +		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
> > +
> >  	/*
> >  	 * When probing the root bridge, which doesn't have any
> >  	 * subordinate PCI devices. We don't have OF node for
> > @@ -491,6 +492,11 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
> >  	/* Save memory bars */
> >  	eeh_save_bars(edev);
> >  
> > +	pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
> > +		__func__, pdn->busno, PCI_SLOT(pdn->devfn),
> > +		PCI_FUNC(pdn->devfn), edev->pe->phb->global_number,
> > +		edev->pe->addr);
> > +
> >  	return NULL;
> >  }
> >  
> > diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> > index 7aa50258dd42..ae06878fbdea 100644
> > --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> > +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> > @@ -65,6 +65,8 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
> >  	if (!pdev->is_virtfn)
> >  		return;
> >  
> > +	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));
> > +
> >  	pdn->device_id  =  pdev->device;
> >  	pdn->vendor_id  =  pdev->vendor;
> >  	pdn->class_code =  pdev->class;
> > @@ -251,6 +253,10 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
> >  	int enable = 0;
> >  	int ret;
> >  
> > +	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
> > +		__func__, pdn->phb->global_number, pdn->busno,
> > +		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
> > +
> >  	/* Retrieve OF node and eeh device */
> >  	edev = pdn_to_eeh_dev(pdn);
> >  	if (!edev || edev->pe)
> > @@ -294,7 +300,12 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
> >  
> >  	/* Enable EEH on the device */
> >  	ret = eeh_ops->set_option(&pe, EEH_OPT_ENABLE);
> > -	if (!ret) {
> > +	if (ret) {
> > +		pr_debug("%s: EEH failed to enable on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
> > +			__func__, pdn->busno, PCI_SLOT(pdn->devfn),
> > +			PCI_FUNC(pdn->devfn), pe.phb->global_number,
> > +			pe.addr, ret);
> > +	} else {
> 
> 
> edev!=NULL here so you could do dev_dbg(&edev->pdev->dev,...) and skip
> PCI_SLOT/PCI_FUNC. Or is (edev!=NULL && edev->pdev==NULL) possible (it
> could be, just asking)?

I can see that edev will be non-NULL here, but that pr_debug() pattern
(using the PDN information to form the PCI address) is quite common
across the EEH code, so I think rather than changing a couple of
specific cases, I should do a separate cleanup patch and introduce
something like pdn_debug(pdn, "...."). What do you think?

(I don't know exactly when edev->pdev can be NULL.)

> 
> >  		/* Retrieve PE address */
> >  		edev->pe_config_addr = eeh_ops->get_pe_addr(&pe);
> >  		pe.addr = edev->pe_config_addr;
> > @@ -310,11 +321,6 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
> >  		if (enable) {
> >  			eeh_add_flag(EEH_ENABLED);
> >  			eeh_add_to_parent_pe(edev);
> > -
> > -			pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
> > -				__func__, pdn->busno, PCI_SLOT(pdn->devfn),
> > -				PCI_FUNC(pdn->devfn), pe.phb->global_number,
> > -				pe.addr);
> >  		} else if (pdn->parent && pdn_to_eeh_dev(pdn->parent) &&
> >  			   (pdn_to_eeh_dev(pdn->parent))->pe) {
> >  			/* This device doesn't support EEH, but it may have an
> > @@ -323,6 +329,11 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
> >  			edev->pe_config_addr = pdn_to_eeh_dev(pdn->parent)->pe_config_addr;
> >  			eeh_add_to_parent_pe(edev);
> >  		}
> > +		pr_debug("%s: EEH %s on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
> > +			__func__, (enable ? "enabled" : "unsupported"),
> > +			pdn->busno, PCI_SLOT(pdn->devfn),
> > +			PCI_FUNC(pdn->devfn), pe.phb->global_number,
> > +			pe.addr, ret);
> 
> Same here. I understand though this one is a cut-n-paste :)
> 
> 
> >  	}
> >  
> >  	/* Save memory bars */
> > 
> 
> -- 
> Alexey

On 19/06/2019 14:27, Sam Bobroff wrote:
> On Tue, Jun 11, 2019 at 03:47:58PM +1000, Alexey Kardashevskiy wrote:
>>
>>
>> On 07/05/2019 14:30, Sam Bobroff wrote:
>>> Also remove useless comment.
>>>
>>> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
>>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>  arch/powerpc/kernel/eeh.c                    |  2 +-
>>>  arch/powerpc/platforms/powernv/eeh-powernv.c | 14 ++++++++----
>>>  arch/powerpc/platforms/pseries/eeh_pseries.c | 23 +++++++++++++++-----
>>>  3 files changed, 28 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>> index 8d3c36a1f194..b14d89547895 100644
>>> --- a/arch/powerpc/kernel/eeh.c
>>> +++ b/arch/powerpc/kernel/eeh.c
>>> @@ -1291,7 +1291,7 @@ void eeh_add_device_late(struct pci_dev *dev)
>>>  	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
>>>  	edev = pdn_to_eeh_dev(pdn);
>>>  	if (edev->pdev == dev) {
>>> -		pr_debug("EEH: Already referenced !\n");
>>> +		pr_debug("EEH: Device %s already referenced!\n", pci_name(dev));
>>>  		return;
>>>  	}
>>>  
>>> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
>>> index 6fc1a463b796..0e374cdba961 100644
>>> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
>>> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
>>> @@ -50,10 +50,7 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
>>>  	if (!pdev->is_virtfn)
>>>  		return;
>>>  
>>> -	/*
>>> -	 * The following operations will fail if VF's sysfs files
>>> -	 * aren't created or its resources aren't finalized.
>>> -	 */
>>> +	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));
>>
>>
>> dev_dbg() seems more appropriate.
> 
> Oh! It does, or even pci_debug() :-)
> 
> I'll change it if I need to do another version, otherwise I'll clean it
> up later.
> 
>>>  	eeh_add_device_early(pdn);
>>>  	eeh_add_device_late(pdev);
>>>  	eeh_sysfs_add_device(pdev);
>>> @@ -397,6 +394,10 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
>>>  	int ret;
>>>  	int config_addr = (pdn->busno << 8) | (pdn->devfn);
>>>  
>>> +	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
>>> +		__func__, hose->global_number, pdn->busno,
>>> +		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
>>> +
>>>  	/*
>>>  	 * When probing the root bridge, which doesn't have any
>>>  	 * subordinate PCI devices. We don't have OF node for
>>> @@ -491,6 +492,11 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
>>>  	/* Save memory bars */
>>>  	eeh_save_bars(edev);
>>>  
>>> +	pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
>>> +		__func__, pdn->busno, PCI_SLOT(pdn->devfn),
>>> +		PCI_FUNC(pdn->devfn), edev->pe->phb->global_number,
>>> +		edev->pe->addr);
>>> +
>>>  	return NULL;
>>>  }
>>>  
>>> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
>>> index 7aa50258dd42..ae06878fbdea 100644
>>> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
>>> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
>>> @@ -65,6 +65,8 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
>>>  	if (!pdev->is_virtfn)
>>>  		return;
>>>  
>>> +	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));
>>> +
>>>  	pdn->device_id  =  pdev->device;
>>>  	pdn->vendor_id  =  pdev->vendor;
>>>  	pdn->class_code =  pdev->class;
>>> @@ -251,6 +253,10 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>>>  	int enable = 0;
>>>  	int ret;
>>>  
>>> +	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
>>> +		__func__, pdn->phb->global_number, pdn->busno,
>>> +		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
>>> +
>>>  	/* Retrieve OF node and eeh device */
>>>  	edev = pdn_to_eeh_dev(pdn);
>>>  	if (!edev || edev->pe)
>>> @@ -294,7 +300,12 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>>>  
>>>  	/* Enable EEH on the device */
>>>  	ret = eeh_ops->set_option(&pe, EEH_OPT_ENABLE);
>>> -	if (!ret) {
>>> +	if (ret) {
>>> +		pr_debug("%s: EEH failed to enable on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
>>> +			__func__, pdn->busno, PCI_SLOT(pdn->devfn),
>>> +			PCI_FUNC(pdn->devfn), pe.phb->global_number,
>>> +			pe.addr, ret);
>>> +	} else {
>>
>>
>> edev!=NULL here so you could do dev_dbg(&edev->pdev->dev,...) and skip
>> PCI_SLOT/PCI_FUNC. Or is (edev!=NULL && edev->pdev==NULL) possible (it
>> could be, just asking)?
> 
> I can see that edev will be non-NULL here, but that pr_debug() pattern
> (using the PDN information to form the PCI address) is quite common
> across the EEH code, so I think rather than changing a couple of
> specific cases, I should do a separate cleanup patch and introduce
> something like pdn_debug(pdn, "...."). What do you think?


I'd switch them all to already existing dev_dbg/pci_debug rather than
adding pdn_debug as imho it should not have been used in the first place
really...

> (I don't know exactly when edev->pdev can be NULL.)

... and if you switch to dev_dbg/pci_debug, I think quite soon you'll
know if it can or cannot be NULL :)



> 
>>
>>>  		/* Retrieve PE address */
>>>  		edev->pe_config_addr = eeh_ops->get_pe_addr(&pe);
>>>  		pe.addr = edev->pe_config_addr;
>>> @@ -310,11 +321,6 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>>>  		if (enable) {
>>>  			eeh_add_flag(EEH_ENABLED);
>>>  			eeh_add_to_parent_pe(edev);
>>> -
>>> -			pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
>>> -				__func__, pdn->busno, PCI_SLOT(pdn->devfn),
>>> -				PCI_FUNC(pdn->devfn), pe.phb->global_number,
>>> -				pe.addr);
>>>  		} else if (pdn->parent && pdn_to_eeh_dev(pdn->parent) &&
>>>  			   (pdn_to_eeh_dev(pdn->parent))->pe) {
>>>  			/* This device doesn't support EEH, but it may have an
>>> @@ -323,6 +329,11 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>>>  			edev->pe_config_addr = pdn_to_eeh_dev(pdn->parent)->pe_config_addr;
>>>  			eeh_add_to_parent_pe(edev);
>>>  		}
>>> +		pr_debug("%s: EEH %s on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
>>> +			__func__, (enable ? "enabled" : "unsupported"),
>>> +			pdn->busno, PCI_SLOT(pdn->devfn),
>>> +			PCI_FUNC(pdn->devfn), pe.phb->global_number,
>>> +			pe.addr, ret);
>>
>> Same here. I understand though this one is a cut-n-paste :)
>>
>>
>>>  	}
>>>  
>>>  	/* Save memory bars */
>>>
>>
>> -- 
>> Alexey

On Thu, Jun 20, 2019 at 12:40 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> On 19/06/2019 14:27, Sam Bobroff wrote:
> > On Tue, Jun 11, 2019 at 03:47:58PM +1000, Alexey Kardashevskiy wrote:
> >>
> >> On 07/05/2019 14:30, Sam Bobroff wrote:
> >>> Also remove useless comment.
> >>>
> >>> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> >>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>> ---
> *snip*
> >
> > I can see that edev will be non-NULL here, but that pr_debug() pattern
> > (using the PDN information to form the PCI address) is quite common
> > across the EEH code, so I think rather than changing a couple of
> > specific cases, I should do a separate cleanup patch and introduce
> > something like pdn_debug(pdn, "...."). What do you think?
>
> I'd switch them all to already existing dev_dbg/pci_debug rather than
> adding pdn_debug as imho it should not have been used in the first place
> really...
>
> > (I don't know exactly when edev->pdev can be NULL.)
>
> ... and if you switch to dev_dbg/pci_debug, I think quite soon you'll
> know if it can or cannot be NULL :)

As far as I can tell edev->pdev is NULL in two cases:

1. Before eeh_device_add_late() has been called on the pdev. The late
part of the add maps the pdev to an edev and sets the pdev's edev
pointer and vis a vis.
2. While recoverying EEH unaware devices. Unaware devices are
destroyed and rescanned and the edev->pdev pointer is cleared by
pcibios_device_release()

In most of these cases it should be safe to use the pci_*() functions
rather than making a new one up for printing pdns. In the cases where
we might not have a PCI dev i'd make a new set of prints that take an
EEH dev rather than a pci_dn since i'd like pci_dn to die sooner
rather than later.

Oliver

On Thu, Jun 20, 2019 at 01:45:24PM +1000, Oliver O'Halloran wrote:
> On Thu, Jun 20, 2019 at 12:40 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> > On 19/06/2019 14:27, Sam Bobroff wrote:
> > > On Tue, Jun 11, 2019 at 03:47:58PM +1000, Alexey Kardashevskiy wrote:
> > >>
> > >> On 07/05/2019 14:30, Sam Bobroff wrote:
> > >>> Also remove useless comment.
> > >>>
> > >>> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > >>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > >>> ---
> > *snip*
> > >
> > > I can see that edev will be non-NULL here, but that pr_debug() pattern
> > > (using the PDN information to form the PCI address) is quite common
> > > across the EEH code, so I think rather than changing a couple of
> > > specific cases, I should do a separate cleanup patch and introduce
> > > something like pdn_debug(pdn, "...."). What do you think?
> >
> > I'd switch them all to already existing dev_dbg/pci_debug rather than
> > adding pdn_debug as imho it should not have been used in the first place
> > really...
> >
> > > (I don't know exactly when edev->pdev can be NULL.)
> >
> > ... and if you switch to dev_dbg/pci_debug, I think quite soon you'll
> > know if it can or cannot be NULL :)
> 
> As far as I can tell edev->pdev is NULL in two cases:
> 
> 1. Before eeh_device_add_late() has been called on the pdev. The late
> part of the add maps the pdev to an edev and sets the pdev's edev
> pointer and vis a vis.
> 2. While recoverying EEH unaware devices. Unaware devices are
> destroyed and rescanned and the edev->pdev pointer is cleared by
> pcibios_device_release()
> 
> In most of these cases it should be safe to use the pci_*() functions
> rather than making a new one up for printing pdns. In the cases where
> we might not have a PCI dev i'd make a new set of prints that take an
> EEH dev rather than a pci_dn since i'd like pci_dn to die sooner
> rather than later.
> 
> Oliver

I'll change the calls in {pnv,pseries}_pcibios_bus_add_device() and
eeh_add_device_late() to use dev_dbg() and post a new version.

For {pnv,pseries}_eeh_probe() I'm not sure what we can do; there's no
pci_dev available yet and while it would be nice to use the eeh_dev
rather than the pdn, it doesn't seem to have the bus/device/fn
information we need. Am I missing something there?  (The code in the
probe functions seems to get it from the pci_dn.)

If there isn't an easy way around this, would it therefore be reasonable
to just leave them open-coded as they are?

Cheers,
Sam.

On Tue, 2019-07-16 at 16:48 +1000, Sam Bobroff wrote:
> On Thu, Jun 20, 2019 at 01:45:24PM +1000, Oliver O'Halloran wrote:
> > On Thu, Jun 20, 2019 at 12:40 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > > On 19/06/2019 14:27, Sam Bobroff wrote:
> > > > On Tue, Jun 11, 2019 at 03:47:58PM +1000, Alexey Kardashevskiy wrote:
> > > > > On 07/05/2019 14:30, Sam Bobroff wrote:
> > > > > > Also remove useless comment.
> > > > > > 
> > > > > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > > > > > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > > > > ---
> > > *snip*
> > > > I can see that edev will be non-NULL here, but that pr_debug() pattern
> > > > (using the PDN information to form the PCI address) is quite common
> > > > across the EEH code, so I think rather than changing a couple of
> > > > specific cases, I should do a separate cleanup patch and introduce
> > > > something like pdn_debug(pdn, "...."). What do you think?
> > > 
> > > I'd switch them all to already existing dev_dbg/pci_debug rather than
> > > adding pdn_debug as imho it should not have been used in the first place
> > > really...
> > > 
> > > > (I don't know exactly when edev->pdev can be NULL.)
> > > 
> > > ... and if you switch to dev_dbg/pci_debug, I think quite soon you'll
> > > know if it can or cannot be NULL :)
> > 
> > As far as I can tell edev->pdev is NULL in two cases:
> > 
> > 1. Before eeh_device_add_late() has been called on the pdev. The late
> > part of the add maps the pdev to an edev and sets the pdev's edev
> > pointer and vis a vis.
> > 2. While recoverying EEH unaware devices. Unaware devices are
> > destroyed and rescanned and the edev->pdev pointer is cleared by
> > pcibios_device_release()
> > 
> > In most of these cases it should be safe to use the pci_*() functions
> > rather than making a new one up for printing pdns. In the cases where
> > we might not have a PCI dev i'd make a new set of prints that take an
> > EEH dev rather than a pci_dn since i'd like pci_dn to die sooner
> > rather than later.
> > 
> > Oliver
> 
> I'll change the calls in {pnv,pseries}_pcibios_bus_add_device() and
> eeh_add_device_late() to use dev_dbg() and post a new version.
> 
> For {pnv,pseries}_eeh_probe() I'm not sure what we can do; there's no
> pci_dev available yet and while it would be nice to use the eeh_dev
> rather than the pdn, it doesn't seem to have the bus/device/fn
> information we need. Am I missing something there?  (The code in the
> probe functions seems to get it from the pci_dn.)

We do have a pci_dev in the powernv case since pnv_eeh_probe() isn't
called until the late probe happens (which is after the pci_dev has
been created). I've got some patches to rework the probe path to make
this a bit clearer, but they need a bit more work.

> 
> If there isn't an easy way around this, would it therefore be reasonable
> to just leave them open-coded as they are?

I've had this patch floating around a while that should do the trick.
The PCI_BUSNO macro is probably unnecessary since I'm sure there is
something that does it in generic code, but I couldn't find it.


From 61ff8c23c4d13ff640fb2d069dcedacdf2ee22dd Mon Sep 17 00:00:00 2001
From: Oliver O'Halloran <oohall@gmail.com>
Date: Thu, 18 Apr 2019 18:25:13 +1000
Subject: [PATCH] powerpc/eeh: Add bdfn field to eeh_dev

Preperation for removing pci_dn from the powernv EEH code. The only thing we
really use pci_dn for is to get the bdfn of the device for config space
accesses, so adding that information to eeh_dev reduces the need to carry
around the pci_dn.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/include/asm/eeh.h     | 2 ++
 arch/powerpc/include/asm/ppc-pci.h | 2 ++
 arch/powerpc/kernel/eeh_dev.c      | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 7fd476d..a208e02 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -131,6 +131,8 @@ static inline bool eeh_pe_passed(struct eeh_pe *pe)
 struct eeh_dev {
 	int mode;			/* EEH mode			*/
 	int class_code;			/* Class code of the device	*/
+	int bdfn; 			/* bdfn of device (for cfg ops) */
+	struct pci_controller *controller;
 	int pe_config_addr;		/* PE config address		*/
 	u32 config_space[16];		/* Saved PCI config space	*/
 	int pcix_cap;			/* Saved PCIx capability	*/
diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h
index cec2d64..72860de 100644
--- a/arch/powerpc/include/asm/ppc-pci.h
+++ b/arch/powerpc/include/asm/ppc-pci.h
@@ -74,6 +74,8 @@ static inline const char *eeh_driver_name(struct pci_dev *pdev)
 
 #endif /* CONFIG_EEH */
 
+#define PCI_BUSNO(bdfn) ((bdfn >> 8) & 0xff)
+
 #else /* CONFIG_PCI */
 static inline void init_pci_config_tokens(void) { }
 #endif /* !CONFIG_PCI */
diff --git a/arch/powerpc/kernel/eeh_dev.c b/arch/powerpc/kernel/eeh_dev.c
index c4317c4..7370185 100644
--- a/arch/powerpc/kernel/eeh_dev.c
+++ b/arch/powerpc/kernel/eeh_dev.c
@@ -47,6 +47,8 @@ struct eeh_dev *eeh_dev_init(struct pci_dn *pdn)
 	/* Associate EEH device with OF node */
 	pdn->edev = edev;
 	edev->pdn = pdn;
+	edev->bdfn = (pdn->busno << 8) | pdn->devfn;
+	edev->controller = pdn->phb;
 
 	return edev;
 }

On Tue, Jul 16, 2019 at 05:00:44PM +1000, Oliver O'Halloran wrote:
> On Tue, 2019-07-16 at 16:48 +1000, Sam Bobroff wrote:
> > On Thu, Jun 20, 2019 at 01:45:24PM +1000, Oliver O'Halloran wrote:
> > > On Thu, Jun 20, 2019 at 12:40 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > > > On 19/06/2019 14:27, Sam Bobroff wrote:
> > > > > On Tue, Jun 11, 2019 at 03:47:58PM +1000, Alexey Kardashevskiy wrote:
> > > > > > On 07/05/2019 14:30, Sam Bobroff wrote:
> > > > > > > Also remove useless comment.
> > > > > > > 
> > > > > > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > > > > > > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > > > > > ---
> > > > *snip*
> > > > > I can see that edev will be non-NULL here, but that pr_debug() pattern
> > > > > (using the PDN information to form the PCI address) is quite common
> > > > > across the EEH code, so I think rather than changing a couple of
> > > > > specific cases, I should do a separate cleanup patch and introduce
> > > > > something like pdn_debug(pdn, "...."). What do you think?
> > > > 
> > > > I'd switch them all to already existing dev_dbg/pci_debug rather than
> > > > adding pdn_debug as imho it should not have been used in the first place
> > > > really...
> > > > 
> > > > > (I don't know exactly when edev->pdev can be NULL.)
> > > > 
> > > > ... and if you switch to dev_dbg/pci_debug, I think quite soon you'll
> > > > know if it can or cannot be NULL :)
> > > 
> > > As far as I can tell edev->pdev is NULL in two cases:
> > > 
> > > 1. Before eeh_device_add_late() has been called on the pdev. The late
> > > part of the add maps the pdev to an edev and sets the pdev's edev
> > > pointer and vis a vis.
> > > 2. While recoverying EEH unaware devices. Unaware devices are
> > > destroyed and rescanned and the edev->pdev pointer is cleared by
> > > pcibios_device_release()
> > > 
> > > In most of these cases it should be safe to use the pci_*() functions
> > > rather than making a new one up for printing pdns. In the cases where
> > > we might not have a PCI dev i'd make a new set of prints that take an
> > > EEH dev rather than a pci_dn since i'd like pci_dn to die sooner
> > > rather than later.
> > > 
> > > Oliver
> > 
> > I'll change the calls in {pnv,pseries}_pcibios_bus_add_device() and
> > eeh_add_device_late() to use dev_dbg() and post a new version.
> > 
> > For {pnv,pseries}_eeh_probe() I'm not sure what we can do; there's no
> > pci_dev available yet and while it would be nice to use the eeh_dev
> > rather than the pdn, it doesn't seem to have the bus/device/fn
> > information we need. Am I missing something there?  (The code in the
> > probe functions seems to get it from the pci_dn.)
> 
> We do have a pci_dev in the powernv case since pnv_eeh_probe() isn't
> called until the late probe happens (which is after the pci_dev has
> been created). I've got some patches to rework the probe path to make
> this a bit clearer, but they need a bit more work.
> 
> > 
> > If there isn't an easy way around this, would it therefore be reasonable
> > to just leave them open-coded as they are?
> 
> I've had this patch floating around a while that should do the trick.
> The PCI_BUSNO macro is probably unnecessary since I'm sure there is
> something that does it in generic code, but I couldn't find it.

Looks good, I'll try including it and create a dev_dbg style function
or macro that takes an edev.

I don't think I can use it in the pcibios bus add device handlers (where
there is no edev, or where it may be attached to the wrong device) but
I'll use it for all the other cases.

If it works out well I can follow up and update more of the EEH logging
to use it :-)

> From 61ff8c23c4d13ff640fb2d069dcedacdf2ee22dd Mon Sep 17 00:00:00 2001
> From: Oliver O'Halloran <oohall@gmail.com>
> Date: Thu, 18 Apr 2019 18:25:13 +1000
> Subject: [PATCH] powerpc/eeh: Add bdfn field to eeh_dev
> 
> Preperation for removing pci_dn from the powernv EEH code. The only thing we
> really use pci_dn for is to get the bdfn of the device for config space
> accesses, so adding that information to eeh_dev reduces the need to carry
> around the pci_dn.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
>  arch/powerpc/include/asm/eeh.h     | 2 ++
>  arch/powerpc/include/asm/ppc-pci.h | 2 ++
>  arch/powerpc/kernel/eeh_dev.c      | 2 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 7fd476d..a208e02 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -131,6 +131,8 @@ static inline bool eeh_pe_passed(struct eeh_pe *pe)
>  struct eeh_dev {
>  	int mode;			/* EEH mode			*/
>  	int class_code;			/* Class code of the device	*/
> +	int bdfn; 			/* bdfn of device (for cfg ops) */
> +	struct pci_controller *controller;
>  	int pe_config_addr;		/* PE config address		*/
>  	u32 config_space[16];		/* Saved PCI config space	*/
>  	int pcix_cap;			/* Saved PCIx capability	*/
> diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h
> index cec2d64..72860de 100644
> --- a/arch/powerpc/include/asm/ppc-pci.h
> +++ b/arch/powerpc/include/asm/ppc-pci.h
> @@ -74,6 +74,8 @@ static inline const char *eeh_driver_name(struct pci_dev *pdev)
>  
>  #endif /* CONFIG_EEH */
>  
> +#define PCI_BUSNO(bdfn) ((bdfn >> 8) & 0xff)
> +
>  #else /* CONFIG_PCI */
>  static inline void init_pci_config_tokens(void) { }
>  #endif /* !CONFIG_PCI */
> diff --git a/arch/powerpc/kernel/eeh_dev.c b/arch/powerpc/kernel/eeh_dev.c
> index c4317c4..7370185 100644
> --- a/arch/powerpc/kernel/eeh_dev.c
> +++ b/arch/powerpc/kernel/eeh_dev.c
> @@ -47,6 +47,8 @@ struct eeh_dev *eeh_dev_init(struct pci_dn *pdn)
>  	/* Associate EEH device with OF node */
>  	pdn->edev = edev;
>  	edev->pdn = pdn;
> +	edev->bdfn = (pdn->busno << 8) | pdn->devfn;
> +	edev->controller = pdn->phb;
>  
>  	return edev;
>  }
> -- 
> 2.9.5
>