Re: [net-next v39] mctp pcc: Implement MCTP over PCC Transport

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

Hi Adam,

> +static void mctp_pcc_client_rx_callback(struct mbox_client *cl, void *mssg)
> +{
> +       struct acpi_pcct_ext_pcc_shared_memory pcc_header;
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       struct mctp_pcc_mailbox *inbox;
> +       struct mctp_skb_cb *cb;
> +       struct sk_buff *skb;
> +       u32 header_length;
> +       int size;
> +
> +       mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, inbox.client);
> +       inbox = &mctp_pcc_ndev->inbox;
> +       memcpy_fromio(&pcc_header, inbox->chan->shmem, sizeof(pcc_header));
> +
> +       // The message must at least have the PCC command indicating it is an MCTP
> +       // message followed by the MCTP header, or we have a malformed message.
> +       // This may be run on big endian system, but the data in the buffer is
> +       // explicitly little endian.
> +       header_length = le32_to_cpu(pcc_header.length);
> +       if (header_length < sizeof(pcc_header.command) + sizeof(struct mctp_hdr)) {
> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +               return;
> +       }
> +       // If the reported size is larger than the shared memory minus headers,
> +       // something is wrong and treat the buffer as corrupted data.
> +       if (header_length > inbox->chan->shmem_size - PCC_EXTRA_LEN) {
> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +               return;
> +       }
> +       if (memcmp(&pcc_header.command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH) != 0) {
> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +               return;
> +       }
> +
> +       size = header_length + PCC_EXTRA_LEN;
> +       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
> +       if (!skb) {
> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +               return;
> +       }

Minor, but you have four instances of the dstats_rx_dropped call, that
may work as a shared error path.

> +       skb_put(skb, size);
> +       skb->protocol = htons(ETH_P_MCTP);
> +       memcpy_fromio(skb->data, inbox->chan->shmem, size);
> +       dev_dstats_rx_add(mctp_pcc_ndev->ndev, size);
> +       skb_pull(skb, sizeof(pcc_header));
> +       skb_reset_mac_header(skb);
> +       skb_reset_network_header(skb);
> +       cb = __mctp_cb(skb);
> +       cb->halen = 0;
> +       netif_rx(skb);
> +}
> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +       struct acpi_pcct_ext_pcc_shared_memory *pcc_header;
> +       struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> +       int len = skb->len;
> +
> +       /* Consolidated a fragmented packet into contiguous memory */
> +       if (skb_is_nonlinear(skb)) {
> +               if (skb_linearize(skb))
> +                       goto error;
> +       }

skb_linearize() already has the skb_is_nonlinear() check.

However, you don't need to call skb_linearize() anyway, as that will
happen for you in validate_xmit_skb(), since the driver does not
advertise support for nonlinear skbs.

> +
> +       if (skb_cow_head(skb, sizeof(*pcc_header)))
> +               goto error;
> +
> +       /**
> +        * This code could potentially be run on A Big Endian
> +        * System.  The ACPI specification requires that values
> +        * in the shared buffer be little endian.
> +        */

Minor, but I don't think this comment is necessary.

More importantly though, you're getting a bunch of warnings from the use
of these endian helpers:

  drivers/net/mctp/mctp-pcc.c:70:25: warning: cast to restricted __le32
  drivers/net/mctp/mctp-pcc.c:125:31: warning: incorrect type in assignment (different base types)
  drivers/net/mctp/mctp-pcc.c:125:31:    expected unsigned int [usertype] signature
  drivers/net/mctp/mctp-pcc.c:125:31:    got restricted __le32 [usertype]
  drivers/net/mctp/mctp-pcc.c:126:27: warning: incorrect type in assignment (different base types)
  drivers/net/mctp/mctp-pcc.c:126:27:    expected unsigned int [usertype] flags
  drivers/net/mctp/mctp-pcc.c:126:27:    got restricted __le32 [usertype]
  drivers/net/mctp/mctp-pcc.c:128:28: warning: incorrect type in assignment (different base types)
  drivers/net/mctp/mctp-pcc.c:128:28:    expected unsigned int [usertype] length
  drivers/net/mctp/mctp-pcc.c:128:28:    got restricted __le32 [usertype]

Looks like struct acpi_pcct_ext_pcc_shared_memory does not specify
fields as little endian, but host endian.

You could change to endian-annotated types to those ACPI structs, but
there are probably reasons why that's not already done. This might be an
indication that there are no big-endian ACPI platforms, so you don't
need the endian conversions?

> +       pcc_header = skb_push(skb, sizeof(*pcc_header));
> +       pcc_header->signature = cpu_to_le32(PCC_SIGNATURE | mpnd->outbox.index);
> +       pcc_header->flags = cpu_to_le32(PCC_CMD_COMPLETION_NOTIFY);
> +       memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
> +       pcc_header->length =  cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
> +
> +       if (mbox_send_message(mpnd->outbox.chan->mchan, skb) < 0) {
> +               // Remove the header in case it gets sent again
> +               skb_pull(skb, sizeof(*pcc_header));
> +               netif_stop_queue(ndev);
> +               return NETDEV_TX_BUSY;
> +       }
> +
> +       return NETDEV_TX_OK;
> +error:
> +       dev_dstats_tx_dropped(ndev);
> +       kfree_skb(skb);
> +       return NETDEV_TX_OK;
> +}
> +
> +static void mctp_pcc_tx_prepare(struct mbox_client *cl, void *mssg)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       struct mctp_pcc_mailbox *outbox;
> +       struct sk_buff *skb = mssg;
> +
> +       mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, outbox.client);
> +       outbox = &mctp_pcc_ndev->outbox;
> +
> +       /* The PCC Mailbox typically does not make use of the mssg pointer
> +        * The mctp-over pcc driver is the only client that uses it.
> +        * This value should always be non-null; it is possible
> +        * that a change in the Mailbox level will break that assumption.
> +        */
> +       if (!skb) {
> +               WARN_ONCE(!skb, "%s called with null message.\n", __func__);
> +               return;
> +       }

I'd suggest netdev_warn_once(), so we know which device is causing this.

> +
> +       if (skb->len > outbox->chan->shmem_size) {
> +               dev_dstats_tx_dropped(mctp_pcc_ndev->ndev);
> +               return;
> +       }
> +       memcpy_toio(outbox->chan->shmem,  skb->data, skb->len);

Super minor: double space before skb->data there.

> +       dev_dstats_tx_add(mctp_pcc_ndev->ndev, skb->len);
> +}
> +
> +static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int r)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       struct sk_buff *skb = mssg;
> +
> +       /*
> +        * If there is a packet in flight during driver cleanup
> +        * It may have been freed already.
> +        */
> +       if (!mssg)
> +               return;
> +       /*
> +        * If the return code is non-zero, we should not report the packet
> +        * as transmitted.  However, we are in IRQ context right now, and we
> +        * cannot safely write transmission statistics.
> +        */

This reads as if you're not updating stats at all, but you do so in
mctp_pcc_tx_prepare(). I don't think this comment is necessary - if
you really want to mention this, add a comment on the
dev_dstats_tx_add() to indicate why you're calling it early.

> +       mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, outbox.client);
> +       dev_consume_skb_any(skb);
> +       netif_wake_queue(mctp_pcc_ndev->ndev);
> +}
> +
> +static int mctp_pcc_open(struct net_device *ndev)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
> +       struct mctp_pcc_mailbox *outbox, *inbox;
> +
> +       outbox = &mctp_pcc_ndev->outbox;
> +       inbox = &mctp_pcc_ndev->inbox;
> +
> +       outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
> +       if (IS_ERR(outbox->chan))
> +               return PTR_ERR(outbox->chan);
> +
> +       if (outbox->chan->shmem_size  <  MCTP_PCC_MIN_SIZE) {

Minor: odd spacing here.

> +               pcc_mbox_free_channel(outbox->chan);
> +               return -EINVAL;
> +       }
> +
> +       inbox->client.rx_callback = mctp_pcc_client_rx_callback;
> +       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);
> +       }
> +       if (inbox->chan->shmem_size < MCTP_PCC_MIN_SIZE) {
> +               pcc_mbox_free_channel(outbox->chan);
> +               pcc_mbox_free_channel(inbox->chan);
> +               return -EINVAL;
> +       }
> +       return 0;
> +}
> +
> +static int mctp_pcc_stop(struct net_device *ndev)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       unsigned int count, idx;
> +       struct mbox_chan *chan;
> +       struct sk_buff *skb;
> +
> +       mctp_pcc_ndev = netdev_priv(ndev);
> +       chan = mctp_pcc_ndev->outbox.chan->mchan;
> +       pcc_mbox_free_channel(mctp_pcc_ndev->inbox.chan);
> +       scoped_guard(spinlock_irqsave, &chan->lock) {
> +               skb = chan->active_req;
> +               chan->active_req = NULL;
> +               if (skb) {
> +                       dev_dstats_tx_dropped(ndev);
> +                       dev_consume_skb_any(skb);
> +               }
> +               while (chan->msg_count > 0) {
> +                       count = chan->msg_count;
> +                       idx = chan->msg_free;
> +                       if (idx >= count)
> +                               idx -= count;
> +                       else
> +                               idx += MBOX_TX_QUEUE_LEN - count;
> +                       skb = chan->msg_data[idx];
> +                       dev_dstats_tx_dropped(ndev);
> +                       dev_consume_skb_any(skb);
> +                       chan->msg_count--;
> +               }
> +       }

I'm going to assume this is all OK with respect to PCC channel activity.

> +       pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
> +       return 0;
> +}
> +
> +static int mctp_pcc_change_mtu(struct net_device *dev, int new_mtu)
> +{
> +       if (new_mtu > dev->max_mtu)
> +               return -EINVAL;
> +       if (new_mtu < MCTP_MIN_MTU)
> +               return -EINVAL;
> +       return 0;
> +}

The core already does these checks, I don't think you need a
ndo_change_mtu callback at all.

Cheers,


Jeremy