Re: [PATCH v2 1/6] hid: new driver for PicoLCD device

On Sun, 21 March 2010 Dmitry Torokhov [off-list ref] wrote:

On Sun, Mar 21, 2010 at 05:37:37PM +0100, Bruno Prémont wrote:

quoted

On Sat, 20 March 2010 Dmitry Torokhov [off-list ref] wrote:

quoted

quoted

+	usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
+	complete_all(&data->ready);
+	INIT_COMPLETION(data->ready);

Umm, what does this do, exactly?

It wakes up anyone waiting on the completion and then resets the completion
as otherwise any future attempt to wait on it would succeed immediately.

You realize that if you re-initialize the completion right after
signalling it there is a big chance the waiters will miss it (they do
check completion->done flags that you reset right away.

In general completions are suited for something that happens once (a
single request - allocated - processed - signalled) but not for
repeating use.

Would below approach be more correct?

- move the completion to struct picolcd_pending
  so it meets the "happens once" requirement

- protect data->pending with a mutex
  (though also use spinlock to prevent race between event which
   signals the completion and picolcd_send_and_wait() around timeout)

- use the data->lock spinlock to protect multi-report requests from
  interleaving

In speudo-code this would be something like:

picolcd_send_and_wait(...)
{
	struct picolcd_pending pending;
	...
	init_completion(&pending->ready);
	aquire_mutex(data->mutex);
	spin_lock_irqsave(&data->lock, flags);
	... // prepare report
	data->pending = &pending;
	usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
	spin_unlock_irqrestore(&data->lock, flags);
	wait_for_completion_interruptible_timeout(&pending->ready, HZ*2);
	spin_lock_irqsave(&data->lock, flags);
	if (data->pending = &pending)
		data->pending = NULL;
	spin_unlock_irqrestore(&data->lock, flags);
	release_mutext(data->mutex);
	...
}

picolcd_raw_event(...)
{
	...
	spin_lock_irqsave(&data->lock, flags);
	if (data->pending) {
		// copy event data to pending
		...
		complete(&pending->ready);
	}
	spin_unlock_irqrestore(&data->lock, flags);
	...
}

In picolcd_remove() and picolcd_reset() I could then do something
similar to picolcd_raw_event() to trigger completion with no
data/error in order to skip timeout on the requester side.

Though how should I prevent races on hot-unplug?

Thanks,
Bruno