Re: [PATCH] PCI/AER: save/restore AER registers during suspend/resume

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jul 26, 2019 at 10:15:24AM -0600, Keith Busch wrote:
> On Fri, Jul 26, 2019 at 02:00:05AM -0700, Patel, Mayurkumar wrote:
> > > The call to pci_save_state most likely occurs long before a user has an
> > > opportunity to alter these regsiters, though. Won't this just restore
> > > what was previously there, and not the state you changed it to?
> > 
> > There were two things (not sure to call them issues),
> > 1. PCI_ERR_ROOT_COMMAND resets to 0  during S3 entry/exit, which disables AER interrupt trigger
> > if AER happens on Endpoint after resume.
> > 
> > Also specified in spec. NCB-PCI_Express_Base_4.0r1.0_September-27-2017-c.pdf in
> > chapter 7.8.4.9 Root Error Command Register (Offset 2Ch) - in bitfields descriptions.
> > i.e. Correctable Error Reporting Enable – When Set, this bit enables the generation of an interrupt when
> > a correctable error is reported by any of the Functions in the Hierarchy Domain associated with this Root Port.
> > 
> > 2. Root port resets to its default configuration of UNCOR_MASK/SEVER/COR_MASK register bits after system resume.
> > This influences user configurations, how errors shall be treated if AER happens on root port itself due to Device (for example
> > Endpoint not answering which results in completion timeouts on root ports).
> > 
> > Following is one example scenario which can handled with this patch.
> > - user configures AER registers using setpci certain masks and severity based on debug requirements. This can be applied on Root port of EP.
> > - triggers system test which includes S3 entry/exit cycles.
> > - system enters s3 -> AER registers settings are saved which has been configured by users.
> > - system exits s3 -> AER registers settings are restored which has been configured by users.
> 
> Right, I was just more curious *where* the aer state was being saved
> during suspend since pci port driver only saves state during probe. This
> patch must be relying on the generic pci-driver's power management. I
> think that works, but I just didn't realize initially that we're relying
> on that path.

I agree that we should save the AER state at suspend-time, not just at
probe-time.  I *think* this should happen already via the
pci_save_state() calls in pci_pm_suspend_noirq(), etc.

> > > You are allocating the capability save buffer in aer_probe(), so this
> > > save/restore applies only to root ports. But you mention above that you
> > > want to restore end devices, right?
> > 
> > That’s correct. I agree that my commit message was not so explicit.
> > But Since I included PCI_ERR_ROOT_COMMAND register for save/restore which is specific to Root ports only & I thought
> > endpoint drivers can handle to save/restore (UNCOR_MASK/SEVER/COR_MASK) themselves with its suspend/resume functions.
> > 
> > However I am fine to move pci_add_ext_cap_save_buffer() to some other function so
> > that it also save/restores UNCOR_MASK/SEVER/COR_MASK registers for endpoints and
> > if it's not useful to save/restore MASK & SEVER registers  then also fine to remove them
> > and just restore PCI_ERR_ROOT_COMMAND & ECRC settings.  Please let me know.
> 
> If users are changing default settings that you want to preserve, that
> reason should apply to bridges and endpoints too, so I think you ought
> to allocate the save buffer for this capability for all devices that
> support it in in pci_aer_init(). Just make sure to check the device type
> in save/restore so you're not accessing root port specific registers.

I agree, I think we need to save/restore AER settings for all devices,
not just bridges.

Bjorn