On 06/07/2020 11:36, Oliver O'Halloran wrote:
> There's a bunch of strange things about this code. First up is that none of
> the fields being written to are functional for a VF. The SR-IOV
> specification lists then as "Reserved, but OS should preserve" so writing
> new values to them doesn't do anything and is clearly wrong from a
> correctness perspective.
>
> However, since VFs are designed to be managed by the OS there is an
> argument to be made that we should be saving and restoring some parts of
> config space. We already sort of do that by saving the first 64 bytes of
> config space in the eeh_dev (see eeh_dev->config_space[]). This is
> inadequate since it doesn't even consider saving and restoring the PCI
> capability structures. However, this is a problem with EEH in general and
> that needs to be fixed for non-VF devices too.
>
> There's no real reason to keep around this around so delete it.
>
> Signed-off-by: Oliver O'Halloran <ooh...@gmail.com>
Reviewed-by: Alexey Kardashevskiy <a...@ozlabs.ru>
> ---
> arch/powerpc/include/asm/eeh.h | 1 -
> arch/powerpc/kernel/eeh.c | 59 --------------------
> arch/powerpc/platforms/powernv/eeh-powernv.c | 20 ++-----
> arch/powerpc/platforms/pseries/eeh_pseries.c | 26 +--------
> 4 files changed, 7 insertions(+), 99 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 1bddc0dfe099..046c5a2fe411 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -314,7 +314,6 @@ int eeh_pe_reset(struct eeh_pe *pe, int option, bool
> include_passed);
> int eeh_pe_configure(struct eeh_pe *pe);
> int eeh_pe_inject_err(struct eeh_pe *pe, int type, int func,
> unsigned long addr, unsigned long mask);
> -int eeh_restore_vf_config(struct pci_dn *pdn);
>
> /**
> * EEH_POSSIBLE_ERROR() -- test for possible MMIO failure.
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 859f76020256..a4df6f6de0bd 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -742,65 +742,6 @@ static void eeh_restore_dev_state(struct eeh_dev *edev,
> void *userdata)
> pci_restore_state(pdev);
> }
>
> -int eeh_restore_vf_config(struct pci_dn *pdn)
> -{
> - struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> - u32 devctl, cmd, cap2, aer_capctl;
> - int old_mps;
> -
> - if (edev->pcie_cap) {
> - /* Restore MPS */
> - old_mps = (ffs(pdn->mps) - 8) << 5;
> - eeh_ops->read_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL,
> - 2, &devctl);
> - devctl &= ~PCI_EXP_DEVCTL_PAYLOAD;
> - devctl |= old_mps;
> - eeh_ops->write_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL,
> - 2, devctl);
> -
> - /* Disable Completion Timeout if possible */
> - eeh_ops->read_config(pdn, edev->pcie_cap + PCI_EXP_DEVCAP2,
> - 4, &cap2);
> - if (cap2 & PCI_EXP_DEVCAP2_COMP_TMOUT_DIS) {
> - eeh_ops->read_config(pdn,
> - edev->pcie_cap + PCI_EXP_DEVCTL2,
> - 4, &cap2);
> - cap2 |= PCI_EXP_DEVCTL2_COMP_TMOUT_DIS;
> - eeh_ops->write_config(pdn,
> - edev->pcie_cap + PCI_EXP_DEVCTL2,
> - 4, cap2);
> - }
> - }
> -
> - /* Enable SERR and parity checking */
> - eeh_ops->read_config(pdn, PCI_COMMAND, 2, &cmd);
> - cmd |= (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
> - eeh_ops->write_config(pdn, PCI_COMMAND, 2, cmd);
> -
> - /* Enable report various errors */
> - if (edev->pcie_cap) {
> - eeh_ops->read_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL,
> - 2, &devctl);
> - devctl &= ~PCI_EXP_DEVCTL_CERE;
> - devctl |= (PCI_EXP_DEVCTL_NFERE |
> - PCI_EXP_DEVCTL_FERE |
> - PCI_EXP_DEVCTL_URRE);
> - eeh_ops->write_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL,
> - 2, devctl);
> - }
> -
> - /* Enable ECRC generation and check */
> - if (edev->pcie_cap && edev->aer_cap) {
> - eeh_ops->read_config(pdn, edev->aer_cap + PCI_ERR_CAP,
> - 4, &aer_capctl);
> - aer_capctl |= (PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE);
> - eeh_ops->write_config(pdn, edev->aer_cap + PCI_ERR_CAP,
> - 4, aer_capctl);
> - }
> -
> - return 0;
> -}
> -
> /**
> * pcibios_set_pcie_reset_state - Set PCI-E reset state
> * @dev: pci device struct
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c
> b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index bcd0515d8f79..8f3a7611efc1 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -1629,20 +1629,12 @@ static int pnv_eeh_restore_config(struct pci_dn *pdn)
> if (!edev)
> return -EEXIST;
>
> - /*
> - * We have to restore the PCI config space after reset since the
> - * firmware can't see SRIOV VFs.
> - *
> - * FIXME: The MPS, error routing rules, timeout setting are worthy
> - * to be exported by firmware in extendible way.
> - */
> - if (edev->physfn) {
> - ret = eeh_restore_vf_config(pdn);
> - } else {
> - phb = pdn->phb->private_data;
> - ret = opal_pci_reinit(phb->opal_id,
> - OPAL_REINIT_PCI_DEV, config_addr);
> - }
> + if (edev->physfn)
> + return 0;
> +
> + phb = edev->controller->private_data;
> + ret = opal_pci_reinit(phb->opal_id,
> + OPAL_REINIT_PCI_DEV, edev->bdfn);
>
> if (ret) {
> pr_warn("%s: Can't reinit PCI dev 0x%x (%lld)\n",
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c
> b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 088771fa38be..83122bf65a8c 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -718,30 +718,6 @@ static int pseries_eeh_write_config(struct pci_dn *pdn,
> int where, int size, u32
> return rtas_write_config(pdn, where, size, val);
> }
>
> -static int pseries_eeh_restore_config(struct pci_dn *pdn)
> -{
> - struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> - s64 ret = 0;
> -
> - if (!edev)
> - return -EEXIST;
> -
> - /*
> - * FIXME: The MPS, error routing rules, timeout setting are worthy
> - * to be exported by firmware in extendible way.
> - */
> - if (edev->physfn)
> - ret = eeh_restore_vf_config(pdn);
> -
> - if (ret) {
> - pr_warn("%s: Can't reinit PCI dev 0x%x (%lld)\n",
> - __func__, edev->pe_config_addr, ret);
> - return -EIO;
> - }
> -
> - return ret;
> -}
> -
> #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,
> #ifdef CONFIG_PCI_IOV
> .notify_resume = pseries_notify_resume
> #endif
>
--
Alexey
