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

Andrew Lunn
2024-05-13T20:08:28Z

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