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

[Takashi Iwai <tiwai@suse.de>]

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?

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.

> +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.

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.

> +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.


thanks,

Takashi