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
