Re: [PATCH v12.update2 02/15] PCI: Let pci_mmap_page_range() take resource address

[Bjorn Helgaas]

On Fri, Jun 17, 2016 at 12:25:49PM -0700, Yinghai Lu wrote:
> On Thu, Jun 16, 2016 at 7:15 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > On Thu, Jun 09, 2016 at 03:25:52PM -0700, Yinghai Lu wrote:
> >> 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?
> >
> >> 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.

> > 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