snd_mixart_send_msg / snd_mixart_send_msg_wait_notif

snd_mixart_send_msg / snd_mixart_send_msg_wait_notif

[Rene Herman]

Hi.

While looking through ALSA for schedule_timeout() calls, I ran into:

int snd_mixart_send_msg(...)
{

	[ ... ]

         set_current_state(TASK_UNINTERRUPTIBLE);
         add_wait_queue(&mgr->msg_sleep, &wait);
         spin_unlock_irq(&mgr->msg_lock);
         timeout = schedule_timeout(MSG_TIMEOUT_JIFFIES);
         remove_wait_queue(&mgr->msg_sleep, &wait);

         if (! timeout) {
                 /* error - no ack */
                 mutex_unlock(&mgr->msg_mutex);
                 snd_printk(KERN_ERR "error: no reponse on msg %x\n", 
msg_frame);
                 return -EIO;
         }

	[ ... ]

}

and the same in snd_mixart_send_msg_wait_notif().

I believe there to be  something wrong with this code. A schedule_timeout() 
in TASK_UNINTERRUPTIBLE is always going to return 0. Didn't you intend to 
use a wait_event_timeout() or something like that? Puzzled, since you err 
out on !timeout, and it seems every run through this would end up there.

Rene.

Re: snd_mixart_send_msg / snd_mixart_send_msg_wait_notif

[Rene Herman]

On 09/18/2007 01:54 PM, Rene Herman wrote:

People just _make_ me reply to myself:

  alsa@digigram.com
     SMTP error from remote mailer after RCPT TO:<alsa@digigram.com>:
     host mail.digigram.com [213.30.172.230]: 550 5.1.1 <alsa@digigram.com>:
     Recipient address rejected: User unknown in relay recipient table

> While looking through ALSA for schedule_timeout() calls, I ran into:
> 
> int snd_mixart_send_msg(...)
> {
> 
>     [ ... ]
> 
>         set_current_state(TASK_UNINTERRUPTIBLE);
>         add_wait_queue(&mgr->msg_sleep, &wait);
>         spin_unlock_irq(&mgr->msg_lock);
>         timeout = schedule_timeout(MSG_TIMEOUT_JIFFIES);
>         remove_wait_queue(&mgr->msg_sleep, &wait);
> 
>         if (! timeout) {
>                 /* error - no ack */
>                 mutex_unlock(&mgr->msg_mutex);
>                 snd_printk(KERN_ERR "error: no reponse on msg %x\n", 
> msg_frame);
>                 return -EIO;
>         }
> 
>     [ ... ]
> 
> }
> 
> and the same in snd_mixart_send_msg_wait_notif().
> 
> I believe there to be  something wrong with this code. A 
> schedule_timeout() in TASK_UNINTERRUPTIBLE is always going to return 0. 
> Didn't you intend to use a wait_event_timeout() or something like that? 
> Puzzled, since you err out on !timeout, and it seems every run through 
> this would end up there.

Rene.

Re: snd_mixart_send_msg / snd_mixart_send_msg_wait_notif

[Takashi Iwai]

At Tue, 18 Sep 2007 13:54:23 +0200,
Rene Herman wrote:
> 
> Hi.
> 
> While looking through ALSA for schedule_timeout() calls, I ran into:
> 
> int snd_mixart_send_msg(...)
> {
> 
> 	[ ... ]
> 
>          set_current_state(TASK_UNINTERRUPTIBLE);
>          add_wait_queue(&mgr->msg_sleep, &wait);
>          spin_unlock_irq(&mgr->msg_lock);
>          timeout = schedule_timeout(MSG_TIMEOUT_JIFFIES);
>          remove_wait_queue(&mgr->msg_sleep, &wait);
> 
>          if (! timeout) {
>                  /* error - no ack */
>                  mutex_unlock(&mgr->msg_mutex);
>                  snd_printk(KERN_ERR "error: no reponse on msg %x\n", 
> msg_frame);
>                  return -EIO;
>          }
> 
> 	[ ... ]
> 
> }
> 
> and the same in snd_mixart_send_msg_wait_notif().
> 
> I believe there to be  something wrong with this code. A schedule_timeout() 
> in TASK_UNINTERRUPTIBLE is always going to return 0.

No, it returns the time left at the point it's woken up.
*_uninterruptible() ignores signals but not wake_up() itself
(otherwise it makes no sense).  The description in the kernel comment
is misleading indeed.


Takashi

Re: snd_mixart_send_msg / snd_mixart_send_msg_wait_notif

[Rene Herman]

On 09/18/2007 02:40 PM, Takashi Iwai wrote:

> At Tue, 18 Sep 2007 13:54:23 +0200,
> Rene Herman wrote:
>> Hi.
>>
>> While looking through ALSA for schedule_timeout() calls, I ran into:
>>
>> int snd_mixart_send_msg(...)
>> {
>>
>> 	[ ... ]
>>
>>          set_current_state(TASK_UNINTERRUPTIBLE);
>>          add_wait_queue(&mgr->msg_sleep, &wait);
>>          spin_unlock_irq(&mgr->msg_lock);
>>          timeout = schedule_timeout(MSG_TIMEOUT_JIFFIES);
>>          remove_wait_queue(&mgr->msg_sleep, &wait);
>>
>>          if (! timeout) {
>>                  /* error - no ack */
>>                  mutex_unlock(&mgr->msg_mutex);
>>                  snd_printk(KERN_ERR "error: no reponse on msg %x\n", 
>> msg_frame);
>>                  return -EIO;
>>          }
>>
>> 	[ ... ]
>>
>> }
>>
>> and the same in snd_mixart_send_msg_wait_notif().
>>
>> I believe there to be  something wrong with this code. A schedule_timeout() 
>> in TASK_UNINTERRUPTIBLE is always going to return 0.
> 
> No, it returns the time left at the point it's woken up.
> *_uninterruptible() ignores signals but not wake_up() itself
> (otherwise it makes no sense).  The description in the kernel comment
> is misleading indeed.

And the wake_up() is done from interrupt here -- yes, okay, I understand 
now. Thanks.

(send email to digigram asking what happened to that alsa@ address by the way).

Rene.