[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c3b147b2-ed54-f1f0-52f0-51d41199acd0@quicinc.com>
Date: Wed, 4 Oct 2023 16:42:39 -0700
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Mathias Nyman <mathias.nyman@...el.com>,
Mathias Nyman <mathias.nyman@...ux.intel.com>,
<gregkh@...uxfoundation.org>, <lgirdwood@...il.com>,
<broonie@...nel.org>, <perex@...ex.cz>, <tiwai@...e.com>,
<agross@...nel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<srinivas.kandagatla@...aro.org>, <bgoswami@...cinc.com>,
<Thinh.Nguyen@...opsys.com>
CC: <linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
<alsa-devel@...a-project.org>, <linux-arm-msm@...r.kernel.org>,
<devicetree@...r.kernel.org>
Subject: Re: [PATCH v7 01/33] xhci: add support to allocate several
interrupters
Hi Mathias,
On 10/4/2023 11:35 AM, Wesley Cheng wrote:
> Hi Mathias,
>
> On 10/4/2023 7:02 AM, Mathias Nyman wrote:
>> On 2.10.2023 23.07, Wesley Cheng wrote:
>>> Hi Mathias,
>>>
>>> On 9/28/2023 3:31 AM, Mathias Nyman wrote:
>>>> On 22.9.2023 0.48, Wesley Cheng wrote:
>>>>> From: Mathias Nyman <mathias.nyman@...ux.intel.com>
>>>>>
>>>>> Modify the XHCI drivers to accommodate for handling multiple event
>>>>> rings in
>>>>> case there are multiple interrupters. Add the required APIs so
>>>>> clients are
>>>>> able to allocate/request for an interrupter ring, and pass this
>>>>> information
>>>>> back to the client driver. This allows for users to handle the
>>>>> resource
>>>>> accordingly, such as passing the event ring base address to an
>>>>> audio DSP.
>>>>> There is no actual support for multiple MSI/MSI-X vectors.
>>>>>
>>>>> Factoring out XHCI interrupter APIs and structures done by Wesley
>>>>> Cheng, in
>>>>> order to allow for USB class drivers to utilze them.
>>>>>
>>>>> }
>>>>> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct
>>>>> xhci_interrupter *ir)
>>>>> +{
>>>>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>>> + unsigned int intr_num;
>>>>> +
>>>>> + /* interrupter 0 is primary interrupter, don't touchit */
>>>>> + if (!ir || !ir->intr_num || ir->intr_num >=
>>>>> xhci->max_interrupters) {
>>>>> + xhci_dbg(xhci, "Invalid secondary interrupter, can't
>>>>> remove\n");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + /* fixme, should we check xhci->interrupter[intr_num] == ir */
>>>>> + spin_lock(&xhci->lock);
>>>>
>>>> Needs to be spin_lock_irq() ir spin_lock_irqsave() as xhci->lock is
>>>> used in interrupt handler.
>>>>
>>>>
>>>>> + intr_num = ir->intr_num;
>>>>> + xhci_free_interrupter(xhci, ir);
>>>>> + xhci->interrupters[intr_num] = NULL;
>>>>> + spin_unlock(&xhci->lock);
>>>>
>>>> likewise
>>>>
>>>
>>> Let me check these again. In general, I think I will use both the
>>> xhci->mutex and xhci->lock where needed, because I believe we'd run
>>> into sleep while atomic issues
>>> while freeing the DMA memory. Will rework this and submit in the
>>> next rev.
>>>
>>
>> Maybe we need to split xhci_free_interrupter() into separate remove
>> and free functions
>>
>
> Thanks for sharing the work you've been doing. Yes, I did something
> similar as well on my end, but will refactor in your code and re-test.
>
>> Did some work on this, and on the sideband api in general.
>>
>> Code still has a lot of FIXMEs, and it's completely untested, but to
>> avoid us
>> from doing duplicate work I pushed it to my feature_interrupters
>> branch anyway
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
>> feature_interrupters
>> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters
>>
>
> Ok. Initial look at it seems like it will be fine, but will integrate
> and make changes where needed.
>
Had to make some minor tweaks here and there, but nothing major. Was
able to validate the changes on my end, and they look good. Will test a
bit more, and include these in my next submission. Will try to address
your FIXME tags as well.
Thanks
Wesley Cheng
Powered by blists - more mailing lists