On 9/21/2023 5:56 AM, Chris Lew wrote:
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.
ok, btw, then would keep your authorship in first place.
In our downstream, there were multiple changes around here and
could not get a clear author here. I fixed this while testing
with Modem SSR recently for our tree, that said, will fix it
next version.
*/
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.
+ 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?
Yes, without the rcu_dereference_raw there is a SPARSE warning.
For some reason, did not see the same in other places where flow is
de-referenced. That said, yeah, will pull this common code and
create a new helper.
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?
Yeah, targets like SDX modems, have a qrtr_fwd_del_proc in the
endpoint_unregister path.
Regards,
Sricharan
