Received-SPF: pass (google.com: domain of linux-kernel+bounces-178060-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 22:08:16 +0200
From: Andrew Lunn <andrew@lunn.ch>
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: <6d3aed83-ee56-4c3c-bb23-0f7d1f471ea4@lunn.ch>
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>

> +struct mctp_pcc_hdr {
> +	u32 signature;
> +	u32  flags;

There looks to be an extra space here, or a tab vs space issue.

> +	u32 length;
> +	char mctp_signature[4];
> +};
> +
> +struct mctp_pcc_packet {
> +	struct mctp_pcc_hdr pcc_header;
> +	union {
> +		struct mctp_hdr     mctp_header;

and more here. I would expect checkpatch to point these out.

> +struct mctp_pcc_hw_addr {
> +	int inbox_index;
> +	int outbox_index;
> +};
> +	physical_link_addr.inbox_index =
> +		htonl(mctp_pcc_dev->hw_addr.inbox_index);


These are {in|out}box_index are u32s right? Otherwise you would not be
using htonl() on them. Maybe specify the type correctly.

> +	physical_link_addr.outbox_index =
> +		htonl(mctp_pcc_dev->hw_addr.outbox_index);

You should also mark the physical_link_addr members as being big
endian so sparse can check you are not missing any byte swaps.

> +	dev_addr_set(ndev, (const u8 *)&physical_link_addr);
> +	rc = register_netdev(ndev);
> +	if (rc)
> +		goto cleanup_in_channel;
> +	list_add_tail(&mctp_pcc_dev->head, &mctp_pcc_ndevs);
> +	return 0;
> +cleanup_in_channel:

It would be normal to add a blink line after the return, just to make
it easier to see where the error cleanup code starts.


> +	mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
> +cleanup_out_channel:
> +	mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
> +free_netdev:
> +	unregister_netdev(ndev);

Can you get here with the ndev actually registered?

> +static acpi_status lookup_pcct_indices(struct acpi_resource *ares, void *context)
> +{
> +	struct acpi_resource_address32 *addr;
> +	struct lookup_context *luc = context;
> +
> +	switch (ares->type) {
> +	case 0x0c:
> +	case 0x0a:

Please replace these magic numbers of #defines.

> +static int mctp_pcc_driver_add(struct acpi_device *adev)
> +{
> +	int inbox_index;
> +	int outbox_index;
> +	acpi_handle dev_handle;
> +	acpi_status status;
> +	struct lookup_context context = {0, 0, 0};
> +
> +	dev_info(&adev->dev, "Adding mctp_pcc device for HID  %s\n", acpi_device_hid(adev));

It would be better to not spam the logs when a driver probes, unless
there is an actual error.

> +	dev_handle = acpi_device_handle(adev);
> +	status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, &context);
> +	if (ACPI_SUCCESS(status)) {
> +		inbox_index = context.inbox_index;
> +		outbox_index = context.outbox_index;
> +		return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index);
> +	}
> +	dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS");
> +	return -EINVAL;
> +};
> +
> +/* pass in adev=NULL to remove all devices
> + */
> +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) {
> +		mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, head);
> +		if (!adev || mctp_pcc_dev->acpi_device == adev) {
> +			struct net_device *ndev;
> +
> +			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;
> +		}
> +	}
> +};
> +
> +static const struct acpi_device_id mctp_pcc_device_ids[] = {
> +	{ "DMT0001", 0},
> +	{ "", 0},
> +};
> +
> +static struct acpi_driver mctp_pcc_driver = {
> +	.name = "mctp_pcc",
> +	.class = "Unknown",
> +	.ids = mctp_pcc_device_ids,
> +	.ops = {
> +		.add = mctp_pcc_driver_add,
> +		.remove = mctp_pcc_driver_remove,
> +		.notify = NULL,
> +	},
> +	.owner = THIS_MODULE,
> +
> +};
> +
> +static int __init mctp_pcc_mod_init(void)
> +{
> +	int rc;
> +
> +	pr_info("initializing MCTP over PCC\n");

More useless log spamming... pr_dbg(), or remove altogether.

	Andrew