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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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?


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux