On 2021/12/3 18:18, Barry Song wrote:
>> + switch (entry->mmap_type) {
>> + case HNS_ROCE_MMAP_TYPE_DB:
>> + prot = pgprot_noncached(vma->vm_page_prot);
>> + break;
>> + case HNS_ROCE_MMAP_TYPE_TPTR:
>> + prot = vma->vm_page_prot;
>> + break;
>> + /*
>> + * The BAR region of direct WQE supports Early Write Ack,
>> + * so pgprot_device is used to improve performance.
>> + */
>> + case HNS_ROCE_MMAP_TYPE_DWQE:
>> + prot = pgprot_device(vma->vm_page_prot);
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>
> i am still not convinced why HNS_ROCE_MMAP_TYPE_DB needs nocache and HNS_ROCE_MMAP_TYPE_DWQE needs
> device. generally people use ioremap() to map pci bar spaces in pci device drivers, and ioremap()
> is pretty much nGnRE:
> #define ioremap(addr, size) __ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
> #define ioremap_np(addr, size) __ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRnE))
>
> i am only seeing four places which are using nE in kernel:
> # line filename / context / line
> 1 866 drivers/of/address.c <<of_iomap>>
> return ioremap_np(res.start, resource_size(&res));
> 2 901 drivers/of/address.c <<of_io_request_and_map>>
> mem = ioremap_np(res.start, resource_size(&res));
> 3 89 include/linux/io.h <<pci_remap_cfgspace>>
> return ioremap_np(offset, size) ?: ioremap(offset, size);
> 4 47 lib/devres.c <<__devm_ioremap>>
> addr = ioremap_np(offset, size);
>
> so i guess nGnRE is quite safe for pci device bar spaces. for config space, it is a different story
> though which is the 3rd one in the above list:
>
> #ifdef CONFIG_PCI
> /*
> * The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
> * Posting") mandate non-posted configuration transactions. This default
> * implementation attempts to use the ioremap_np() API to provide this
> * on arches that support it, and falls back to ioremap() on those that
> * don't. Overriding this function is deprecated; arches that properly
> * support non-posted accesses should implement ioremap_np() instead, which
> * this default implementation can then use to return mappings compliant with
> * the PCI specification.
> */
> #ifndef pci_remap_cfgspace
> #define pci_remap_cfgspace pci_remap_cfgspace
> static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
> size_t size)
> {
> return ioremap_np(offset, size) ?: ioremap(offset, size);
> }
> #endif
> #endif
>
> Thanks
> Barry
>
> .
>
Thank you for your comment. After my reconsideration, HNS_ROCE_MMAP_TYPE_DB should use
the device attribute. I will submit another patch to fix this problem first.
Thanks
Wenpeng
