Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <349a7b1c-915f-4f58-260f-900aa7e3db65@quicinc.com>
Date:   Wed, 20 Sep 2023 17:26:25 -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>
From:   Chris Lew <quic_clew@quicinc.com>
In-Reply-To: <20230920053317.2165867-3-quic_srichara@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 9/19/2023 10:33 PM, Sricharan Ramabadhran wrote:

> @@ -122,6 +123,9 @@ static DEFINE_XARRAY_ALLOC(qrtr_ports);
>    * @qrtr_tx_lock: lock for qrtr_tx_flow inserts
>    * @rx_queue: receive queue
>    * @item: list item for broadcast list
> + * @kworker: worker thread for recv work
> + * @task: task to run the worker thread
> + * @read_data: scheduled work for recv work

I think I made these descriptions a bit ambiguous with "recv work". 
Since we are only parsing DEL_PROC messages at the moment, the 
descriptions should be more accurate on what they are for.

>    */
>   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.

> +		if (cb->type == QRTR_TYPE_DEL_PROC) {
> +			/* Free tx flow counters */
> +			mutex_lock(&node->qrtr_tx_lock);
> +			radix_tree_for_each_slot(slot, &node->qrtr_tx_flow, &iter, 0) {
> +				flow = rcu_dereference_raw(*slot);
> +				wake_up_interruptible_all(&flow->resume_tx);
> +			}
> +			mutex_unlock(&node->qrtr_tx_lock);
> +

I don't see any other places where we use rcu_dereference_raw for the 
flow. Does this need to be updated for the rest of the places we get the 
flow?

The same loop is done in qrtr_endpoint_unregister() so maybe we should 
look into adding a helper for this logic?

> +			/* Translate DEL_PROC to BYE for local enqueue */
> +			cb->type = QRTR_TYPE_BYE;
> +			pkt = (struct qrtr_ctrl_pkt *)skb->data;
> +			memset(pkt, 0, sizeof(*pkt));
> +			pkt->cmd = cpu_to_le32(QRTR_TYPE_BYE);
> +

Are we relying on the remote to program the destination of this packet 
to be the control port?

Thanks,
Chris