linux-kernel - Re: [PATCH v3 08/28] ASoC: qcom: Add USB backend ASoC driver for Q6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 10 Mar 2023 07:21:20 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Wesley Cheng <quic_wcheng@...cinc.com>, mathias.nyman@...el.com,
        perex@...ex.cz, broonie@...nel.org, lgirdwood@...il.com,
        krzysztof.kozlowski+dt@...aro.org, agross@...nel.org,
        Thinh.Nguyen@...opsys.com, bgoswami@...cinc.com,
        andersson@...nel.org, robh+dt@...nel.org,
        gregkh@...uxfoundation.org, tiwai@...e.com
Cc:     linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-usb@...r.kernel.org, quic_jackp@...cinc.com,
        quic_plai@...cinc.com
Subject: Re: [PATCH v3 08/28] ASoC: qcom: Add USB backend ASoC driver for Q6



On 09/03/2023 19:38, Wesley Cheng wrote:
> Hi Srinivas,
> 
> On 3/9/2023 1:01 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 08/03/2023 23:57, Wesley Cheng wrote:
>>> Create a USB BE component that will register a new USB port to the 
>>> ASoC USB
>>> framework.  This will handle determination on if the requested audio
>>> profile is supported by the USB device currently selected.
>>>
>>> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
>>
>> Thanks Wesley for the patch, I have few minor comments.
>>
> 
> Thanks for the review!
> 
>>> ---
>>>   include/sound/q6usboffload.h  |  20 ++++
>>>   sound/soc/qcom/Kconfig        |   4 +
>>>   sound/soc/qcom/qdsp6/Makefile |   1 +
>>>   sound/soc/qcom/qdsp6/q6usb.c  | 208 ++++++++++++++++++++++++++++++++++
>>>   4 files changed, 233 insertions(+)
>>>   create mode 100644 include/sound/q6usboffload.h
>>>   create mode 100644 sound/soc/qcom/qdsp6/q6usb.c
>>>
>>> diff --git a/include/sound/q6usboffload.h b/include/sound/q6usboffload.h
>>> new file mode 100644
>>> index 000000000000..4fb1912d9f55
>>> --- /dev/null
>>> +++ b/include/sound/q6usboffload.h
>>> @@ -0,0 +1,20 @@
>>> +/* SPDX-License-Identifier: GPL-2.0
>>> + *
>>> + * linux/sound/q6usboffload.h -- QDSP6 USB offload
>>> + *
>>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 
>>> rights reserved.
>>> + */
>>> +
>>> +/**
>>> + * struct q6usb_offload
>>> + * @dev - dev handle to usb be
>>> + * @sid - streamID for iommu
>>> + * @intr_num - usb interrupter number
>>> + * @domain - allocated iommu domain
>>> + **/
>>> +struct q6usb_offload {
>>> +    struct device *dev;
>>> +    long long sid;
>>> +    u32 intr_num;
>>> +    struct iommu_domain *domain;
>> Why do we need to store this domain, You can remove this along with 
>> the one line that gets domain in probe function.
>>
> 
> We'll need a reference to the iommu domain, because the QC USB offload 
> driver will be the one that is going to map the XHCI interrupter and 
> transfer ring regions for the audio DSP.  This happens when a USB QMI 

this is okay, AFAIU, as long as uaudio_qdev->dev pointer is used in dma 
alloc apis like dma_map*, dma_alloc_* you would not need to handle 
iommu_domain directly like this in drivers.


--srini

> enable stream request is received in the USB offload driver.  Please 
> refer to:
> 
> static int prepare_qmi_response(struct snd_usb_substream *subs,
>          struct qmi_uaudio_stream_req_msg_v01 *req_msg,
>          struct qmi_uaudio_stream_resp_msg_v01 *resp, int info_idx)
> {
> ...
>      xhci_pa = xhci_get_ir_resource(subs->dev, ir);
>      if (!xhci_pa) {
>          dev_err(uaudio_qdev->dev,
>              "failed to get sec event ring address\n");
>          ret = -ENODEV;
>          goto free_sec_ring;
>      }
> ...
>      va = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, xhci_pa, 
> PAGE_SIZE,
>              NULL);
>      if (!va) {
>          ret = -ENOMEM;
>          goto free_sec_ring;
>      }
> 
> This is just an example for mapping the XHCI secondary interrupter.  We 
> will also do the same for the transfer ring.
> 
> Thanks
> Wesley Cheng