[bug report] Input: atkbd - restore repeat rate when resuming

[bug report] Input: atkbd - restore repeat rate when resuming

[Dan Carpenter]

[ This bug seems really ancient.  But so far as I can see it really is
  super old.  I'm not sure why it's only showing up now.  -dan ]

Hello Dmitry Torokhov,

Commit 3d0f0fa0cb55 ("Input: atkbd - restore repeat rate when
resuming") from Aug 4, 2006 (linux-next), leads to the following
Smatch static checker warning:

	drivers/input/keyboard/atkbd.c:604 atkbd_set_repeat_rate()
	warn: sleeping in atomic context

drivers/input/keyboard/atkbd.c
    583 static int atkbd_set_repeat_rate(struct atkbd *atkbd)
    584 {
    585         const short period[32] =
    586                 { 33,  37,  42,  46,  50,  54,  58,  63,  67,  75,  83,  92, 100, 109, 116, 125,
    587                  133, 149, 167, 182, 200, 217, 232, 250, 270, 303, 333, 370, 400, 435, 470, 500 };
    588         const short delay[4] =
    589                 { 250, 500, 750, 1000 };
    590 
    591         struct input_dev *dev = atkbd->dev;
    592         unsigned char param;
    593         int i = 0, j = 0;
    594 
    595         while (i < ARRAY_SIZE(period) - 1 && period[i] < dev->rep[REP_PERIOD])
    596                 i++;
    597         dev->rep[REP_PERIOD] = period[i];
    598 
    599         while (j < ARRAY_SIZE(delay) - 1 && delay[j] < dev->rep[REP_DELAY])
    600                 j++;
    601         dev->rep[REP_DELAY] = delay[j];
    602 
    603         param = i | (j << 5);
--> 604         return ps2_command(&atkbd->ps2dev, &param, ATKBD_CMD_SETREP);

This fucntion call takes a mutex.

    605 }

The call tree is:

atkbd_reconnect() <- disables preempt
-> atkbd_set_repeat_rate()

In atkbd_reconnect() it's the atkbd_disable(atkbd) which takes a
spinlock.

regards,
dan carpenter

Re: [bug report] Input: atkbd - restore repeat rate when resuming

[Dmitry Torokhov]

Hi Dan,

On Mon, Mar 03, 2025 at 11:11:35AM +0300, Dan Carpenter wrote:
> [ This bug seems really ancient.  But so far as I can see it really is
>   super old.  I'm not sure why it's only showing up now.  -dan ]
> 
> Hello Dmitry Torokhov,
> 
> Commit 3d0f0fa0cb55 ("Input: atkbd - restore repeat rate when
> resuming") from Aug 4, 2006 (linux-next), leads to the following
> Smatch static checker warning:
> 
> 	drivers/input/keyboard/atkbd.c:604 atkbd_set_repeat_rate()
> 	warn: sleeping in atomic context
> 
> drivers/input/keyboard/atkbd.c
>     583 static int atkbd_set_repeat_rate(struct atkbd *atkbd)
>     584 {
>     585         const short period[32] =
>     586                 { 33,  37,  42,  46,  50,  54,  58,  63,  67,  75,  83,  92, 100, 109, 116, 125,
>     587                  133, 149, 167, 182, 200, 217, 232, 250, 270, 303, 333, 370, 400, 435, 470, 500 };
>     588         const short delay[4] =
>     589                 { 250, 500, 750, 1000 };
>     590 
>     591         struct input_dev *dev = atkbd->dev;
>     592         unsigned char param;
>     593         int i = 0, j = 0;
>     594 
>     595         while (i < ARRAY_SIZE(period) - 1 && period[i] < dev->rep[REP_PERIOD])
>     596                 i++;
>     597         dev->rep[REP_PERIOD] = period[i];
>     598 
>     599         while (j < ARRAY_SIZE(delay) - 1 && delay[j] < dev->rep[REP_DELAY])
>     600                 j++;
>     601         dev->rep[REP_DELAY] = delay[j];
>     602 
>     603         param = i | (j << 5);
> --> 604         return ps2_command(&atkbd->ps2dev, &param, ATKBD_CMD_SETREP);
> 
> This fucntion call takes a mutex.
> 
>     605 }
> 
> The call tree is:
> 
> atkbd_reconnect() <- disables preempt
> -> atkbd_set_repeat_rate()
> 
> In atkbd_reconnect() it's the atkbd_disable(atkbd) which takes a
> spinlock.

I think your tools are confused by the guard notation. atkbd_disable()
not only takes but also releases the spinlock.

Thanks.

-- 
Dmitry

Re: [bug report] Input: atkbd - restore repeat rate when resuming

[Dan Carpenter]

On Tue, Mar 04, 2025 at 11:09:35PM -0800, Dmitry Torokhov wrote:
> Hi Dan,
> 
> On Mon, Mar 03, 2025 at 11:11:35AM +0300, Dan Carpenter wrote:
> > [ This bug seems really ancient.  But so far as I can see it really is
> >   super old.  I'm not sure why it's only showing up now.  -dan ]
> > 
> > Hello Dmitry Torokhov,
> > 
> > Commit 3d0f0fa0cb55 ("Input: atkbd - restore repeat rate when
> > resuming") from Aug 4, 2006 (linux-next), leads to the following
> > Smatch static checker warning:
> > 
> > 	drivers/input/keyboard/atkbd.c:604 atkbd_set_repeat_rate()
> > 	warn: sleeping in atomic context
> > 
> > drivers/input/keyboard/atkbd.c
> >     583 static int atkbd_set_repeat_rate(struct atkbd *atkbd)
> >     584 {
> >     585         const short period[32] =
> >     586                 { 33,  37,  42,  46,  50,  54,  58,  63,  67,  75,  83,  92, 100, 109, 116, 125,
> >     587                  133, 149, 167, 182, 200, 217, 232, 250, 270, 303, 333, 370, 400, 435, 470, 500 };
> >     588         const short delay[4] =
> >     589                 { 250, 500, 750, 1000 };
> >     590 
> >     591         struct input_dev *dev = atkbd->dev;
> >     592         unsigned char param;
> >     593         int i = 0, j = 0;
> >     594 
> >     595         while (i < ARRAY_SIZE(period) - 1 && period[i] < dev->rep[REP_PERIOD])
> >     596                 i++;
> >     597         dev->rep[REP_PERIOD] = period[i];
> >     598 
> >     599         while (j < ARRAY_SIZE(delay) - 1 && delay[j] < dev->rep[REP_DELAY])
> >     600                 j++;
> >     601         dev->rep[REP_DELAY] = delay[j];
> >     602 
> >     603         param = i | (j << 5);
> > --> 604         return ps2_command(&atkbd->ps2dev, &param, ATKBD_CMD_SETREP);
> > 
> > This fucntion call takes a mutex.
> > 
> >     605 }
> > 
> > The call tree is:
> > 
> > atkbd_reconnect() <- disables preempt
> > -> atkbd_set_repeat_rate()
> > 
> > In atkbd_reconnect() it's the atkbd_disable(atkbd) which takes a
> > spinlock.
> 
> I think your tools are confused by the guard notation. atkbd_disable()
> not only takes but also releases the spinlock.

Argh...  Yeah.  Sorry.  I'll fix that.  I guess the problem is that
Smatch is confused by the if statement in class_serio_pause_rx_destructor()

regards,
dan carpenter

From b045578c121afaba0522b26a617b45e8c2307da8 Mon Sep 17 00:00:00 2001
From: Dan Carpenter <dan.carpenter@linaro.org>
Date: Wed, 5 Mar 2025 11:47:37 +0300
Subject: [PATCH] preempt_info: add class_serio_pause_rx_destructor()

The class_serio_pause_rx_destructor() function enables preemption again.
This silences some false positives in atkbd.c.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 check_preempt_info.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/check_preempt_info.c b/check_preempt_info.c
index 8fabd64cff24..2f5ab7648951 100644
--- a/check_preempt_info.c
+++ b/check_preempt_info.c
@@ -129,6 +129,7 @@ static struct preempt_info func_table[] = {
 	{ "rht_unlock",				PREEMPT_SUB },
 	{ "bit_spin_unlock",			PREEMPT_SUB },
 	{ "__bit_spin_unlock",			PREEMPT_SUB },
+	{ "class_serio_pause_rx_destructor",	PREEMPT_SUB },
 
 	{ "mtk_wed_device_attach",		PREEMPT_IGNORE },
 };
-- 
2.47.2