diff mbox series

[v9,34/34] ASoC: usb: Rediscover USB SND devices on USB port add

Message ID 20231017200109.11407-35-quic_wcheng@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Introduce QC USB SND audio offloading support | expand

Commit Message

Wesley Cheng Oct. 17, 2023, 8:01 p.m. UTC
In case the USB backend device has not been initialized/probed, USB SND
device connections can still occur.  When the USB backend is eventually
made available, previous USB SND device connections are not communicated to
the USB backend.  Call snd_usb_rediscover_devices() to generate the connect
callbacks for all USB SND devices connected.  This will allow for the USB
backend to be updated with the current set of devices available.

The chip array entries are all populated and removed while under the
register_mutex, so going over potential race conditions:

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() it won't notify of any
  connection for that particular chip index.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 sound/soc/soc-usb.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Pierre-Louis Bossart Oct. 17, 2023, 11:11 p.m. UTC | #1
On 10/17/23 15:01, Wesley Cheng wrote:
> In case the USB backend device has not been initialized/probed, USB SND
> device connections can still occur.  When the USB backend is eventually
> made available, previous USB SND device connections are not communicated to
> the USB backend.  Call snd_usb_rediscover_devices() to generate the connect
> callbacks for all USB SND devices connected.  This will allow for the USB
> backend to be updated with the current set of devices available.
> 
> The chip array entries are all populated and removed while under the
> register_mutex, so going over potential race conditions:
> 
> 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() it won't notify of any
>   connection for that particular chip index.
Looks like you are assuming the regular USB audio stuff is probed first?

What if it's not the case? Have you tested with a manual 'blacklist' and
"modprobe" sequence long after all the DSP stuff is initialized?

It really reminds me of audio+display issues, and the same opens apply IMHO.
Wesley Cheng Oct. 23, 2023, 9:54 p.m. UTC | #2
Hi Pierre,

On 10/17/2023 4:11 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 10/17/23 15:01, Wesley Cheng wrote:
>> In case the USB backend device has not been initialized/probed, USB SND
>> device connections can still occur.  When the USB backend is eventually
>> made available, previous USB SND device connections are not communicated to
>> the USB backend.  Call snd_usb_rediscover_devices() to generate the connect
>> callbacks for all USB SND devices connected.  This will allow for the USB
>> backend to be updated with the current set of devices available.
>>
>> The chip array entries are all populated and removed while under the
>> register_mutex, so going over potential race conditions:
>>
>> 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() it won't notify of any
>>    connection for that particular chip index.
> Looks like you are assuming the regular USB audio stuff is probed first?
> 
> What if it's not the case? Have you tested with a manual 'blacklist' and
> "modprobe" sequence long after all the DSP stuff is initialized?
> 
> It really reminds me of audio+display issues, and the same opens apply IMHO.

Not necessarily...if the USB audio driver is not probed, then that is 
the same scenario as when there is no USB audio capable device plugged 
in, while the offload path is waiting for the connect event. I think 
this is the standard scenario.

In the situation where the platform sound card hasn't probed yet and USB 
audio devices are being identified, then that is basically the scenario 
that would be more of an issue, since its USB SND that notifies of the 
connection state (at the time of connect/disconnect).

I've tried with building these drivers as modules and probing them at 
different times/sequences, and I haven't seen an issue so far.

Thanks
Wesley Cheng
Pierre-Louis Bossart Oct. 24, 2023, 1:35 p.m. UTC | #3
On 10/23/23 16:54, Wesley Cheng wrote:
> Hi Pierre,
> 
> On 10/17/2023 4:11 PM, Pierre-Louis Bossart wrote:
>>
>>
>> On 10/17/23 15:01, Wesley Cheng wrote:
>>> In case the USB backend device has not been initialized/probed, USB SND
>>> device connections can still occur.  When the USB backend is eventually
>>> made available, previous USB SND device connections are not
>>> communicated to
>>> the USB backend.  Call snd_usb_rediscover_devices() to generate the
>>> connect
>>> callbacks for all USB SND devices connected.  This will allow for the
>>> USB
>>> backend to be updated with the current set of devices available.
>>>
>>> The chip array entries are all populated and removed while under the
>>> register_mutex, so going over potential race conditions:
>>>
>>> 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() it won't notify of any
>>>    connection for that particular chip index.
>> Looks like you are assuming the regular USB audio stuff is probed first?
>>
>> What if it's not the case? Have you tested with a manual 'blacklist' and
>> "modprobe" sequence long after all the DSP stuff is initialized?
>>
>> It really reminds me of audio+display issues, and the same opens apply
>> IMHO.
> 
> Not necessarily...if the USB audio driver is not probed, then that is
> the same scenario as when there is no USB audio capable device plugged
> in, while the offload path is waiting for the connect event. I think
> this is the standard scenario.
> 
> In the situation where the platform sound card hasn't probed yet and USB
> audio devices are being identified, then that is basically the scenario
> that would be more of an issue, since its USB SND that notifies of the
> connection state (at the time of connect/disconnect).

Not following if this scenario is covered?

> I've tried with building these drivers as modules and probing them at
> different times/sequences, and I haven't seen an issue so far.

The scenario I have in mind is this:

the platform driver is on the deny list, the USB driver detects a
device. When the platform driver probes at a later time (with a manual
modprobe to make delays really long), how would the notification be handled?

Between audio and display, we use the 'drm_audio_component' layer to
model these sort of run-time binding between independent driver stacks.
It's not used here but we need a moral equivalent, don't we?

It would really help if you documented a bit more the dependencies or
timing assumptions, to make sure we have a stable solution to build on.
Wesley Cheng Oct. 25, 2023, 10:31 p.m. UTC | #4
Hi Pierre,

On 10/24/2023 6:35 AM, Pierre-Louis Bossart wrote:
> 
> 
> On 10/23/23 16:54, Wesley Cheng wrote:
>> Hi Pierre,
>>
>> On 10/17/2023 4:11 PM, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 10/17/23 15:01, Wesley Cheng wrote:
>>>> In case the USB backend device has not been initialized/probed, USB SND
>>>> device connections can still occur.  When the USB backend is eventually
>>>> made available, previous USB SND device connections are not
>>>> communicated to
>>>> the USB backend.  Call snd_usb_rediscover_devices() to generate the
>>>> connect
>>>> callbacks for all USB SND devices connected.  This will allow for the
>>>> USB
>>>> backend to be updated with the current set of devices available.
>>>>
>>>> The chip array entries are all populated and removed while under the
>>>> register_mutex, so going over potential race conditions:
>>>>
>>>> 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() it won't notify of any
>>>>     connection for that particular chip index.
>>> Looks like you are assuming the regular USB audio stuff is probed first?
>>>
>>> What if it's not the case? Have you tested with a manual 'blacklist' and
>>> "modprobe" sequence long after all the DSP stuff is initialized?
>>>
>>> It really reminds me of audio+display issues, and the same opens apply
>>> IMHO.
>>
>> Not necessarily...if the USB audio driver is not probed, then that is
>> the same scenario as when there is no USB audio capable device plugged
>> in, while the offload path is waiting for the connect event. I think
>> this is the standard scenario.
>>
>> In the situation where the platform sound card hasn't probed yet and USB
>> audio devices are being identified, then that is basically the scenario
>> that would be more of an issue, since its USB SND that notifies of the
>> connection state (at the time of connect/disconnect).
> 
> Not following if this scenario is covered?
> 

Yes, this is covered.  For example, if there are already devices 
connected, but the platform sound card is still unbound.  Then this 
rediscover API will be called to traverse through the list of connected 
USB sound devices, so that the USB DPCM dai can know about their 
existence when it is probed.

>> I've tried with building these drivers as modules and probing them at
>> different times/sequences, and I haven't seen an issue so far.
> 
> The scenario I have in mind is this:
> 
> the platform driver is on the deny list, the USB driver detects a
> device. When the platform driver probes at a later time (with a manual
> modprobe to make delays really long), how would the notification be handled?
> 

So that is essentially the same scenario as when there is no USB device 
connected, ie no USB class driver is bounded to anything.  Since the 
notifications are all handled within USB SND (USB class driver) then if 
the module isn't loaded yet, no notification is sent to the DPCM USB 
backend.  Once you say...modprobe the USB SND driver, then the USB 
interface probe occurs, and that would issue the connect callback from 
the USB SND probe routine. (keep in mind these are not platform devices, 
we're working with devices under the usb bus)

> Between audio and display, we use the 'drm_audio_component' layer to
> model these sort of run-time binding between independent driver stacks.
> It's not used here but we need a moral equivalent, don't we?
> 
> It would really help if you documented a bit more the dependencies or
> timing assumptions, to make sure we have a stable solution to build on.
> 

I can add this to the RST that I'll make in detail, and add a summary 
here in the commit message.

Thanks
Wesley Cheng
diff mbox series

Patch

diff --git a/sound/soc/soc-usb.c b/sound/soc/soc-usb.c
index 7407678a993e..60aafbe87c36 100644
--- a/sound/soc/soc-usb.c
+++ b/sound/soc/soc-usb.c
@@ -104,6 +104,8 @@  struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev, void *priv,
 	list_add_tail(&usb->list, &usb_ctx_list);
 	mutex_unlock(&ctx_mutex);
 
+	snd_usb_rediscover_devices();
+
 	return usb;
 }
 EXPORT_SYMBOL_GPL(snd_soc_usb_add_port);