| Message ID | 20230405150554.30540-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arch: Consolidate <asm/fb.h> | expand |
On Wed, Apr 5, 2023, at 17:05, Thomas Zimmermann wrote: > Generic implementations of fb_pgprotect() and fb_is_primary_device() > have been in the source code for a long time. Prepare the header file > to make use of them. > > Improve the code by using an inline function for fb_pgprotect() and > by removing include statements. > > Symbols are protected by preprocessor guards. Architectures that > provide a symbol need to define a preprocessor token of the same > name and value. Otherwise the header file will provide a generic > implementation. This pattern has been taken from <asm/io.h>. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Moving this into generic code is good, but I'm not sure about the default for fb_pgprotect(): > + > +#ifndef fb_pgprotect > +#define fb_pgprotect fb_pgprotect > +static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, > + unsigned long off) > +{ } > +#endif I think most architectures will want the version we have on arc, arm, arm64, loongarch, and sh already: static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, unsigned long off) { vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); } so I'd suggest making that version the default, and treating the empty ones (m68knommu, sparc32) as architecture specific workarounds. I see that sparc64 and parisc use pgprot_uncached here, but as they don't define a custom pgprot_writecombine, this ends up being the same, and they can use the above definition as well. mips defines pgprot_writecombine but uses pgprot_noncached in fb_pgprotect(), which is probably a mistake and should have been updated as part of commit 4b050ba7a66c ("MIPS: pgtable.h: Implement the pgprot_writecombine function for MIPS"). Arnd
On Wed, Apr 05, 2023 at 05:53:03PM +0200, Arnd Bergmann wrote: > On Wed, Apr 5, 2023, at 17:05, Thomas Zimmermann wrote: > > Generic implementations of fb_pgprotect() and fb_is_primary_device() > > have been in the source code for a long time. Prepare the header file > > to make use of them. > > > > Improve the code by using an inline function for fb_pgprotect() and > > by removing include statements. > > > > Symbols are protected by preprocessor guards. Architectures that > > provide a symbol need to define a preprocessor token of the same > > name and value. Otherwise the header file will provide a generic > > implementation. This pattern has been taken from <asm/io.h>. > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > Moving this into generic code is good, but I'm not sure > about the default for fb_pgprotect(): > > > + > > +#ifndef fb_pgprotect > > +#define fb_pgprotect fb_pgprotect > > +static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, > > + unsigned long off) > > +{ } > > +#endif > > I think most architectures will want the version we have on > arc, arm, arm64, loongarch, and sh already: > > static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, > unsigned long off) > { > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > } > > so I'd suggest making that version the default, and treating the > empty ones (m68knommu, sparc32) as architecture specific > workarounds. Yeah I was about to type the same suggestion :-) -Daniel > I see that sparc64 and parisc use pgprot_uncached here, but as > they don't define a custom pgprot_writecombine, this ends up being > the same, and they can use the above definition as well. > > mips defines pgprot_writecombine but uses pgprot_noncached > in fb_pgprotect(), which is probably a mistake and should have > been updated as part of commit 4b050ba7a66c ("MIPS: pgtable.h: > Implement the pgprot_writecombine function for MIPS"). > > Arnd
Hi Am 05.04.23 um 17:53 schrieb Arnd Bergmann: > On Wed, Apr 5, 2023, at 17:05, Thomas Zimmermann wrote: >> Generic implementations of fb_pgprotect() and fb_is_primary_device() >> have been in the source code for a long time. Prepare the header file >> to make use of them. >> >> Improve the code by using an inline function for fb_pgprotect() and >> by removing include statements. >> >> Symbols are protected by preprocessor guards. Architectures that >> provide a symbol need to define a preprocessor token of the same >> name and value. Otherwise the header file will provide a generic >> implementation. This pattern has been taken from <asm/io.h>. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > Moving this into generic code is good, but I'm not sure > about the default for fb_pgprotect(): > >> + >> +#ifndef fb_pgprotect >> +#define fb_pgprotect fb_pgprotect >> +static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, >> + unsigned long off) >> +{ } >> +#endif > > I think most architectures will want the version we have on > arc, arm, arm64, loongarch, and sh already: > > static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, > unsigned long off) > { > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > } > > so I'd suggest making that version the default, and treating the > empty ones (m68knommu, sparc32) as architecture specific > workarounds. Make sense, thanks for the feedback. I'll send out an update soon. Best regards Thomas > > I see that sparc64 and parisc use pgprot_uncached here, but as > they don't define a custom pgprot_writecombine, this ends up being > the same, and they can use the above definition as well. > > mips defines pgprot_writecombine but uses pgprot_noncached > in fb_pgprotect(), which is probably a mistake and should have > been updated as part of commit 4b050ba7a66c ("MIPS: pgtable.h: > Implement the pgprot_writecombine function for MIPS"). > > Arnd
Hi Am 05.04.23 um 17:53 schrieb Arnd Bergmann: > On Wed, Apr 5, 2023, at 17:05, Thomas Zimmermann wrote: >> Generic implementations of fb_pgprotect() and fb_is_primary_device() >> have been in the source code for a long time. Prepare the header file >> to make use of them. >> >> Improve the code by using an inline function for fb_pgprotect() and >> by removing include statements. >> >> Symbols are protected by preprocessor guards. Architectures that >> provide a symbol need to define a preprocessor token of the same >> name and value. Otherwise the header file will provide a generic >> implementation. This pattern has been taken from <asm/io.h>. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > Moving this into generic code is good, but I'm not sure > about the default for fb_pgprotect(): > >> + >> +#ifndef fb_pgprotect >> +#define fb_pgprotect fb_pgprotect >> +static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, >> + unsigned long off) >> +{ } >> +#endif > > I think most architectures will want the version we have on > arc, arm, arm64, loongarch, and sh already: > > static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, > unsigned long off) > { > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > } > > so I'd suggest making that version the default, and treating the > empty ones (m68knommu, sparc32) as architecture specific > workarounds. > > I see that sparc64 and parisc use pgprot_uncached here, but as > they don't define a custom pgprot_writecombine, this ends up being > the same, and they can use the above definition as well. > > mips defines pgprot_writecombine but uses pgprot_noncached > in fb_pgprotect(), which is probably a mistake and should have > been updated as part of commit 4b050ba7a66c ("MIPS: pgtable.h: > Implement the pgprot_writecombine function for MIPS"). I would not want to change any of the other platform's functions unless the rsp platform maintainers ask me to. Best regards Thomas > > Arnd
diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h index f9f18101ed36..cb42166e7e11 100644 --- a/include/asm-generic/fb.h +++ b/include/asm-generic/fb.h @@ -1,13 +1,29 @@ /* SPDX-License-Identifier: GPL-2.0 */ + #ifndef __ASM_GENERIC_FB_H_ #define __ASM_GENERIC_FB_H_ -#include <linux/fb.h> -#define fb_pgprotect(...) do {} while (0) +/* + * Only include this header file from your architecture's <asm/fb.h>. + */ + +struct fb_info; +struct file; +struct vm_area_struct; + +#ifndef fb_pgprotect +#define fb_pgprotect fb_pgprotect +static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, + unsigned long off) +{ } +#endif +#ifndef fb_is_primary_device +#define fb_is_primary_device fb_is_primary_device static inline int fb_is_primary_device(struct fb_info *info) { return 0; } +#endif #endif /* __ASM_GENERIC_FB_H_ */
Generic implementations of fb_pgprotect() and fb_is_primary_device() have been in the source code for a long time. Prepare the header file to make use of them. Improve the code by using an inline function for fb_pgprotect() and by removing include statements. Symbols are protected by preprocessor guards. Architectures that provide a symbol need to define a preprocessor token of the same name and value. Otherwise the header file will provide a generic implementation. This pattern has been taken from <asm/io.h>. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- include/asm-generic/fb.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)