Re: [PATCH V2 net-next 2/2] net: qrtr: Add support for processing DEL_PROC type control message

[Chris Lew]

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