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

On Mon, May 13, 2024 at 01:35:44PM -0400, admiyo@os.amperecomputing.com wrote:

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.

Signed-off-by: Adam Young <redacted>

Hi Adam,

Some minor feedback from my side.

...

+static struct mctp_pcc_packet *mctp_pcc_extract_data(struct sk_buff *old_skb,
+						     void *buffer, int outbox_index)
+{
+	struct mctp_pcc_packet *mpp;
+
+	mpp = buffer;
+	writel(PCC_MAGIC | outbox_index, &mpp->pcc_header.signature);
+	writel(0x1, &mpp->pcc_header.flags);
+	memcpy_toio(mpp->pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
+	writel(old_skb->len + SIGNATURE_LENGTH,  &mpp->pcc_header.length);
+	memcpy_toio(mpp->header_data,    old_skb->data, old_skb->len);
+	return mpp;
+}
+
+static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *)

Please include a name for all function parameters.

Flagged by W=1 builds.

+{
+	struct sk_buff *skb;
+	struct mctp_pcc_packet *mpp;
+	struct mctp_skb_cb *cb;
+	int data_len;
+	unsigned long buf_ptr_val;

buf_ptr_val is assigned but otherwise unused.

Flagged by W=1 builds.

+	struct mctp_pcc_ndev *mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
+	void *skb_buf;

For Networking code please consider:

1. Using reverse xmas tree order - longest line to shortest - for local
   variable declarations.

   This tool can be of assistance: https://github.com/ecree-solarflare/xmastree

2. Restricting lines to 80 columns wide where this can be trivially achieved.

   ./scripts/checkpatch.pl --max-line-length=80

In this case, perhaps:

	struct mctp_pcc_ndev *mctp_pcc_dev;
	struct mctp_pcc_packet *mpp;
	struct mctp_skb_cb *cb;
	struct sk_buff *skb;
	void *skb_buf;
	int data_len;

	mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);

+
+	mpp = (struct mctp_pcc_packet *)mctp_pcc_dev->pcc_comm_inbox_addr;
+	buf_ptr_val = (unsigned long)mpp;
+	data_len = readl(&mpp->pcc_header.length) + 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;
+	}
+	skb->protocol = htons(ETH_P_MCTP);
+	skb_buf = skb_put(skb, data_len);
+	memcpy_fromio(skb_buf, mpp, 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;
+	netif_rx(skb);
+}
+
+static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+	unsigned char *buffer;
+	struct mctp_pcc_ndev *mpnd;
+	struct mctp_pcc_packet  *mpp;
+	unsigned long flags;
+	int rc;
+
+	netif_stop_queue(ndev);
+	ndev->stats.tx_bytes += skb->len;
+	mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
+	spin_lock_irqsave(&mpnd->lock, flags);
+	buffer =  mpnd->pcc_comm_outbox_addr;

buffer is assigned but otherwise unused in this function.

Flagged by W=1 builds.

+	mpp = mctp_pcc_extract_data(skb, mpnd->pcc_comm_outbox_addr, mpnd->hw_addr.outbox_index);
+	rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, mpp);
+	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;
+}

...

-- 
pw-bot: changes-requested