| Message ID | 20140409061128.GC16465@1wt.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Willy Tarreau, On Wed, 9 Apr 2014 08:11:29 +0200, Willy Tarreau wrote: > The WARN is correctly emitted for both igb ports here, but unfortunately > despite EINVAL, I still get the panic. Also I find it surprizing that it > reports sizes ending in ffff. I read the patch and it looks correct, so > we possibly have something else wrong here. Yes, the panic is expected: Jason's patch is not *fixing* anything, it's just telling you *why* it's going to panic. Thomas
On Wed, Apr 09, 2014 at 09:12:34AM +0200, Thomas Petazzoni wrote: > Dear Willy Tarreau, > > On Wed, 9 Apr 2014 08:11:29 +0200, Willy Tarreau wrote: > > > The WARN is correctly emitted for both igb ports here, but unfortunately > > despite EINVAL, I still get the panic. Also I find it surprizing that it > > reports sizes ending in ffff. I read the patch and it looks correct, so > > we possibly have something else wrong here. > > Yes, the panic is expected: Jason's patch is not *fixing* anything, > it's just telling you *why* it's going to panic. I just thought that the EINVAL would prevent one from registering the device, which would be more useful (if at all possible). Willy
Dear Willy Tarreau, On Wed, 9 Apr 2014 09:47:52 +0200, Willy Tarreau wrote: > > Yes, the panic is expected: Jason's patch is not *fixing* anything, > > it's just telling you *why* it's going to panic. > > I just thought that the EINVAL would prevent one from registering > the device, which would be more useful (if at all possible). Unfortunately, I don't think that's possible: the function that sets up the MBus window is called by the emulated bridge code, i.e when the Linux PCI core thinks it is doing a write to a bridge register. And I don't think there is a way of "refusing" the write to let the Linux PCI core know that it was not possible to set up the bridge BAR as it was requested. Maybe this is something that Jason can confirm/infirm. I remember having a quick look at the core Linux PCI core to see if it was somehow checking whether the bridge BAR has been properly configured, but I think I concluded it was not the case, and it was just assuming that write the memory base/limit in the bridge registers was sufficient. Thomas
On Wed, Apr 09, 2014 at 09:53:50AM +0200, Thomas Petazzoni wrote: > Dear Willy Tarreau, > > On Wed, 9 Apr 2014 09:47:52 +0200, Willy Tarreau wrote: > > > > Yes, the panic is expected: Jason's patch is not *fixing* anything, > > > it's just telling you *why* it's going to panic. > > > > I just thought that the EINVAL would prevent one from registering > > the device, which would be more useful (if at all possible). > > Unfortunately, I don't think that's possible: the function that sets up > the MBus window is called by the emulated bridge code, i.e when the > Linux PCI core thinks it is doing a write to a bridge register. And I > don't think there is a way of "refusing" the write to let the Linux > PCI core know that it was not possible to set up the bridge BAR as it > was requested. > > Maybe this is something that Jason can confirm/infirm. I remember > having a quick look at the core Linux PCI core to see if it was > somehow checking whether the bridge BAR has been properly configured, > but I think I concluded it was not the case, and it was just assuming > that write the memory base/limit in the bridge registers was > sufficient. OK thanks for the detailed explanation! Willy
On Wed, Apr 09, 2014 at 08:11:29AM +0200, Willy Tarreau wrote: > OK I just got it by adding two printk() in pci-mvebu.c. Both functions > mvebu_pcie_handle_iobase_change() and mvebu_pcie_handle_membase_change() > do pass a size which is in fact a mask (size - 1) and not the real size. > So the mbus is fed with an incorrect size which is off by one : Yes, that is right. I tested my patch here and didn't see any problem, but I realize now that the mbus code is bailing early due to this: kernel: mvebu_mbus: cannot add window '4:e8', conflicts with another window Which I've never got around to fixing.. (whole other story there) Your patch looks fine, and it obviously needs to be sequenced before mine. (Thomas/Jason C: how do you want to do this?) Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > From de000611015c7490a07ced6e36bfffbfdd136832 Mon Sep 17 00:00:00 2001 > From: Willy Tarreau <w@1wt.eu> > Date: Wed, 9 Apr 2014 08:05:09 +0200 > Subject: pci: mvebu: fix off-by-one in the computed size of the mbus windows > > mvebu_pcie_handle_membase_change() and mvebu_pcie_handle_iobase_change() > compute a window size which is in fact a mask. This size is fed to > mvebu_mbus_add_window_by_id() which itself subtracts 1 to get the > mask. So clearly the two functions above are wrong. Mask isn't the right word, maybe: mvebu_pcie_handle_membase_change() and mvebu_pcie_handle_iobase_change() do not correctly compute the window size. PCI uses an inclusive start/end address pair, which requires a +1 when converting to size. This only worked because a bug in the mbus driver allowed it to silently accept and round up bogus sizes. Fix this by adding one to the computed size. Regards, Jason
On Wed, Apr 09, 2014 at 09:53:50AM +0200, Thomas Petazzoni wrote: > Maybe this is something that Jason can confirm/infirm. I remember > having a quick look at the core Linux PCI core to see if it was > somehow checking whether the bridge BAR has been properly configured, > but I think I concluded it was not the case, and it was just assuming > that write the memory base/limit in the bridge registers was > sufficient. I think the best way to fail would be to return an error from mvebu_pcie_align_resource - but we can't create the window in that call. Once things get to the BAR write stage it shouldn't fail unfortunately. Jason
Dear Jason Gunthorpe, On Wed, 9 Apr 2014 10:20:40 -0600, Jason Gunthorpe wrote: > > OK I just got it by adding two printk() in pci-mvebu.c. Both functions > > mvebu_pcie_handle_iobase_change() and mvebu_pcie_handle_membase_change() > > do pass a size which is in fact a mask (size - 1) and not the real size. > > So the mbus is fed with an incorrect size which is off by one : > > Yes, that is right. I tested my patch here and didn't see any problem, > but I realize now that the mbus code is bailing early due to this: > > kernel: mvebu_mbus: cannot add window '4:e8', conflicts with another window > > Which I've never got around to fixing.. (whole other story there) > > Your patch looks fine, and it obviously needs to be sequenced before > mine. (Thomas/Jason C: how do you want to do this?) What I can propose is that I accumulate in a branch all the patches needed to solve the various PCIe/Mbus problems we've identified: * Your patch adding warnings to the mvebu-mbus driver * Willy's patch fixing the off-by-one on the size * Neil's patch fixing the MSI teardown function * My two patches fixing the rest of the MSI logic * And patches to come for the link problem, and the cutting of non-power-of-two BARs into power-of-two windows This way, everybody will be able to test than in his specific hardware situations, the patches are solving all the problems. Then I can take care of formally submitting those patches to the relevant maintainers, of course keeping the authorship as appropriate. How does that sound? Thomas
On Thu, Apr 10, 2014 at 08:53:46AM +0200, Thomas Petazzoni wrote: > Dear Jason Gunthorpe, > > On Wed, 9 Apr 2014 10:20:40 -0600, Jason Gunthorpe wrote: > > > > OK I just got it by adding two printk() in pci-mvebu.c. Both functions > > > mvebu_pcie_handle_iobase_change() and mvebu_pcie_handle_membase_change() > > > do pass a size which is in fact a mask (size - 1) and not the real size. > > > So the mbus is fed with an incorrect size which is off by one : > > > > Yes, that is right. I tested my patch here and didn't see any problem, > > but I realize now that the mbus code is bailing early due to this: > > > > kernel: mvebu_mbus: cannot add window '4:e8', conflicts with another window > > > > Which I've never got around to fixing.. (whole other story there) > > > > Your patch looks fine, and it obviously needs to be sequenced before > > mine. (Thomas/Jason C: how do you want to do this?) > > What I can propose is that I accumulate in a branch all the patches > needed to solve the various PCIe/Mbus problems we've identified: > > * Your patch adding warnings to the mvebu-mbus driver > * Willy's patch fixing the off-by-one on the size > * Neil's patch fixing the MSI teardown function > * My two patches fixing the rest of the MSI logic > * And patches to come for the link problem, and the cutting of > non-power-of-two BARs into power-of-two windows > > This way, everybody will be able to test than in his specific > hardware situations, the patches are solving all the problems. Then I > can take care of formally submitting those patches to the relevant > maintainers, of course keeping the authorship as appropriate. > > How does that sound? Great! Please do. thx, Jason.
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index 0e79665..eff0ab5 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -329,7 +329,7 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port) port->iowin_base = port->pcie->io.start + iobase; port->iowin_size = ((0xFFF | ((port->bridge.iolimit & 0xF0) << 8) | (port->bridge.iolimitupper << 16)) - - iobase); + iobase) + 1; mvebu_mbus_add_window_remap_by_id(port->io_target, port->io_attr, port->iowin_base, port->iowin_size, @@ -362,7 +362,7 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port) port->memwin_base = ((port->bridge.membase & 0xFFF0) << 16); port->memwin_size = (((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) - - port->memwin_base; + port->memwin_base + 1; mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr, port->memwin_base, port->memwin_size);