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

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

Hi Adam,

> diff --git a/drivers/net/ethernet/intel/stYNFrCT b/drivers/net/ethernet/intel/stYNFrCT
> new file mode 100644
> index 0000000000000000000000000000000000000000..9b01d1e909f20fe037eea685116d344a2165db52
> GIT binary patch
> literal 564073

[1255 lines of b85 data]

I think something leaked into your commit?

> +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);

Is this still OK with pcc_header.length being a u32 (and not a __le32?)

> +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;
> +
> +       if (skb_cow_head(skb, sizeof(*pcc_header)))
> +               goto error;
> +
> +       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;

You're doing the endian conversion on read, but not on write. What was
the decision on whether you need it or not?

> +
> +       if (skb->len > mpnd->outbox.chan->shmem_size)
> +               goto error;
> +
> +       if (mbox_send_message(mpnd->outbox.chan->mchan, skb) < 0) {
> +               netif_stop_queue(ndev);
> +               /*
> +                * There is a possibility that the mailbox was cleared on
> +                * another thread between the failed send attempt and
> +                * stopping the queue.  If that is the case, and we don't restart
> +                * the queue, it will remain permanently stopped.  To test,
> +                * try submitting the message again. If successful, restart the
> +                * queue.
> +                */
> +               if (mbox_send_message(mpnd->outbox.chan->mchan, skb) >= 0) {
> +                       netif_wake_queue(ndev);

The retry is a bit odd; you can simplify this race handling by doing the
stop_queue pessimistically, and the re-starting the queue on success:

    netif_stop_queue();
    if (mbox_send_message(...) >= 0) {
        netif_start_queue();
    }

I don't think you need a wake (vs start), since you're running in the
same softirq context as the stop.

Cheers,


Jeremy