linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Sam Bobroff <sbobroff@linux.ibm.com>
To: Russell Currey <ruscur@russell.cc>
Cc: linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 07/13] powerpc/eeh: Clean up pci_ers_result handling
Date: Tue, 8 May 2018 11:09:15 +1000	[thread overview]
Message-ID: <20180508010915.GA11802@tungsten.ozlabs.ibm.com> (raw)
In-Reply-To: <1525417081.2560.10.camel@russell.cc>

[-- Attachment #1: Type: text/plain, Size: 4248 bytes --]

On Fri, May 04, 2018 at 04:58:01PM +1000, Russell Currey wrote:
> On Wed, 2018-05-02 at 16:36 +1000, Sam Bobroff wrote:
> > As EEH event handling progresses, a cumulative result of type
> > pci_ers_result is built up by (some of) the eeh_report_*() functions
> > using either:
> > 	if (rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> > 	if (*res == PCI_ERS_RESULT_NONE) *res = rc;
> > or:
> > 	if ((*res == PCI_ERS_RESULT_NONE) ||
> > 	    (*res == PCI_ERS_RESULT_RECOVERED)) *res = rc;
> > 	if (*res == PCI_ERS_RESULT_DISCONNECT &&
> > 	    rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> > (Where *res is the accumulator.)
> > 
> > However, the intent is not immediately clear and the result in some
> > situations is order dependent.
> > 
> > Address this by assigning a priority to each result value, and always
> > merging to the highest priority. This renders the intent clear, and
> > provides a stable value for all orderings.
> > 
> > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > ---
> >  arch/powerpc/kernel/eeh_driver.c | 36 ++++++++++++++++++++++++++--
> > --------
> >  1 file changed, 26 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/eeh_driver.c
> > b/arch/powerpc/kernel/eeh_driver.c
> > index 188d15c4fe3a..f33dd68a9ca2 100644
> > --- a/arch/powerpc/kernel/eeh_driver.c
> > +++ b/arch/powerpc/kernel/eeh_driver.c
> > @@ -39,6 +39,29 @@ struct eeh_rmv_data {
> >  	int removed;
> >  };
> >  
> > +static int eeh_result_priority(enum pci_ers_result result)
> > +{
> > +	switch (result) {
> > +	case PCI_ERS_RESULT_NONE: return 0;
> > +	case PCI_ERS_RESULT_NO_AER_DRIVER: return 1;
> > +	case PCI_ERS_RESULT_RECOVERED: return 2;
> > +	case PCI_ERS_RESULT_CAN_RECOVER: return 3;
> > +	case PCI_ERS_RESULT_DISCONNECT: return 4;
> > +	case PCI_ERS_RESULT_NEED_RESET: return 5;
> > +	default:
> > +		WARN_ONCE(1, "Unknown pci_ers_result value");
> 
> Missing newline and also would be good to print the value was

Sounds good, will do! I'm not sure if it's mentioned elsewhere but I'll
fix the same issues in pci_ers_result_name() as well.

> > +		return 0;
> > +	}
> > +};
> > +
> > +static enum pci_ers_result merge_result(enum pci_ers_result old,
> > +					enum pci_ers_result new)
> 
> merge_result() sounds like something really generic, maybe call it
> pci_ers_merge_result() or something?

Sounds good, will do.

> Note: just learned that it stands for Error Recovery System and that's
> a thing (?)
> 
> > +{
> > +	if (eeh_result_priority(new) > eeh_result_priority(old))
> > +		return new;
> > +	return old;
> 
> MAX would be nicer as per mpe's suggestion

Sorry, I don't understand. The return value isn't MAX(new, old) so
how can MAX() do this?

> > +}
> > +
> >  /**
> >   * eeh_pcid_get - Get the PCI device driver
> >   * @pdev: PCI device
> > @@ -206,9 +229,7 @@ static void *eeh_report_error(struct eeh_dev
> > *edev, void *userdata)
> >  
> >  	rc = driver->err_handler->error_detected(dev,
> > pci_channel_io_frozen);
> >  
> > -	/* A driver that needs a reset trumps all others */
> > -	if (rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> > -	if (*res == PCI_ERS_RESULT_NONE) *res = rc;
> > +	*res = merge_result(*res, rc);
> >  
> >  	edev->in_error = true;
> >  	pci_uevent_ers(dev, PCI_ERS_RESULT_NONE);
> > @@ -249,9 +270,7 @@ static void *eeh_report_mmio_enabled(struct
> > eeh_dev *edev, void *userdata)
> >  
> >  	rc = driver->err_handler->mmio_enabled(dev);
> >  
> > -	/* A driver that needs a reset trumps all others */
> > -	if (rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> > -	if (*res == PCI_ERS_RESULT_NONE) *res = rc;
> > +	*res = merge_result(*res, rc);
> >  
> >  out:
> >  	eeh_pcid_put(dev);
> > @@ -294,10 +313,7 @@ static void *eeh_report_reset(struct eeh_dev
> > *edev, void *userdata)
> >  		goto out;
> >  
> >  	rc = driver->err_handler->slot_reset(dev);
> > -	if ((*res == PCI_ERS_RESULT_NONE) ||
> > -	    (*res == PCI_ERS_RESULT_RECOVERED)) *res = rc;
> > -	if (*res == PCI_ERS_RESULT_DISCONNECT &&
> > -	     rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
> > +	*res = merge_result(*res, rc);
> >  
> >  out:
> >  	eeh_pcid_put(dev);
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2018-05-08  1:09 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-02  6:34 [PATCH 00/13] EEH refactoring 2 Sam Bobroff
2018-05-02  6:34 ` [PATCH 01/13] powerpc/eeh: Add eeh_max_freezes to initial EEH log line Sam Bobroff
2018-05-04  5:46   ` Russell Currey
2018-05-02  6:35 ` [PATCH 02/13] powerpc/eeh: Add final message for successful recovery Sam Bobroff
2018-05-04  2:55   ` Michael Ellerman
2018-05-04  6:08     ` Russell Currey
2018-05-07  5:35       ` Sam Bobroff
2018-05-07  5:29     ` Sam Bobroff
2018-05-02  6:35 ` [PATCH 03/13] powerpc/eeh: Fix use-after-release of EEH driver Sam Bobroff
2018-05-04  2:56   ` Michael Ellerman
2018-05-07  5:38     ` Sam Bobroff
2018-05-02  6:35 ` [PATCH 04/13] powerpc/eeh: Remove unused eeh_pcid_name() Sam Bobroff
2018-05-04  6:29   ` Russell Currey
2018-05-02  6:35 ` [PATCH 05/13] powerpc/eeh: Strengthen types of eeh traversal functions Sam Bobroff
2018-05-04  6:32   ` Russell Currey
2018-05-02  6:35 ` [PATCH 06/13] powerpc/eeh: Add message when PE processing at parent Sam Bobroff
2018-05-04  6:51   ` Russell Currey
2018-05-02  6:36 ` [PATCH 07/13] powerpc/eeh: Clean up pci_ers_result handling Sam Bobroff
2018-05-04  2:58   ` Michael Ellerman
2018-05-04  6:58   ` Russell Currey
2018-05-08  1:09     ` Sam Bobroff [this message]
2018-05-02  6:36 ` [PATCH 08/13] powerpc/eeh: Introduce eeh_for_each_pe() Sam Bobroff
2018-05-04  6:59   ` Russell Currey
2018-05-02  6:36 ` [PATCH 09/13] powerpc/eeh: Introduce eeh_edev_actionable() Sam Bobroff
2018-05-02  6:36 ` [PATCH 10/13] powerpc/eeh: Introduce eeh_set_channel_state() Sam Bobroff
2018-05-02  6:36 ` [PATCH 11/13] powerpc/eeh: Introduce eeh_set_irq_state() Sam Bobroff
2018-05-04  3:02   ` Michael Ellerman
2018-05-08  1:12     ` Sam Bobroff
2018-05-02  6:36 ` [PATCH 12/13] powerpc/eeh: Cleaner handling of EEH_DEV_NO_HANDLER Sam Bobroff
2018-05-02  6:36 ` [PATCH 13/13] powerpc/eeh: Refactor report functions Sam Bobroff
2018-05-03 13:27   ` Michael Ellerman
2018-05-07  5:23     ` Sam Bobroff
2018-05-09 14:51       ` Michael Ellerman

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=20180508010915.GA11802@tungsten.ozlabs.ibm.com \
    --to=sbobroff@linux.ibm.com \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).