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: <dffdbd5c-3d21-5cee-03d6-b89e0caf9a41@quicinc.com>
Date:   Mon, 25 Sep 2023 16:55:38 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V2 net-next 2/2] net: qrtr: Add support for processing
 DEL_PROC type control message
Content-Language: en-US
To:     Sricharan Ramabadhran <quic_srichara@quicinc.com>,
        <mani@kernel.org>, <davem@davemloft.net>, <edumazet@google.com>,
        <kuba@kernel.org>, <pabeni@redhat.com>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <netdev@vger.kernel.org>, <quic_viswanat@quicinc.com>,
        <horms@kernel.org>
References: <20230920053317.2165867-1-quic_srichara@quicinc.com>
 <20230920053317.2165867-3-quic_srichara@quicinc.com>
 <349a7b1c-915f-4f58-260f-900aa7e3db65@quicinc.com>
 <0db026e7-6017-83ed-4071-c2ea6d72e3d7@quicinc.com>
From:   Chris Lew <quic_clew@quicinc.com>
In-Reply-To: <0db026e7-6017-83ed-4071-c2ea6d72e3d7@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 9/23/2023 6:49 PM, Sricharan Ramabadhran wrote:
>>>    */
>>>   struct qrtr_node {
>>>       struct mutex ep_lock;
>>> @@ -134,6 +138,9 @@ struct qrtr_node {
>>>       struct sk_buff_head rx_queue;
>>>       struct list_head item;
>>> +    struct kthread_worker kworker;
>>> +    struct task_struct *task;
>>> +    struct kthread_work read_data;
>>
>> I think our own kthread here might have been overkill. I forget why we 
>> needed it instead of using a workqueue.
> 
>    I added a workqueue here because endpoint post is getting called from
>    atomic contexts and below DEL_PROC handling acquires qrtr_tx_lock.


Got it, I think deferring the processing makes sense. I was more 
focusing on the fact that we are creating our own kthread instead of 
using the system workqueues.

Prior to commit e04df98adf7d ("net: qrtr: Remove receive worker"), this 
was a work_struct. I think we should keep it as a work_struct until we 
can motivate why a qrtr_node needs it's own kthread.