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
