Message ID | 20210220230248.320870-1-ztong0001@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | video: fbdev: pm2fb: avoid stall on fb_sync | expand |
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, 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
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(-)