linux-kernel - Re: [PATCH v4 4/8] PCI/DPC: Use pci_aer_clear_status() in dpc_process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211021170507.GA2701463@bhelgaas>
Date:   Thu, 21 Oct 2021 12:05:07 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Naveen Naidu <naveennaidu479@...il.com>
Cc:     bhelgaas@...gle.com, ruscur@...sell.cc, oohall@...il.com,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH v4 4/8] PCI/DPC: Use pci_aer_clear_status() in
 dpc_process_error()

On Thu, Oct 21, 2021 at 10:06:11PM +0530, Naveen Naidu wrote:
> On 20/10, Bjorn Helgaas wrote:
> > On Tue, Oct 05, 2021 at 10:48:11PM +0530, Naveen Naidu wrote:
> > > dpc_process_error() clears both AER fatal and non fatal status
> > > registers. Instead of clearing each status registers via a different
> > > function call use pci_aer_clear_status().
> > > 
> > > This helps clean up the code a bit.
> > > 
> > > Signed-off-by: Naveen Naidu <naveennaidu479@...il.com>
> > > ---
> > >  drivers/pci/pcie/dpc.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > index df3f3a10f8bc..faf4a1e77fab 100644
> > > --- a/drivers/pci/pcie/dpc.c
> > > +++ b/drivers/pci/pcie/dpc.c
> > > @@ -288,8 +288,7 @@ void dpc_process_error(struct pci_dev *pdev)
> > >  		 dpc_get_aer_uncorrect_severity(pdev, &info) &&
> > >  		 aer_get_device_error_info(pdev, &info)) {
> > >  		aer_print_error(pdev, &info);
> > > -		pci_aer_clear_nonfatal_status(pdev);
> > > -		pci_aer_clear_fatal_status(pdev);
> > > +		pci_aer_clear_status(pdev);
> > 
> > The commit log suggests that this is a simple cleanup that doesn't
> > change any behavior, but that's not quite true:
> > 
> >   - The new code would clear PCI_ERR_ROOT_STATUS, but the old code
> >     does not.
> > 
> >   - The old code masks the status bits with the severity bits before
> >     clearing, but the new code does not.
> > 
> > The commit log needs to show why these changes are what we want.
> >
> 
> Reading through the code again, I realize how wrong(stupid) I was when
> making this patch. I was thinking that:
> 
>   pci_aer_clear_status() = pci_aer_clear_fatal_status() + pci_aer_clear_nonfatal_status()
> 
> Now I understand, that it is not at all the case. I apologize for the
> mistake. I'll make sure to be meticulous while reading functions and not
> just assume their behaviour just from their function names.

No problem, one could argue that the collection of pci_aer_clear_*()
functions that do slightly different things is itself a defect.