Hi Adam,

A few comments inline, mainly on the locking & list traversal.

+struct mctp_pcc_mailbox {
+       u32 index;
+       struct pcc_mbox_chan *chan;
+       struct mbox_client client;
+       struct sk_buff_head packets;
+};
+
+/* The netdev structure. One of these per PCC adapter. */
+struct mctp_pcc_ndev {
+       /* spinlock to serialize access to queue that holds a copy of the
+        * sk_buffs that are also in the ring buffers of the mailbox.
+        */
+       spinlock_t lock;
+       struct net_device *ndev;
+       struct acpi_device *acpi_device;
+       struct mctp_pcc_mailbox inbox;
+       struct mctp_pcc_mailbox outbox;
+};
+
+static void *mctp_pcc_rx_alloc(struct mbox_client *c, int size)
+{
+       struct mctp_pcc_ndev *mctp_pcc_ndev;
+       struct mctp_pcc_mailbox *box;
+       struct sk_buff *skb;
+
+       mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
+       box = &mctp_pcc_ndev->inbox;
+
+       if (size > mctp_pcc_ndev->ndev->mtu)
+               return NULL;
+       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
+       if (!skb)
+               return NULL;
+       skb_put(skb, size);
+       skb->protocol = htons(ETH_P_MCTP);
+
+       spin_lock(&mctp_pcc_ndev->lock);
+       skb_queue_head(&box->packets, skb);
+       spin_unlock(&mctp_pcc_ndev->lock);

Given you're using your own locking here (which seems sensible, since
you're iterating elsewhere), you're not relying on the list's lock. In
which case you can use __skb_queue_head() (and __skb_unlink below) as
lockless variants.

+
+       return skb->data;
+}
+
+static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
+{
+       struct mctp_pcc_ndev *mctp_pcc_ndev;
+       struct pcc_header pcc_header;
+       struct sk_buff *skb = NULL;
+       struct mctp_skb_cb *cb;
+
+       mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
+       if (!buffer) {
+               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
+               return;
+       }
+
+       spin_lock(&mctp_pcc_ndev->lock);
+       skb_queue_walk(&mctp_pcc_ndev->inbox.packets, skb) {
+               if (skb->data != buffer)
+                       continue;
+               skb_unlink(skb, &mctp_pcc_ndev->inbox.packets);
+               break;
+       }
+       spin_unlock(&mctp_pcc_ndev->lock);
+
+       if (skb) {

The termination case of skb_queue_walk() will not leave skb as null.

Instead, you will probably want to use a temporary variable for the
iterator, and set skb when you find a match.

+               dev_dstats_rx_add(mctp_pcc_ndev->ndev, skb->len);
+               skb_reset_mac_header(skb);
+               skb_pull(skb, sizeof(pcc_header));
+               skb_reset_network_header(skb);
+               cb = __mctp_cb(skb);
+               cb->halen = 0;
+               netif_rx(skb);
+       }
+}
+
+static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int r)
+{
+       struct mctp_pcc_ndev *mctp_pcc_ndev;
+       struct mctp_pcc_mailbox *box;
+       struct sk_buff *skb = NULL;
+
+       mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, outbox.client);
+       box = container_of(c, struct mctp_pcc_mailbox, client);
+       spin_lock(&mctp_pcc_ndev->lock);
+       skb_queue_walk(&box->packets, skb) {
+               if (skb->data == mssg) {
+                       skb_unlink(skb, &box->packets);
+                       break;
+               }
+       }
+       spin_unlock(&mctp_pcc_ndev->lock);
+
+       if (skb)

Same as above; this will not be null in the not-found case, but will
point to the list head.

+               dev_consume_skb_any(skb);

... but even if so, you could pass skb as NULL here and avoid the if.

+}
+
+static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+       struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
+       struct pcc_header *pcc_header;
+       int len = skb->len;
+       int rc;
+
+       rc = skb_cow_head(skb, sizeof(*pcc_header));
+       if (rc) {
+               dev_dstats_tx_dropped(ndev);
+               kfree_skb(skb);
+               return NETDEV_TX_OK;
+       }
+
+       pcc_header = skb_push(skb, sizeof(*pcc_header));
+       pcc_header->signature = PCC_SIGNATURE | mpnd->outbox.index;
+       pcc_header->flags = PCC_CMD_COMPLETION_NOTIFY;
+       memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
+       pcc_header->length = len + MCTP_SIGNATURE_LENGTH;
+
+       spin_lock(&mpnd->lock);
+       skb_queue_head(&mpnd->outbox.packets, skb);
+       spin_unlock(&mpnd->lock);
+
+       rc = mbox_send_message(mpnd->outbox.chan->mchan, skb->data);
+
+       if (rc < 0) {
+               skb_unlink(skb, &mpnd->outbox.packets);

This does not hold the lock you are using for the list traversal. If you
want to rely on the list-internal locking, that's fine, but you need to
be consistent in your approach.

+               return NETDEV_TX_BUSY;
+       }
+
+       dev_dstats_tx_add(ndev, len);
+       return NETDEV_TX_OK;
+}
+
+static void drain_packets(struct sk_buff_head *list)
+{
+       struct sk_buff *skb;
+
+       while (!skb_queue_empty(list)) {
+               skb = skb_dequeue(list);
+               dev_consume_skb_any(skb);
+       }
+}
+
+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?

+       int mctp_pcc_mtu;
+
+       outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
+       if (IS_ERR(outbox->chan))
+               return PTR_ERR(outbox->chan);
+
+       inbox->chan = pcc_mbox_request_channel(&inbox->client, inbox->index);
+       if (IS_ERR(inbox->chan)) {
+               pcc_mbox_free_channel(outbox->chan);
+               return PTR_ERR(inbox->chan);
+       }
+
+       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"?

+       mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
+               sizeof(struct pcc_header);
+       ndev->mtu = MCTP_MIN_MTU;
+       ndev->max_mtu = mctp_pcc_mtu;
+       ndev->min_mtu = MCTP_MIN_MTU;
+
+       return 0;
+}
+
+static int mctp_pcc_ndo_stop(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;

No wrapping needed here either.

Cheers,


Jeremy