Re: [PATCH pci-next] pci/edr: Ignore Surprise Down error on hot removal

[Ethan Zhao]

On 3/5/2024 3:33 AM, Smita Koralahalli wrote:
> Hi Ethan,
>
> On 3/4/2024 3:58 AM, Lukas Wunner wrote:
> > On Mon, Mar 04, 2024 at 04:08:19AM -0500, Ethan Zhao wrote:
> > > Per PCI firmware spec r3.3 sec 4.6.12, for firmware first mode DPC
> > > handling path, FW should clear UC errors logged by port and bring link
> > > out of DPC, but because of ambiguity of wording in the spec, some 
> > > BIOSes
> > > doesn't clear the surprise down error and the error bits in pci status,
> > > still notify OS to handle it. thus following trick is needed in EDR 
> > > when
> > > double reporting (hot removal interrupt && dpc notification) is hit.
>
> Please correct me if I'm wrong.
>
> When there is double reporting (hot removal interrupt && dpc 
> notification), won't the DPC handler be called always which takes care 
> of clearing the surprise down errors? Do we need it again from EDR 
> handler?

My understanding, if firmware first mode is enabled, DPC driver wouldn't
be enabled, EDR is notified instead, though some of the common functions
are used in EDR, such as dpc_process_error() is called in edr_handle_event(),
but dpc_handler() isn't called, so does the dpc_handle_surprise_removal().

Thanks,
Ethan

> Thanks
> Smita
>
> > Please provide more detailed information about the hardware and BIOS
> > affected by this.
> >
> >
> > > -static void dpc_handle_surprise_removal(struct pci_dev *pdev)
> > > +bool  dpc_handle_surprise_removal(struct pci_dev *pdev)
> > >   {
> > > +    if (!dpc_is_surprise_removal(pdev))
> > > +        return false;
> >
> > This change of moving dpc_is_surprise_removal() into
> > dpc_handle_surprise_removal() seems unrelated to the problem at hand.
> >
> > Please drop it if it's unnecessary to fix the issue.
> >
> >
> > > --- a/drivers/pci/pcie/edr.c
> > > +++ b/drivers/pci/pcie/edr.c
> > > @@ -184,6 +184,9 @@ static void edr_handle_event(acpi_handle handle, 
> > > u32 event, void *data)
> > >           goto send_ost;
> > >       }
> > >   +    if (dpc_handle_surprise_removal(edev))
> > > +        goto send_ost;
> > > +
> > >       dpc_process_error(edev);
> > >       pci_aer_raw_clear_status(edev);
> >
> > This seems to be the only necessary change.  Please reduce the
> > patch to contain only it and no other refactoring.
> >
> > Please capitalize the "PCI/EDR: " prefix in the subject and add
> > a Fixes tag.
> >
> > Thanks,
> >
> > Lukas