| Message ID | 20181114082314.8965-13-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/34] powerpc: use mm zones more sensibly | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 44 lines checked |
Christoph Hellwig <hch@lst.de> writes: > Configure the dma settings at device setup time, and stop playing games > with get_pci_dma_ops. This prepares for using the common dma_configure > code later on. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > arch/powerpc/platforms/cell/iommu.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) This one's crashing, haven't dug into why yet: [ 1.347085] Unable to handle kernel paging request for data at address 0x00000040 [ 1.391505] Faulting instruction address: 0xc0000000006b6e6c cpu 0x0: Vector: 380 (Data SLB Access) at [c0000007fc9032d0] pc: c0000000006b6e6c: .of_n_addr_cells+0x34/0xc0 lr: c000000000070b30: .cell_iommu_get_fixed_address+0x58/0x2b0 sp: c0000007fc903560 msr: 9000000000009032 dar: 40 current = 0xc0000007fc8d0000 paca = 0xc000000000f60000 irqmask: 0x03 irq_happened: 0x01 pid = 1, comm = swapper/0 Linux version 4.20.0-rc2-gcc7x-g1e32f48 (kerkins@p82) (gcc version 7.4.1 20181208 (Custom eb377405ab2d1900)) #1 SMP Sun Dec 9 12:16:48 AEDT 2018 enter ? for help [c0000007fc9035f0] c000000000070b30 .cell_iommu_get_fixed_address+0x58/0x2b0 [c0000007fc9036c0] c0000000000711ac .cell_dma_dev_setup.part.1+0x24/0x118 [c0000007fc903740] c000000000071374 .cell_of_bus_notify+0x6c/0xbc [c0000007fc9037c0] c0000000000e7ef0 .notifier_call_chain+0x90/0xf8 [c0000007fc903860] c0000000000e8c2c .blocking_notifier_call_chain+0x84/0xb8 [c0000007fc9038f0] c000000000597544 .device_add+0x584/0x7b8 [c0000007fc9039c0] c0000000005a0308 .platform_device_add+0x148/0x2f0 [c0000007fc903a60] c0000000005a1508 .platform_device_register_full+0x148/0x168 [c0000007fc903ae0] c000000000a9a8a0 .__machine_initcall_cell_cell_publish_devices+0x1bc/0x210 [c0000007fc903be0] c00000000000eca4 .do_one_initcall+0x64/0x2d8 [c0000007fc903cc0] c000000000a844ec .kernel_init_freeable+0x3dc/0x4e4 [c0000007fc903da0] c00000000000f06c .kernel_init+0x24/0x150 [c0000007fc903e20] c00000000000a9c0 .ret_from_kernel_thread+0x58/0x78 cheers > diff --git a/arch/powerpc/platforms/cell/iommu.c b/arch/powerpc/platforms/cell/iommu.c > index 12352a58072a..cce5bf9515e5 100644 > --- a/arch/powerpc/platforms/cell/iommu.c > +++ b/arch/powerpc/platforms/cell/iommu.c > @@ -657,14 +657,21 @@ static const struct dma_map_ops dma_iommu_fixed_ops = { > .mapping_error = dma_iommu_mapping_error, > }; > > +static u64 cell_iommu_get_fixed_address(struct device *dev); > + > static void cell_dma_dev_setup(struct device *dev) > { > - if (get_pci_dma_ops() == &dma_iommu_ops) > + if (get_pci_dma_ops() == &dma_iommu_ops) { > + u64 addr = cell_iommu_get_fixed_address(dev); > + > + if (addr != OF_BAD_ADDR) > + set_dma_offset(dev, addr + dma_iommu_fixed_base); > set_iommu_table_base(dev, cell_get_iommu_table(dev)); > - else if (get_pci_dma_ops() == &dma_nommu_ops) > + } else if (get_pci_dma_ops() == &dma_nommu_ops) { > set_dma_offset(dev, cell_dma_nommu_offset); > - else > + } else { > BUG(); > + } > } > > static void cell_pci_dma_dev_setup(struct pci_dev *dev) > @@ -950,19 +957,14 @@ static int dma_suported_and_switch(struct device *dev, u64 dma_mask) > { > if (dma_mask == DMA_BIT_MASK(64) && > cell_iommu_get_fixed_address(dev) != OF_BAD_ADDR) { > - u64 addr = cell_iommu_get_fixed_address(dev) + > - dma_iommu_fixed_base; > dev_dbg(dev, "iommu: 64-bit OK, using fixed ops\n"); > - dev_dbg(dev, "iommu: fixed addr = %llx\n", addr); > set_dma_ops(dev, &dma_iommu_fixed_ops); > - set_dma_offset(dev, addr); > return 1; > } > > if (dma_iommu_dma_supported(dev, dma_mask)) { > dev_dbg(dev, "iommu: not 64-bit, using default ops\n"); > - set_dma_ops(dev, get_pci_dma_ops()); > - cell_dma_dev_setup(dev); > + set_dma_ops(dev, &dma_iommu_ops); > return 1; > } > > -- > 2.19.1
On Sun, Dec 09, 2018 at 09:23:39PM +1100, Michael Ellerman wrote: > Christoph Hellwig <hch@lst.de> writes: > > > Configure the dma settings at device setup time, and stop playing games > > with get_pci_dma_ops. This prepares for using the common dma_configure > > code later on. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > arch/powerpc/platforms/cell/iommu.c | 20 +++++++++++--------- > > 1 file changed, 11 insertions(+), 9 deletions(-) > > This one's crashing, haven't dug into why yet: Can you provide a gdb assembly of the exact crash site? This looks like for some odd reason the DT structures aren't fully setup by the time we are probing the device, which seems odd. Either way, something like the patch below would ensure we call cell_iommu_get_fixed_address from a similar context as before, can you check if that fixes the issue? diff --git a/arch/powerpc/platforms/cell/iommu.c b/arch/powerpc/platforms/cell/iommu.c index 93c7e4aef571..4891b338bf9f 100644 --- a/arch/powerpc/platforms/cell/iommu.c +++ b/arch/powerpc/platforms/cell/iommu.c @@ -569,19 +569,12 @@ static struct iommu_table *cell_get_iommu_table(struct device *dev) return &window->table; } -static u64 cell_iommu_get_fixed_address(struct device *dev); - static void cell_dma_dev_setup(struct device *dev) { - if (cell_iommu_enabled) { - u64 addr = cell_iommu_get_fixed_address(dev); - - if (addr != OF_BAD_ADDR) - set_dma_offset(dev, addr + dma_iommu_fixed_base); + if (cell_iommu_enabled) set_iommu_table_base(dev, cell_get_iommu_table(dev)); - } else { + else set_dma_offset(dev, cell_dma_nommu_offset); - } } static void cell_pci_dma_dev_setup(struct pci_dev *dev) @@ -865,8 +858,16 @@ static u64 cell_iommu_get_fixed_address(struct device *dev) static bool cell_pci_iommu_bypass_supported(struct pci_dev *pdev, u64 mask) { - return mask == DMA_BIT_MASK(64) && - cell_iommu_get_fixed_address(&pdev->dev) != OF_BAD_ADDR; + if (mask == DMA_BIT_MASK(64)) { + u64 addr = cell_iommu_get_fixed_address(&pdev->dev); + + if (addr != OF_BAD_ADDR) { + set_dma_offset(&pdev->dev, dma_iommu_fixed_base + addr); + return true; + } + } + + return true; } static void insert_16M_pte(unsigned long addr, unsigned long *ptab,
Christoph Hellwig <hch@lst.de> writes: > On Sun, Dec 09, 2018 at 09:23:39PM +1100, Michael Ellerman wrote: >> Christoph Hellwig <hch@lst.de> writes: >> >> > Configure the dma settings at device setup time, and stop playing games >> > with get_pci_dma_ops. This prepares for using the common dma_configure >> > code later on. >> > >> > Signed-off-by: Christoph Hellwig <hch@lst.de> >> > --- >> > arch/powerpc/platforms/cell/iommu.c | 20 +++++++++++--------- >> > 1 file changed, 11 insertions(+), 9 deletions(-) >> >> This one's crashing, haven't dug into why yet: > > Can you provide a gdb assembly of the exact crash site? This looks > like for some odd reason the DT structures aren't fully setup by the > time we are probing the device, which seems odd. It's dev->of_node which is NULL. Because we were passed a platform_device which doesn't have an of_node. It's the cbe-mic device created in cell_publish_devices(). I can fix that by simply checking for a NULL node, then the system boots but then I have no network devices, due to: tg3 0000:00:01.0: enabling device (0140 -> 0142) tg3 0000:00:01.0: DMA engine test failed, aborting tg3: probe of 0000:00:01.0 failed with error -12 tg3 0000:00:01.1: enabling device (0140 -> 0142) tg3 0000:00:01.1: DMA engine test failed, aborting tg3: probe of 0000:00:01.1 failed with error -12 I think the problem is that we don't want to set iommu_bypass_supported unless cell_iommu_fixed_mapping_init() succeeds. Yep. This makes it work for me on cell on top of your v5. cheers diff --git a/arch/powerpc/platforms/cell/iommu.c b/arch/powerpc/platforms/cell/iommu.c index 348a815779c1..8329fda17cc8 100644 --- a/arch/powerpc/platforms/cell/iommu.c +++ b/arch/powerpc/platforms/cell/iommu.c @@ -813,6 +813,10 @@ static u64 cell_iommu_get_fixed_address(struct device *dev) int i, len, best, naddr, nsize, pna, range_size; np = of_node_get(dev->of_node); + if (!np) + /* We can be called for platform devices that have no of_node */ + goto out; + while (1) { naddr = of_n_addr_cells(np); nsize = of_n_size_cells(np); @@ -1065,8 +1069,11 @@ static int __init cell_iommu_init(void) /* Setup various callbacks */ cell_pci_controller_ops.dma_dev_setup = cell_pci_dma_dev_setup; - if (!iommu_fixed_disabled && cell_iommu_fixed_mapping_init() == 0) + if (!iommu_fixed_disabled && cell_iommu_fixed_mapping_init() == 0) { + cell_pci_controller_ops.iommu_bypass_supported = + cell_pci_iommu_bypass_supported; goto done; + } /* Create an iommu for each /axon node. */ for_each_node_by_name(np, "axon") { @@ -1085,10 +1092,6 @@ static int __init cell_iommu_init(void) } done: /* Setup default PCI iommu ops */ - if (!iommu_fixed_disabled) { - cell_pci_controller_ops.iommu_bypass_supported = - cell_pci_iommu_bypass_supported; - } set_pci_dma_ops(&dma_iommu_ops); cell_iommu_enabled = true; bail:
On Sat, Dec 15, 2018 at 12:29:11AM +1100, Michael Ellerman wrote: > I think the problem is that we don't want to set iommu_bypass_supported > unless cell_iommu_fixed_mapping_init() succeeds. > > Yep. This makes it work for me on cell on top of your v5. Thanks, this looks good. I've folded it with the slight change of moving the iommu_bypass_supported setup into cell_iommu_fixed_mapping_init.
diff --git a/arch/powerpc/platforms/cell/iommu.c b/arch/powerpc/platforms/cell/iommu.c index 12352a58072a..cce5bf9515e5 100644 --- a/arch/powerpc/platforms/cell/iommu.c +++ b/arch/powerpc/platforms/cell/iommu.c @@ -657,14 +657,21 @@ static const struct dma_map_ops dma_iommu_fixed_ops = { .mapping_error = dma_iommu_mapping_error, }; +static u64 cell_iommu_get_fixed_address(struct device *dev); + static void cell_dma_dev_setup(struct device *dev) { - if (get_pci_dma_ops() == &dma_iommu_ops) + if (get_pci_dma_ops() == &dma_iommu_ops) { + u64 addr = cell_iommu_get_fixed_address(dev); + + if (addr != OF_BAD_ADDR) + set_dma_offset(dev, addr + dma_iommu_fixed_base); set_iommu_table_base(dev, cell_get_iommu_table(dev)); - else if (get_pci_dma_ops() == &dma_nommu_ops) + } else if (get_pci_dma_ops() == &dma_nommu_ops) { set_dma_offset(dev, cell_dma_nommu_offset); - else + } else { BUG(); + } } static void cell_pci_dma_dev_setup(struct pci_dev *dev) @@ -950,19 +957,14 @@ static int dma_suported_and_switch(struct device *dev, u64 dma_mask) { if (dma_mask == DMA_BIT_MASK(64) && cell_iommu_get_fixed_address(dev) != OF_BAD_ADDR) { - u64 addr = cell_iommu_get_fixed_address(dev) + - dma_iommu_fixed_base; dev_dbg(dev, "iommu: 64-bit OK, using fixed ops\n"); - dev_dbg(dev, "iommu: fixed addr = %llx\n", addr); set_dma_ops(dev, &dma_iommu_fixed_ops); - set_dma_offset(dev, addr); return 1; } if (dma_iommu_dma_supported(dev, dma_mask)) { dev_dbg(dev, "iommu: not 64-bit, using default ops\n"); - set_dma_ops(dev, get_pci_dma_ops()); - cell_dma_dev_setup(dev); + set_dma_ops(dev, &dma_iommu_ops); return 1; }
Configure the dma settings at device setup time, and stop playing games with get_pci_dma_ops. This prepares for using the common dma_configure code later on. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/powerpc/platforms/cell/iommu.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)