Re: [PATCH v2 07/13] powerpc/eeh: Clean up pci_ers_result handling

[Michael Ellerman]

Sam Bobroff  writes:

> On Sat, Jun 02, 2018 at 01:40:46AM +1000, Michael Ellerman wrote:
>> Sam Bobroff  writes:
>> 
>> > 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 
>> > ---
>> > == v1 -> v2: ==
>> >
>> > * Added the value, and missing newline, to some WARN()s.
>> > * Improved name of merge_result() to pci_ers_merge_result().
>> > * Adjusted the result priorities so that unknown doesn't overlap with 
>> > _NONE.
>> 
>> These === markers seem to have confused patchwork, they ended up in the
>> patch, and then git put them in the changelog.
>> 
>> http://patchwork.ozlabs.org/patch/920194/
>> 
>> The usual format is just something like:
>> 
>> v2 - Added the value, and missing newline, to some WARN()s.
>>- Improved name of merge_result() to pci_ers_merge_result().
>>- Adjusted the result priorities so that unknown doesn't overlap with 
>> _NONE.
>> 
>> cheers
>
> Oh! I'll change it!

Thanks.

cheers

Re: [PATCH v2 07/13] powerpc/eeh: Clean up pci_ers_result handling

[Sam Bobroff]

On Sat, Jun 02, 2018 at 01:40:46AM +1000, Michael Ellerman wrote:
> Sam Bobroff  writes:
> 
> > 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 
> > ---
> > == v1 -> v2: ==
> >
> > * Added the value, and missing newline, to some WARN()s.
> > * Improved name of merge_result() to pci_ers_merge_result().
> > * Adjusted the result priorities so that unknown doesn't overlap with _NONE.
> 
> These === markers seem to have confused patchwork, they ended up in the
> patch, and then git put them in the changelog.
> 
> http://patchwork.ozlabs.org/patch/920194/
> 
> The usual format is just something like:
> 
> v2 - Added the value, and missing newline, to some WARN()s.
>- Improved name of merge_result() to pci_ers_merge_result().
>- Adjusted the result priorities so that unknown doesn't overlap with 
> _NONE.
> 
> cheers

Oh! I'll change it!

Sam.

> 
> >  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..2d3cac584899 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 1;
> > +   case PCI_ERS_RESULT_NO_AER_DRIVER: return 2;
> > +   case PCI_ERS_RESULT_RECOVERED: return 3;
> > +   case PCI_ERS_RESULT_CAN_RECOVER: return 4;
> > +   case PCI_ERS_RESULT_DISCONNECT: return 5;
> > +   case PCI_ERS_RESULT_NEED_RESET: return 6;
> > +   default:
> > +   WARN_ONCE(1, "Unknown pci_ers_result value: %d\n", (int)result);
> > +   return 0;
> > +   }
> > +};
> > +
> > +static enum pci_ers_result pci_ers_merge_result(enum pci_ers_result old,
> > +   enum pci_ers_result new)
> > +{
> > +   if (eeh_result_priority(new) > eeh_result_priority(old))
> > +   return new;
> > +   return old;
> > +}
> > +
> >  /**
> >   * 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 = pci_ers_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 = pci_ers_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 = pci_ers_merge_result(*res, rc);
> >  
> >  out:
> > eeh_pcid_put(dev);
> > -- 
> > 2.16.1.74.g9b0b1f47b
> 


signature.asc
Description: PGP signature

Re: [PATCH v2 07/13] powerpc/eeh: Clean up pci_ers_result handling

[Michael Ellerman]

Sam Bobroff  writes:

> 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 
> ---
> == v1 -> v2: ==
>
> * Added the value, and missing newline, to some WARN()s.
> * Improved name of merge_result() to pci_ers_merge_result().
> * Adjusted the result priorities so that unknown doesn't overlap with _NONE.

These === markers seem to have confused patchwork, they ended up in the
patch, and then git put them in the changelog.

http://patchwork.ozlabs.org/patch/920194/

The usual format is just something like:

v2 - Added the value, and missing newline, to some WARN()s.
   - Improved name of merge_result() to pci_ers_merge_result().
   - Adjusted the result priorities so that unknown doesn't overlap with _NONE.

cheers

>  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..2d3cac584899 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 1;
> + case PCI_ERS_RESULT_NO_AER_DRIVER: return 2;
> + case PCI_ERS_RESULT_RECOVERED: return 3;
> + case PCI_ERS_RESULT_CAN_RECOVER: return 4;
> + case PCI_ERS_RESULT_DISCONNECT: return 5;
> + case PCI_ERS_RESULT_NEED_RESET: return 6;
> + default:
> + WARN_ONCE(1, "Unknown pci_ers_result value: %d\n", (int)result);
> + return 0;
> + }
> +};
> +
> +static enum pci_ers_result pci_ers_merge_result(enum pci_ers_result old,
> + enum pci_ers_result new)
> +{
> + if (eeh_result_priority(new) > eeh_result_priority(old))
> + return new;
> + return old;
> +}
> +
>  /**
>   * 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 = pci_ers_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 = pci_ers_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 = pci_ers_merge_result(*res, rc);
>  
>  out:
>   eeh_pcid_put(dev);
> -- 
> 2.16.1.74.g9b0b1f47b

[PATCH v2 07/13] powerpc/eeh: Clean up pci_ers_result handling

[Sam Bobroff]

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 
---
== v1 -> v2: ==
* Added the value, and missing newline, to some WARN()s.
* Improved name of merge_result() to pci_ers_merge_result().
* Adjusted the result priorities so that unknown doesn't overlap with _NONE.

 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..2d3cac584899 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 1;
+   case PCI_ERS_RESULT_NO_AER_DRIVER: return 2;
+   case PCI_ERS_RESULT_RECOVERED: return 3;
+   case PCI_ERS_RESULT_CAN_RECOVER: return 4;
+   case PCI_ERS_RESULT_DISCONNECT: return 5;
+   case PCI_ERS_RESULT_NEED_RESET: return 6;
+   default:
+   WARN_ONCE(1, "Unknown pci_ers_result value: %d\n", (int)result);
+   return 0;
+   }
+};
+
+static enum pci_ers_result pci_ers_merge_result(enum pci_ers_result old,
+   enum pci_ers_result new)
+{
+   if (eeh_result_priority(new) > eeh_result_priority(old))
+   return new;
+   return old;
+}
+
 /**
  * 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 = pci_ers_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 = pci_ers_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 = pci_ers_merge_result(*res, rc);
 
 out:
eeh_pcid_put(dev);
-- 
2.16.1.74.g9b0b1f47b