Re: [RFC 2/4] PCI: generic: Add support for ARM64 and MSI(x)

[Lorenzo Pieralisi]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/