All of lore.kernel.org
 help / color / mirror / Atom feed
From: Naveen Naidu <naveennaidu479@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: bhelgaas@google.com, ruscur@russell.cc, oohall@gmail.com,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH v4 4/8] PCI/DPC: Use pci_aer_clear_status() in dpc_process_error()
Date: Thu, 21 Oct 2021 22:06:11 +0530	[thread overview]
Message-ID: <20211021163611.rsfmberxw6dqn5gx@theprophet> (raw)
In-Reply-To: <20211021014029.GA2657684@bhelgaas>

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@gmail.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.

I'll drop this patch in the next version of the patch series I make.

Apologies again ^^'

> >  	}
> >  }
> >  
> > -- 
> > 2.25.1
> > 
> > _______________________________________________
> > Linux-kernel-mentees mailing list
> > Linux-kernel-mentees@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

WARNING: multiple messages have this Message-ID (diff)
From: Naveen Naidu <naveennaidu479@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	oohall@gmail.com, ruscur@russell.cc, bhelgaas@google.com,
	linuxppc-dev@lists.ozlabs.org,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH v4 4/8] PCI/DPC: Use pci_aer_clear_status() in dpc_process_error()
Date: Thu, 21 Oct 2021 22:06:11 +0530	[thread overview]
Message-ID: <20211021163611.rsfmberxw6dqn5gx@theprophet> (raw)
In-Reply-To: <20211021014029.GA2657684@bhelgaas>

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@gmail.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.

I'll drop this patch in the next version of the patch series I make.

Apologies again ^^'

> >  	}
> >  }
> >  
> > -- 
> > 2.25.1
> > 
> > _______________________________________________
> > Linux-kernel-mentees mailing list
> > Linux-kernel-mentees@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

WARNING: multiple messages have this Message-ID (diff)
From: Naveen Naidu <naveennaidu479@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	oohall@gmail.com, bhelgaas@google.com,
	linuxppc-dev@lists.ozlabs.org,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH v4 4/8] PCI/DPC: Use pci_aer_clear_status() in dpc_process_error()
Date: Thu, 21 Oct 2021 22:06:11 +0530	[thread overview]
Message-ID: <20211021163611.rsfmberxw6dqn5gx@theprophet> (raw)
In-Reply-To: <20211021014029.GA2657684@bhelgaas>

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@gmail.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.

I'll drop this patch in the next version of the patch series I make.

Apologies again ^^'

> >  	}
> >  }
> >  
> > -- 
> > 2.25.1
> > 
> > _______________________________________________
> > Linux-kernel-mentees mailing list
> > Linux-kernel-mentees@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

  reply	other threads:[~2021-10-21 16:36 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-05 17:18 [PATCH v4 0/8] Fix long standing AER Error Handling Issues Naveen Naidu
2021-10-05 17:18 ` Naveen Naidu
2021-10-05 17:18 ` Naveen Naidu
2021-10-05 17:18 ` [PATCH v4 1/8] PCI/AER: Remove ID from aer_agent_string[] Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-21  1:28   ` Bjorn Helgaas
2021-10-21  1:28     ` Bjorn Helgaas
2021-10-21  1:28     ` Bjorn Helgaas
2021-10-21 16:30     ` Naveen Naidu
2021-10-21 16:30       ` Naveen Naidu
2021-10-21 16:30       ` Naveen Naidu
2021-10-21 17:03       ` Bjorn Helgaas
2021-10-21 17:03         ` Bjorn Helgaas
2021-10-21 17:03         ` Bjorn Helgaas
2021-10-05 17:18 ` [PATCH v4 2/8] PCI: Cleanup struct aer_err_info Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18 ` [PATCH v4 3/8] PCI/DPC: Initialize info->id in dpc_process_error() Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-21  1:28   ` Bjorn Helgaas
2021-10-21  1:28     ` Bjorn Helgaas
2021-10-21  1:28     ` Bjorn Helgaas
2021-10-21 16:31     ` Naveen Naidu
2021-10-21 16:31       ` Naveen Naidu
2021-10-21 16:31       ` Naveen Naidu
2021-10-05 17:18 ` [PATCH v4 4/8] PCI/DPC: Use pci_aer_clear_status() " Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-21  1:40   ` Bjorn Helgaas
2021-10-21  1:40     ` Bjorn Helgaas
2021-10-21  1:40     ` Bjorn Helgaas
2021-10-21 16:36     ` Naveen Naidu [this message]
2021-10-21 16:36       ` Naveen Naidu
2021-10-21 16:36       ` Naveen Naidu
2021-10-21 17:05       ` Bjorn Helgaas
2021-10-21 17:05         ` Bjorn Helgaas
2021-10-21 17:05         ` Bjorn Helgaas
2021-10-05 17:18 ` [PATCH v4 5/8] PCI/DPC: Converge EDR and DPC Path of clearing AER registers Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-21  2:09   ` Bjorn Helgaas
2021-10-21  2:09     ` Bjorn Helgaas
2021-10-21  2:09     ` Bjorn Helgaas
2021-10-21 16:53     ` Naveen Naidu
2021-10-21 16:53       ` Naveen Naidu
2021-10-21 16:53       ` Naveen Naidu
2021-10-21 17:11       ` Bjorn Helgaas
2021-10-21 17:11         ` Bjorn Helgaas
2021-10-21 17:11         ` Bjorn Helgaas
2021-10-05 17:18 ` [PATCH v4 6/8] PCI/AER: Clear error device AER registers in aer_irq() Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18 ` [PATCH v4 7/8] PCI/ERR: Remove redundant clearing of AER register in pcie_do_recovery() Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18 ` [PATCH v4 8/8] PCI/AER: Include DEVCTL in aer_print_error() Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu
2021-10-05 17:18   ` Naveen Naidu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211021163611.rsfmberxw6dqn5gx@theprophet \
    --to=naveennaidu479@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=helgaas@kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=oohall@gmail.com \
    --cc=ruscur@russell.cc \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.