Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 28 May 2024 15:18:23 -0400 admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@amperecomputing.com>
> 
> Implementation of DMTF DSP:0292
> Management Control Transport Protocol(MCTP)  over
> Platform Communication Channel(PCC)
> 
> 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.

Missing your SoB, but please wait for more feedback before reposting.

> +#include <net/pkt_sched.h>

Hm, what do you need this include for?

> +#define SPDM_VERSION_OFFSET 1
> +#define SPDM_REQ_RESP_OFFSET 2
> +#define MCTP_PAYLOAD_LENGTH 256
> +#define MCTP_CMD_LENGTH 4
> +#define MCTP_PCC_VERSION     0x1 /* DSP0253 defines a single version: 1 */
> +#define MCTP_SIGNATURE "MCTP"
> +#define SIGNATURE_LENGTH 4
> +#define MCTP_HEADER_LENGTH 12
> +#define MCTP_MIN_MTU 68
> +#define PCC_MAGIC 0x50434300
> +#define PCC_DWORD_TYPE 0x0c

Could you align the values using tabs?

> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_dev;
> +	struct mctp_skb_cb *cb;
> +	struct sk_buff *skb;
> +	u32 length_offset;
> +	u32 flags_offset;
> +	void *skb_buf;
> +	u32 data_len;
> +	u32 flags;
> +
> +	mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> +	length_offset = offsetof(struct mctp_pcc_hdr, length);
> +	data_len = readl(mctp_pcc_dev->pcc_comm_inbox_addr + length_offset) +
> +		   MCTP_HEADER_LENGTH;
> +
> +	skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len);
> +	if (!skb) {
> +		mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
> +		return;
> +	}
> +	mctp_pcc_dev->mdev.dev->stats.rx_packets++;
> +	mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len;

Please implement ndo_get_stats64, use of the core dev stats in drivers
is deprecated:

 *	@stats:		Statistics struct, which was left as a legacy, use
 *			rtnl_link_stats64 instead

> +	skb->protocol = htons(ETH_P_MCTP);
> +	skb_buf = skb_put(skb, data_len);
> +	memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len);
> +	skb_reset_mac_header(skb);
> +	skb_pull(skb, sizeof(struct mctp_pcc_hdr));
> +	skb_reset_network_header(skb);
> +	cb = __mctp_cb(skb);
> +	cb->halen = 0;
> +	skb->dev =  mctp_pcc_dev->mdev.dev;

netdev_alloc_skb() already sets dev

> +	netif_rx(skb);
> +
> +	flags_offset = offsetof(struct mctp_pcc_hdr, flags);
> +	flags = readl(mctp_pcc_dev->pcc_comm_inbox_addr + flags_offset);
> +	mctp_pcc_dev->in_chan->ack_rx = (flags & 1) > 0;
> +}
> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct mctp_pcc_hdr pcc_header;
> +	struct mctp_pcc_ndev *mpnd;
> +	void __iomem *buffer;
> +	unsigned long flags;
> +	int rc;
> +
> +	netif_stop_queue(ndev);

Why?

> +	ndev->stats.tx_bytes += skb->len;
> +	ndev->stats.tx_packets++;
> +	mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
> +
> +	spin_lock_irqsave(&mpnd->lock, flags);
> +	buffer = mpnd->pcc_comm_outbox_addr;
> +	pcc_header.signature = PCC_MAGIC;
> +	pcc_header.flags = 0x1;
> +	memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
> +	pcc_header.length = skb->len + SIGNATURE_LENGTH;
> +	memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr));
> +	memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len);
> +	rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
> +							 NULL);
> +	spin_unlock_irqrestore(&mpnd->lock, flags);
> +
> +	dev_consume_skb_any(skb);
> +	netif_start_queue(ndev);
> +	if (!rc)
> +		return NETDEV_TX_OK;
> +	return NETDEV_TX_BUSY;
> +}
> +
> +static const struct net_device_ops mctp_pcc_netdev_ops = {
> +	.ndo_start_xmit = mctp_pcc_tx,
> +	.ndo_uninit = NULL

No need to init things to NULL

> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
> +	struct list_head *ptr;
> +	struct list_head *tmp;
> +
> +	list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
> +		struct net_device *ndev;
> +
> +		mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
> +		if (adev && mctp_pcc_dev->acpi_device == adev)
> +			continue;
> +
> +		mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
> +		mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
> +		ndev = mctp_pcc_dev->mdev.dev;
> +		if (ndev)
> +			mctp_unregister_netdev(ndev);
> +		list_del(ptr);
> +		if (adev)
> +			break;
> +	}
> +};

spurious ;


> +	.owner = THIS_MODULE,
> +

suprious new line

> +};
> +
-- 
pw-bot: cr