Received-SPF: pass (google.com: domain of linux-kernel+bounces-177962-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 13 May 2024 19:31:11 +0100
From: Simon Horman <horms@kernel.org>
To: admiyo@os.amperecomputing.com
Cc: Jeremy Kerr <jk@codeconstruct.com.au>,
	Matt Johnston <matt@codeconstruct.com.au>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport
Message-ID: <20240513183111.GV2787@kernel.org>
References: <20240513173546.679061-1-admiyo@os.amperecomputing.com>
 <20240513173546.679061-2-admiyo@os.amperecomputing.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240513173546.679061-2-admiyo@os.amperecomputing.com>

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