| Message ID | 20191105143218.5948-9-andrew_gabbasov@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: aloop: Support sound timer as clock source instead of jiffies | expand |
On Tue, 05 Nov 2019 15:32:18 +0100, Andrew Gabbasov wrote: > > Show and change sound card timer source with read-write info > file in proc filesystem. Initial string can still be set as > module parameter. > > The timer source string value can be changed at any time, > but it is latched by PCM substream open callback (the first one > for a particular cable). At this point it is actually used, that > is the string is parsed, and the timer is looked up and opened. > > The timer source is set for a loopback card (the same as initial > setting by module parameter), but every cable uses the value, > current at the moment of open. > > Setting the value to empty string switches the timer to jiffies. > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> Unfortunately the whole code here are racy. It may lead to a crash or use-after-free easily. Some locking is needed definitely. thanks, Takashi
Hello Takashi, Thank you very much for your feedback! > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Thursday, November 07, 2019 11:06 AM > To: Gabbasov, Andrew > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav > Kysela; Takashi Iwai; Timo Wischer > Subject: Re: [PATCH v2 8/8] ALSA: aloop: Support runtime change of > snd_timer via info interface > > On Tue, 05 Nov 2019 15:32:18 +0100, > Andrew Gabbasov wrote: > > > > Show and change sound card timer source with read-write info > > file in proc filesystem. Initial string can still be set as > > module parameter. > > > > The timer source string value can be changed at any time, > > but it is latched by PCM substream open callback (the first one > > for a particular cable). At this point it is actually used, that > > is the string is parsed, and the timer is looked up and opened. > > > > The timer source is set for a loopback card (the same as initial > > setting by module parameter), but every cable uses the value, > > current at the moment of open. > > > > Setting the value to empty string switches the timer to jiffies. > > > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > Unfortunately the whole code here are racy. It may lead to a crash or > use-after-free easily. Some locking is needed definitely. You are right, using and changing of loopback->timer_source should be protected. I'll add locking with loopback->cable_lock to the bodies of print_timer_source_info() and change_timer_source_info() (like in the example diff below), similarly to other /proc files and mixer controls. All other uses of loopback->timer_source are already covered by loopback->cable_lock, except for loopback_set_timer_source() call from loopback_probe(), that is done at the very early stage and doesn't conflict with other uses. I think, in order to avoid racing problems, this will be enough, won't it? Thanks. Best regards, Andrew diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 415128a97774..ca9307dd780e 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -1684,8 +1684,10 @@ static void print_timer_source_info(struct snd_info_entry *entry, { struct loopback *loopback = entry->private_data; + mutex_lock(&loopback->cable_lock); snd_iprintf(buffer, "%s\n", loopback->timer_source ? loopback->timer_source : ""); + mutex_unlock(&loopback->cable_lock); } static void change_timer_source_info(struct snd_info_entry *entry, @@ -1694,8 +1696,10 @@ static void change_timer_source_info(struct snd_info_entry *entry, struct loopback *loopback = entry->private_data; char line[64]; + mutex_lock(&loopback->cable_lock); if (!snd_info_get_line(buffer, line, sizeof(line))) loopback_set_timer_source(loopback, strim(line)); + mutex_unlock(&loopback->cable_lock); } static int loopback_timer_source_proc_new(struct loopback *loopback)
On Thu, 07 Nov 2019 11:40:18 +0100, Gabbasov, Andrew wrote: > > Hello Takashi, > > Thank you very much for your feedback! > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Thursday, November 07, 2019 11:06 AM > > To: Gabbasov, Andrew > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav > > Kysela; Takashi Iwai; Timo Wischer > > Subject: Re: [PATCH v2 8/8] ALSA: aloop: Support runtime change of > > snd_timer via info interface > > > > On Tue, 05 Nov 2019 15:32:18 +0100, > > Andrew Gabbasov wrote: > > > > > > Show and change sound card timer source with read-write info > > > file in proc filesystem. Initial string can still be set as > > > module parameter. > > > > > > The timer source string value can be changed at any time, > > > but it is latched by PCM substream open callback (the first one > > > for a particular cable). At this point it is actually used, that > > > is the string is parsed, and the timer is looked up and opened. > > > > > > The timer source is set for a loopback card (the same as initial > > > setting by module parameter), but every cable uses the value, > > > current at the moment of open. > > > > > > Setting the value to empty string switches the timer to jiffies. > > > > > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > > > Unfortunately the whole code here are racy. It may lead to a crash or > > use-after-free easily. Some locking is needed definitely. > > You are right, using and changing of loopback->timer_source should be protected. > I'll add locking with loopback->cable_lock to the bodies of print_timer_source_info() > and change_timer_source_info() (like in the example diff below), similarly to other > /proc files and mixer controls. All other uses of loopback->timer_source are already > covered by loopback->cable_lock, except for loopback_set_timer_source() call from > loopback_probe(), that is done at the very early stage and doesn't conflict with other > uses. I think, in order to avoid racing problems, this will be enough, won't it? Yes, I guess so. loopback_set_timer_source() replaces only the timer_source and it's referred only at opening a stream. thanks, Takashi > > Thanks. > > Best regards, > Andrew > > diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c > index 415128a97774..ca9307dd780e 100644 > --- a/sound/drivers/aloop.c > +++ b/sound/drivers/aloop.c > @@ -1684,8 +1684,10 @@ static void print_timer_source_info(struct snd_info_entry *entry, > { > struct loopback *loopback = entry->private_data; > > + mutex_lock(&loopback->cable_lock); > snd_iprintf(buffer, "%s\n", > loopback->timer_source ? loopback->timer_source : ""); > + mutex_unlock(&loopback->cable_lock); > } > > static void change_timer_source_info(struct snd_info_entry *entry, > @@ -1694,8 +1696,10 @@ static void change_timer_source_info(struct snd_info_entry *entry, > struct loopback *loopback = entry->private_data; > char line[64]; > > + mutex_lock(&loopback->cable_lock); > if (!snd_info_get_line(buffer, line, sizeof(line))) > loopback_set_timer_source(loopback, strim(line)); > + mutex_unlock(&loopback->cable_lock); > } > > static int loopback_timer_source_proc_new(struct loopback *loopback) >
diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 6db70ebd46f6..16444a34d4b9 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -1655,7 +1655,7 @@ static void print_cable_info(struct snd_info_entry *entry, mutex_unlock(&loopback->cable_lock); } -static int loopback_proc_new(struct loopback *loopback, int cidx) +static int loopback_cable_proc_new(struct loopback *loopback, int cidx) { char name[32]; @@ -1676,6 +1676,40 @@ static void loopback_set_timer_source(struct loopback *loopback, value, GFP_KERNEL); } +static void print_timer_source_info(struct snd_info_entry *entry, + struct snd_info_buffer *buffer) +{ + struct loopback *loopback = entry->private_data; + + snd_iprintf(buffer, "%s\n", + loopback->timer_source ? loopback->timer_source : ""); +} + +static void change_timer_source_info(struct snd_info_entry *entry, + struct snd_info_buffer *buffer) +{ + struct loopback *loopback = entry->private_data; + char line[64]; + + if (!snd_info_get_line(buffer, line, sizeof(line))) + loopback_set_timer_source(loopback, strim(line)); +} + +static int loopback_timer_source_proc_new(struct loopback *loopback) +{ + struct snd_info_entry *entry; + int err; + + err = snd_card_proc_new(loopback->card, "timer_source", &entry); + if (err < 0) + return err; + + snd_info_set_text_ops(entry, loopback, print_timer_source_info); + entry->mode |= S_IWUSR; + entry->c.text.write = change_timer_source_info; + return 0; +} + static int loopback_probe(struct platform_device *devptr) { struct snd_card *card; @@ -1708,8 +1742,9 @@ static int loopback_probe(struct platform_device *devptr) err = loopback_mixer_new(loopback, pcm_notify[dev] ? 1 : 0); if (err < 0) goto __nodev; - loopback_proc_new(loopback, 0); - loopback_proc_new(loopback, 1); + loopback_cable_proc_new(loopback, 0); + loopback_cable_proc_new(loopback, 1); + loopback_timer_source_proc_new(loopback); strcpy(card->driver, "Loopback"); strcpy(card->shortname, "Loopback"); sprintf(card->longname, "Loopback %i", dev + 1);
Show and change sound card timer source with read-write info file in proc filesystem. Initial string can still be set as module parameter. The timer source string value can be changed at any time, but it is latched by PCM substream open callback (the first one for a particular cable). At this point it is actually used, that is the string is parsed, and the timer is looked up and opened. The timer source is set for a loopback card (the same as initial setting by module parameter), but every cable uses the value, current at the moment of open. Setting the value to empty string switches the timer to jiffies. Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> --- sound/drivers/aloop.c | 41 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-)