| Message ID | 20230406143019.6709-2-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | arch: Consolidate <asm/fb.h> | expand |
Hi Thomas, On Thu, Apr 6, 2023 at 4:30 PM Thomas Zimmermann <tzimmermann@suse.de> 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. The default mode set by > fb_pgprotect() is now writecombine, which is what most platforms > want. > > 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>. > > v2: > * use writecombine mappings by default (Arnd) > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Thanks for your patch! > --- a/include/asm-generic/fb.h > +++ b/include/asm-generic/fb.h > @@ -1,13 +1,32 @@ > /* 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>. > + */ > + > +#include <asm/page.h> > + > +struct fb_info; > +struct file; > + > +#ifndef fb_pgprotect > +#define fb_pgprotect fb_pgprotect > +static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, > + unsigned long off) Does this affect any noMMU platforms that relied on fb_pgprotect() doing nothing before? Perhaps the body below should be protected by "#ifdef CONFIG_MMU"? > +{ > + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); Shouldn't this use the pgprot_val() wrapper? > +} > +#endif Gr{oetje,eeting}s, Geert
Hi Geert Am 11.04.23 um 10:08 schrieb Geert Uytterhoeven: > Hi Thomas, > > On Thu, Apr 6, 2023 at 4:30 PM Thomas Zimmermann <tzimmermann@suse.de> 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. The default mode set by >> fb_pgprotect() is now writecombine, which is what most platforms >> want. >> >> 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>. >> >> v2: >> * use writecombine mappings by default (Arnd) >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > Thanks for your patch! > >> --- a/include/asm-generic/fb.h >> +++ b/include/asm-generic/fb.h >> @@ -1,13 +1,32 @@ >> /* 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>. >> + */ >> + >> +#include <asm/page.h> >> + >> +struct fb_info; >> +struct file; >> + >> +#ifndef fb_pgprotect >> +#define fb_pgprotect fb_pgprotect >> +static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, >> + unsigned long off) > > Does this affect any noMMU platforms that relied on fb_pgprotect() > doing nothing before? > Perhaps the body below should be protected by "#ifdef CONFIG_MMU"? I cannot conclusively answer this question, but I did some grep'ing ('git grep ndef | grep CONFIG_MMU'): Only the architectures in this patchset provide <asm/fb.h> but nothing anywhere uses <asm-generic/fb.h> yet. And of those architectures, only arm and m68k have !CONFIG_MMU cases. Those are handled in the rsp patches. I think we're good. > >> +{ >> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > > Shouldn't this use the pgprot_val() wrapper? No, I think. Grep'ing for vm_page_prot, I'm not seeing it being used in such assignments. Best regards Thomas > >> +} >> +#endif > > Gr{oetje,eeting}s, > > Geert >
On Mon, Apr 17, 2023, at 11:03, Thomas Zimmermann wrote: > Am 11.04.23 um 10:08 schrieb Geert Uytterhoeven: >> On Thu, Apr 6, 2023 at 4:30 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >>> +#ifndef fb_pgprotect >>> +#define fb_pgprotect fb_pgprotect >>> +static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma, >>> + unsigned long off) >> >> Does this affect any noMMU platforms that relied on fb_pgprotect() >> doing nothing before? >> Perhaps the body below should be protected by "#ifdef CONFIG_MMU"? > > I cannot conclusively answer this question, but I did some grep'ing > ('git grep ndef | grep CONFIG_MMU'): > > Only the architectures in this patchset provide <asm/fb.h> but nothing > anywhere uses <asm-generic/fb.h> yet. And of those architectures, only > arm and m68k have !CONFIG_MMU cases. Those are handled in the rsp > patches. I think we're good. Agreed. The generic version is just a more elaborate way to do nothing here, as the vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); line on nommu just turns into a self-assignment of the same member that was set the line before. Arnd
diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h index f9f18101ed36..631d97c507ca 100644 --- a/include/asm-generic/fb.h +++ b/include/asm-generic/fb.h @@ -1,13 +1,32 @@ /* 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>. + */ + +#include <asm/page.h> + +struct fb_info; +struct file; + +#ifndef fb_pgprotect +#define fb_pgprotect fb_pgprotect +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); +} +#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. The default mode set by fb_pgprotect() is now writecombine, which is what most platforms want. 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>. v2: * use writecombine mappings by default (Arnd) Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- include/asm-generic/fb.h | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)