On Fri, Jun 17, 2016 at 12:25:49PM -0700, Yinghai Lu wrote:

On Thu, Jun 16, 2016 at 7:15 PM, Bjorn Helgaas [off-list ref] wrote:

quoted

On Thu, Jun 09, 2016 at 03:25:52PM -0700, Yinghai Lu wrote:

quoted

In 8c05cd08a7 ("PCI: fix offset check for sysfs mmapped files"), try
to check exposed value with resource start/end in proc mmap path.

|        start = vma->vm_pgoff;
|        size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1;
|        pci_start = (mmap_api == PCI_MMAP_PROCFS) ?
|                        pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0;
|        if (start >= pci_start && start < pci_start + size &&
|                        start + nr <= pci_start + size)

That breaks sparc that exposed value is BAR value, and need to be offseted
to resource address.

I'm not quite sure what you're saying here.  Are you saying that sparc
is currently broken, and this patch fixes it?  If so, what exactly is
broken?  Can you give a small example of an mmap that is currently
broken?

quoted

Original pci_mmap_page_range() is taking PCI BAR value aka usr_address.

Bjorn found out that it would be much simple to pass resource address
directly and avoid extra those __pci_mmap_make_offset.

In this patch:
1. in proc path: proc_bus_pci_mmap, try convert back to resource
   before calling pci_mmap_page_range
2. in sysfs path: pci_mmap_resource will just offset with resource start.
3. all pci_mmap_page_range will have vma->vm_pgoff with in resource
   range instead of BAR value.
4. remove __pci_mmap_make_offset, as the checking is done
   in pci_mmap_fits().

This is a pretty big patch.  It would help a lot to split it up.

Looks like they are tight together after change api. vm_pgoff meaning changes.

I could split item 4 to another patch, but compiler could complain or
even refuse to
go on if static functions are defined but not used.

Yeah, I was afraid they might be too tightly coupled to split up.
Still, every little bit helps.

quoted

I think the comment about "re-enabling the 2 lines below" is pointless
because doing that would break applications, which I don't think we'll
do.

I propose the microblaze, powerpc, and sparc patches below, which
remove simplify pci_resource_to_user() and clean up this comment.

Agreed. Actually I have the change for sparc/PCI in patch 3
   sparc/PCI: Use correct offset for bus address to resource
according to previous review.

Sure enough, I see it there now.  I think it's easier to review when
split out, so I'll keep it separate, since it's not actually dependent
on the rest of the changes in "sparc/PCI: Use correct offset for bus
address to resource".

Will drop related change in sparc/PCI: Use correct offset for bus
address to resource

and respin the whole patchset today.

I added your acks and pushed the result to pci/resource.  I'll also
post these formally on the list so they're easier to find.

Bjorn