Bjorn,
On Sat, Oct 3, 2020 at 1:29 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> [+cc Sinan]
>
> On Wed, Sep 30, 2020 at 03:05:36AM -0400, Ethan Zhao wrote:
> > When uncorrectable error happens, AER driver and DPC driver interrupt
> > handlers likely call
> >
> > pcie_do_recovery()
> > ->pci_walk_bus()
> > ->report_frozen_detected()
> >
> > with pci_channel_io_frozen the same time.
> > If pci_dev_set_io_state() return true even if the original state is
> > pci_channel_io_frozen, that will cause AER or DPC handler re-enter
> > the error detecting and recovery procedure one after another.
> > The result is the recovery flow mixed between AER and DPC.
> > So simplify the pci_dev_set_io_state() function to only return true
> > when dev->error_state is changed.
> >
> > Signed-off-by: Ethan Zhao <haifeng.zhao@xxxxxxxxx>
> > Tested-by: Wen Jin <wen.jin@xxxxxxxxx>
> > Tested-by: Shanshan Zhang <ShanshanX.Zhang@xxxxxxxxx>
> > Reviewed-by: Alexandru Gagniuc <mr.nuke.me@xxxxxxxxx>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > ---
> > Changnes:
> > v2: revise description and code according to suggestion from Andy.
> > v3: change code to simpler.
> > v4: no change.
> > v5: no change.
> > v6: no change.
> >
> > drivers/pci/pci.h | 37 +++++--------------------------------
> > 1 file changed, 5 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 455b32187abd..f2beeaeda321 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -359,39 +359,12 @@ struct pci_sriov {
> > static inline bool pci_dev_set_io_state(struct pci_dev *dev,
> > pci_channel_state_t new)
> > {
> > - bool changed = false;
> > -
> > device_lock_assert(&dev->dev);
> > - switch (new) {
> > - case pci_channel_io_perm_failure:
> > - switch (dev->error_state) {
> > - case pci_channel_io_frozen:
> > - case pci_channel_io_normal:
> > - case pci_channel_io_perm_failure:
> > - changed = true;
> > - break;
> > - }
> > - break;
> > - case pci_channel_io_frozen:
> > - switch (dev->error_state) {
> > - case pci_channel_io_frozen:
> > - case pci_channel_io_normal:
> > - changed = true;
> > - break;
> > - }
> > - break;
> > - case pci_channel_io_normal:
> > - switch (dev->error_state) {
> > - case pci_channel_io_frozen:
> > - case pci_channel_io_normal:
> > - changed = true;
> > - break;
> > - }
> > - break;
> > - }
> > - if (changed)
> > - dev->error_state = new;
> > - return changed;
> > + if (dev->error_state == new)
> > + return false;
> > +
> > + dev->error_state = new;
> > + return true;
> > }
>
> IIUC this changes the behavior of the function, but it's difficult to
> analyze because it does a lot of simplification at the same time.
>
> Please consider the following, which is intended to simplify the
> function while preserving the behavior (but please verify; it's been a
> long time since I looked at this). Then maybe see how your patch
> could be done on top of this?
>
> Alternatively, come up with your own simplification patch + the
> functionality change.
>
>
> commit 983d9b1f8177 ("PCI/ERR: Simplify pci_dev_set_io_state()")
> Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Date: Tue May 19 12:28:57 2020 -0500
>
> PCI/ERR: Simplify pci_dev_set_io_state()
>
> Truth table:
>
> requested new state
> current ------------------------------------------
> state normal frozen perm_failure
> ------------ + ------------- ------------- ------------
> normal | normal frozen perm_failure
> frozen | normal frozen perm_failure
> perm_failure | perm_failure* perm_failure* perm_failure
>
> * "not changed", returns false
>
> No functional change intended.
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6d3f75867106..81408552f7c9 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -358,39 +358,21 @@ struct pci_sriov {
> static inline bool pci_dev_set_io_state(struct pci_dev *dev,
> pci_channel_state_t new)
> {
> - bool changed = false;
> -
> device_lock_assert(&dev->dev);
> - switch (new) {
> - case pci_channel_io_perm_failure:
> - switch (dev->error_state) {
> - case pci_channel_io_frozen:
> - case pci_channel_io_normal:
> - case pci_channel_io_perm_failure:
> - changed = true;
> - break;
> - }
> - break;
> - case pci_channel_io_frozen:
> - switch (dev->error_state) {
> - case pci_channel_io_frozen:
> - case pci_channel_io_normal:
> - changed = true;
> - break;
> - }
> - break;
> - case pci_channel_io_normal:
> - switch (dev->error_state) {
> - case pci_channel_io_frozen:
> - case pci_channel_io_normal:
> - changed = true;
> - break;
> - }
> - break;
> +
> + /* Can always put a device in perm_failure state */
> + if (new == pci_channel_io_perm_failure) {
> + dev->error_state == pci_channel_io_perm_failure;
> + return true;
> }
> - if (changed)
> - dev->error_state = new;
> - return changed;
> +
> + /* If already in perm_failure, can't set to normal or frozen */
> + if (dev->error_state == pci_channel_io_perm_failure)
> + return false;
This line goes to the first.
> +
> + /* Can always change normal to frozen or vice versa */
> + dev->error_state = new;
> + return true;
> }
Per code grepping, there are two kinds of cases
pci_dev_set_io_state() are called.
1. doesn't care about the return value, just set it to
pci_chnnel_io_perm_failure.
and later handling depends on dev->error_state, whatever the
value changed or not.
pciehp_unconfigure_device()
-> pci_walk_bus()
-> pci_dev_set_disconnected()
->pci_dev_set_io_state() with pci_chnnel_io_perm_failure
2. cares about the return value or the dev->error_state is changed or not. and
the later handling also depends on if the value of dev->error_state
is changed or not.
pcie_do_recovery()
->pci_walk_bus()
->report_frozen_detected()
->report_error_detected()
->pci_dev_set_io_state() with pci_channel_io_frozen.
This case, if the original value was pci_channel_io_frozen, no
need to set it again
and say it is *changed*. ----- return true only it is really changed.
pcie_do_recovery()
->pci_walk_bus()
->report_resume()
->pcI_dev_set_ios_state() with pci_channel_io_normal.
The same.
pcie_do_recovery()
->pci_walk_bus()
->report_normal_detected() with pci_channel_io_normal
The same.
I will combat the 'Truth Table' to keep the original functions
and let it return true
only when it really changes to avoid re-enter the later handling process.
Will send v7 for your review.
Thanks,
Ethan
>
> static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
