On Mon, 2010-02-22 at 19:38 +0100, ext Dmitry Torokhov wrote:

On Mon, Feb 22, 2010 at 03:19:36PM +0200, Jari Vanhala wrote:

quoted

On Mon, 2010-02-22 at 07:40 +0100, ext Dmitry Torokhov wrote:

quoted

quoted

+struct vibra_info {
+	struct device		*dev;
+	struct input_dev	*input_dev;
+
+	struct workqueue_struct *workqueue;

Why do we need a separate workqueue? Can't keventd serve us?

There were problems with too long delays once in a while, own workqueue
made things much better. Also, you can change priority if needed.

OK, in this case you shoudl consider starting it only when input device
is opened and shutting it down when it is closed.

Hum.. Delayed workqueue creating/destroying makes driver much more
complex. I create separate patch for it.

quoted

quoted

quoted

+static int vibra_play(struct input_dev *dev, void *data,
+		      struct ff_effect *effect)
+{
+	struct vibra_info *info = data;
+
+	if (!info->workqueue)
+		return 0;

How can workqueue be missig here?

It's missing when we remove driver. ff-memless wants to stop vibra (if
it was running) and destroys info. And also we really don't want to
start worker anymore.

I would expect the code to make sure workqueue is present while there
are any outstanding playback requests. Otherwise you need more locking
(what stops workqueue from being deleted right after you checked it?)

I think locking is kind of overkill. Vibra_play is run in atomic-context
and.. but I can add spinlock to protect work.

	++Jam