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

[Simon Horman <horms@kernel.org>]

On Mon, May 13, 2024 at 01:35:44PM -0400, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.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.
> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>

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