Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 25 Jul 2023 11:27:27 +0200
Message-ID: <87wmyotk74.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc:     Wesley Cheng <quic_wcheng@quicinc.com>, agross@kernel.org,
        andersson@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        catalin.marinas@arm.com, will@kernel.org, mathias.nyman@intel.com,
        gregkh@linuxfoundation.org, lgirdwood@gmail.com,
        broonie@kernel.org, perex@perex.cz, tiwai@suse.com,
        srinivas.kandagatla@linaro.org, bgoswami@quicinc.com,
        Thinh.Nguyen@synopsys.com, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-usb@vger.kernel.org,
        alsa-devel@alsa-project.org, quic_jackp@quicinc.com,
        oneukum@suse.com, albertccwang@google.com, o-takashi@sakamocchi.jp
Subject: Re: [PATCH v4 31/32] sound: usb: card: Allow for rediscovery of connected USB SND devices
In-Reply-To: <671a524d-b4c8-78d8-33de-40170a23d189@linux.intel.com>
References: <20230725023416.11205-1-quic_wcheng@quicinc.com>
        <20230725023416.11205-32-quic_wcheng@quicinc.com>
        <671a524d-b4c8-78d8-33de-40170a23d189@linux.intel.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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(&register_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