| Message ID | 20230725023416.11205-32-quic_wcheng@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
On 7/25/23 04:34, Wesley Cheng wrote: > In case of notifying SND platform drivers of connection events, some of > these use cases, such as offloading, require an ASoC USB backend device to > be initialized before the events can be handled. If the USB backend device > has not yet been probed, this leads to missing initial USB audio device > connection events. > > Expose an API that traverses the usb_chip array for connected devices, and > to call the respective connection callback registered to the SND platform > driver. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > sound/usb/card.c | 19 +++++++++++++++++++ > sound/usb/card.h | 2 ++ > 2 files changed, 21 insertions(+) > > diff --git a/sound/usb/card.c b/sound/usb/card.c > index 365f6d978608..27a89aaa0bf3 100644 > --- a/sound/usb/card.c > +++ b/sound/usb/card.c > @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > } > EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); > > +/* > + * in case the platform driver was not ready at the time of USB SND > + * device connect, expose an API to discover all connected USB devices > + * so it can populate any dependent resources/structures. > + */ > +void snd_usb_rediscover_devices(void) > +{ > + int i; > + > + mutex_lock(®ister_mutex); > + for (i = 0; i < SNDRV_CARDS; i++) { > + if (usb_chip[i]) > + if (platform_ops && platform_ops->connect_cb) > + platform_ops->connect_cb(usb_chip[i]); what happens if the USB device is removed while the platform device adds a port? This sounds super-racy to me. It's the same set of problems we're having between audio and display/DRM, I would be surprised if this function dealt with all corner cases of insertion/removal, bind/unbind. > + } > + mutex_unlock(®ister_mutex); > +} > +EXPORT_SYMBOL_GPL(snd_usb_rediscover_devices); > + > /* > * disconnect streams > * called from usb_audio_disconnect() > diff --git a/sound/usb/card.h b/sound/usb/card.h > index db735680d155..dfd241334e3d 100644 > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -219,6 +219,7 @@ int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); > int snd_usb_unregister_platform_ops(void); > struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > struct snd_pcm_hw_params *params, int direction); > +void snd_usb_rediscover_devices(void); > #else > static int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops) > { > @@ -235,5 +236,6 @@ static struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > { > return NULL; > } > +void snd_usb_rediscover_devices(void) { } > #endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ > #endif /* __USBAUDIO_CARD_H */
On Tue, 25 Jul 2023 11:15:11 +0200, Pierre-Louis Bossart wrote: > > > > On 7/25/23 04:34, Wesley Cheng wrote: > > In case of notifying SND platform drivers of connection events, some of > > these use cases, such as offloading, require an ASoC USB backend device to > > be initialized before the events can be handled. If the USB backend device > > has not yet been probed, this leads to missing initial USB audio device > > connection events. > > > > Expose an API that traverses the usb_chip array for connected devices, and > > to call the respective connection callback registered to the SND platform > > driver. > > > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > > --- > > sound/usb/card.c | 19 +++++++++++++++++++ > > sound/usb/card.h | 2 ++ > > 2 files changed, 21 insertions(+) > > > > diff --git a/sound/usb/card.c b/sound/usb/card.c > > index 365f6d978608..27a89aaa0bf3 100644 > > --- a/sound/usb/card.c > > +++ b/sound/usb/card.c > > @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > > } > > EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); > > > > +/* > > + * in case the platform driver was not ready at the time of USB SND > > + * device connect, expose an API to discover all connected USB devices > > + * so it can populate any dependent resources/structures. > > + */ > > +void snd_usb_rediscover_devices(void) > > +{ > > + int i; > > + > > + mutex_lock(®ister_mutex); > > + for (i = 0; i < SNDRV_CARDS; i++) { > > + if (usb_chip[i]) > > + if (platform_ops && platform_ops->connect_cb) > > + platform_ops->connect_cb(usb_chip[i]); > > what happens if the USB device is removed while the platform device adds > a port? That should be protected by the register_mutex. But there can be other races (see below :) > This sounds super-racy to me. It's the same set of problems we're having > between audio and display/DRM, I would be surprised if this function > dealt with all corner cases of insertion/removal, bind/unbind. Yes, we need to be more careful about binding. For example, in the current patch set, I see no way to prevent unloading snd-usb-audio-qmi module, and it allows user to cut off the stuff during operation, which may break things while the kernel is running the code of the unloaded module. You need to have a proper module refcount management for avoiding such a scenario. Most of drivers don't need it because ALSA core part already takes care of it. But in this case, it requires a manual adjustment. Takashi
Hi Pierre, On 7/25/2023 2:15 AM, Pierre-Louis Bossart wrote: > > > On 7/25/23 04:34, Wesley Cheng wrote: >> In case of notifying SND platform drivers of connection events, some of >> these use cases, such as offloading, require an ASoC USB backend device to >> be initialized before the events can be handled. If the USB backend device >> has not yet been probed, this leads to missing initial USB audio device >> connection events. >> >> Expose an API that traverses the usb_chip array for connected devices, and >> to call the respective connection callback registered to the SND platform >> driver. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> sound/usb/card.c | 19 +++++++++++++++++++ >> sound/usb/card.h | 2 ++ >> 2 files changed, 21 insertions(+) >> >> diff --git a/sound/usb/card.c b/sound/usb/card.c >> index 365f6d978608..27a89aaa0bf3 100644 >> --- a/sound/usb/card.c >> +++ b/sound/usb/card.c >> @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >> } >> EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); >> >> +/* >> + * in case the platform driver was not ready at the time of USB SND >> + * device connect, expose an API to discover all connected USB devices >> + * so it can populate any dependent resources/structures. >> + */ >> +void snd_usb_rediscover_devices(void) >> +{ >> + int i; >> + >> + mutex_lock(®ister_mutex); >> + for (i = 0; i < SNDRV_CARDS; i++) { >> + if (usb_chip[i]) >> + if (platform_ops && platform_ops->connect_cb) >> + platform_ops->connect_cb(usb_chip[i]); > > what happens if the USB device is removed while the platform device adds > a port? > > This sounds super-racy to me. It's the same set of problems we're having > between audio and display/DRM, I would be surprised if this function > dealt with all corner cases of insertion/removal, bind/unbind. > The chip array entries are all populated and removed while under the register_mutex, so going over your race condition, we should see: Thread#1: q6usb_component_probe() --> snd_soc_usb_add_port() --> snd_usb_rediscover_devices() --> mutex_lock(register_mutex) Thread#2 --> usb_audio_disconnect() --> mutex_lock(register_mutex) So either thread#1 or thread#2 will complete first. If Thread#1 completes before thread#2: SOC USB will notify DPCM backend of the device connection. Shortly after, once thread#2 runs, we will get a disconnect event for the connected device. Thread#2 completes before thread#1: Then during snd_usb_rediscover_devices() we won't notify of any connection for that particular chip index. Thanks Wesley Cheng
On 8/15/23 20:38, Wesley Cheng wrote: > Hi Pierre, > > On 7/25/2023 2:15 AM, Pierre-Louis Bossart wrote: >> >> >> On 7/25/23 04:34, Wesley Cheng wrote: >>> In case of notifying SND platform drivers of connection events, some of >>> these use cases, such as offloading, require an ASoC USB backend >>> device to >>> be initialized before the events can be handled. If the USB backend >>> device >>> has not yet been probed, this leads to missing initial USB audio device >>> connection events. >>> >>> Expose an API that traverses the usb_chip array for connected >>> devices, and >>> to call the respective connection callback registered to the SND >>> platform >>> driver. >>> >>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >>> --- >>> sound/usb/card.c | 19 +++++++++++++++++++ >>> sound/usb/card.h | 2 ++ >>> 2 files changed, 21 insertions(+) >>> >>> diff --git a/sound/usb/card.c b/sound/usb/card.c >>> index 365f6d978608..27a89aaa0bf3 100644 >>> --- a/sound/usb/card.c >>> +++ b/sound/usb/card.c >>> @@ -170,6 +170,25 @@ struct snd_usb_stream >>> *snd_usb_find_suppported_substream(int card_idx, >>> } >>> EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); >>> +/* >>> + * in case the platform driver was not ready at the time of USB SND >>> + * device connect, expose an API to discover all connected USB devices >>> + * so it can populate any dependent resources/structures. >>> + */ >>> +void snd_usb_rediscover_devices(void) >>> +{ >>> + int i; >>> + >>> + mutex_lock(®ister_mutex); >>> + for (i = 0; i < SNDRV_CARDS; i++) { >>> + if (usb_chip[i]) >>> + if (platform_ops && platform_ops->connect_cb) >>> + platform_ops->connect_cb(usb_chip[i]); >> >> what happens if the USB device is removed while the platform device adds >> a port? >> >> This sounds super-racy to me. It's the same set of problems we're having >> between audio and display/DRM, I would be surprised if this function >> dealt with all corner cases of insertion/removal, bind/unbind. >> > > The chip array entries are all populated and removed while under the > register_mutex, so going over your race condition, we should see: > > Thread#1: > q6usb_component_probe() > --> snd_soc_usb_add_port() > --> snd_usb_rediscover_devices() > --> mutex_lock(register_mutex) > > Thread#2 > --> usb_audio_disconnect() > --> mutex_lock(register_mutex) > > So either thread#1 or thread#2 will complete first. If > > Thread#1 completes before thread#2: > SOC USB will notify DPCM backend of the device connection. Shortly > after, once thread#2 runs, we will get a disconnect event for the > connected device. > > Thread#2 completes before thread#1: > Then during snd_usb_rediscover_devices() we won't notify of any > connection for that particular chip index. You really want to capture this locking model as part of the commit messages or code, it'll help reviewers figure things out.
Hi Takashi, On 7/25/2023 2:27 AM, Takashi Iwai wrote: > On Tue, 25 Jul 2023 11:15:11 +0200, > Pierre-Louis Bossart wrote: >> >> >> >> On 7/25/23 04:34, Wesley Cheng wrote: >>> In case of notifying SND platform drivers of connection events, some of >>> these use cases, such as offloading, require an ASoC USB backend device to >>> be initialized before the events can be handled. If the USB backend device >>> has not yet been probed, this leads to missing initial USB audio device >>> connection events. >>> >>> Expose an API that traverses the usb_chip array for connected devices, and >>> to call the respective connection callback registered to the SND platform >>> driver. >>> >>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >>> --- >>> sound/usb/card.c | 19 +++++++++++++++++++ >>> sound/usb/card.h | 2 ++ >>> 2 files changed, 21 insertions(+) >>> >>> diff --git a/sound/usb/card.c b/sound/usb/card.c >>> index 365f6d978608..27a89aaa0bf3 100644 >>> --- a/sound/usb/card.c >>> +++ b/sound/usb/card.c >>> @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, >>> } >>> EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); >>> >>> +/* >>> + * in case the platform driver was not ready at the time of USB SND >>> + * device connect, expose an API to discover all connected USB devices >>> + * so it can populate any dependent resources/structures. >>> + */ >>> +void snd_usb_rediscover_devices(void) >>> +{ >>> + int i; >>> + >>> + mutex_lock(®ister_mutex); >>> + for (i = 0; i < SNDRV_CARDS; i++) { >>> + if (usb_chip[i]) >>> + if (platform_ops && platform_ops->connect_cb) >>> + platform_ops->connect_cb(usb_chip[i]); >> >> what happens if the USB device is removed while the platform device adds >> a port? > > That should be protected by the register_mutex. But there can be > other races (see below :) > >> This sounds super-racy to me. It's the same set of problems we're having >> between audio and display/DRM, I would be surprised if this function >> dealt with all corner cases of insertion/removal, bind/unbind. > > Yes, we need to be more careful about binding. > > For example, in the current patch set, I see no way to prevent > unloading snd-usb-audio-qmi module, and it allows user to cut off the > stuff during operation, which may break things while the kernel is > running the code of the unloaded module. You need to have a proper > module refcount management for avoiding such a scenario. Most of > drivers don't need it because ALSA core part already takes care of > it. But in this case, it requires a manual adjustment. > Sorry for the delayed response...this was routed to another folder, and missed it. I see, that is a valid situation, so I think the best way to address it is to do something like the following: static void __exit qc_usb_audio_offload_exit(void) { ... snd_usb_unregister_platform_ops(); for (idx = 0; idx < SNDRV_CARDS; idx++) qc_usb_audio_offload_disconnect(uadev[idx].chip); We'll first unregister the platform ops, so that we get no further connect/disconnect CBs, and then we'll forcefully ensure that all devices are removed/cleaned. Part of the USB offload disconnect sequence will also forcefully stop the offload path on the external DSP by issuing a disconnect indication QMI message. Then we can safely clean up the rest of the resources. We do have refcounting in place for several of the other structures, but I think in the module exit case, we need to ensure the offload path is stopped fully. Thanks Wesley Cheng
On Mon, 28 Aug 2023 23:25:14 +0200, Wesley Cheng wrote: > > Hi Takashi, > > On 7/25/2023 2:27 AM, Takashi Iwai wrote: > > On Tue, 25 Jul 2023 11:15:11 +0200, > > Pierre-Louis Bossart wrote: > >> > >> > >> > >> On 7/25/23 04:34, Wesley Cheng wrote: > >>> In case of notifying SND platform drivers of connection events, some of > >>> these use cases, such as offloading, require an ASoC USB backend device to > >>> be initialized before the events can be handled. If the USB backend device > >>> has not yet been probed, this leads to missing initial USB audio device > >>> connection events. > >>> > >>> Expose an API that traverses the usb_chip array for connected devices, and > >>> to call the respective connection callback registered to the SND platform > >>> driver. > >>> > >>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > >>> --- > >>> sound/usb/card.c | 19 +++++++++++++++++++ > >>> sound/usb/card.h | 2 ++ > >>> 2 files changed, 21 insertions(+) > >>> > >>> diff --git a/sound/usb/card.c b/sound/usb/card.c > >>> index 365f6d978608..27a89aaa0bf3 100644 > >>> --- a/sound/usb/card.c > >>> +++ b/sound/usb/card.c > >>> @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, > >>> } > >>> EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); > >>> +/* > >>> + * in case the platform driver was not ready at the time of USB SND > >>> + * device connect, expose an API to discover all connected USB devices > >>> + * so it can populate any dependent resources/structures. > >>> + */ > >>> +void snd_usb_rediscover_devices(void) > >>> +{ > >>> + int i; > >>> + > >>> + mutex_lock(®ister_mutex); > >>> + for (i = 0; i < SNDRV_CARDS; i++) { > >>> + if (usb_chip[i]) > >>> + if (platform_ops && platform_ops->connect_cb) > >>> + platform_ops->connect_cb(usb_chip[i]); > >> > >> what happens if the USB device is removed while the platform device adds > >> a port? > > > > That should be protected by the register_mutex. But there can be > > other races (see below :) > > > >> This sounds super-racy to me. It's the same set of problems we're having > >> between audio and display/DRM, I would be surprised if this function > >> dealt with all corner cases of insertion/removal, bind/unbind. > > > > Yes, we need to be more careful about binding. > > > > For example, in the current patch set, I see no way to prevent > > unloading snd-usb-audio-qmi module, and it allows user to cut off the > > stuff during operation, which may break things while the kernel is > > running the code of the unloaded module. You need to have a proper > > module refcount management for avoiding such a scenario. Most of > > drivers don't need it because ALSA core part already takes care of > > it. But in this case, it requires a manual adjustment. > > > > Sorry for the delayed response...this was routed to another folder, > and missed it. > > I see, that is a valid situation, so I think the best way to address > it is to do something like the following: > > static void __exit qc_usb_audio_offload_exit(void) > { > ... > snd_usb_unregister_platform_ops(); > for (idx = 0; idx < SNDRV_CARDS; idx++) > qc_usb_audio_offload_disconnect(uadev[idx].chip); > > We'll first unregister the platform ops, so that we get no further > connect/disconnect CBs, and then we'll forcefully ensure that all > devices are removed/cleaned. Part of the USB offload disconnect > sequence will also forcefully stop the offload path on the external > DSP by issuing a disconnect indication QMI message. > > Then we can safely clean up the rest of the resources. We do have > refcounting in place for several of the other structures, but I think > in the module exit case, we need to ensure the offload path is stopped > fully. Yes, the proper disconnection procedure is mandatory. In addition, you can have a module refcount increment upon the card binding, too. This may prevent the unexpected exit (although it's often still possible to unbind via sysfs). thanks, Takashi
diff --git a/sound/usb/card.c b/sound/usb/card.c index 365f6d978608..27a89aaa0bf3 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, } EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); +/* + * in case the platform driver was not ready at the time of USB SND + * device connect, expose an API to discover all connected USB devices + * so it can populate any dependent resources/structures. + */ +void snd_usb_rediscover_devices(void) +{ + int i; + + mutex_lock(®ister_mutex); + for (i = 0; i < SNDRV_CARDS; i++) { + if (usb_chip[i]) + if (platform_ops && platform_ops->connect_cb) + platform_ops->connect_cb(usb_chip[i]); + } + mutex_unlock(®ister_mutex); +} +EXPORT_SYMBOL_GPL(snd_usb_rediscover_devices); + /* * disconnect streams * called from usb_audio_disconnect() diff --git a/sound/usb/card.h b/sound/usb/card.h index db735680d155..dfd241334e3d 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -219,6 +219,7 @@ int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); int snd_usb_unregister_platform_ops(void); struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, struct snd_pcm_hw_params *params, int direction); +void snd_usb_rediscover_devices(void); #else static int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops) { @@ -235,5 +236,6 @@ static struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, { return NULL; } +void snd_usb_rediscover_devices(void) { } #endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ #endif /* __USBAUDIO_CARD_H */
In case of notifying SND platform drivers of connection events, some of these use cases, such as offloading, require an ASoC USB backend device to be initialized before the events can be handled. If the USB backend device has not yet been probed, this leads to missing initial USB audio device connection events. Expose an API that traverses the usb_chip array for connected devices, and to call the respective connection callback registered to the SND platform driver. Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> --- sound/usb/card.c | 19 +++++++++++++++++++ sound/usb/card.h | 2 ++ 2 files changed, 21 insertions(+)