Received-SPF: pass (google.com: domain of linux-kernel+bounces-193383-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 28 May 2024 19:45:57 -0700
From: Jakub Kicinski <kuba@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>, Paolo Abeni <pabeni@redhat.com>,
 netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport
Message-ID: <20240528194557.7a8f522d@kernel.org>
In-Reply-To: <20240528191823.17775-4-admiyo@os.amperecomputing.com>
References: <20240513173546.679061-1-admiyo@os.amperecomputing.com>
	<20240528191823.17775-1-admiyo@os.amperecomputing.com>
	<20240528191823.17775-4-admiyo@os.amperecomputing.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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