On 10/11/22 at 05:16pm, Niklas Schnelle wrote:
> On Sun, 2022-10-09 at 18:31 +0800, Baoquan He wrote:
> > By taking GENERIC_IOREMAP method, the generic ioremap_prot() and
> > iounmap() are visible and available to arch. Arch only needs to
> > provide implementation of arch_ioremap() or arch_iounmap() if there's
> > arch specific handling needed in its ioremap() or iounmap(). This
> > change will simplify implementation by removing duplicated codes with
> > generic ioremap() and iounmap(), and has the equivalent functioality
> > as before.
> >
> > For s390, add hooks arch_ioremap() and arch_iounmap() for s390's special
> > operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> > converted to use ioremap_prot() from GENERIC_IOREMAP.
> >
> > Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
> > Cc: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> > Cc: Gerald Schaefer <gerald.schaefer@xxxxxxxxxxxxx>
> > Cc: Heiko Carstens <hca@xxxxxxxxxxxxx>
> > Cc: Vasily Gorbik <gor@xxxxxxxxxxxxx>
> > Cc: Alexander Gordeev <agordeev@xxxxxxxxxxxxx>
> > Cc: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>
> > Cc: Sven Schnelle <svens@xxxxxxxxxxxxx>
> > Cc: linux-s390@xxxxxxxxxxxxxxx
> > ---
> > v2->v3:
> > - Add code comment inside arch_ioremap() to help uderstand the
> > obsucre code. Christoph suggested this, Niklas provided the
> > paragraph of text.
> >
> > arch/s390/Kconfig | 1 +
> > arch/s390/include/asm/io.h | 25 +++++++++------
> > arch/s390/pci/pci.c | 65 ++++++++------------------------------
> > 3 files changed, 30 insertions(+), 61 deletions(-)
> >
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index 318fce77601d..c59e1b25f59d 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -135,6 +135,7 @@ config S390
> > select GENERIC_SMP_IDLE_THREAD
> > select GENERIC_TIME_VSYSCALL
> > select GENERIC_VDSO_TIME_NS
> > + select GENERIC_IOREMAP
>
> I think you should add the "if PCI" from the diff in your last mail to
> this patch.
That's reasonable, will do.
The code change in driver should be posted separately to get reviewing
from the relevant drvier maintainers. I may wrap it into this series in
next post so that people know its background.
>
> > select HAVE_ALIGNED_STRUCT_PAGE if SLUB
> > select HAVE_ARCH_AUDITSYSCALL
> > select HAVE_ARCH_JUMP_LABEL
......
> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > index 73cdc5539384..3c00dc7d79bc 100644
> > --- a/arch/s390/pci/pci.c
> > +++ b/arch/s390/pci/pci.c
> > @@ -244,64 +244,25 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
> > zpci_memcpy_toio(to, from, count);
> > }
> >
> > -static void __iomem *__ioremap(phys_addr_t addr, size_t size, pgprot_t prot)
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
> > {
> > - unsigned long offset, vaddr;
> > - struct vm_struct *area;
> > - phys_addr_t last_addr;
> > -
> > - last_addr = addr + size - 1;
> > - if (!size || last_addr < addr)
> > - return NULL;
> > -
> > + /*
> > + * When PCI MIO instructions are unavailable the "physical" address
> > + * encodes a hint for accessing the PCI memory space it represents.
> > + * Just pass it unchanged such that ioread/iowrite can decode it.
> > + */
> > if (!static_branch_unlikely(&have_mio))
> > - return (void __iomem *) addr;
> > -
> > - offset = addr & ~PAGE_MASK;
> > - addr &= PAGE_MASK;
> > - size = PAGE_ALIGN(size + offset);
> > - area = get_vm_area(size, VM_IOREMAP);
> > - if (!area)
> > - return NULL;
> > -
> > - vaddr = (unsigned long) area->addr;
> > - if (ioremap_page_range(vaddr, vaddr + size, addr, prot)) {
> > - free_vm_area(area);
> > - return NULL;
> > - }
> > - return (void __iomem *) ((unsigned long) area->addr + offset);
> > + return (void __iomem *) *paddr;
>
> nit: no space after the cast
Sorry, remember you pointed this out in v2, while I didn't get what
it is. Could you be more specific or give the right line of code?
Are you suggesting below line?
- return (void __iomem *) ((unsigned long) area->addr + offset);
+ return (void __iomem *)*paddr;
>
> > + return NULL;
> > }
> >
> > -void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
> > +bool arch_iounmap(void __iomem *addr)
> > {
> > - return __ioremap(addr, size, __pgprot(prot));
> > -}
> > -EXPORT_SYMBOL(ioremap_prot);
> > -
> > -void __iomem *ioremap(phys_addr_t addr, size_t size)
> > -{
> > - return __ioremap(addr, size, PAGE_KERNEL);
> > -}
> > -EXPORT_SYMBOL(ioremap);
> > -
> > -void __iomem *ioremap_wc(phys_addr_t addr, size_t size)
> > -{
> > - return __ioremap(addr, size, pgprot_writecombine(PAGE_KERNEL));
> > -}
> > -EXPORT_SYMBOL(ioremap_wc);
> > -
> > -void __iomem *ioremap_wt(phys_addr_t addr, size_t size)
> > -{
> > - return __ioremap(addr, size, pgprot_writethrough(PAGE_KERNEL));
> > -}
> > -EXPORT_SYMBOL(ioremap_wt);
> > -
> > -void iounmap(volatile void __iomem *addr)
> > -{
> > - if (static_branch_likely(&have_mio))
> > - vunmap((__force void *) ((unsigned long) addr & PAGE_MASK));
> > + if (!static_branch_likely(&have_mio))
> > + return false;
> > + return true;
> > }
> > -EXPORT_SYMBOL(iounmap);
> >
> > /* Create a virtual mapping cookie for a PCI BAR */
> > static void __iomem *pci_iomap_range_fh(struct pci_dev *pdev, int bar,
>
> Gave this a round of testing on s390 with both the MIO and non-MIO
> cases. I also see you addressed my previous comments and it looks good
> to me. As you showed in the other mail the compile error is a pre
> existing problem so shouldn't influence this change. So assuming you
> add the "if PCI" and the nit above you can add my:
Thanks a lot, will add that part to this patch and add your tags.
>
> Tested-by: Niklas
> Schnelle <schnelle@xxxxxxxxxxxxx>
> Reviewed-by: Niklas Schnelle
> <schnelle@xxxxxxxxxxxxx>
>
>
