Re: [PATCH 06/14] powerpc/eeh: Remove VF config space restoration

On 13/07/2020 20:55, Oliver O'Halloran wrote:

On Mon, Jul 13, 2020 at 8:32 PM Alexey Kardashevskiy [off-list ref] wrote:

quoted

quoted

 #ifdef CONFIG_PCI_IOV
 int pseries_send_allow_unfreeze(struct pci_dn *pdn,
                              u16 *vf_pe_array, int cur_vfs)

@@ -848,7 +824,7 @@ static struct eeh_ops pseries_eeh_ops = {
      .read_config            = pseries_eeh_read_config,
      .write_config           = pseries_eeh_write_config,
      .next_error             = NULL,
-     .restore_config         = pseries_eeh_restore_config,
+     .restore_config         = NULL, /* NB: configure_bridge() does this */

configure_bridge() calls rtas_call(ibm_configure_pe, 3, 1, NULL...)
which reconfigures the PE and which is quite different from what
pseries_eeh_restore_config() used to do although the comment suggests it
is about the same thing. I am pretty sure the new code produces a better
result so I suggest ditching this comment and adding a note to the
commit log may be. Thanks,

I put the comment there largely because the EEH core seems to think
that restore_config() is what should be called to reset the device's
config space to the defaults set be firmware. On PowerNV it does
actually do that and configure_bridge is this:

static int pnv_eeh_configure_bridge(struct eeh_pe *pe)
{
        return 0;
}

So... there's definitely something strange going on there. I don't
remember the exact details, but I think the generic EEH code calls
into RTAS to collect debug data and apparently that requires the
device to be accessible via MMIO (i.e BARs need to be restored) which
is why the pseries .configure_bridge() calls configure_pe. 

ah ok, makes more now, cool. thanks,

It might
work out better, but having something called "restore_config" that
doesn't actually restore the config is uh... modern. It's something
that probably needs a rework at some point. Anyway, I think the
comment is more helpful than it is misleading. Especially if you
consider the PowerNV behaviour.

quoted

quoted

 #ifdef CONFIG_PCI_IOV
      .notify_resume          = pseries_notify_resume
 #endif

--
Alexey

-- 
Alexey