| Message ID | 20230912135050.17155-2-tzimmermann@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ppc, fbdev: Clean up fbdev mmap helper | expand |
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, > Only PowerPC's fb_pgprotect() needs the file argument, although > the implementation does not use it. Pass NULL to the internal Can you please mention the function that's the implementation for PowerPC ? If I'm looking at the code correctly, that function is phys_mem_access_prot() defined in the arch/powerpc/mm/mem.c file: pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, unsigned long size, pgprot_t vma_prot) { if (ppc_md.phys_mem_access_prot) return ppc_md.phys_mem_access_prot(file, pfn, size, vma_prot); if (!page_is_ram(pfn)) vma_prot = pgprot_noncached(vma_prot); return vma_prot; } and if set, ppc_md.phys_mem_access_prot is pci_phys_mem_access_prot() that is defined in the arch/powerpc/kernel/pci-common.c source file: https://elixir.bootlin.com/linux/v6.6-rc2/source/arch/powerpc/kernel/pci-common.c#L524 That function indeed doesn't use the file argument. So your patch looks correct to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Javier Am 20.09.23 um 10:01 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > Hello Thomas, > >> Only PowerPC's fb_pgprotect() needs the file argument, although >> the implementation does not use it. Pass NULL to the internal > > Can you please mention the function that's the implementation for Sure > PowerPC ? If I'm looking at the code correctly, that function is > phys_mem_access_prot() defined in the arch/powerpc/mm/mem.c file: > > pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, > unsigned long size, pgprot_t vma_prot) > { > if (ppc_md.phys_mem_access_prot) > return ppc_md.phys_mem_access_prot(file, pfn, size, vma_prot); > > if (!page_is_ram(pfn)) > vma_prot = pgprot_noncached(vma_prot); > > return vma_prot; > } > > and if set, ppc_md.phys_mem_access_prot is pci_phys_mem_access_prot() > that is defined in the arch/powerpc/kernel/pci-common.c source file: > > https://elixir.bootlin.com/linux/v6.6-rc2/source/arch/powerpc/kernel/pci-common.c#L524 Yes, that's correct. The only value for that function pointer appears to be pci_phys_mem_access_prot() > > That function indeed doesn't use the file argument. So your patch looks > correct to me. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Thanks Best regards Thomas >
diff --git a/arch/powerpc/include/asm/fb.h b/arch/powerpc/include/asm/fb.h index 5f1a2e5f76548..61e3b8806db69 100644 --- a/arch/powerpc/include/asm/fb.h +++ b/arch/powerpc/include/asm/fb.h @@ -9,7 +9,12 @@ static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, unsigned long off) { - vma->vm_page_prot = phys_mem_access_prot(file, off >> PAGE_SHIFT, + /* + * PowerPC's implementation of phys_mem_access_prot() does + * not use the file argument. Set it to NULL in preparation + * of later updates to the interface. + */ + vma->vm_page_prot = phys_mem_access_prot(NULL, PHYS_PFN(off), vma->vm_end - vma->vm_start, vma->vm_page_prot); }
Only PowerPC's fb_pgprotect() needs the file argument, although the implementation does not use it. Pass NULL to the internal helper in preparation of further updates. A later patch will remove the file parameter from fb_pgprotect(). While at it, replace the shift operation with PHYS_PFN(). Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- arch/powerpc/include/asm/fb.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)