Re: [PATCH v13 4/8] PCI/DPC: Add Error Disconnect Recover (EDR) support

[Bjorn Helgaas]

On Sat, Jan 25, 2020 at 03:25:00PM -0800, Kuppuswamy Sathyanarayanan wrote:
> On Fri, Jan 24, 2020 at 09:04:50AM -0600, Bjorn Helgaas wrote:
> > On Sat, Jan 18, 2020 at 08:00:33PM -0800, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > > 
> > > As per ACPI specification r6.3, sec 5.6.6, when firmware owns Downstream
> > > Port Containment (DPC), its expected to use the "Error Disconnect
> > > Recover" (EDR) notification to alert OSPM of a DPC event and if OS
> > > supports EDR, its expected to handle the software state invalidation and
> > > port recovery in OS, and also let firmware know the recovery status via
> > > _OST ACPI call. Related _OST status codes can be found in ACPI
> > > specification r6.3, sec 6.3.5.2.
> > > 
> > > Also, as per PCI firmware specification r3.2 Downstream Port Containment
> > > Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by
> > > firmware (firmware first mode), firmware is responsible for
> > > configuring the DPC and OS is responsible for error recovery. Also, OS
> > > is allowed to modify DPC registers only during the EDR notification
> > > window. So with EDR support, OS should provide DPC port services even in
> > > firmware first mode.
> ...

> > > +		acpi_status astatus;
> > > +
> > > +		dpc->adev = adev;
> > > +
> > > +		astatus = acpi_install_notify_handler(adev->handle,
> > > +						      ACPI_SYSTEM_NOTIFY,
> > > +						      edr_handle_event,
> > > +						      dpc);
> > 
> > I think there are a couple issues with the ECN here:
> > 
> >   - The ECN allows EDR notifications on host bridges (sec 4.5.1, table
> >     4-4), but it does not allow the "Locate" _DSM under host bridges
> >     (sec 4.6.13).
> > 
> >   - The ECN allows EDR notifications on root ports or switch ports
> >     that do not support DPC (sec 4.5.1), but it does not allow the
> >     "Locate" _DSM on those ports (sec 4.6.13).
> 
> Can you please give more details on where its mentioned? Following is
> the copy of "Locate" _DSM location related specification. All it says is,
> this object can be placed under any object representing root port or
> switch port. It does not seem to add any restrictions. Please let me  know
> your comments.
> 
> Location:
> This object can be placed under any object representing a DPC capable
> PCI Express Root Port or Switch Downstream Port. If a port implements
> this DSM, its child devices cannot instantiate this DSM function

You quoted it: "This object [the 'Locate' _DSM] can be placed under
any object representing a *DPC capable* PCI Express Root Port or Switch
Downstream Port."

If the intention was to allow the Locate _DSM for *any* root ports or
switch downstream ports, the spec should not include the "DPC capable"
restriction.

Bjorn