Re: [net-next v40] 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))
> +		goto error;
> +	// 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)
> +		goto error;
> +	if (memcmp(&pcc_header.command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH) != 0)
> +		goto error;
> +
> +	size = header_length + PCC_EXTRA_LEN;
> +	skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
> +	if (!skb)
> +		goto error;
> +	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);
> +	return;
> +error:
> +	dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +}

Super minor, but odd spacing here. This should probably be:

            if (!skb)
                goto error;

            [...]
            netif_rx(skb);
            return;
    
    error:
            dev_dstats_rx_dropped(..);
    }

(ie, with a bit of spacing after the goto and the return)

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

This is still not needed.

> +
> +	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: You've taken out BE support, but left this comment in.

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

Cheers,


Jeremy