Message ID | 1459988405-13991-2-git-send-email-gpiccoli@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
On Wed, Apr 06, 2016 at 09:20:05PM -0300, Guilherme G. Piccoli wrote: >Commit 39baadbf36ce ("powerpc/eeh: Remove eeh information from pci_dn") >changed the pci_dn struct by removing its EEH-related members. >As part of this clean-up, DDW mechanism was modified to read the device >configuration address from eeh_dev struct. > >As a consequence, now if we disable EEH mechanism on kernel command-line >for example, the DDW mechanism will fail, generating a kernel oops by >dereferencing a NULL pointer (which turns to be the eeh_dev pointer). > >This patch just changes the configuration address calculation on DDW >functions to a manual calculation based on pci_dn members instead of >using eeh_dev-based address. > >No functional changes were made. This was tested on pSeries, both >in PHyp and qemu guest. > >Fixes: 39baadbf36ce ("powerpc/eeh: Remove eeh information from pci_dn") >Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com> Thanks, Guilherme. Please make sure if it needs to be backported to stable kernel. I assume it probably needs to be in 3.10+. >--- > arch/powerpc/platforms/pseries/iommu.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > >diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c >index bd98ce2..3db2cea 100644 >--- a/arch/powerpc/platforms/pseries/iommu.c >+++ b/arch/powerpc/platforms/pseries/iommu.c >@@ -912,7 +912,8 @@ machine_arch_initcall(pseries, find_existing_ddw_windows); > static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail, > struct ddw_query_response *query) > { >- struct eeh_dev *edev; >+ struct device_node *dn; >+ struct pci_dn *pdn; > u32 cfg_addr; > u64 buid; > int ret; >@@ -923,11 +924,10 @@ static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail, > * Retrieve them from the pci device, not the node with the > * dma-window property > */ >- edev = pci_dev_to_eeh_dev(dev); >- cfg_addr = edev->config_addr; >- if (edev->pe_config_addr) >- cfg_addr = edev->pe_config_addr; >- buid = edev->phb->buid; >+ dn = pci_device_to_OF_node(dev); >+ pdn = PCI_DN(dn); >+ cfg_addr = (pdn->busno << 8) | (pdn->devfn); >+ buid = pdn->phb->buid; No parentheses required on the second operand when getting @cfg_addr. It would be nicer to initialize @buid and then @cfg_addr. When developers look at the code, they usually check @buid and then @cfg_addr. > > ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query, > cfg_addr, BUID_HI(buid), BUID_LO(buid)); >@@ -941,7 +941,8 @@ static int create_ddw(struct pci_dev *dev, const u32 *ddw_avail, > struct ddw_create_response *create, int page_shift, > int window_shift) > { >- struct eeh_dev *edev; >+ struct device_node *dn; >+ struct pci_dn *pdn; > u32 cfg_addr; > u64 buid; > int ret; >@@ -952,11 +953,10 @@ static int create_ddw(struct pci_dev *dev, const u32 *ddw_avail, > * Retrieve them from the pci device, not the node with the > * dma-window property > */ >- edev = pci_dev_to_eeh_dev(dev); >- cfg_addr = edev->config_addr; >- if (edev->pe_config_addr) >- cfg_addr = edev->pe_config_addr; >- buid = edev->phb->buid; >+ dn = pci_device_to_OF_node(dev); >+ pdn = PCI_DN(dn); >+ cfg_addr = (pdn->busno << 8) | (pdn->devfn); >+ buid = pdn->phb->buid; Same comment as above. Thanks, Gavin > > do { > /* extra outputs are LIOBN and dma-addr (hi, lo) */ >-- >2.1.0 >
On 04/06/2016 09:48 PM, Gavin Shan wrote: > On Wed, Apr 06, 2016 at 09:20:05PM -0300, Guilherme G. Piccoli wrote: >> Fixes: 39baadbf36ce ("powerpc/eeh: Remove eeh information from pci_dn") >> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > > Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > > Thanks, Guilherme. Please make sure if it needs to be backported > to stable kernel. I assume it probably needs to be in 3.10+. Thanks for your quick review Gavin! I guess we only need to backport this to kernels containing 39baadbf36ce. Stable is a good idea. >> + dn = pci_device_to_OF_node(dev); >> + pdn = PCI_DN(dn); >> + cfg_addr = (pdn->busno << 8) | (pdn->devfn); >> + buid = pdn->phb->buid; > > No parentheses required on the second operand when getting @cfg_addr. > It would be nicer to initialize @buid and then @cfg_addr. When > developers look at the code, they usually check @buid and then > @cfg_addr. OK, I'll change this in v3. buid first, cfg_addr second. Also I'll remove the parentheses - it's a bad habit of mine =) Cheers, Guilherme
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c index bd98ce2..3db2cea 100644 --- a/arch/powerpc/platforms/pseries/iommu.c +++ b/arch/powerpc/platforms/pseries/iommu.c @@ -912,7 +912,8 @@ machine_arch_initcall(pseries, find_existing_ddw_windows); static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail, struct ddw_query_response *query) { - struct eeh_dev *edev; + struct device_node *dn; + struct pci_dn *pdn; u32 cfg_addr; u64 buid; int ret; @@ -923,11 +924,10 @@ static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail, * Retrieve them from the pci device, not the node with the * dma-window property */ - edev = pci_dev_to_eeh_dev(dev); - cfg_addr = edev->config_addr; - if (edev->pe_config_addr) - cfg_addr = edev->pe_config_addr; - buid = edev->phb->buid; + dn = pci_device_to_OF_node(dev); + pdn = PCI_DN(dn); + cfg_addr = (pdn->busno << 8) | (pdn->devfn); + buid = pdn->phb->buid; ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query, cfg_addr, BUID_HI(buid), BUID_LO(buid)); @@ -941,7 +941,8 @@ static int create_ddw(struct pci_dev *dev, const u32 *ddw_avail, struct ddw_create_response *create, int page_shift, int window_shift) { - struct eeh_dev *edev; + struct device_node *dn; + struct pci_dn *pdn; u32 cfg_addr; u64 buid; int ret; @@ -952,11 +953,10 @@ static int create_ddw(struct pci_dev *dev, const u32 *ddw_avail, * Retrieve them from the pci device, not the node with the * dma-window property */ - edev = pci_dev_to_eeh_dev(dev); - cfg_addr = edev->config_addr; - if (edev->pe_config_addr) - cfg_addr = edev->pe_config_addr; - buid = edev->phb->buid; + dn = pci_device_to_OF_node(dev); + pdn = PCI_DN(dn); + cfg_addr = (pdn->busno << 8) | (pdn->devfn); + buid = pdn->phb->buid; do { /* extra outputs are LIOBN and dma-addr (hi, lo) */
Commit 39baadbf36ce ("powerpc/eeh: Remove eeh information from pci_dn") changed the pci_dn struct by removing its EEH-related members. As part of this clean-up, DDW mechanism was modified to read the device configuration address from eeh_dev struct. As a consequence, now if we disable EEH mechanism on kernel command-line for example, the DDW mechanism will fail, generating a kernel oops by dereferencing a NULL pointer (which turns to be the eeh_dev pointer). This patch just changes the configuration address calculation on DDW functions to a manual calculation based on pci_dn members instead of using eeh_dev-based address. No functional changes were made. This was tested on pSeries, both in PHyp and qemu guest. Fixes: 39baadbf36ce ("powerpc/eeh: Remove eeh information from pci_dn") Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> --- arch/powerpc/platforms/pseries/iommu.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)