| 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: <s5hzl0wxbir.wl%tiwai@suse.de>
Date: Thu, 22 Apr 2010 08:10:52 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Pavel Machek <pavel@....cz>, Richard Purdie <rpurdie@...ys.net>,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] input: Add LED support to Synaptics device
At Wed, 21 Apr 2010 10:23:22 -0700,
Dmitry Torokhov wrote:
>
> On Wed, Apr 21, 2010 at 04:46:39PM +0200, Takashi Iwai wrote:
> > The new Synaptics devices have an LED on the top-left corner.
> > This patch adds a new LED class device to control it. It's created
> > dynamically upon synaptics device probing.
> >
> > The LED is controlled via the command 0x0a with parameters 0x88 or 0x10.
> > This seems only on/off control although other value might be accepted.
> >
> > The detection of the LED isn't clear yet. It should have been the new
> > capability bits that indicate the presence, but on real machines, it
> > doesn't fit. So, for the time being, the driver checks the product id
> > in the ext capability bits and assumes that LED exists on the known
> > devices.
> >
>
> Tkashi,
>
> Does it build if you disable LED subsystem? Note that I don't want
> psmouse to depend on LEDs...
Oh, I missed that dependency, yes.
I'll fix to change to select LEDS on demand.
> > Signed-off-by: Takashi Iwai <tiwai@...e.de>
> > ---
> >
> > Fixed a typo in v2 patch, sorry.
> >
> > drivers/input/mouse/synaptics.c | 88 +++++++++++++++++++++++++++++++++++++++
> > drivers/input/mouse/synaptics.h | 3 +
> > 2 files changed, 91 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index c7b5285..fcc007c 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -28,6 +28,7 @@
> > #include <linux/input.h>
> > #include <linux/serio.h>
> > #include <linux/libps2.h>
> > +#include <linux/leds.h>
> > #include <linux/slab.h>
> > #include "psmouse.h"
> > #include "synaptics.h"
> > @@ -335,6 +336,79 @@ static void synaptics_pt_create(struct psmouse *psmouse)
> > serio_register_port(serio);
> > }
> >
> > +/*
> > + * LED handling:
> > + * Some Synaptics devices have an embeded LED at the top-left corner.
> > + */
> > +
> > +struct synaptics_led {
> > + enum led_brightness status;
> > + struct psmouse *psmouse;
> > + struct work_struct work;
> > + struct led_classdev cdev;
> > +};
> > +
> > +static void synaptics_set_led(struct psmouse *psmouse, int on)
> > +{
> > + unsigned char param[1];
> > +
> > + if (psmouse_sliced_command(psmouse, on ? 0x88 : 0x10))
> > + return;
> > + param[0] = 0x0a;
> > + ps2_command(&psmouse->ps2dev, param, PSMOUSE_CMD_SETRATE);
> > +}
> > +
> > +static void synaptics_led_work(struct work_struct *work)
> > +{
> > + struct synaptics_led *led;
> > +
> > + led = container_of(work, struct synaptics_led, work);
> > + synaptics_set_led(led->psmouse, led->status);
>
> 2+ instances of work may run simultaneously on 2 CPUs; I think you need
> lcokign here. Also you need locking to prevent psmouse core access the
> device (send other commands) when user accesses sysfs attributes.
ps2_command() and co have already mutex in it. I thought this should
suffice. Meanwhile, it'd be better to protect the whole command
sequence at once. I'll change to call ps2_begin_command/end_command()
explicitly.
> Also,
> doesn't the device have to be in command mode before you start sending
> commands to it?
Looks not.
> > +}
> > +
> > +static void synaptics_led_cdev_brightness_set(struct led_classdev *cdev,
> > + enum led_brightness value)
> > +{
> > + struct synaptics_led *led;
> > +
> > + led = container_of(cdev, struct synaptics_led, cdev);
> > + if (value != led->status) {
> > + led->status = value;
> > + schedule_work(&led->work);
> > + }
> > +}
> > +
> > +static int synaptics_init_led(struct psmouse *psmouse)
> > +{
> > + struct synaptics_data *priv = psmouse->private;
> > + struct synaptics_led *led;
> > + int err;
> > +
> > + /* FIXME: LED is supposedly detectable in cap0c[1] 0x20, but it seems
> > + * not working on real machines.
> > + * So we check the product id to be sure.
> > + */
> > + if (!priv->ext_cap_0c || SYN_CAP_PRODUCT_ID(priv->ext_cap) != 0xe4)
> > + return 0;
> > +
> > + printk(KERN_INFO "synaptics: support LED control\n");
> > + led = kzalloc(sizeof(struct synaptics_led), GFP_KERNEL);
> > + if (!led)
> > + return -ENOMEM;
>
> I do not think synaptics_led structure uses that much space to be
> allocated separately and conditionally; why don't you just put lde, work
> and value directly into synaptics_data?
Because I didn't want to pollute synaptics.h by dependency of led
stuff, wanted to make synaptics.c rather self-contained. It'll be
ifdef'ed, and it's not beautiful to see much in the header that is
referred in other file.
thanks,
Takashi
--
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