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
| ||
|
Date: Thu, 25 Jun 2009 22:08:26 +0800 From: Alek Du <alek.du@...el.com> To: Jani Nikula <ext-jani.1.nikula@...ia.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, 25 Jun 2009 21:31:33 +0800 Jani Nikula <ext-jani.1.nikula@...ia.com> wrote: > 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. > If you schedule the timer when you decide it "stabilized", the final gpio_get_value() could still return 0 in the timer handler, if the key released at that time. So your previous "stabilized" state is useless. Isn't the delay work itself the mechanism to decide the "stabilized" ? The work will finally call gpio_get_value to determine the state to be sent to input layer. I don't think there is any defect here. > IMHO it should be either fixed or reverted. > No, the original timer handler will crash kernel if you are using a I2C GPIO or SPI GPIO expander Since it try to call sleep-able gpio_get_value in atomic context. -- 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/
Powered by blists - more mailing lists