On Mon, Oct 12, 2020 at 04:03:28PM +0200, Niklas Schnelle wrote: > Hi Daniel, > > freshly back from my vacation I've just taken a look at your patch. > First thanks for this fix and the detailed commit description. > Definitely makes sense to fix this and you can add my > > Acked-by: Niklas Schnelle <schne...@linux.ibm.com> > > Content wise it all looks sane and clear and since Gerald did the testing, > I would have applied it to our tree already, but I got some trivial > checkpatch violations that probably apply to the whole series. > I've commented them inline below. > If you confirm there I can do the fixups when applying or you can resend. > > On 10/9/20 9:59 AM, Daniel Vetter wrote: > > Way back it was a reasonable assumptions that iomem mappings never > > change the pfn range they point at. But this has changed: > > > > - gpu drivers dynamically manage their memory nowadays, invalidating > > ptes with unmap_mapping_range when buffers get moved > > > > - contiguous dma allocations have moved from dedicated carvetouts to > > cma regions. This means if we miss the unmap the pfn might contain > > pagecache or anon memory (well anything allocated with GFP_MOVEABLE) > > > > - even /dev/mem now invalidates mappings when the kernel requests that > > iomem region when CONFIG_IO_STRICT_DEVMEM is set, see 3234ac664a87 > > The above commit mention should use the format > 'commit 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the > region")' > otherwise this results in a checkpatch ERROR. > > > ("/dev/mem: Revoke mappings when a driver claims the region") > > > > Accessing pfns obtained from ptes without holding all the locks is > > therefore no longer a good idea. Fix this. > > > > Since zpci_memcpy_from|toio seems to not do anything nefarious with > > locks we just need to open code get_pfn and follow_pfn and make sure > > we drop the locks only after we've done. The write function also needs > > just a typo but just saw it "we're" instead of "we've" > > > the copy_from_user move, since we can't take userspace faults while > > holding the mmap sem. > > > > Reviewed-by: Gerald Schaefer <gerald.schae...@linux.ibm.com> > > > No empty line after the Revied-by tag. > > > Signed-off-by: Daniel Vetter <daniel.vet...@intel.com> > > Your Signed-off-by mail address does not match the one you're sending from, > this yields a checkpatch warning when using git am with your mail. > This is probably just a silly misconfiguration but since Signed-offs > are signatures should I change this to > "Daniel Vetter <daniel.vet...@ffwll.ch>" which is the one you're > sending from and also in the MAINTAINERS file? > > > > Cc: Jason Gunthorpe <j...@ziepe.ca> > > Cc: Dan Williams <dan.j.willi...@intel.com> > > Cc: Kees Cook <keesc...@chromium.org> > > Cc: Andrew Morton <a...@linux-foundation.org> > > Cc: John Hubbard <jhubb...@nvidia.com> > > Cc: Jérôme Glisse <jgli...@redhat.com> > > Cc: Jan Kara <j...@suse.cz> > > Cc: Dan Williams <dan.j.willi...@intel.com> > > The above Cc: line for Dan Williams is a duplicate > > > Cc: linux...@kvack.org > > Cc: linux-arm-ker...@lists.infradead.org > > Cc: linux-samsung-...@vger.kernel.org > > Cc: linux-me...@vger.kernel.org > > Cc: Niklas Schnelle <schne...@linux.ibm.com> > > Cc: Gerald Schaefer <gerald.schae...@linux.ibm.com> > > Cc: linux-s...@vger.kernel.org > > -- > > v2: Move VM_IO | VM_PFNMAP checks around so they keep returning EINVAL > > like before (Gerard) > > I think the above should go before the CC/Signed-off/Reviewev block.
This is a per-subsystem bikeshed :-) drivers/gpu definitely wants it above, but most core subsystems want it below. I'll move it. > > --- > > arch/s390/pci/pci_mmio.c | 98 +++++++++++++++++++++++----------------- > > 1 file changed, 57 insertions(+), 41 deletions(-) > > > > diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c > > index 401cf670a243..1a6adbc68ee8 100644 > > --- a/arch/s390/pci/pci_mmio.c > > +++ b/arch/s390/pci/pci_mmio.c > > @@ -119,33 +119,15 @@ static inline int __memcpy_toio_inuser(void __iomem > > *dst, > > return rc; > > } > > > > -static long get_pfn(unsigned long user_addr, unsigned long access, > > - unsigned long *pfn) > > -{ > > - struct vm_area_struct *vma; > > - long ret; > > - > > - mmap_read_lock(current->mm); > > - ret = -EINVAL; > > - vma = find_vma(current->mm, user_addr); > > - if (!vma) > > - goto out; > > - ret = -EACCES; > > - if (!(vma->vm_flags & access)) > > - goto out; > > - ret = follow_pfn(vma, user_addr, pfn); > > -out: > > - mmap_read_unlock(current->mm); > > - return ret; > > -} > > - > > SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, > > const void __user *, user_buffer, size_t, length) > > { > > u8 local_buf[64]; > > void __iomem *io_addr; > > void *buf; > > - unsigned long pfn; > > + struct vm_area_struct *vma; > > + pte_t *ptep; > > + spinlock_t *ptl; > > With checkpatch.pl --strict the above yields a complained > "CHECK: spinlock_t definition without comment" but I think > that's really okay since your commit description is very clear. > Same oin line 277. I think this is a falls positive, checkpatch doesn't realize that SYSCALL_DEFINE3 is a function, not a structure. And in a structure I'd have added the kerneldoc or comment. I'll fix up all the nits you've found for the next round. Thanks for taking a look. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel