| Message ID | 20171213153242.98015-5-bryantly@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, 2017-12-13 at 09:32 -0600, Bryant G. Ly wrote: > When pseries SR-IOV is enabled and after a PF driver > has resumed from EEH, platform has to be notified > of the event so the child VFs can be allowed to > resume their normal recovery path. > > This patch makes the EEH operation allow unfreeze > platform dependent code and adds the call to > pseries EEH code. > > Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> > Signed-off-by: Juan J. Alvarez <jjalvare@linux.vnet.ibm.com> Just some nitpicks, there's a lot of weird whitespace in this patch > --- > arch/powerpc/include/asm/eeh.h | 1 + > arch/powerpc/kernel/eeh_driver.c | 4 ++ > arch/powerpc/platforms/powernv/eeh-powernv.c | 3 +- > arch/powerpc/platforms/pseries/eeh_pseries.c | 100 > ++++++++++++++++++++++++++- > 4 files changed, 106 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/eeh.h > b/arch/powerpc/include/asm/eeh.h > index 5161c37dd039..12d52a0cd447 100644 > --- a/arch/powerpc/include/asm/eeh.h > +++ b/arch/powerpc/include/asm/eeh.h > @@ -214,6 +214,7 @@ struct eeh_ops { > int (*write_config)(struct pci_dn *pdn, int where, int size, > u32 val); > int (*next_error)(struct eeh_pe **pe); > int (*restore_config)(struct pci_dn *pdn); > + int (*notify_resume)(struct pci_dn *pdn); > }; > > extern int eeh_subsystem_flags; > diff --git a/arch/powerpc/kernel/eeh_driver.c > b/arch/powerpc/kernel/eeh_driver.c > index c61bf770282b..dbda0cda559b 100644 > --- a/arch/powerpc/kernel/eeh_driver.c > +++ b/arch/powerpc/kernel/eeh_driver.c > @@ -361,6 +361,7 @@ static void *eeh_report_resume(void *data, void > *userdata) > bool was_in_error; > struct pci_driver *driver; > char *envp[] = {"EVENT=EEH_RESUME", "ONLINE=1", NULL}; > + struct pci_dn *pdn = eeh_dev_to_pdn(edev); > > if (!dev || eeh_dev_removed(edev) || eeh_pe_passed(edev- > >pe)) > return NULL; > @@ -384,6 +385,9 @@ static void *eeh_report_resume(void *data, void > *userdata) > driver->err_handler->resume(dev); > eeh_pcid_put(dev); > kobject_uevent_env(&dev->dev.kobj, KOBJ_CHANGE, envp); > +#ifdef CONFIG_PCI_IOV > + eeh_ops->notify_resume(pdn); > +#endif > return NULL; > } > > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c > b/arch/powerpc/platforms/powernv/eeh-powernv.c > index 961e64115d92..8575b3a29e7c 100644 > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > @@ -1763,7 +1763,8 @@ static struct eeh_ops pnv_eeh_ops = { > .read_config = pnv_eeh_read_config, > .write_config = pnv_eeh_write_config, > .next_error = pnv_eeh_next_error, > - .restore_config = pnv_eeh_restore_config > + .restore_config = pnv_eeh_restore_config, > + .notify_resume = NULL > }; > > #ifdef CONFIG_PCI_IOV > diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c > b/arch/powerpc/platforms/pseries/eeh_pseries.c > index 5bdd1678a9ff..2b36fbf4ce74 100644 > --- a/arch/powerpc/platforms/pseries/eeh_pseries.c > +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c > @@ -798,6 +798,103 @@ static int pseries_eeh_restore_config(struct > pci_dn *pdn) > return 0; > } > > +#ifdef CONFIG_PCI_IOV > +int pseries_send_allow_unfreeze(struct eeh_pe *pe, > + u16 *vf_pe_array, int cur_vfs) > +{ > + int rc, config_addr; extra space > + int ibm_allow_unfreeze = rtas_token("ibm,open-sriov-allow- > unfreeze"); > + > + config_addr = pe->config_addr; > + spin_lock(&rtas_data_buf_lock); > + memcpy(rtas_data_buf, vf_pe_array, RTAS_DATA_BUF_SIZE); > + rc = rtas_call(ibm_allow_unfreeze, 5, 1, NULL, > + config_addr, > + BUID_HI(pe->phb->buid), > + BUID_LO(pe->phb->buid), > + rtas_data_buf, cur_vfs * sizeof(u16)); > + spin_unlock(&rtas_data_buf_lock); > + if (rc) > + pr_warn("%s: Failed to allow unfreeze for PHB#%x- > PE#%x, rc=%x\n", > + __func__, > + pe->phb->global_number, > + pe->config_addr, rc); > + return rc; > +} > + > +static int pseries_call_allow_unfreeze(struct eeh_dev *edev) > +{ > + struct eeh_pe *pe; > + struct pci_dn *pdn, *tmp, *parent, *physfn_pdn; > + int cur_vfs, rc, vf_index; > + u16 *vf_pe_array; > + > + vf_pe_array = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL); > + if (!vf_pe_array) > + return -ENOMEM; > + > + memset(vf_pe_array, 0, RTAS_DATA_BUF_SIZE); > + cur_vfs = 0; > + rc = 0; > + if (edev->pdev->is_physfn) { > + pe = eeh_dev_to_pe(edev); > + cur_vfs = pci_num_vf(edev->pdev); > + pdn = eeh_dev_to_pdn(edev); > + parent = pdn->parent; extra space > + /* For each of its VF > + * call allow unfreeze > + */ Weird line split here, and the comment doesn't read very well, and doesn't describe anything that isn't easily figured out by looking at the code. > + for (vf_index = 0; vf_index < cur_vfs; vf_index++) > + vf_pe_array[vf_index] = > + be16_to_cpu(pdn- > >pe_num_map[vf_index]); > + > + rc = pseries_send_allow_unfreeze(pe, vf_pe_array, > cur_vfs); > + pdn->last_allow_rc = rc; > + for (vf_index = 0; vf_index < cur_vfs; vf_index++) { > + list_for_each_entry_safe(pdn, tmp, &parent- > >child_list, > + list) { > + if (pdn->busno > + != pci_iov_virtfn_bus(edev- > >pdev, > + vf_index) > || > + pdn->devfn > + != pci_iov_virtfn_devfn(edev- > >pdev, > + vf_index > )) I know you're running out of room here but it looks really gross. Could this possibly be refactored? > + continue; > + pdn->last_allow_rc = rc; > + } > + } > + } else { > + pdn = pci_get_pdn(edev->pdev); > + vf_pe_array[0] = be16_to_cpu(pdn->pe_number); > + physfn_pdn = pci_get_pdn(edev->physfn); more extra space > + edev = pdn_to_eeh_dev(physfn_pdn); > + pe = eeh_dev_to_pe(edev); > + rc = pseries_send_allow_unfreeze(pe, vf_pe_array, > 1); > + pdn->last_allow_rc = rc; > + } > + > + kfree(vf_pe_array); > + return rc; > +} > + > +static int pseries_notify_resume(struct pci_dn *pdn) > +{ > + struct eeh_dev *edev = pdn_to_eeh_dev(pdn); > + > + if (!edev) > + return -EEXIST; > + > + if (rtas_token("ibm,open-sriov-allow-unfreeze") > + == RTAS_UNKNOWN_SERVICE) > + return -EINVAL; > + > + if (edev->pdev->is_physfn || edev->pdev->is_virtfn) extra space > + return pseries_call_allow_unfreeze(edev); > + > + return 0; > +} > +#endif > + > static struct eeh_ops pseries_eeh_ops = { > .name = "pseries", > .init = pseries_eeh_init, > @@ -813,7 +910,8 @@ 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 = pseries_eeh_restore_config, > + .notify_resume = pseries_notify_resume > }; > > /**
On 14/12/17 02:32, Bryant G. Ly wrote: > When pseries SR-IOV is enabled and after a PF driver > has resumed from EEH, platform has to be notified > of the event so the child VFs can be allowed to > resume their normal recovery path. > > This patch makes the EEH operation allow unfreeze > platform dependent code and adds the call to > pseries EEH code. > > Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> > Signed-off-by: Juan J. Alvarez <jjalvare@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/eeh.h | 1 + > arch/powerpc/kernel/eeh_driver.c | 4 ++ > arch/powerpc/platforms/powernv/eeh-powernv.c | 3 +- > arch/powerpc/platforms/pseries/eeh_pseries.c | 100 ++++++++++++++++++++++++++- > 4 files changed, 106 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > index 5161c37dd039..12d52a0cd447 100644 > --- a/arch/powerpc/include/asm/eeh.h > +++ b/arch/powerpc/include/asm/eeh.h > @@ -214,6 +214,7 @@ struct eeh_ops { > int (*write_config)(struct pci_dn *pdn, int where, int size, u32 val); > int (*next_error)(struct eeh_pe **pe); > int (*restore_config)(struct pci_dn *pdn); > + int (*notify_resume)(struct pci_dn *pdn); > }; > > extern int eeh_subsystem_flags; > diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c > index c61bf770282b..dbda0cda559b 100644 > --- a/arch/powerpc/kernel/eeh_driver.c > +++ b/arch/powerpc/kernel/eeh_driver.c > @@ -361,6 +361,7 @@ static void *eeh_report_resume(void *data, void *userdata) > bool was_in_error; > struct pci_driver *driver; > char *envp[] = {"EVENT=EEH_RESUME", "ONLINE=1", NULL}; > + struct pci_dn *pdn = eeh_dev_to_pdn(edev); > > if (!dev || eeh_dev_removed(edev) || eeh_pe_passed(edev->pe)) > return NULL; > @@ -384,6 +385,9 @@ static void *eeh_report_resume(void *data, void *userdata) > driver->err_handler->resume(dev); > eeh_pcid_put(dev); > kobject_uevent_env(&dev->dev.kobj, KOBJ_CHANGE, envp); > +#ifdef CONFIG_PCI_IOV > + eeh_ops->notify_resume(pdn); eeh_ops->notify_resume(eeh_dev_to_pdn(edev)); otherwise the compiler will complain at @pdn declaration if !defined(CONFIG_PCI_IOV). Just try compiling without CONFIG_PCI_IOV. > +#endif > return NULL; > } > > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c > index 961e64115d92..8575b3a29e7c 100644 > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > @@ -1763,7 +1763,8 @@ static struct eeh_ops pnv_eeh_ops = { > .read_config = pnv_eeh_read_config, > .write_config = pnv_eeh_write_config, > .next_error = pnv_eeh_next_error, > - .restore_config = pnv_eeh_restore_config > + .restore_config = pnv_eeh_restore_config, > + .notify_resume = NULL .notify_resume is NULL already. > }; > > #ifdef CONFIG_PCI_IOV > diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c > index 5bdd1678a9ff..2b36fbf4ce74 100644 > --- a/arch/powerpc/platforms/pseries/eeh_pseries.c > +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c > @@ -798,6 +798,103 @@ static int pseries_eeh_restore_config(struct pci_dn *pdn) > return 0; > } > > +#ifdef CONFIG_PCI_IOV > +int pseries_send_allow_unfreeze(struct eeh_pe *pe, > + u16 *vf_pe_array, int cur_vfs) > +{ > + int rc, config_addr; > + int ibm_allow_unfreeze = rtas_token("ibm,open-sriov-allow-unfreeze"); > + > + config_addr = pe->config_addr; @config_addr is pointless - it is used just once, even pr_warn few lines below uses pe->config_addr. > + spin_lock(&rtas_data_buf_lock); > + memcpy(rtas_data_buf, vf_pe_array, RTAS_DATA_BUF_SIZE); > + rc = rtas_call(ibm_allow_unfreeze, 5, 1, NULL, > + config_addr, > + BUID_HI(pe->phb->buid), > + BUID_LO(pe->phb->buid), > + rtas_data_buf, cur_vfs * sizeof(u16)); > + spin_unlock(&rtas_data_buf_lock); > + if (rc) > + pr_warn("%s: Failed to allow unfreeze for PHB#%x-PE#%x, rc=%x\n", > + __func__, > + pe->phb->global_number, > + pe->config_addr, rc); > + return rc; > +} > + > +static int pseries_call_allow_unfreeze(struct eeh_dev *edev) > +{ > + struct eeh_pe *pe; > + struct pci_dn *pdn, *tmp, *parent, *physfn_pdn; > + int cur_vfs, rc, vf_index; > + u16 *vf_pe_array; > + > + vf_pe_array = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL); > + if (!vf_pe_array) > + return -ENOMEM; > + > + memset(vf_pe_array, 0, RTAS_DATA_BUF_SIZE); "z" from kzalloc says it reset the memory. > + cur_vfs = 0; > + rc = 0; > + if (edev->pdev->is_physfn) { > + pe = eeh_dev_to_pe(edev); > + cur_vfs = pci_num_vf(edev->pdev); > + pdn = eeh_dev_to_pdn(edev); > + parent = pdn->parent; > + /* For each of its VF > + * call allow unfreeze > + */ > + for (vf_index = 0; vf_index < cur_vfs; vf_index++) > + vf_pe_array[vf_index] = > + be16_to_cpu(pdn->pe_num_map[vf_index]); It is kind of assumed that the number of VFs is always less than 2048 minus rtas call parameters (RTAS_DATA_BUF_SIZE==4096 now)? Is the upper limit of cur_vfs checked anywhere? We can afford 4K on stack if we know for sure this is all we n > + > + rc = pseries_send_allow_unfreeze(pe, vf_pe_array, cur_vfs); > + pdn->last_allow_rc = rc; Is this PF or VF pdn (I assume PF)? > + for (vf_index = 0; vf_index < cur_vfs; vf_index++) { > + list_for_each_entry_safe(pdn, tmp, &parent->child_list, > + list) { > + if (pdn->busno > + != pci_iov_virtfn_bus(edev->pdev, > + vf_index) || > + pdn->devfn > + != pci_iov_virtfn_devfn(edev->pdev, > + vf_index)) > + continue; > + pdn->last_allow_rc = rc; I am missing the point of copying last_allow_rc - cannot eeh_notify_resume_show() just return it from the PF? May be just add another flag to eeh_pe::state for last_allow_rc? How many different @rc do we expect here? > + } > + } > + } else { > + pdn = pci_get_pdn(edev->pdev); > + vf_pe_array[0] = be16_to_cpu(pdn->pe_number); > + physfn_pdn = pci_get_pdn(edev->physfn); Way too many spaces, why? :) > + edev = pdn_to_eeh_dev(physfn_pdn); > + pe = eeh_dev_to_pe(edev); > + rc = pseries_send_allow_unfreeze(pe, vf_pe_array, 1); > + pdn->last_allow_rc = rc; > + } > + > + kfree(vf_pe_array); > + return rc; > +} > + > +static int pseries_notify_resume(struct pci_dn *pdn) > +{ > + struct eeh_dev *edev = pdn_to_eeh_dev(pdn); > + > + if (!edev) > + return -EEXIST; > + > + if (rtas_token("ibm,open-sriov-allow-unfreeze") > + == RTAS_UNKNOWN_SERVICE) > + return -EINVAL; Is this the only possible error code? > + > + if (edev->pdev->is_physfn || edev->pdev->is_virtfn) > + return pseries_call_allow_unfreeze(edev); > + > + return 0; > +} > +#endif > + > static struct eeh_ops pseries_eeh_ops = { > .name = "pseries", > .init = pseries_eeh_init, > @@ -813,7 +910,8 @@ 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 = pseries_eeh_restore_config, > + .notify_resume = pseries_notify_resume > }; > > /** >
Yes, way less. So our current design only supports less than or equal to 256 VFs per PF. That is 256*2 bytes. On 12/17/17 11:02 PM, Alexey Kardashevskiy wrote: > It is kind of assumed that the number of VFs is always less than 2048 minus > rtas call parameters (RTAS_DATA_BUF_SIZE==4096 now)?
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h index 5161c37dd039..12d52a0cd447 100644 --- a/arch/powerpc/include/asm/eeh.h +++ b/arch/powerpc/include/asm/eeh.h @@ -214,6 +214,7 @@ struct eeh_ops { int (*write_config)(struct pci_dn *pdn, int where, int size, u32 val); int (*next_error)(struct eeh_pe **pe); int (*restore_config)(struct pci_dn *pdn); + int (*notify_resume)(struct pci_dn *pdn); }; extern int eeh_subsystem_flags; diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c index c61bf770282b..dbda0cda559b 100644 --- a/arch/powerpc/kernel/eeh_driver.c +++ b/arch/powerpc/kernel/eeh_driver.c @@ -361,6 +361,7 @@ static void *eeh_report_resume(void *data, void *userdata) bool was_in_error; struct pci_driver *driver; char *envp[] = {"EVENT=EEH_RESUME", "ONLINE=1", NULL}; + struct pci_dn *pdn = eeh_dev_to_pdn(edev); if (!dev || eeh_dev_removed(edev) || eeh_pe_passed(edev->pe)) return NULL; @@ -384,6 +385,9 @@ static void *eeh_report_resume(void *data, void *userdata) driver->err_handler->resume(dev); eeh_pcid_put(dev); kobject_uevent_env(&dev->dev.kobj, KOBJ_CHANGE, envp); +#ifdef CONFIG_PCI_IOV + eeh_ops->notify_resume(pdn); +#endif return NULL; } diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c index 961e64115d92..8575b3a29e7c 100644 --- a/arch/powerpc/platforms/powernv/eeh-powernv.c +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c @@ -1763,7 +1763,8 @@ static struct eeh_ops pnv_eeh_ops = { .read_config = pnv_eeh_read_config, .write_config = pnv_eeh_write_config, .next_error = pnv_eeh_next_error, - .restore_config = pnv_eeh_restore_config + .restore_config = pnv_eeh_restore_config, + .notify_resume = NULL }; #ifdef CONFIG_PCI_IOV diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c index 5bdd1678a9ff..2b36fbf4ce74 100644 --- a/arch/powerpc/platforms/pseries/eeh_pseries.c +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c @@ -798,6 +798,103 @@ static int pseries_eeh_restore_config(struct pci_dn *pdn) return 0; } +#ifdef CONFIG_PCI_IOV +int pseries_send_allow_unfreeze(struct eeh_pe *pe, + u16 *vf_pe_array, int cur_vfs) +{ + int rc, config_addr; + int ibm_allow_unfreeze = rtas_token("ibm,open-sriov-allow-unfreeze"); + + config_addr = pe->config_addr; + spin_lock(&rtas_data_buf_lock); + memcpy(rtas_data_buf, vf_pe_array, RTAS_DATA_BUF_SIZE); + rc = rtas_call(ibm_allow_unfreeze, 5, 1, NULL, + config_addr, + BUID_HI(pe->phb->buid), + BUID_LO(pe->phb->buid), + rtas_data_buf, cur_vfs * sizeof(u16)); + spin_unlock(&rtas_data_buf_lock); + if (rc) + pr_warn("%s: Failed to allow unfreeze for PHB#%x-PE#%x, rc=%x\n", + __func__, + pe->phb->global_number, + pe->config_addr, rc); + return rc; +} + +static int pseries_call_allow_unfreeze(struct eeh_dev *edev) +{ + struct eeh_pe *pe; + struct pci_dn *pdn, *tmp, *parent, *physfn_pdn; + int cur_vfs, rc, vf_index; + u16 *vf_pe_array; + + vf_pe_array = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL); + if (!vf_pe_array) + return -ENOMEM; + + memset(vf_pe_array, 0, RTAS_DATA_BUF_SIZE); + cur_vfs = 0; + rc = 0; + if (edev->pdev->is_physfn) { + pe = eeh_dev_to_pe(edev); + cur_vfs = pci_num_vf(edev->pdev); + pdn = eeh_dev_to_pdn(edev); + parent = pdn->parent; + /* For each of its VF + * call allow unfreeze + */ + for (vf_index = 0; vf_index < cur_vfs; vf_index++) + vf_pe_array[vf_index] = + be16_to_cpu(pdn->pe_num_map[vf_index]); + + rc = pseries_send_allow_unfreeze(pe, vf_pe_array, cur_vfs); + pdn->last_allow_rc = rc; + for (vf_index = 0; vf_index < cur_vfs; vf_index++) { + list_for_each_entry_safe(pdn, tmp, &parent->child_list, + list) { + if (pdn->busno + != pci_iov_virtfn_bus(edev->pdev, + vf_index) || + pdn->devfn + != pci_iov_virtfn_devfn(edev->pdev, + vf_index)) + continue; + pdn->last_allow_rc = rc; + } + } + } else { + pdn = pci_get_pdn(edev->pdev); + vf_pe_array[0] = be16_to_cpu(pdn->pe_number); + physfn_pdn = pci_get_pdn(edev->physfn); + edev = pdn_to_eeh_dev(physfn_pdn); + pe = eeh_dev_to_pe(edev); + rc = pseries_send_allow_unfreeze(pe, vf_pe_array, 1); + pdn->last_allow_rc = rc; + } + + kfree(vf_pe_array); + return rc; +} + +static int pseries_notify_resume(struct pci_dn *pdn) +{ + struct eeh_dev *edev = pdn_to_eeh_dev(pdn); + + if (!edev) + return -EEXIST; + + if (rtas_token("ibm,open-sriov-allow-unfreeze") + == RTAS_UNKNOWN_SERVICE) + return -EINVAL; + + if (edev->pdev->is_physfn || edev->pdev->is_virtfn) + return pseries_call_allow_unfreeze(edev); + + return 0; +} +#endif + static struct eeh_ops pseries_eeh_ops = { .name = "pseries", .init = pseries_eeh_init, @@ -813,7 +910,8 @@ 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 = pseries_eeh_restore_config, + .notify_resume = pseries_notify_resume }; /**