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