From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0E811C4360F for ; Thu, 4 Apr 2019 10:18:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B727A20882 for ; Thu, 4 Apr 2019 10:18:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729347AbfDDKSp (ORCPT ); Thu, 4 Apr 2019 06:18:45 -0400 Received: from smtp1.de.adit-jv.com ([93.241.18.167]:46971 "EHLO smtp1.de.adit-jv.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726694AbfDDKSp (ORCPT ); Thu, 4 Apr 2019 06:18:45 -0400 Received: from localhost (smtp1.de.adit-jv.com [127.0.0.1]) by smtp1.de.adit-jv.com (Postfix) with ESMTP id AD48A3C013F; Thu, 4 Apr 2019 12:18:41 +0200 (CEST) Received: from smtp1.de.adit-jv.com ([127.0.0.1]) by localhost (smtp1.de.adit-jv.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 3StZ6-NXbAFl; Thu, 4 Apr 2019 12:18:34 +0200 (CEST) Received: from HI2EXCH01.adit-jv.com (hi2exch01.adit-jv.com [10.72.92.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by smtp1.de.adit-jv.com (Postfix) with ESMTPS id 1FAEE3C00C3; Thu, 4 Apr 2019 12:18:34 +0200 (CEST) Received: from [10.72.93.172] (10.72.93.172) by HI2EXCH01.adit-jv.com (10.72.92.24) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 4 Apr 2019 12:18:33 +0200 Subject: Re: [PATCH 09/10] ALSA: pcm: Add snd_pcm_ops for snd_pcm_link() To: Takashi Iwai CC: , , , References: <1553529644-5654-1-git-send-email-twischer@de.adit-jv.com> <1553586574-18608-1-git-send-email-twischer@de.adit-jv.com> <1553586574-18608-5-git-send-email-twischer@de.adit-jv.com> <3ad5c313-ba2c-b541-8930-3f2515dfca72@de.adit-jv.com> <68cfbf7a-5f1e-50d2-2b97-4d3c5d360b36@de.adit-jv.com> <00f4c8db-44cf-b555-f3ca-42916b1e7ceb@de.adit-jv.com> From: Timo Wischer Message-ID: <00d97a34-c055-c7ef-dbfd-e17c0f6cfff3@de.adit-jv.com> Date: Thu, 4 Apr 2019 12:18:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [10.72.93.172] Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 3/27/19 10:38, Takashi Iwai wrote: > On Wed, 27 Mar 2019 10:26:56 +0100, > Timo Wischer wrote: >> On 3/27/19 10:11, Takashi Iwai wrote: >>> On Wed, 27 Mar 2019 09:34:40 +0100, >>> Timo Wischer wrote: >>>> On 3/26/19 17:00, Takashi Iwai wrote: >>>>> On Tue, 26 Mar 2019 16:16:54 +0100, >>>>> Timo Wischer wrote: >>>>>> On 3/26/19 15:23, Takashi Iwai wrote: >>>>>>> On Tue, 26 Mar 2019 12:25:37 +0100, >>>>>>> Timo Wischer wrote: >>>>>>>> On 3/26/19 09:35, Takashi Iwai wrote: >>>>>>>> >>>>>>>> On Tue, 26 Mar 2019 08:49:33 +0100, >>>>>>>> wrote: >>>>>>>> From: Timo Wischer >>>>>>>> snd_pcm_link() can be called by the user as long >>>>>>>> as the device is not >>>>>>>> yet started. Therefore currently a driver which wants to iterate over >>>>>>>> the linked substreams has to do this at the start trigger. But the start >>>>>>>> trigger should not block for a long time. Therefore there is no callback >>>>>>>> which can be used to iterate over the linked substreams without delaying >>>>>>>> the start trigger. >>>>>>>> This patch introduces a new callback function which will be called after >>>>>>>> the linked substream list was updated by snd_pcm_link(). This callback >>>>>>>> function is allowed to block for a longer time without interfering the >>>>>>>> synchronized start up of linked substreams. >>>>>>>> Signed-off-by: Timo Wischer >>>>>>>> >>>>>>>> Well, the idea appears interesting, but I'm afraid >>>>>>>> that the >>>>>>>> implementation is still racy. The place you're calling the new >>>>>>>> callback isn't protected, hence the stream can be triggered while >>>>>>>> calling it. That is, even during operating your loopback link_changed >>>>>>>> callback, another thread is able to start the stream. >>>>>>>> Hi Takashi, >>>>>>>> >>>>>>>> As far as I got you mean the following scenario: >>>>>>>> >>>>>>>> * snd_pcm_link() is called for a HW sound card >>>>>>>> + loopback_snd_timer_link_changed() >>>>>>> The start may happen at this point. >>>>>> In this case the last link status will be used and aloop will print a >>>>>> warning "Another sound timer was requested but at least one device is >>>>>> already running...". >>>>>> >>>>>> Without this patch set a similar issue already exists. When calling >>>>>> snd_pcm_start() before snd_pcm_link() was done the additional device >>>>>> linked by the snd_pcm_link() will not be started. >>>>>> Therefore the application has already to take care about the order of >>>>>> the calls. >>>>> Yes, but it doesn't matter for now, just because other drivers do care >>>>> the PCM links only for trigger callback. Now you're trying to add >>>>> something new but in an incomplete manner. >>>>> >>>>>>>> + loopback_snd_timer_open() >>>>>>>> + spin_lock_irqsave(&dpcm->cable->lock, flags); >>>>>>>> * snd_pcm_start() called for aloop sound card >>>>>>>> + loopback_trigger() >>>>>>>> + spin_lock(&cable->lock) -> has to wait till loopback_snd_timer_open() >>>>>>>> calls spin_unlock_irqrestore() >>>>>>>> >>>>>>>> So far snd_pcm_start() has to wait for loopback_snd_timer_open(). >>>>>>>> >>>>>>>> * loopback_snd_timer_open() will continue with >>>>>>>> + dpcm->cable->snd_timer.instance = NULL; >>>>>>>> + spin_unlock_irqrestore() >>>>>>>> * loopback_trigger() can enter the lock >>>>>>>> + loopback_snd_timer_start() will fail with -EINVAL due to >>>>>>>> (loopback_trigger == NULL) >>>>>>>> >>>>>>>> At least this will not result into memory corruption due to race or any other >>>>>>>> wired behavior. >>>>>>> I don't expect the memory corruption, but my point is that dealing >>>>>>> with linked streams is still tricky. It was considered for the >>>>>>> lightweight coupled start/stop operation, and something intensively >>>>>>> depending on the linked status was out of the original design... >>>>>>> >>>>>>>> But my expectation is that snd_pcm_link(hw, aloop) or snd_pcm_link(aloop, hw) >>>>>>>> is only called by the application calling snd_pcm_start(aloop) >>>>>>>> because the same aloop device cannot be opened by multiple applications at the >>>>>>>> same time. >>>>>>>> >>>>>>>> Do you see an use case where one application would call snd_pcm_start() in >>>>>>>> parallel with snd_pcm_link() (somehow configuring the device)? >>>>>>> It's not about the actual application usages but rather against the >>>>>>> malicious attacks. Especially aloop is a virtual device that is >>>>>>> available allover the places, it may be deployed / attacked easily. >>>>>> The attack we are identifying here can only be done by the application >>>>>> opening the aloop device. >>>>>> An application allowed to open the aloop device is anyway able to >>>>>> manipulate the audio streaming. >>>>> Right, and if it such a racy access may lead to a driver misbehavior, >>>>> it's a big concern. The proposed callback usage is racy, so some >>>>> other implementation might be broken easily in future. >>>>> >>>>>> Or do you see an attack which would influence any other device/stream >>>>>> not opened by this application? >>>>>> >>>>>>>> May be we should add an additional synchronization mechanism in pcm_native.c >>>>>>>> to avoid call of snd_pcm_link() in parallel with snd_pcm_start(). >>>>>>> If it really matters... Honestly speaking, I'm not fully convinced >>>>>>> whether we want to deal with this using the PCM link mechanism. >>>>>>> >>>>>>> What's the motivation for using the linked streams at the first place? >>>>>>> That's one of the biggest missing piece in the whole picture. >>>>>> In general when the user uses snd_pcm_link() it expects that the >>>>>> linked devices are somehow synchronized. >>>>>> Any applications already using snd_pcm_link() do not need to be >>>>>> adapted to use the new feature of aloop (for example JACK or ALSA >>>>>> multi plugin) >>>>>> >>>>>> But when linking a HW sound card and aloop without this patch set, >>>>>> both devices will be started in sync but >>>>>> the snd_pcm_period_eleapsed() calls of the different devices will >>>>>> drift. To avoid this the aloop plugin can automatically use the right >>>>>> timer. >>>>>> If this feature is not implemented the user has to use snd_pcm_link() >>>>>> to trigger snd_pcm_start() in sync but also has to configure the aloop >>>>>> plugin to use the right sound timer. >>>>>> May be the linked cards can change during runtime of the >>>>>> system. Without this feature the aloop kernel driver has to be loaded >>>>>> with different kernel parameters >>>>>> to select the right timer. >>>>>> >>>>>> ALSA controls cannot be used easily. Selecting the sound timer by the >>>>>> card number could be error prone because the card ID could change >>>>>> between system starts. >>>>>> Therefore an ALSA control supporting the card name should be >>>>>> used. This could be for example done via an ALSA enum control. But in >>>>>> this case the names of all sound cards of the system has to be >>>>>> available >>>>>> on aloop probe() call. But at this point in time the sound cards >>>>>> probed after aloop are not available. Therefore only the sound timers >>>>>> of the sound cards probed before aloop are selectable. >>>>> Hm. For me this patch series looks very hackish. As mentioned, the >>>>> PCM link usage is rather just a synchronous trigger start/stop for >>>>> multiple streams belonging to the same hardware; in that sense, it'd >>>>> be possible to adapt some mechanism for aloop, but at most it should >>>>> be much less intrusive change, e.g. just doing the multiple >>>>> loopback_timer_start() in a single loop. >>>> What do you mean by "just doing the multiple loopback_timer_start() in >>>> a single loop."? >>>> >>>> The snd_pcm_link() call information are mainly used to select the >>>> right sound timer. Starting the timer in sync is a nice add-on. >>>> Therefore by simple starting all timers in a for loop I would not >>>> select the right sound timer. >>>> (May be I misunderstood your example) >>> My example is only about the system timer. >>> >>>> I could call the link_changed() callback inside >>>> snd_pcm_stream_lock_irq() same lock also hold by snd_pcm_start(). >>>> But an application could still call snd_pcm_link() and snd_pcm_start() >>>> in parallel e.g.: >>>> >>>> snd_pcm_common_ioctl(IOCTL_LINK) >>>>> task switch >>>> snd_pcm_common_ioctl(IOCTL_START) >>>> snd_pcm_start() >>>> done >>>>> continue previous task >>>> The same could for example happen with snd_pcm_start() and snd_hw_free(): >>>> >>>> snd_pcm_common_ioctl(IOCTL_START) >>>>> task switch >>>> snd_pcm_common_ioctl(IOCTL_HW_FREE) >>>> snd_hw_free() >>>> Done >>>>> continue previous task >>>> snd_pcm_start() will fail with an error code >>>> >>>> Therefore I do not think we have to synchronize the IOCTL calls (this >>>> can only be done in the user application or ALSAlib). We only have to >>>> return an proper error code in case of misusage. >>>> >>>> As far as I understand ALSA was not designed for multithread usage. So >>>> the user (or ALSAlib) has to be aware of calling the functions in the >>>> right order. >>>> Otherwise the user could expect returned error codes. >>> Well, your assumption is plain wrong. ALSA is designed for >>> multithread usage. And the kernel works for multithread. >>> >>> It's not about the right usage. It's about the robustness, about >>> whether any malicious code may lead to a serious defunct. The opened >>> race in calls may easily introduce such a failure. >>> >>>> In case of my patch set snd_pcm_start() will always fail with a proper >>>> error code as long as no sound timer is running. This could be the >>>> case if snd_pcm_start() is called before the first snd_pcm_link() >>>> call. >>>> This is also the case if snd_pcm_start() is called when the >>>> snd_pcm_link() call has closed the old timer but not yet opened the >>>> new one. >>>> In case of snd_pcm_link() is called after snd_pcm_start() aloop will >>>> write a proper warning (that the snd_pcm_link() call will have no >>>> effect because the stream is already running). >>>> All member variables used in loopback_snd_timer_open(), >>>> loopback_snd_timer_source_update() and loopback_snd_timer_start() are >>>> always protected by cable->lock. Therefore no one can read an invalid >>>> state of these variables and the misusage can always be detected. >>>> >>>> Therefore I am not sure what I should change. May be you could >>>> reference to a source code line which looks hackish for you? >>> In principle, the PCM ops is supposed to be safe for operating a >>> certain stuff. If a state change may happen during the operation, >>> this should be called inside PCM stream lock. So the design of the >>> new callback itself is fragile. >>> >>> then, it comes to a point to re-setup the timer at the link change. >>> The idea is interesting, but it's a wrong usage of PCM link feature, >>> to be honest. >>> >>> That said, I find the changes up to patch 8 are acceptable (with some >>> fixes expected), but the link feature isn't. Hi Takashi, could you provide your feedback to the other patches? I would like to apply these 8 patches first and then I would provide an addition patch to get an ALSA control to select the sound timer. I fear this would be the best idea (replacing the snd_pcm_link() feature) because the ALSA control is accessible by any audio user and we could use ALSA hooks to select the right sound timer when opening an ALSA device. >> Thanks for clarification. Do you think there is a chance to get an >> acceptable snd_pcm_link() feature when using PCM stream lock for >> snd_pcm_link()? >> In this case I think snd_timer_open() has to be adapted to use >> non-blocking calls only (do not use mutexs). > This needs a good amount of consideration: currently the PCM link > feature is designed for the synchronous start/stop, especially for the > streams *on the same hardware*. The link between different devices is > merely a best-effort. This is the current design. > > Your target is obviously above that level. So we have to consider > either extending the usage and the implementation, or introducing > another API. Both are things we'd like to avoid as much as possible > from the maintainability POV. > >> Or do I have to use another API to configure the sound timer at >> runtime (e.g. ALSA controls or RW kernel module parameters)? > The kernel parameter would be handy. An alternative would be a > configuration via proc or sysfs file. Even configfs or debugfs is > another possibility, but the proc file would be easiest, I suppose. > > Of course, the disadvantage of these is that it's usually writable > only by root. > > > Takashi From mboxrd@z Thu Jan 1 00:00:00 1970 From: Timo Wischer Subject: Re: [PATCH 09/10] ALSA: pcm: Add snd_pcm_ops for snd_pcm_link() Date: Thu, 4 Apr 2019 12:18:33 +0200 Message-ID: <00d97a34-c055-c7ef-dbfd-e17c0f6cfff3@de.adit-jv.com> References: <1553529644-5654-1-git-send-email-twischer@de.adit-jv.com> <1553586574-18608-1-git-send-email-twischer@de.adit-jv.com> <1553586574-18608-5-git-send-email-twischer@de.adit-jv.com> <3ad5c313-ba2c-b541-8930-3f2515dfca72@de.adit-jv.com> <68cfbf7a-5f1e-50d2-2b97-4d3c5d360b36@de.adit-jv.com> <00f4c8db-44cf-b555-f3ca-42916b1e7ceb@de.adit-jv.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Takashi Iwai Cc: broonie@kernel.org, perex@perex.cz, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org List-Id: alsa-devel@alsa-project.org On 3/27/19 10:38, Takashi Iwai wrote: > On Wed, 27 Mar 2019 10:26:56 +0100, > Timo Wischer wrote: >> On 3/27/19 10:11, Takashi Iwai wrote: >>> On Wed, 27 Mar 2019 09:34:40 +0100, >>> Timo Wischer wrote: >>>> On 3/26/19 17:00, Takashi Iwai wrote: >>>>> On Tue, 26 Mar 2019 16:16:54 +0100, >>>>> Timo Wischer wrote: >>>>>> On 3/26/19 15:23, Takashi Iwai wrote: >>>>>>> On Tue, 26 Mar 2019 12:25:37 +0100, >>>>>>> Timo Wischer wrote: >>>>>>>> On 3/26/19 09:35, Takashi Iwai wrote: >>>>>>>> >>>>>>>> On Tue, 26 Mar 2019 08:49:33 +0100, >>>>>>>> wrote: >>>>>>>> From: Timo Wischer >>>>>>>> snd_pcm_link() can be called by the user as long >>>>>>>> as the device is not >>>>>>>> yet started. Therefore currently a driver which wants to iterate over >>>>>>>> the linked substreams has to do this at the start trigger. But the start >>>>>>>> trigger should not block for a long time. Therefore there is no callback >>>>>>>> which can be used to iterate over the linked substreams without delaying >>>>>>>> the start trigger. >>>>>>>> This patch introduces a new callback function which will be called after >>>>>>>> the linked substream list was updated by snd_pcm_link(). This callback >>>>>>>> function is allowed to block for a longer time without interfering the >>>>>>>> synchronized start up of linked substreams. >>>>>>>> Signed-off-by: Timo Wischer >>>>>>>> >>>>>>>> Well, the idea appears interesting, but I'm afraid >>>>>>>> that the >>>>>>>> implementation is still racy. The place you're calling the new >>>>>>>> callback isn't protected, hence the stream can be triggered while >>>>>>>> calling it. That is, even during operating your loopback link_changed >>>>>>>> callback, another thread is able to start the stream. >>>>>>>> Hi Takashi, >>>>>>>> >>>>>>>> As far as I got you mean the following scenario: >>>>>>>> >>>>>>>> * snd_pcm_link() is called for a HW sound card >>>>>>>> + loopback_snd_timer_link_changed() >>>>>>> The start may happen at this point. >>>>>> In this case the last link status will be used and aloop will print a >>>>>> warning "Another sound timer was requested but at least one device is >>>>>> already running...". >>>>>> >>>>>> Without this patch set a similar issue already exists. When calling >>>>>> snd_pcm_start() before snd_pcm_link() was done the additional device >>>>>> linked by the snd_pcm_link() will not be started. >>>>>> Therefore the application has already to take care about the order of >>>>>> the calls. >>>>> Yes, but it doesn't matter for now, just because other drivers do care >>>>> the PCM links only for trigger callback. Now you're trying to add >>>>> something new but in an incomplete manner. >>>>> >>>>>>>> + loopback_snd_timer_open() >>>>>>>> + spin_lock_irqsave(&dpcm->cable->lock, flags); >>>>>>>> * snd_pcm_start() called for aloop sound card >>>>>>>> + loopback_trigger() >>>>>>>> + spin_lock(&cable->lock) -> has to wait till loopback_snd_timer_open() >>>>>>>> calls spin_unlock_irqrestore() >>>>>>>> >>>>>>>> So far snd_pcm_start() has to wait for loopback_snd_timer_open(). >>>>>>>> >>>>>>>> * loopback_snd_timer_open() will continue with >>>>>>>> + dpcm->cable->snd_timer.instance = NULL; >>>>>>>> + spin_unlock_irqrestore() >>>>>>>> * loopback_trigger() can enter the lock >>>>>>>> + loopback_snd_timer_start() will fail with -EINVAL due to >>>>>>>> (loopback_trigger == NULL) >>>>>>>> >>>>>>>> At least this will not result into memory corruption due to race or any other >>>>>>>> wired behavior. >>>>>>> I don't expect the memory corruption, but my point is that dealing >>>>>>> with linked streams is still tricky. It was considered for the >>>>>>> lightweight coupled start/stop operation, and something intensively >>>>>>> depending on the linked status was out of the original design... >>>>>>> >>>>>>>> But my expectation is that snd_pcm_link(hw, aloop) or snd_pcm_link(aloop, hw) >>>>>>>> is only called by the application calling snd_pcm_start(aloop) >>>>>>>> because the same aloop device cannot be opened by multiple applications at the >>>>>>>> same time. >>>>>>>> >>>>>>>> Do you see an use case where one application would call snd_pcm_start() in >>>>>>>> parallel with snd_pcm_link() (somehow configuring the device)? >>>>>>> It's not about the actual application usages but rather against the >>>>>>> malicious attacks. Especially aloop is a virtual device that is >>>>>>> available allover the places, it may be deployed / attacked easily. >>>>>> The attack we are identifying here can only be done by the application >>>>>> opening the aloop device. >>>>>> An application allowed to open the aloop device is anyway able to >>>>>> manipulate the audio streaming. >>>>> Right, and if it such a racy access may lead to a driver misbehavior, >>>>> it's a big concern. The proposed callback usage is racy, so some >>>>> other implementation might be broken easily in future. >>>>> >>>>>> Or do you see an attack which would influence any other device/stream >>>>>> not opened by this application? >>>>>> >>>>>>>> May be we should add an additional synchronization mechanism in pcm_native.c >>>>>>>> to avoid call of snd_pcm_link() in parallel with snd_pcm_start(). >>>>>>> If it really matters... Honestly speaking, I'm not fully convinced >>>>>>> whether we want to deal with this using the PCM link mechanism. >>>>>>> >>>>>>> What's the motivation for using the linked streams at the first place? >>>>>>> That's one of the biggest missing piece in the whole picture. >>>>>> In general when the user uses snd_pcm_link() it expects that the >>>>>> linked devices are somehow synchronized. >>>>>> Any applications already using snd_pcm_link() do not need to be >>>>>> adapted to use the new feature of aloop (for example JACK or ALSA >>>>>> multi plugin) >>>>>> >>>>>> But when linking a HW sound card and aloop without this patch set, >>>>>> both devices will be started in sync but >>>>>> the snd_pcm_period_eleapsed() calls of the different devices will >>>>>> drift. To avoid this the aloop plugin can automatically use the right >>>>>> timer. >>>>>> If this feature is not implemented the user has to use snd_pcm_link() >>>>>> to trigger snd_pcm_start() in sync but also has to configure the aloop >>>>>> plugin to use the right sound timer. >>>>>> May be the linked cards can change during runtime of the >>>>>> system. Without this feature the aloop kernel driver has to be loaded >>>>>> with different kernel parameters >>>>>> to select the right timer. >>>>>> >>>>>> ALSA controls cannot be used easily. Selecting the sound timer by the >>>>>> card number could be error prone because the card ID could change >>>>>> between system starts. >>>>>> Therefore an ALSA control supporting the card name should be >>>>>> used. This could be for example done via an ALSA enum control. But in >>>>>> this case the names of all sound cards of the system has to be >>>>>> available >>>>>> on aloop probe() call. But at this point in time the sound cards >>>>>> probed after aloop are not available. Therefore only the sound timers >>>>>> of the sound cards probed before aloop are selectable. >>>>> Hm. For me this patch series looks very hackish. As mentioned, the >>>>> PCM link usage is rather just a synchronous trigger start/stop for >>>>> multiple streams belonging to the same hardware; in that sense, it'd >>>>> be possible to adapt some mechanism for aloop, but at most it should >>>>> be much less intrusive change, e.g. just doing the multiple >>>>> loopback_timer_start() in a single loop. >>>> What do you mean by "just doing the multiple loopback_timer_start() in >>>> a single loop."? >>>> >>>> The snd_pcm_link() call information are mainly used to select the >>>> right sound timer. Starting the timer in sync is a nice add-on. >>>> Therefore by simple starting all timers in a for loop I would not >>>> select the right sound timer. >>>> (May be I misunderstood your example) >>> My example is only about the system timer. >>> >>>> I could call the link_changed() callback inside >>>> snd_pcm_stream_lock_irq() same lock also hold by snd_pcm_start(). >>>> But an application could still call snd_pcm_link() and snd_pcm_start() >>>> in parallel e.g.: >>>> >>>> snd_pcm_common_ioctl(IOCTL_LINK) >>>>> task switch >>>> snd_pcm_common_ioctl(IOCTL_START) >>>> snd_pcm_start() >>>> done >>>>> continue previous task >>>> The same could for example happen with snd_pcm_start() and snd_hw_free(): >>>> >>>> snd_pcm_common_ioctl(IOCTL_START) >>>>> task switch >>>> snd_pcm_common_ioctl(IOCTL_HW_FREE) >>>> snd_hw_free() >>>> Done >>>>> continue previous task >>>> snd_pcm_start() will fail with an error code >>>> >>>> Therefore I do not think we have to synchronize the IOCTL calls (this >>>> can only be done in the user application or ALSAlib). We only have to >>>> return an proper error code in case of misusage. >>>> >>>> As far as I understand ALSA was not designed for multithread usage. So >>>> the user (or ALSAlib) has to be aware of calling the functions in the >>>> right order. >>>> Otherwise the user could expect returned error codes. >>> Well, your assumption is plain wrong. ALSA is designed for >>> multithread usage. And the kernel works for multithread. >>> >>> It's not about the right usage. It's about the robustness, about >>> whether any malicious code may lead to a serious defunct. The opened >>> race in calls may easily introduce such a failure. >>> >>>> In case of my patch set snd_pcm_start() will always fail with a proper >>>> error code as long as no sound timer is running. This could be the >>>> case if snd_pcm_start() is called before the first snd_pcm_link() >>>> call. >>>> This is also the case if snd_pcm_start() is called when the >>>> snd_pcm_link() call has closed the old timer but not yet opened the >>>> new one. >>>> In case of snd_pcm_link() is called after snd_pcm_start() aloop will >>>> write a proper warning (that the snd_pcm_link() call will have no >>>> effect because the stream is already running). >>>> All member variables used in loopback_snd_timer_open(), >>>> loopback_snd_timer_source_update() and loopback_snd_timer_start() are >>>> always protected by cable->lock. Therefore no one can read an invalid >>>> state of these variables and the misusage can always be detected. >>>> >>>> Therefore I am not sure what I should change. May be you could >>>> reference to a source code line which looks hackish for you? >>> In principle, the PCM ops is supposed to be safe for operating a >>> certain stuff. If a state change may happen during the operation, >>> this should be called inside PCM stream lock. So the design of the >>> new callback itself is fragile. >>> >>> then, it comes to a point to re-setup the timer at the link change. >>> The idea is interesting, but it's a wrong usage of PCM link feature, >>> to be honest. >>> >>> That said, I find the changes up to patch 8 are acceptable (with some >>> fixes expected), but the link feature isn't. Hi Takashi, could you provide your feedback to the other patches? I would like to apply these 8 patches first and then I would provide an addition patch to get an ALSA control to select the sound timer. I fear this would be the best idea (replacing the snd_pcm_link() feature) because the ALSA control is accessible by any audio user and we could use ALSA hooks to select the right sound timer when opening an ALSA device. >> Thanks for clarification. Do you think there is a chance to get an >> acceptable snd_pcm_link() feature when using PCM stream lock for >> snd_pcm_link()? >> In this case I think snd_timer_open() has to be adapted to use >> non-blocking calls only (do not use mutexs). > This needs a good amount of consideration: currently the PCM link > feature is designed for the synchronous start/stop, especially for the > streams *on the same hardware*. The link between different devices is > merely a best-effort. This is the current design. > > Your target is obviously above that level. So we have to consider > either extending the usage and the implementation, or introducing > another API. Both are things we'd like to avoid as much as possible > from the maintainability POV. > >> Or do I have to use another API to configure the sound timer at >> runtime (e.g. ALSA controls or RW kernel module parameters)? > The kernel parameter would be handy. An alternative would be a > configuration via proc or sysfs file. Even configfs or debugfs is > another possibility, but the proc file would be easiest, I suppose. > > Of course, the disadvantage of these is that it's usually writable > only by root. > > > Takashi