schedule inside spin_lock_irqsave?

PATCH In review Richard Zidlicky 3 participants 8 messages May 30, 2010 – Jul 6, 2010 Media (V4L/DVB)

Share & tools

Jiri Slaby

2010-05-30 15:24 UTC #1 lore raw

Quoted 22 lines · Richard Zidlicky

Hi,
came across following snippet of code (2.6.34:drivers/media/dvb/siano/smscoreapi.c) and since prepare_to_wait is new for me I am wondering if this is can work?
struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
{
	struct smscore_buffer_t *cb = NULL;
	unsigned long flags;

	DEFINE_WAIT(wait);

	spin_lock_irqsave(&coredev->bufferslock, flags);

	/* This function must return a valid buffer, since the buffer list is
	 * finite, we check that there is an available buffer, if not, we wait
	 * until such buffer become available.
	 */

	prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);

	if (list_empty(&coredev->buffers))

Quoted 8 lines

		schedule();

	finish_wait(&coredev->buffer_mng_waitq, &wait);

	cb = (struct smscore_buffer_t *) coredev->buffers.next;
	list_del(&cb->entry);

	spin_unlock_irqrestore(&coredev->bufferslock, flags);

Yep, that's a double bug.
1) If the waiting is interrupted, it will die because the list is still empty.
2) If there is no entry in the list, it will deadlock at least on UP. This should be
wait_event(&coredev->buffer_mng_waitq, cb = get_entry()); with get_entry like:
struct smscore_buffer_t *get_entry(void)
{
struct smscore_buffer_t *cb = NULL; spin_lock_irqsave(&coredev->bufferslock, flags);

  if (!list_empty(&coredev->buffers)) {
    cb = (struct smscore_buffer_t *) coredev->buffers.next;
    list_del(&cb->entry);

} spin_unlock_irqrestore(&coredev->bufferslock, flags); return cb;
}

regards,

Signature · 1 line

js

Jiri Slaby

2010-05-30 15:32 UTC #2 lore raw

Quoted 6 lines · Jiri Slaby

struct smscore_buffer_t *get_entry(void)
{
struct smscore_buffer_t *cb = NULL; spin_lock_irqsave(&coredev->bufferslock, flags);
  if (!list_empty(&coredev->buffers)) {
    cb = (struct smscore_buffer_t *) coredev->buffers.next;

Looking at the smscore_buffer_t definition, this is really ugly since it relies on entry being the first in the structure. It should be list_first_entry(&coredev->buffers, ...) instead, cast-less.

Quoted 5 lines

    list_del(&cb->entry);
} spin_unlock_irqrestore(&coredev->bufferslock, flags); return cb;
}

Signature · 1 line

js

Richard Zidlicky

author

2010-06-01 17:13 UTC #3 lore raw

On Sun, May 30, 2010 at 05:24:38PM +0200, Jiri Slaby wrote:

Hi,

Quoted 1 line

Quoted 1 line
came across following snippet of code (2.6.34:drivers/media/dvb/siano/smscoreapi.c) and

...
...
...

Quoted 14 lines

This should be wait_event(&coredev->buffer_mng_waitq, cb = get_entry()); with get_entry like:
struct smscore_buffer_t *get_entry(void)
{
struct smscore_buffer_t *cb = NULL; spin_lock_irqsave(&coredev->bufferslock, flags);
  if (!list_empty(&coredev->buffers)) {
    cb = (struct smscore_buffer_t *) coredev->buffers.next;
    list_del(&cb->entry);
} spin_unlock_irqrestore(&coredev->bufferslock, flags); return cb;
}

...
...
...

Quoted 10 lines

Looking at the smscore_buffer_t definition, this is really ugly since it relies on entry being the first in the structure. It should be list_first_entry(&coredev->buffers, ...) instead, cast-less.
Quoted 5 lines
    list_del(&cb->entry);
} spin_unlock_irqrestore(&coredev->bufferslock, flags); return cb;
}

thanks for the suggestions, is it on anyones TODO list or should I cook up my own patch? Would take a few more days till I can get at it.

BTW does anyone have knowledge how to enable IR receiver code for the smsxxx devices? Looks like its just the matter of setting sms_board_gpio_cfg.ir to the "right" value
- which value?