On Mon, Dec 9, 2013 at 6:31 PM, Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote: > On Mon, Dec 09, 2013 at 05:18:30PM -0700, Bjorn Helgaas wrote: > >> > Basically, your fix to pci_setup_bridge_io is fine - but your >> > observation led me to realize that the HW drivers implementing RMW for >> > 8 and 16 bit ops under their struct pci_ops.read function have exactly >> > the same flaw you are fixing here - they will silently wipe out write >> > 1 to clear bits. >> >> Ah, OK. I thought you were saying that we couldn't change >> pci_setup_bridge_io() to use 16-bit accesses because of this problem. > > No, not really - just that this bug you discovered is broader than just > that one place :) > >> Sure, if somebody can come up with a reasonable way to share this sort >> of code, that sounds like a good thing. Maybe extending struct >> pci_ops is the way to do this, but I hope not, because it seems like > > Well, sharing the code is no problem, it is exactly the same for every > 32 bit only host driver. > > But supporting RW1C bits in capability lists seems fairly hard once > the context at the call site is lost. Strictly speaking, fixing this only requires knowledge of the hardware register layout, so at least in principle, it can be done without the call site context. It does sound like it might be messy, though. QEMU deals with this issue somehow; maybe there's something there we can copy. > Which makes me wonder if supporting sub-dword writes to dwords with > RW1C bits makes sense at all :( > > That was why my initial reaction was to not do sub dword writes if you > know it will conflict with a RW1C bit. Keeping the dword read/write and clearing the upper 16 bits before the write is an option. But I'm inclined to do the 16-bit accesses, i.e., use the patch as posted, because that makes the issue more visible. If we keep the dword access, we can fix this particular issue, but it does nothing to help fix other similar cases. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
