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

[Sricharan Ramabadhran]

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