| 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
| ||
|
Message-ID: <1356911071.26227.10.camel@gitbox>
Date: Mon, 31 Dec 2012 12:44:31 +1300
From: Tony Prisk <linux@...sktech.co.nz>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
vt8500-wm8505-linux-kernel@...glegroups.com,
linux-input@...r.kernel.org
Subject: Re: [PATCH] input: vt8500: Add power button keypad driver
> > +Example:
> > + powerkey: pwrkey@0 {
> > + compatible = "wm,power-keypad";
> > + interrupts = <22>;
> > + keymap = <116>; /* KEY_POWER */
>
> Do we really need this in DT? I'd say just having it manageable from
> userspace is enough.
Just seemed easier this way. Will be changed.
> > +config KEYBOARD_WMT_POWER_KEYPAD
> > + tristate "Wondermedia Power keypad support"
> > + depends on ARCH_VT8500
>
> I'd feel safer if we added "depends on OF" as well.
ARCH_VT8500 always selects USE_OF, but I can add it as well if you think
its necessary.
> > +
> > +static void __iomem *pmc_base;
> > +static struct input_dev *kpad_power;
> > +static spinlock_t kpad_power_lock;
> > +static int power_button_pressed;
> > +static struct timer_list kpad_power_timer;
> > +static unsigned int kpad_power_code;
>
> Maybe wrap it in a structure?
Will do.
>
> > +
> > +static inline void kpad_power_timeout(unsigned long fcontext)
>
> Why inline? It can't be inlined anyway since its address is used.
>
Umm, no idea why this is inline. Will remove.
> > +{
> > + u32 status;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&kpad_power_lock, flags);
> > +
> > + status = readl(pmc_base + 0x14);
> > +
> > + if (power_button_pressed) {
>
> This check is useless, you won't ever get here if button hasn't been
> pressed.
>
Hmm, correct. Will fix.
> > + status = readl(pmc_base + 0x14);
> > + udelay(100);
> > + writel(status, pmc_base + 0x14);
> > +
> > + if (status & BIT(14)) {
> > + if (!power_button_pressed) {
>
> No need to do this check.
>
The hardware generates multiple interrupts when the button is held.
Without the !power_button_pressed, it will generate multiple pressed
events without releases, because the timer doesn't get to finish.
The interrupt is non-maskable.
> > +static int vt8500_pwr_kpad_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *np;
> > + u32 status;
> > + int err;
> > + int irq;
> > +
> > + np = of_find_compatible_node(NULL, NULL, "via,vt8500-pmc");
> > + if (!np) {
> > + dev_err(&pdev->dev, "pmc node not found\n");
> > + return -EINVAL;
> > + }
> > +
> > + pmc_base = of_iomap(np, 0);
> > + if (!pmc_base) {
> > + dev_err(&pdev->dev, "unable to map pmc memory\n");
> > + return -ENOMEM;
> > + }
> > +
> > + np = pdev->dev.of_node;
> > + if (!np) {
> > + dev_err(&pdev->dev, "devicenode not found\n");
> > + return -ENODEV;
> > + }
> > +
> > + err = of_property_read_u32(np, "keymap", &kpad_power_code);
> > + if (err) {
> > + dev_warn(&pdev->dev, "defaulting to KEY_POWER\n");
> > + kpad_power_code = KEY_POWER;
> > + }
> > +
> > + /* set power button to soft-power */
> > + status = readl(pmc_base + 0x54);
> > + writel(status | 1, pmc_base + 0x54);
> > +
> > + /* clear any pending interrupts */
> > + status = readl(pmc_base + 0x14);
> > + writel(status, pmc_base + 0x14);
> > +
> > + kpad_power = input_allocate_device();
> > + if (!kpad_power) {
> > + dev_err(&pdev->dev, "failed to allocate input device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + spin_lock_init(&kpad_power_lock);
> > + setup_timer(&kpad_power_timer, kpad_power_timeout,
> > + (unsigned long)kpad_power);
> > +
> > + irq = irq_of_parse_and_map(np, 0);
> > + err = request_irq(irq, &kpad_power_isr, 0, "pwrbtn", NULL);
> > + if (err < 0) {
> > + dev_err(&pdev->dev, "failed to request irq\n");
> > + return err;
> > + }
> > +
> > + kpad_power->evbit[0] = BIT_MASK(EV_KEY);
> > + set_bit(kpad_power_code, kpad_power->keybit);
> > +
> > + kpad_power->name = "wmt_power_keypad";
> > + kpad_power->phys = "wmt_power_keypad";
> > + kpad_power->keycode = &kpad_power_code;
> > + kpad_power->keycodesize = sizeof(unsigned int);
> > + kpad_power->keycodemax = 1;
> > +
> > + err = input_register_device(kpad_power);
> > + if (err < 0) {
> > + dev_err(&pdev->dev, "failed to register input device\n");
>
> You either need to use managed resources or clean up after yourself;
> leaking memory and other resources is not nice. Given that you are using
> timer, which needs to be canceled, and I am not sure if your device
> allows enabling/disabling generating interrupts while keeping the
> interrupt handler registered, manual cleanup looks like the only option
> for you.
>
> BTW, where is your remove() method?
Eeek. Too much turkey :) I will tidy this up and resubmit.
Thanks for the quick review.
Regards
Tony P
--
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