Re: [PATCH] video: fbdev: pm2fb: avoid stall on fb_sync

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 [off-list ref] wrote:

Hi--

On 2/20/21 3:02 PM, Tong Zhang wrote:

quoted

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?

quoted

Signed-off-by: Tong Zhang <redacted>
---
 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?

quoted

+{
+     int timeout = 10000;
+     while (pm2_RD(p, PM2R_IN_FIFO_SPACE) < a) {
+             cpu_relax();
+             if (--timeout==0)

spaces around ==

quoted

+                     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 ==

quoted

+                             goto end;
+             }
+             if (pm2_RD(par, PM2R_OUT_FIFO) == PM2TAG(PM2R_SYNC))
+                     break;
+     } while (--timeout_sync>0);

spaces around >

quoted

+end:
+     if ((!timeout_sync) || (!timeout_fifo))
+             printk_ratelimited(KERN_WARNING "pm2fb: sync timeout!\n");
      return 0;
 }

thanks.
--
~Randy