On Wed, Feb 26, 2020 at 10:42:27AM -0800, Kuppuswamy Sathyanarayanan wrote:if its costly to carry it in pci_dev, we can always re-read them.
On 2/25/20 5:02 PM, Bjorn Helgaas wrote:
On Thu, Feb 13, 2020 at 10:20:16AM -0800, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
...
Yes, we could remove it. But it might need some more changes toI think this is basically what I proposed with the sample patch in my
dpc driver functions. I can think of two ways,
1. Re-factor the DPC driver not to use dpc_dev structure and just use
pci_dev in their functions implementation. But this might lead to
re-reading following dpc_dev structure members every time we
use it in dpc driver functions.
(Currently in dpc driver probe they cache the following device parameters )
 9 u16 cap_pos;
Â10ÂÂÂÂÂÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rp_extensions;
Â11ÂÂÂÂÂÂÂÂ u8ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rp_log_size;
Â12ÂÂÂÂÂÂÂÂ u16ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ctl;
Â13ÂÂÂÂÂÂÂÂ u16ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ cap;
response to your 3/5 patch. But I don't see the ctl/cap part, so
maybe I missed something.
This message should be expanded somehow. I think the point is that weYes, if we hit this error then its a firmware defect. Either
got an EDR notification, but firmware couldn't tell us where the
containment event occurred. Should that ever happen? Or is it a
firmware defect if it *does* happen?
I will use your suggestion here along with above mentioned change.
In any event, I think the message should say something like "Can't
identify source of EDR notification".
Yes, your patch will resolve this issue.
I *think* maybe if we move the DPC info into the struct pci_dev itThis seems... I'm not sure what. I guess it's really just reading
the DPC capability for use by dpc_process_error(), so maybe it's OK.
But it's a little strange to read.
will solve this issue too? I.e., we won't have a struct dpc_dev, so
we won't have this funny-looking dpc_dev_init().
Got it. Adding pci_info here will be helpful to understand the flow.
No this is a valid case. it will only happen if we have a non-acpiI agree this is a valid case (as I mentioned below). My point was
based switch attached to root port.
just that if it is a valid case, we might not want to use pci_warn()
here. Maybe pci_info() if you think it's important, or maybe no
message at all. I don't think "Initializing dpc again" is going to be
useful to a user.
Actually Alex G already proposed a patch to fix it.
Yes, ownership should be based on _OSC negotiation. I will add necessaryWhy are we not doing this via _OSC negotiation in this series? It
comments here.
would be much better if we could just do it instead of adding a
comment that we *should* do it. Nobody knows more about this than you
do, so probably nobody else is going to come along and finish this
up :)
--