linux-kernel - Re: [PATCH]input: Change timer function to workqueue for gpio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1245936693.20530.107.camel@jani-desktop>
Date:	Thu, 25 Jun 2009 16:31:33 +0300
From:	Jani Nikula <ext-jani.1.nikula@...ia.com>
To:	ext Alek Du <alek.du@...el.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Trilok Soni <soni.trilok@...il.com>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	"ben-linux@...ff.org" <ben-linux@...ff.org>
Subject: Re: [PATCH]input: Change timer function to workqueue for gpio_keys
	driver

On Thu, 2009-06-25 at 15:06 +0200, ext Alek Du wrote:
> On Thu, 25 Jun 2009 18:29:25 +0800
> Jani Nikula <ext-jani.1.nikula@...ia.com> wrote:
> 
> > On Fri, Jun 12, 2009 at 8:40 PM, Trilok Soni<soni.trilok@...il.com> wrote:
> > >>  static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > >>  {
> > >>        struct gpio_button_data *bdata = dev_id;
> > >> @@ -62,10 +61,10 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > >>        BUG_ON(irq != gpio_to_irq(button->gpio));
> > >>
> > >>        if (button->debounce_interval)
> > >> -               mod_timer(&bdata->timer,
> > >> -                       jiffies + msecs_to_jiffies(button->debounce_interval));
> > >> +               schedule_delayed_work(&bdata->work,
> > >> +                       msecs_to_jiffies(button->debounce_interval));
> > >>        else
> > >> -               gpio_keys_report_event(bdata);
> > >> +               schedule_work(&bdata->work.work);
> > >>
> > >>        return IRQ_HANDLED;
> > >>  }
> > 
> > Correct me if I'm wrong, but as far as I can tell,
> > schedule_delayed_work doesn't modify the timer if the work was already
> > pending. The result is not the same as with the timer. This breaks the
> > debouncing.
> 
> No. The workqueue is per button, if the work is already pending, then last
> key press is not handled yet. That keeps the debouncing. Why you want the second
> key press to break the first one? The second key press should be ignored, that's
> the meaning of debouncing right?

No, debouncing is supposed to let the gpio line stabilize to either
state before doing *anything*. You only want to schedule the work (and
send the input event) once the line has been in the same state for
debounce_interval ms. This is what the original code did, by kicking the
timer further at each state change.

> > It looks like a slightly modified version of this patch has already
> > been committed [1], but it has the same problem.
> > 
> > [1] 0b346838c5862bfe911432956a106d602535d030 Input: gpio-keys - change
> > timer to workqueue
> 
> Yes, the patch is already in Linus tree.

IMHO it should be either fixed or reverted.


BR,
Jani.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/