| Message ID | 20210220230248.320870-1-ztong0001@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | video: fbdev: pm2fb: avoid stall on fb_sync | expand |
Hi-- On 2/20/21 3:02 PM, Tong Zhang wrote: > pm2fb_sync is called when doing /dev/fb read or write. > The original pm2fb_sync wait indefinitely on hardware flags which can > possibly stall kernel and make everything unresponsive. > Instead of waiting indefinitely, we can timeout to give user a chance to > get back control. Is this a real problem or theoretical? Does someone still use this driver? > Signed-off-by: Tong Zhang <ztong0001@gmail.com> > --- > drivers/video/fbdev/pm2fb.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/fbdev/pm2fb.c b/drivers/video/fbdev/pm2fb.c > index 27893fa139b0..8578c64a0c54 100644 > --- a/drivers/video/fbdev/pm2fb.c > +++ b/drivers/video/fbdev/pm2fb.c > @@ -183,12 +183,23 @@ static inline void pm2v_RDAC_WR(struct pm2fb_par *p, s32 idx, u32 v) > > #ifdef CONFIG_FB_PM2_FIFO_DISCONNECT > #define WAIT_FIFO(p, a) > +#define WAIT_FIFO_TIMEOUT(p, a) (0) > #else > static inline void WAIT_FIFO(struct pm2fb_par *p, u32 a) > { > while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a) > cpu_relax(); > } > +static int inline void WAIT_FIFO_TIMEOUT(struct pm2fb_par *p, u32 a) drop void ^^^ It's already "int". Did you compile this? > +{ > + int timeout = 10000; > + while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a) { > + cpu_relax(); > + if (--timeout==0) spaces around == > + return 1; > + } > + return 0; > +} > #endif > > /* > @@ -1031,15 +1042,27 @@ static int pm2fb_blank(int blank_mode, struct fb_info *info) > static int pm2fb_sync(struct fb_info *info) > { > struct pm2fb_par *par = info->par; > + int timeout_sync = 10000; > + int timeout_fifo; > > - WAIT_FIFO(par, 1); > + if (WAIT_FIFO_TIMEOUT(par, 1)) > + goto end; > pm2_WR(par, PM2R_SYNC, 0); > mb(); > do { > - while (pm2_RD(par, PM2R_OUT_FIFO_WORDS) == 0) > + timeout_fifo = 10000; > + while (pm2_RD(par, PM2R_OUT_FIFO_WORDS) == 0) { > cpu_relax(); > - } while (pm2_RD(par, PM2R_OUT_FIFO) != PM2TAG(PM2R_SYNC)); > + if (--timeout_fifo==0) spaces around == > + goto end; > + } > + if (pm2_RD(par, PM2R_OUT_FIFO) == PM2TAG(PM2R_SYNC)) > + break; > + } while (--timeout_sync>0); spaces around > > > +end: > + if ((!timeout_sync) || (!timeout_fifo)) > + printk_ratelimited(KERN_WARNING "pm2fb: sync timeout!\n"); > return 0; > } > > thanks.
Hi Randy, Thanks for the comment. I currently have this problem on my machine. I have submitted a revised patch -- which includes the console log. Thanks! - Tong On Sat, Feb 20, 2021 at 6:33 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > Hi-- > > On 2/20/21 3:02 PM, Tong Zhang wrote: > > pm2fb_sync is called when doing /dev/fb read or write. > > The original pm2fb_sync wait indefinitely on hardware flags which can > > possibly stall kernel and make everything unresponsive. > > Instead of waiting indefinitely, we can timeout to give user a chance to > > get back control. > > Is this a real problem or theoretical? > Does someone still use this driver? > > > > Signed-off-by: Tong Zhang <ztong0001@gmail.com> > > --- > > drivers/video/fbdev/pm2fb.c | 29 ++++++++++++++++++++++++++--- > > 1 file changed, 26 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/video/fbdev/pm2fb.c b/drivers/video/fbdev/pm2fb.c > > index 27893fa139b0..8578c64a0c54 100644 > > --- a/drivers/video/fbdev/pm2fb.c > > +++ b/drivers/video/fbdev/pm2fb.c > > @@ -183,12 +183,23 @@ static inline void pm2v_RDAC_WR(struct pm2fb_par *p, s32 idx, u32 v) > > > > #ifdef CONFIG_FB_PM2_FIFO_DISCONNECT > > #define WAIT_FIFO(p, a) > > +#define WAIT_FIFO_TIMEOUT(p, a) (0) > > #else > > static inline void WAIT_FIFO(struct pm2fb_par *p, u32 a) > > { > > while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a) > > cpu_relax(); > > } > > +static int inline void WAIT_FIFO_TIMEOUT(struct pm2fb_par *p, u32 a) > > drop void ^^^ > It's already "int". > Did you compile this? > > > +{ > > + int timeout = 10000; > > + while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a) { > > + cpu_relax(); > > + if (--timeout==0) > > spaces around == > > > + return 1; > > + } > > + return 0; > > +} > > #endif > > > > /* > > @@ -1031,15 +1042,27 @@ static int pm2fb_blank(int blank_mode, struct fb_info *info) > > static int pm2fb_sync(struct fb_info *info) > > { > > struct pm2fb_par *par = info->par; > > + int timeout_sync = 10000; > > + int timeout_fifo; > > > > - WAIT_FIFO(par, 1); > > + if (WAIT_FIFO_TIMEOUT(par, 1)) > > + goto end; > > pm2_WR(par, PM2R_SYNC, 0); > > mb(); > > do { > > - while (pm2_RD(par, PM2R_OUT_FIFO_WORDS) == 0) > > + timeout_fifo = 10000; > > + while (pm2_RD(par, PM2R_OUT_FIFO_WORDS) == 0) { > > cpu_relax(); > > - } while (pm2_RD(par, PM2R_OUT_FIFO) != PM2TAG(PM2R_SYNC)); > > + if (--timeout_fifo==0) > > spaces around == > > > + goto end; > > + } > > + if (pm2_RD(par, PM2R_OUT_FIFO) == PM2TAG(PM2R_SYNC)) > > + break; > > + } while (--timeout_sync>0); > > spaces around > > > > > > +end: > > + if ((!timeout_sync) || (!timeout_fifo)) > > + printk_ratelimited(KERN_WARNING "pm2fb: sync timeout!\n"); > > return 0; > > } > > > > > > > thanks. > -- > ~Randy
Hi Tong,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.11 next-20210219]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Tong-Zhang/video-fbdev-pm2fb-avoid-stall-on-fb_sync/20210221-070421
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
config: i386-randconfig-s002-20210221 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-229-g60c1f270-dirty
# https://github.com/0day-ci/linux/commit/77b85e0ba17f78b0335bf283901691ec3942dec3
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tong-Zhang/video-fbdev-pm2fb-avoid-stall-on-fb_sync/20210221-070421
git checkout 77b85e0ba17f78b0335bf283901691ec3942dec3
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
>> drivers/video/fbdev/pm2fb.c:193:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
193 | static int inline void WAIT_FIFO_TIMEOUT(struct pm2fb_par *p, u32 a)
| ^~~~~~
>> drivers/video/fbdev/pm2fb.c:193:19: error: two or more data types in declaration specifiers
193 | static int inline void WAIT_FIFO_TIMEOUT(struct pm2fb_par *p, u32 a)
| ^~~~
vim +193 drivers/video/fbdev/pm2fb.c
183
184 #ifdef CONFIG_FB_PM2_FIFO_DISCONNECT
185 #define WAIT_FIFO(p, a)
186 #define WAIT_FIFO_TIMEOUT(p, a) (0)
187 #else
188 static inline void WAIT_FIFO(struct pm2fb_par *p, u32 a)
189 {
190 while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a)
191 cpu_relax();
192 }
> 193 static int inline void WAIT_FIFO_TIMEOUT(struct pm2fb_par *p, u32 a)
194 {
195 int timeout = 10000;
196 while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a) {
197 cpu_relax();
198 if (--timeout==0)
199 return 1;
200 }
201 return 0;
202 }
203 #endif
204
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Tong,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.11 next-20210219]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Tong-Zhang/video-fbdev-pm2fb-avoid-stall-on-fb_sync/20210221-070421
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
config: x86_64-randconfig-a003-20210221 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/77b85e0ba17f78b0335bf283901691ec3942dec3
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tong-Zhang/video-fbdev-pm2fb-avoid-stall-on-fb_sync/20210221-070421
git checkout 77b85e0ba17f78b0335bf283901691ec3942dec3
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/video/fbdev/pm2fb.c:193:19: error: cannot combine with previous 'int' declaration specifier
static int inline void WAIT_FIFO_TIMEOUT(struct pm2fb_par *p, u32 a)
^
1 error generated.
vim +/int +193 drivers/video/fbdev/pm2fb.c
183
184 #ifdef CONFIG_FB_PM2_FIFO_DISCONNECT
185 #define WAIT_FIFO(p, a)
186 #define WAIT_FIFO_TIMEOUT(p, a) (0)
187 #else
188 static inline void WAIT_FIFO(struct pm2fb_par *p, u32 a)
189 {
190 while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a)
191 cpu_relax();
192 }
> 193 static int inline void WAIT_FIFO_TIMEOUT(struct pm2fb_par *p, u32 a)
194 {
195 int timeout = 10000;
196 while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a) {
197 cpu_relax();
198 if (--timeout==0)
199 return 1;
200 }
201 return 0;
202 }
203 #endif
204
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Tong, On Sun, Feb 21, 2021 at 1:05 AM Tong Zhang <ztong0001@gmail.com> wrote: > On Sat, Feb 20, 2021 at 6:33 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > On 2/20/21 3:02 PM, Tong Zhang wrote: > > > pm2fb_sync is called when doing /dev/fb read or write. > > > The original pm2fb_sync wait indefinitely on hardware flags which can > > > possibly stall kernel and make everything unresponsive. > > > Instead of waiting indefinitely, we can timeout to give user a chance to > > > get back control. > > > > Is this a real problem or theoretical? > > Does someone still use this driver? > > I currently have this problem on my machine. > I have submitted a revised patch -- which includes the console log. Your machine is "QEMU Standard"? Can this happen on real hardware, too, or is this a deficiency in QEMU, which should be fixed there? Gr{oetje,eeting}s, Geert
Hi Geert, IMHO - QEMU is irrelevant here. since I can do passthrough -- in fact -- many drivers do use timeout in .fb_sync e.g. i810fb_sync(), nouveau_fbcon_sync(), sm501fb_sync() etc.. I believe the correct behaviour should be a timeout wait instead of waiting indefinitely. - Tong On Wed, Feb 24, 2021 at 6:35 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Tong, > > On Sun, Feb 21, 2021 at 1:05 AM Tong Zhang <ztong0001@gmail.com> wrote: > > On Sat, Feb 20, 2021 at 6:33 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > > On 2/20/21 3:02 PM, Tong Zhang wrote: > > > > pm2fb_sync is called when doing /dev/fb read or write. > > > > The original pm2fb_sync wait indefinitely on hardware flags which can > > > > possibly stall kernel and make everything unresponsive. > > > > Instead of waiting indefinitely, we can timeout to give user a chance to > > > > get back control. > > > > > > Is this a real problem or theoretical? > > > Does someone still use this driver? > > > > I currently have this problem on my machine. > > I have submitted a revised patch -- which includes the console log. > > Your machine is "QEMU Standard"? > Can this happen on real hardware, too, or is this a deficiency in QEMU, > which should be fixed there? > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
diff --git a/drivers/video/fbdev/pm2fb.c b/drivers/video/fbdev/pm2fb.c index 27893fa139b0..8578c64a0c54 100644 --- a/drivers/video/fbdev/pm2fb.c +++ b/drivers/video/fbdev/pm2fb.c @@ -183,12 +183,23 @@ static inline void pm2v_RDAC_WR(struct pm2fb_par *p, s32 idx, u32 v) #ifdef CONFIG_FB_PM2_FIFO_DISCONNECT #define WAIT_FIFO(p, a) +#define WAIT_FIFO_TIMEOUT(p, a) (0) #else static inline void WAIT_FIFO(struct pm2fb_par *p, u32 a) { while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a) cpu_relax(); } +static int inline void WAIT_FIFO_TIMEOUT(struct pm2fb_par *p, u32 a) +{ + int timeout = 10000; + while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a) { + cpu_relax(); + if (--timeout==0) + return 1; + } + return 0; +} #endif /* @@ -1031,15 +1042,27 @@ static int pm2fb_blank(int blank_mode, struct fb_info *info) static int pm2fb_sync(struct fb_info *info) { struct pm2fb_par *par = info->par; + int timeout_sync = 10000; + int timeout_fifo; - WAIT_FIFO(par, 1); + if (WAIT_FIFO_TIMEOUT(par, 1)) + goto end; pm2_WR(par, PM2R_SYNC, 0); mb(); do { - while (pm2_RD(par, PM2R_OUT_FIFO_WORDS) == 0) + timeout_fifo = 10000; + while (pm2_RD(par, PM2R_OUT_FIFO_WORDS) == 0) { cpu_relax(); - } while (pm2_RD(par, PM2R_OUT_FIFO) != PM2TAG(PM2R_SYNC)); + if (--timeout_fifo==0) + goto end; + } + if (pm2_RD(par, PM2R_OUT_FIFO) == PM2TAG(PM2R_SYNC)) + break; + } while (--timeout_sync>0); +end: + if ((!timeout_sync) || (!timeout_fifo)) + printk_ratelimited(KERN_WARNING "pm2fb: sync timeout!\n"); return 0; }
pm2fb_sync is called when doing /dev/fb read or write. The original pm2fb_sync wait indefinitely on hardware flags which can possibly stall kernel and make everything unresponsive. Instead of waiting indefinitely, we can timeout to give user a chance to get back control. Signed-off-by: Tong Zhang <ztong0001@gmail.com> --- drivers/video/fbdev/pm2fb.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)