On Fri, Oct 10, 2014 at 07:31:26PM +0100, Arnd Bergmann wrote: > On Friday 10 October 2014 14:58:04 Lorenzo Pieralisi wrote: > > On Thu, Oct 09, 2014 at 11:51:43AM +0100, Arnd Bergmann wrote: > > > > > > Last changes where introduced by commit 8c05cd08a, whose commit log adds > > > > to my confusion: > > > > > > > > "[...] I think what we want here is for pci_start to be 0 when mmap_api == > > > > PCI_MMAP_PROCFS.[...]" > > > > > > > > But that's not what the code does. > > > > > > My best guess is that this is a typo and that Darrick meant PCI_MMAP_SYSFS > > > in the changelog, which is the same thing that the code does. It's also > > > the sensible thing to do. > > > > > > This probably means that the procfs interface is now also broken on > > > sparc. > > > > > > > I will try to grab an integrator board to give it a go. > > > > > > Ok, good idea. > > > > Grabbed, tested it, my theory was correct, I can't map PCI resources > > to userspace. Actually, if I pass resource offset as a fixed-up address, mmap > > succeeds through proc, but it does not mmap the resource, it maps > > the resource + mem_offset that happens to be RAM :D for the PCI slot I am > > using. > > > > I am testing on an oldish (3.16) kernel since I am having trouble with > > mainline PCI and my network adapter on integrator, but I do not see why this > > is a problem, this bug has been there forever. > > I would guess that almost the only users of the sysfs and procfs > interfaces are Xorg drivers, you certainly don't need it to get > a network adapter working. The issue I am facing is not related to the PCI mmap implementation, that's certainly broken but does not stop me from using the board. [...] > > By removing mem_offset from pci_mmap_page_range() everything works fine, > > both proc and sys mappings are ok. > > Ok, thanks for confirming! > > > > However, given what you found, the procfs interface being broken since > > > 2010 on both architectures (arm32 and sparc) that try to honor the offset, > > > we should probably go back to your previous suggestion of removing > > > the offset handling, which would make it possible to use the procfs > > > interface and the sysfs interface on all architectures. > > > > > > Would you be able to prepare a patch that does this and circulate that > > > with the sparc, powerpc and microblaze maintainers as well as Darrick > > > and Martin who were involved with the pci_mmap_fits change? > > > > Yes, but let's step back a second. I think that the proc interface > > should expect an offset as passed to the user in /proc/bus/pci/devices, > > and there the resource is exposed through pci_resource_to_user(). > > > > Hence, the pci_mmap_fits() should check the offset against the > > resource filtered through pci_resource_to_user(), job done, patch > > is trivial, and does what pci_resource_to_user() was meant for IMHO. > > My point was that there is no reason why sparc and powerpc should > do this differently. At the moment they do and sparc is broken > as you proved. We can either fix sparc to restore the old behavior > by adding pci_resource_to_user to pci_mmap_fits, or by making it > do what powerpc does, essentially removing the memory space handling > from pci_resource_to_user. > > Whatever we do for sparc is probably what we need to do on ARM as well, > except that ARM has been broken for a longer time than sparc. > > > Then we have to decide what to do with arm32 code: > > > > 1) we remove mem_offset from pci_mmap_page_range() (and rely on default > > pci_resource_to_user()) > > > > or > > > > 2) we provide pci_resource_to_user() for arm32 which does the CPU->bus > > conversion for us (and leave mem_offset as-is in pci_mmap_range()) > > I'd vote for 1) to get it in line with the only working architectures > that currently use a nonzero offset, but Russell needs to have the final > word on this, and I still think we have to involve the sparc and powerpc > maintainers as well, hoping to find a common solution for everybody. > > Making a user space interface behave differently based on the CPU > architecture is a bad idea. I agree with you, I will put together a patchset and copy all people who should be involved. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
