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

On 5/28/24 22:45, Jakub Kicinski wrote:

On Tue, 28 May 2024 15:18:23 -0400 admiyo@os.amperecomputing.com wrote:

quoted

From: Adam Young <redacted>

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.

Sorry, must have dropped it when redoing the last patch,  It was on the 
original and will be on the next version.

quoted

+#include <net/pkt_sched.h>

Hm, what do you need this include for?

Gets the symbolic constant

quoted

+#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?

Will do

quoted

+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

Thanks.  Was not aware.  The other MCTP drivers need this as well.

quoted

+	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

quoted

+	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?

I saw this in other network drivers, both Ethernet and MCTP.  As I 
understand it, without stopping the queue, we could take another packet 
before this one is sent and we only have one buffer;  that said, it 
should be protected by the spin lock.

I am tempted to leave this in here, but move all of the statistics into 
the spin lock.  Is there a reason to not stop the queue?

quoted

+	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

quoted

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

quoted

+	.owner = THIS_MODULE,
+

suprious new line

quoted

+};
+