Re: [PATCH net-next v25 1/1] mctp pcc: Implement MCTP over PCC Transport

[Jeremy Kerr <jk@codeconstruct.com.au>]

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