Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <c3b147b2-ed54-f1f0-52f0-51d41199acd0@quicinc.com>
Date:   Wed, 4 Oct 2023 16:42:39 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v7 01/33] xhci: add support to allocate several
 interrupters
Content-Language: en-US
From:   Wesley Cheng <quic_wcheng@quicinc.com>
To:     Mathias Nyman <mathias.nyman@intel.com>,
        Mathias Nyman <mathias.nyman@linux.intel.com>,
        <gregkh@linuxfoundation.org>, <lgirdwood@gmail.com>,
        <broonie@kernel.org>, <perex@perex.cz>, <tiwai@suse.com>,
        <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <srinivas.kandagatla@linaro.org>, <bgoswami@quicinc.com>,
        <Thinh.Nguyen@synopsys.com>
CC:     <linux-kernel@vger.kernel.org>, <linux-usb@vger.kernel.org>,
        <alsa-devel@alsa-project.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>
References: <20230921214843.18450-1-quic_wcheng@quicinc.com>
 <20230921214843.18450-2-quic_wcheng@quicinc.com>
 <10ad0613-7e88-dbe8-c5a2-d535f8e9db03@linux.intel.com>
 <e3f3c8cd-6338-da08-d988-4d2ed68280e6@quicinc.com>
 <843897f1-3ce5-f8da-5f10-7d8a68849fd2@intel.com>
 <2127ef61-e263-2a0e-438a-6baa125aa70d@quicinc.com>
In-Reply-To: <2127ef61-e263-2a0e-438a-6baa125aa70d@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@linux.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