Re: [EXTERNAL] Re: [PATCH v2 3/6] powerpc/eeh: Improve debug messages around device addition

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:

quoted

On Thu, Jun 20, 2019 at 01:45:24PM +1000, Oliver O'Halloran wrote:

quoted

On Thu, Jun 20, 2019 at 12:40 PM Alexey Kardashevskiy [off-list ref] wrote:

quoted

On 19/06/2019 14:27, Sam Bobroff wrote:

quoted

On Tue, Jun 11, 2019 at 03:47:58PM +1000, Alexey Kardashevskiy wrote:

quoted

On 07/05/2019 14:30, Sam Bobroff wrote:

quoted

Also remove useless comment.

Signed-off-by: Sam Bobroff <redacted>
Reviewed-by: Alexey Kardashevskiy <redacted>
---

*snip*

quoted

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

quoted

(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.

quoted

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 :-)