Hi Adam,

In addition to the below comment, I am removing the additional lock on
the skb lists and using the internal one for all operations.  It leads
to leaner and cleaner code.

Ok, neat!

Just be careful with locking as you're iterating the queues. Your
current approach of doing the drain under one lock acquire is probably
the best, if you can do the same with the queue-internal locking.

quoted

quoted

+       mctp_pcc_ndev->inbox.chan->rx_alloc = mctp_pcc_rx_alloc;
+       mctp_pcc_ndev->outbox.chan->manage_writes = true;
+
+       /* There is no clean way to pass the MTU to the callback function
+        * used for registration, so set the values ahead of time.
+        */

For my own clarity, what's "the callback function used for
registration"?

Actually, this is not longer true: we can do it in ndo_open, and it
is clean.  Removed the comment.

OK, The current patch *does* do it in ndo_open though, hence my
confusion.

From your other reply:

quoted

quoted

+static int mctp_pcc_ndo_open(struct net_device *ndev)
+{
+       struct mctp_pcc_ndev *mctp_pcc_ndev =
+           netdev_priv(ndev);
+       struct mctp_pcc_mailbox *outbox =
+           &mctp_pcc_ndev->outbox;
+       struct mctp_pcc_mailbox *inbox =
+           &mctp_pcc_ndev->inbox;

Minor: I don't think these need wrapping?

The outbox and inbox lines are longer than the mctp_pcc_ndev line, and
they depend on it.  This ordering and wrapping passes the xmas  tree
check and keeps assignment with declaration.

If you need a specific ordering for actual correctness, no need to
force that into a reverse-christmas-tree. Spacing requirements like that
are secondary.

The only remaining query I had was the TX flow control. You're returning
NETDEV_TX_BUSY while the queues are still running, so are likely to get
repeated TX in a loop there.

Cheers,


Jeremy