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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 13/07/2020 20:55, Oliver O'Halloran wrote:
> On Mon, Jul 13, 2020 at 8:32 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>>  #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.
> 
>>>  #ifdef CONFIG_PCI_IOV
>>>       .notify_resume          = pseries_notify_resume
>>>  #endif
>>>
>>
>> --
>> Alexey

-- 
Alexey