On Wed, May 4, 2016 at 8:17 AM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> What mess do you mean? The fact that you could only use
> pcibios_bus_to_resource() for MEM, and something else for IO? Even
> if we could only use pcibios_bus_to_resource() for MEM, that sounds
> like an improvement, not a mess.
I means that we need create another 5 pci_user_to_resource
arch/microblaze/pci/pci-common.c:void pci_resource_to_user(const
struct pci_dev *dev, int bar,
arch/mips/include/asm/pci.h:static inline void
pci_resource_to_user(const struct pci_dev *dev, int bar,
arch/powerpc/kernel/pci-common.c:void pci_resource_to_user(const
struct pci_dev *dev, int bar,
arch/sparc/kernel/pci.c:void pci_resource_to_user(const struct pci_dev
*pdev, int bar,
include/linux/pci.h:static inline void pci_resource_to_user(const
struct pci_dev *dev, int bar,
>
> Most of __pci_mmap_make_offset() is pointless.
we can clean up them later.
>
> We might need something there for I/O regions, but for MEM, the
> vma->vm_pgoff coming into pci_mmap_page_range() should be exactly what
> we need and we shouldn't touch it. I think __pci_mmap_make_offset()
> actually does leave it alone for MEM, but you have to read the code
> carefully to figure that out.
>
> All the validation stuff ("Check that the offset requested corresponds
> to one of the resources...") should be removed or moved to
> pci_mmap_fits().
ok, will give it try.
>> @@ -231,13 +231,26 @@ static int proc_bus_pci_mmap(struct file
>> {
>> struct pci_dev *dev = PDE_DATA(file_inode(file));
>> struct pci_filp_private *fpriv = file->private_data;
>> + resource_size_t start, end, offset;
>> + struct resource *res;
>> int i, ret;
>>
>> if (!capable(CAP_SYS_RAWIO))
>> return -EPERM;
>>
>> + offset = vma->vm_pgoff << PAGE_SHIFT;
>> +
>> /* Make sure the caller is mapping a real resource for this device */
>> for (i = 0; i < PCI_ROM_RESOURCE; i++) {
>> + res = &dev->resource[i];
>> + if (!res->flags)
>> + continue;
>> +
>> + pci_resource_to_user(dev, i, res, &start, &end);
>> + if (!(offset >= start && offset <= end))
>> + continue;
>> +
>> + vma->vm_pgoff = (res->start + (offset - start)) >> PAGE_SHIFT;
>> if (pci_mmap_fits(dev, i, vma, PCI_MMAP_PROCFS))
>
> This is sort of OK, but I think we can do better. I don't see any
> problem with introducing pci_user_to_resource() as the inverse of
> pci_resource_to_user(). I think it will make this code read much
> better.
>
> The default pci_user_to_resource() would do nothing, just like the
> default pci_resource_to_user().
>
> For sparc, I think pci_user_to_resource() can use
> pcibios_bus_to_resource(), and pci_resource_to_user() can be rewritten
> to use pcibios_resource_to_bus(). That makes it much more obvious
> what's happening.
>
> It looks like microblaze and powerpc should use
> pcibios_resource_to_bus() for I/O resources and the default "user
> address == resource address" for MMIO (?)
>
> My goal is to make pci_mmap_resource() and proc_bus_pci_mmap() look
> very similar, e.g.,
>
> /* locate resource */
> pci_user_to_resource() # only in proc_bus_pci_mmap()
> if (!pci_mmap_fits()) {
> WARN(...);
> return -EINVAL;
> }
> pci_mmap_page_range();
>
> Obviously there are several steps in getting here. Reworking
> pci_resource_to_user() to use pcibios_resource_to_bus() when possible
> would be a good start.
I would like to avoid adding pci_user_to_resource, and put extra calling
pci_resource_to_user pci_mmap_fits instead.
Thanks
Yinghai
--
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
