Re: [PATCH v9 1/1] mctp pcc: Implement MCTP over PCC Transport

[Joe Damato <jdamato@fastly.com>]

On Tue, Dec 17, 2024 at 01:25:28PM -0500, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> Implementation of network driver for
> Management Control Transport Protocol(MCTP) over
> Platform Communication Channel(PCC)
> 
> DMTF DSP:0292
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0292_1.0.0WIP50.pdf
> 
> MCTP devices are specified by entries in DSDT/SDST and
> reference channels specified in the PCCT.
> 
> Communication with other devices use the PCC based
> doorbell mechanism.
> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
>  drivers/net/mctp/Kconfig    |  13 ++
>  drivers/net/mctp/Makefile   |   1 +
>  drivers/net/mctp/mctp-pcc.c | 320 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 334 insertions(+)
>  create mode 100644 drivers/net/mctp/mctp-pcc.c

[...]
 
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,320 @@

[...]

> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_ndev;
> +	struct mctp_pcc_hdr mctp_pcc_hdr;
> +	struct pcpu_dstats *dstats;
> +	struct mctp_skb_cb *cb;
> +	struct sk_buff *skb;
> +	void *skb_buf;
> +	u32 data_len;
> +
> +	mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
> +	memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_ndev->inbox.chan->shmem,
> +		      sizeof(struct mctp_pcc_hdr));
> +	data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH;
> +	skb = netdev_alloc_skb(mctp_pcc_ndev->mdev.dev, data_len);
> +
> +	dstats = this_cpu_ptr(mctp_pcc_ndev->mdev.dev->dstats);
> +	u64_stats_update_begin(&dstats->syncp);
> +	if (data_len > mctp_pcc_ndev->mdev.dev->mtu) {
> +		u64_stats_inc(&dstats->rx_drops);
> +		u64_stats_inc(&dstats->rx_drops);

Double counting rx_drops ?

> +		u64_stats_update_end(&dstats->syncp);
> +		return;
> +	}
> +	if (!skb) {
> +		u64_stats_inc(&dstats->rx_drops);
> +		u64_stats_update_end(&dstats->syncp);
> +		return;
> +	}
> +	u64_stats_inc(&dstats->rx_packets);
> +	u64_stats_add(&dstats->rx_bytes, data_len);
> +	u64_stats_update_end(&dstats->syncp);

I suspect what Jeremy meant (but please feel free to correct me if
I'm mistaken, Jeremy) was that you may want to use the helpers in:

include/linux/netdevice.h

e.g. 

  dev_dstats_rx_add(mctp_pcc_ndev->mdev.dev, data_len);
  dev_dstats_rx_dropped(mctp_pcc_ndev->mdev.dev);

etc.

[...]

> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> +	struct mctp_pcc_hdr  *mctp_pcc_header;
> +	struct pcpu_dstats *dstats;
> +	void __iomem *buffer;
> +	unsigned long flags;
> +	int len = skb->len;
> +
> +	dstats = this_cpu_ptr(ndev->dstats);
> +	u64_stats_update_begin(&dstats->syncp);
> +	u64_stats_inc(&dstats->tx_packets);
> +	u64_stats_add(&dstats->tx_bytes, skb->len);
> +	u64_stats_update_end(&dstats->syncp);

Likewise, as above with the helpers from include/linux/netdevice.h:

  dev_dstats_tx_add( ... );
  dev_dstats_tx_dropped( ... );

But, I'll let Jeremy weigh-in to make sure I've not misspoken.

[...]

> +
> +static void  mctp_pcc_setup(struct net_device *ndev)
              ^^  nit: double space ?

[...]

> +static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
> +				       void *context)
> +{
> +	struct  mctp_pcc_lookup_context *luc = context;
              ^^ nit: double space?

[...]

> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{

[...]

> +
> +	rc =  devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
            ^^ nit: double space