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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6d3aed83-ee56-4c3c-bb23-0f7d1f471ea4@lunn.ch>
Date: Mon, 13 May 2024 22:08:16 +0200
From: Andrew Lunn <andrew@...n.ch>
To: admiyo@...amperecomputing.com
Cc: Jeremy Kerr <jk@...econstruct.com.au>,
	Matt Johnston <matt@...econstruct.com.au>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

> +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