Re: [bug report] ALSA: firewire-motu: notify event for parameter change in register DSP model

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Hi,

On Fri, Jan 06, 2023 at 12:31:12PM +0300, Dan Carpenter wrote:
> Hello Takashi Sakamoto,
> 
> The patch 634ec0b2906e: "ALSA: firewire-motu: notify event for
> parameter change in register DSP model" from Oct 15, 2021, leads to
> the following Smatch static checker warning:
> 
> 	sound/firewire/motu/motu-hwdep.c:92 hwdep_read()
> 	warn: inconsistent returns '&motu->lock'.
 
Indeed. When no event is available, the bug appears. Later, I'll post
quick fix. Thanks.

> sound/firewire/motu/motu-hwdep.c
>     27 static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
>     28                        loff_t *offset)
>     29 {
>     30         struct snd_motu *motu = hwdep->private_data;
>     31         DEFINE_WAIT(wait);
>     32         union snd_firewire_event event;
>     33 
>     34         spin_lock_irq(&motu->lock);
>     35 
>     36         while (!motu->dev_lock_changed && motu->msg == 0 && !has_dsp_event(motu)) {
>     37                 prepare_to_wait(&motu->hwdep_wait, &wait, TASK_INTERRUPTIBLE);
>     38                 spin_unlock_irq(&motu->lock);
>     39                 schedule();
>     40                 finish_wait(&motu->hwdep_wait, &wait);
>     41                 if (signal_pending(current))
>     42                         return -ERESTARTSYS;
>     43                 spin_lock_irq(&motu->lock);
>     44         }
>     45 
>     46         memset(&event, 0, sizeof(event));
>     47         if (motu->dev_lock_changed) {
>     48                 event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS;
>     49                 event.lock_status.status = (motu->dev_lock_count > 0);
>     50                 motu->dev_lock_changed = false;
>     51                 spin_unlock_irq(&motu->lock);
>     52 
>     53                 count = min_t(long, count, sizeof(event));
>     54                 if (copy_to_user(buf, &event, count))
>     55                         return -EFAULT;
>     56         } else if (motu->msg > 0) {
>     57                 event.motu_notification.type = SNDRV_FIREWIRE_EVENT_MOTU_NOTIFICATION;
>     58                 event.motu_notification.message = motu->msg;
>     59                 motu->msg = 0;
>     60                 spin_unlock_irq(&motu->lock);
>     61 
>     62                 count = min_t(long, count, sizeof(event));
>     63                 if (copy_to_user(buf, &event, count))
>     64                         return -EFAULT;
>     65         } else if (has_dsp_event(motu)) {
>     66                 size_t consumed = 0;
>     67                 u32 __user *ptr;
>     68                 u32 ev;
>     69 
>     70                 spin_unlock_irq(&motu->lock);
>     71 
>     72                 // Header is filled later.
>     73                 consumed += sizeof(event.motu_register_dsp_change);
>     74 
>     75                 while (consumed < count &&
>     76                        snd_motu_register_dsp_message_parser_copy_event(motu, &ev)) {
>     77                         ptr = (u32 __user *)(buf + consumed);
>     78                         if (put_user(ev, ptr))
>     79                                 return -EFAULT;
>     80                         consumed += sizeof(ev);
>     81                 }
>     82 
>     83                 event.motu_register_dsp_change.type = SNDRV_FIREWIRE_EVENT_MOTU_REGISTER_DSP_CHANGE;
>     84                 event.motu_register_dsp_change.count =
>     85                         (consumed - sizeof(event.motu_register_dsp_change)) / 4;
>     86                 if (copy_to_user(buf, &event, sizeof(event.motu_register_dsp_change)))
>     87                         return -EFAULT;
>     88 
>     89                 count = consumed;
>     90         }
> 
> Smatch complains that there is no "} else {" path which unlocks.
> 
> 
>     91 
> --> 92         return count;
>     93 }
> 
> regards,
> dan carpenter

Takashi Sakamoto