Re: [PATCH v4 18/32] sound: usb: Introduce QC USB SND offloading support

[Wesley Cheng]

Hi Takashi,

On 7/25/2023 12:26 AM, Takashi Iwai wrote:
> On Tue, 25 Jul 2023 04:34:02 +0200,
> Wesley Cheng wrote:
> > --- a/sound/usb/Kconfig
> > +++ b/sound/usb/Kconfig
> > @@ -165,6 +165,21 @@ config SND_BCD2000
> >   	  To compile this driver as a module, choose M here: the module
> >   	  will be called snd-bcd2000.
> >   
> > +config QC_USB_AUDIO_OFFLOAD
> > +	tristate "Qualcomm Audio Offload driver"
> > +	depends on QCOM_QMI_HELPERS
> > +	select SND_PCM
>
> So the driver can be enabled without CONFIG_SND_USB_AUDIO?  It makes
> little sense without it.
> Or is it set so intentionally for testing purpose?

Thanks for the review.  I'll change this to be dependent on 
CONFIG_SND_USB_AUDIO...it shouldn't exist in the end use case w/o it.

> About the code:
>
> > +/* Offloading IOMMU management */
> > +static unsigned long uaudio_get_iova(unsigned long *curr_iova,
> > +	size_t *curr_iova_size, struct list_head *head, size_t size)
> > +{
> > +	struct iova_info *info, *new_info = NULL;
> > +	struct list_head *curr_head;
> > +	unsigned long va = 0;
> > +	size_t tmp_size = size;
> > +	bool found = false;
> > +
> > +	if (size % PAGE_SIZE) {
> > +		dev_err(uaudio_qdev->dev, "size %zu is not page size multiple\n",
> > +			size);
> > +		goto done;
>
> This can be easily triggered by user-space as it's passed directly
> from the mmap call, and it implies that you can fill up the messages
> easily.  It's safer to make it debug message or add the rate limit.
>
> Ditto for other error messages.

Got it, I'll make sure to address the above dev_err().

> > +static void disable_audio_stream(struct snd_usb_substream *subs)
> > +{
> > +	struct snd_usb_audio *chip = subs->stream->chip;
> > +
> > +	if (subs->data_endpoint || subs->sync_endpoint) {
> > +		close_endpoints(chip, subs);
> > +
> > +		mutex_lock(&chip->mutex);
> > +		subs->cur_audiofmt = NULL;
> > +		mutex_unlock(&chip->mutex);
> > +	}
>
> Now looking at this and...
>
> > +static int enable_audio_stream(struct snd_usb_substream *subs,
> > +				snd_pcm_format_t pcm_format,
> > +				unsigned int channels, unsigned int cur_rate,
> > +				int datainterval)
> > +{
>
> ... this implementation, I wonder whether it'd be better to modify and
> export  snd_usb_hw_params() snd snd_usb_hw_free() to fit with qcom
> driver.  Then you can avoid lots of open code.

I think the problem is that snd_usb_hw_params assumes that we've already 
done a PCM open on the PCM device created by USB SND.  However, with the 
offload path, we don't reference the USB PCM device, but the one created 
by the platform sound card.  Hence, I don't have access to the 
snd_pcm_substream.

I attempted to derive snd_pcm_substream from snd_usb_substream, but 
since PCM open isn't run, it doesn't provide a valid structure.

What do you think about adding a wrapper to snd_usb_hw_params?  Have a 
version that will take in snd_usb_substream, and another that is 
registered to hw_params().

> In general, if you see a direct use of chip->mutex, it can be often
> done better in a different form.  The use of an internal lock or such
> from an external driver is always fragile and error-prone.
>
> Also, the current open-code misses the potential race against the
> disconnection during the operation.  In snd-usb-audio, it protects
> with snd_usb_lock_shutdown() and snd_usb_unlock_shutdown() pairs.

I agree...I think then the best approach would be something like the 
above, ie:

int snd_usb_hw_params(struct snd_pcm_substream *substream,
			     struct snd_pcm_hw_params *hw_params)
{
	struct snd_usb_substream *subs = substream->runtime->private_data;

	snd_usb_ep_attach(subs, hw_params);
...

int snd_usb_ep_attach(...)
{
	//implementation of current code in snd_usb_hw_params()
}
EXPORT_SYMBOL(snd_usb_ep_attach);

> > +static int __init qc_usb_audio_offload_init(void)
> > +{
> > +	struct uaudio_qmi_svc *svc;
> > +	int ret;
> > +
> > +	ret = snd_usb_register_platform_ops(&offload_ops);
> > +	if (ret < 0)
> > +		return ret;
>
> Registering the ops at the very first opens a potential access to the
> uninitialized stuff.  Imagine a suspend happens right after this
> point.  As the ops is already registered, it'll enter to the
> suspend_cb callback and straight to Oops.
>
> > +static void __exit qc_usb_audio_offload_exit(void)
> > +{
> > +	struct uaudio_qmi_svc *svc = uaudio_svc;
> > +
> > +	qmi_handle_release(svc->uaudio_svc_hdl);
> > +	flush_workqueue(svc->uaudio_wq);
> > +	destroy_workqueue(svc->uaudio_wq);
> > +	kfree(svc);
> > +	uaudio_svc = NULL;
> > +	snd_usb_unregister_platform_ops();
>
> Similarly, the unregister order has to be careful, too.

Let me re-organize it a bit more.

Thanks
Wesley Cheng