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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 1 Dec 2009 22:40:48 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> 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?

Yeah, that would probably be best.  Let the caller allocate the space
and save/restore it all it wants for special cases like error handling.

-- 
Jesse Barnes, Intel Open Source Technology Center