Re: [GIT PATCH] SCSI fixes for 2.6.32-rc8

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday 01 December 2009, Jesse Barnes wrote:
> On Tue, 01 Dec 2009 12:13:47 -0500
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > On Tue, 2009-12-01 at 14:54 -0200, Kleber Sacilotto de Souza wrote:
> > > Can you please add the patch from "[PATCH] ipr: fix EEH recovery"
> > > sent to this list?
> > 
> > Adding linux-pci because this hack actually tampers with internal PCI
> > device state, which looks wrong.
> > 
> > The thread is here:
> > 
> > http://marc.info/?l=linux-scsi&m=125918723218627
> > 
> > and the proposed full patch and explanation below.
> > 
> > PCI people, is this correct, or is there a better way to do it?
> > 
> > James
> > 
> > ---
> > 
> > Hi,
> > 
> > After commits c82f63e411f1b58427c103bd95af2863b1c96dd1 (PCI: check
> > saved state before restore) and
> > 4b77b0a2ba27d64f58f16d8d4d48d8319dda36ff (PCI: Clear saved_state
> > after the state has been restored) PCI drivers are prevented from
> > restoring the device standard configuration registers twice in a row.
> > These changes introduced a regression on ipr EEH recovery.
> > 
> > The ipr device driver saves the PCI state only during the device probe
> > and restores it on ipr_reset_restore_cfg_space() during IOA resets.
> > This behavior is causing the EEH recovery to fail after the second
> > error detected, since the registers are not being restored.
> > 
> > One possible solution would be saving the registers after restoring
> > them. The problem with this approach is that while recovering from an
> > EEH error if pci_save_state() results in an EEH error, the
> > adapter/slot will be reset, and end up back in
> > ipr_reset_restore_cfg_space(), but it won't have a valid saved state
> > to restore, so pci_restore_state() will fail.
> > 
> > The following patch introduces a workaround for this problem, hacking
> > around the PCI API by setting pdev->state_saved = true before we do
> > the restore. It fixes the EEH regression and prevents that we hit
> > another EEH error during EEH recovery.
> > 
> > 
> > Thanks,
> > Kleber
> > 
> > 
> > 
> > Signed-off-by: Kleber Sacilotto de Souza <klebers@linux.vnet.ibm.com>
> > ---
> >  drivers/scsi/ipr.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> > index 76d294f..c3ff9a6 100644
> > --- a/drivers/scsi/ipr.c
> > +++ b/drivers/scsi/ipr.c
> > @@ -6516,6 +6516,7 @@ static int ipr_reset_restore_cfg_space(struct
> > ipr_cmnd *ipr_cmd)
> >         int rc;
> >  
> >         ENTER;
> > +       ioa_cfg->pdev->state_saved = true;
> >         rc = pci_restore_state(ioa_cfg->pdev);
> >  
> >         if (rc != PCIBIOS_SUCCESSFUL) {
> 
> Rafael may have input here, but it seems like we need a low level
> save/restore routine that ignores the flag (which is generally used for
> suspend/resume I think?).

There are some other users, but they are only a few.

> Maybe adding low level _pci_save_state/_pci_restore_state that don't
> check/set the flags would help?

That might work, but how do we know that the state we're going to restore
is actually valid at this particular point?

Perhaps we need a version using a separate storage area?

Rafael