| Message ID | 20191120012859.23300-7-oohall@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [Very,RFC,01/46] powerpc/eeh: Don't attempt to restore VF config space after reset | expand |
On 20/11/2019 12:28, Oliver O'Halloran wrote: > Move this out of the PHB's dma_dev_setup() callback and into the > ppc_md.pcibios_fixup_iov callback. This ensures that the VF PE's > pdev pointer is always valid for the whole time the device is > added the bus. Yeah it would be nice if dma setup did just dma stuff. > This isn't strictly required, but it's slightly a slightly more logical s/slightly a slightly/slightly (slightly)/ ? :) > place to do the fixup and it makes dma_dev_setup a bit simpler. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > --- > arch/powerpc/platforms/powernv/pci-ioda.c | 35 +++++++++++------------ > 1 file changed, 17 insertions(+), 18 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > index 45f974258766..c6ea7a504e04 100644 > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > @@ -2910,9 +2910,6 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) > struct pci_dn *pdn; > int mul, total_vfs; > > - if (!pdev->is_physfn || pci_dev_is_added(pdev)) > - return; > - > pdn = pci_get_pdn(pdev); > pdn->vfs_expanded = 0; > pdn->m64_single_mode = false; > @@ -2987,6 +2984,22 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) > res->end = res->start - 1; > } > } > + > +static void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev) > +{ > + if (WARN_ON(pci_dev_is_added(pdev))) > + return; > + > + if (pdev->is_virtfn) { > + /* Fix the VF PE's pdev pointer */ > + struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev); > + pe->pdev = pdev; > + > + WARN_ON(!(pe->flags & PNV_IODA_PE_VF)); return; > + } else if (pdev->is_physfn) { > + pnv_pci_ioda_fixup_iov_resources(pdev); and open code pnv_pci_ioda_fixup_iov_resources() right here? Either way Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> > + } > +} > #endif /* CONFIG_PCI_IOV */ > > static void pnv_ioda_setup_pe_res(struct pnv_ioda_pe *pe, > @@ -3641,20 +3654,6 @@ void pnv_pci_dma_dev_setup(struct pci_dev *pdev) > { > struct pci_controller *hose = pci_bus_to_host(pdev->bus); > struct pnv_phb *phb = hose->private_data; > -#ifdef CONFIG_PCI_IOV > - struct pnv_ioda_pe *pe; > - > - /* Fix the VF pdn PE number */ > - if (pdev->is_virtfn) { > - list_for_each_entry(pe, &phb->ioda.pe_list, list) { > - if (pe->rid == ((pdev->bus->number << 8) | > - (pdev->devfn & 0xff))) { > - pe->pdev = pdev; > - break; > - } > - } > - } > -#endif /* CONFIG_PCI_IOV */ > > pnv_pci_ioda_dma_dev_setup(phb, pdev); > } > @@ -3945,7 +3944,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, > ppc_md.pcibios_default_alignment = pnv_pci_default_alignment; > > #ifdef CONFIG_PCI_IOV > - ppc_md.pcibios_fixup_sriov = pnv_pci_ioda_fixup_iov_resources; > + ppc_md.pcibios_fixup_sriov = pnv_pci_ioda_fixup_iov; > ppc_md.pcibios_iov_resource_alignment = pnv_pci_iov_resource_alignment; > ppc_md.pcibios_sriov_enable = pnv_pcibios_sriov_enable; > ppc_md.pcibios_sriov_disable = pnv_pcibios_sriov_disable; >
On Wed, Nov 20, 2019 at 12:28:19PM +1100, Oliver O'Halloran wrote: > Move this out of the PHB's dma_dev_setup() callback and into the > ppc_md.pcibios_fixup_iov callback. This ensures that the VF PE's > pdev pointer is always valid for the whole time the device is > added the bus. > > This isn't strictly required, but it's slightly a slightly more logical > place to do the fixup and it makes dma_dev_setup a bit simpler. Ok, this removes the code I commented on earlier, so I take my comment there back. > + if (pdev->is_virtfn) { > + /* Fix the VF PE's pdev pointer */ > + struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev); > + pe->pdev = pdev; Maybe add an empty line after the variable declaration? > @@ -3641,20 +3654,6 @@ void pnv_pci_dma_dev_setup(struct pci_dev *pdev) > { > struct pci_controller *hose = pci_bus_to_host(pdev->bus); > struct pnv_phb *phb = hose->private_data; > > pnv_pci_ioda_dma_dev_setup(phb, pdev); > } Can you just merge pnv_pci_dma_dev_setup and pnv_pci_ioda_dma_dev_setup now?
On Thu, Nov 21, 2019 at 6:48 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Nov 20, 2019 at 12:28:19PM +1100, Oliver O'Halloran wrote: > > Move this out of the PHB's dma_dev_setup() callback and into the > > ppc_md.pcibios_fixup_iov callback. This ensures that the VF PE's > > pdev pointer is always valid for the whole time the device is > > added the bus. > > > > This isn't strictly required, but it's slightly a slightly more logical > > place to do the fixup and it makes dma_dev_setup a bit simpler. > > Ok, this removes the code I commented on earlier, so I take my > comment there back. It is a bit weird. I'll re-order the two patches so we're not shovelling around the fixup junk. > > + if (pdev->is_virtfn) { > > + /* Fix the VF PE's pdev pointer */ > > + struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev); > > + pe->pdev = pdev; > > Maybe add an empty line after the variable declaration? ok > > @@ -3641,20 +3654,6 @@ void pnv_pci_dma_dev_setup(struct pci_dev *pdev) > > { > > struct pci_controller *hose = pci_bus_to_host(pdev->bus); > > struct pnv_phb *phb = hose->private_data; > > > > pnv_pci_ioda_dma_dev_setup(phb, pdev); > > } > > Can you just merge pnv_pci_dma_dev_setup and pnv_pci_ioda_dma_dev_setup > now? Oh cool, looks like we can. I'll do that.
On Thu, Nov 21, 2019 at 3:34 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > > On 20/11/2019 12:28, Oliver O'Halloran wrote: > > Move this out of the PHB's dma_dev_setup() callback and into the > > ppc_md.pcibios_fixup_iov callback. This ensures that the VF PE's > > pdev pointer is always valid for the whole time the device is > > added the bus. > > Yeah it would be nice if dma setup did just dma stuff. > > > This isn't strictly required, but it's slightly a slightly more logical > > s/slightly a slightly/slightly (slightly)/ ? :) > > > > place to do the fixup and it makes dma_dev_setup a bit simpler. > > > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > > --- > > arch/powerpc/platforms/powernv/pci-ioda.c | 35 +++++++++++------------ > > 1 file changed, 17 insertions(+), 18 deletions(-) > > > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > > index 45f974258766..c6ea7a504e04 100644 > > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > > @@ -2910,9 +2910,6 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) > > struct pci_dn *pdn; > > int mul, total_vfs; > > > > - if (!pdev->is_physfn || pci_dev_is_added(pdev)) > > - return; > > - > > pdn = pci_get_pdn(pdev); > > pdn->vfs_expanded = 0; > > pdn->m64_single_mode = false; > > @@ -2987,6 +2984,22 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) > > res->end = res->start - 1; > > } > > } > > + > > +static void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev) > > +{ > > + if (WARN_ON(pci_dev_is_added(pdev))) > > + return; > > + > > + if (pdev->is_virtfn) { > > + /* Fix the VF PE's pdev pointer */ > > + struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev); > > + pe->pdev = pdev; > > + > > + WARN_ON(!(pe->flags & PNV_IODA_PE_VF)); > > > return; > > > + } else if (pdev->is_physfn) { > > > > > + pnv_pci_ioda_fixup_iov_resources(pdev); > > > and open code pnv_pci_ioda_fixup_iov_resources() right here? pnv_pci_ioda_fixup_iov_resources() is pretty hairy so I'd rather keep it as a separate function. I'd like to get rid of it entirely at some point, but that's a problem for another day.
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index 45f974258766..c6ea7a504e04 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -2910,9 +2910,6 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) struct pci_dn *pdn; int mul, total_vfs; - if (!pdev->is_physfn || pci_dev_is_added(pdev)) - return; - pdn = pci_get_pdn(pdev); pdn->vfs_expanded = 0; pdn->m64_single_mode = false; @@ -2987,6 +2984,22 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) res->end = res->start - 1; } } + +static void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev) +{ + if (WARN_ON(pci_dev_is_added(pdev))) + return; + + if (pdev->is_virtfn) { + /* Fix the VF PE's pdev pointer */ + struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev); + pe->pdev = pdev; + + WARN_ON(!(pe->flags & PNV_IODA_PE_VF)); + } else if (pdev->is_physfn) { + pnv_pci_ioda_fixup_iov_resources(pdev); + } +} #endif /* CONFIG_PCI_IOV */ static void pnv_ioda_setup_pe_res(struct pnv_ioda_pe *pe, @@ -3641,20 +3654,6 @@ void pnv_pci_dma_dev_setup(struct pci_dev *pdev) { struct pci_controller *hose = pci_bus_to_host(pdev->bus); struct pnv_phb *phb = hose->private_data; -#ifdef CONFIG_PCI_IOV - struct pnv_ioda_pe *pe; - - /* Fix the VF pdn PE number */ - if (pdev->is_virtfn) { - list_for_each_entry(pe, &phb->ioda.pe_list, list) { - if (pe->rid == ((pdev->bus->number << 8) | - (pdev->devfn & 0xff))) { - pe->pdev = pdev; - break; - } - } - } -#endif /* CONFIG_PCI_IOV */ pnv_pci_ioda_dma_dev_setup(phb, pdev); } @@ -3945,7 +3944,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, ppc_md.pcibios_default_alignment = pnv_pci_default_alignment; #ifdef CONFIG_PCI_IOV - ppc_md.pcibios_fixup_sriov = pnv_pci_ioda_fixup_iov_resources; + ppc_md.pcibios_fixup_sriov = pnv_pci_ioda_fixup_iov; ppc_md.pcibios_iov_resource_alignment = pnv_pci_iov_resource_alignment; ppc_md.pcibios_sriov_enable = pnv_pcibios_sriov_enable; ppc_md.pcibios_sriov_disable = pnv_pcibios_sriov_disable;
Move this out of the PHB's dma_dev_setup() callback and into the ppc_md.pcibios_fixup_iov callback. This ensures that the VF PE's pdev pointer is always valid for the whole time the device is added the bus. This isn't strictly required, but it's slightly a slightly more logical place to do the fixup and it makes dma_dev_setup a bit simpler. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- arch/powerpc/platforms/powernv/pci-ioda.c | 35 +++++++++++------------ 1 file changed, 17 insertions(+), 18 deletions(-)