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

[Jeremy Kerr]

Hi Adam,

Looking good. I see that there are some warnings reported from the
accesses to the __be32 fields in the header struct:

  https://netdev.bots.linux.dev/static/nipa/919612/13915538/build_allmodconfig_warn/stderr

[there are some pre-existing warnings there; but they're fine to ignore
for your purposes]

You'll likely need some endian conversion there; are those defined as
big endian though?

And if you're re-rolling, I would suggest:

> +static void mctp_cleanup_netdev(void *data)
> +{
> +       struct net_device *ndev = data;
> +
> +       mctp_unregister_netdev(ndev);
> +}
>
> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{
> +       struct mctp_pcc_lookup_context context = {0, 0, 0};
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       struct device *dev = &acpi_dev->dev;
> +       struct net_device *ndev;
> +       acpi_handle dev_handle;
> +       acpi_status status;
> +       int mctp_pcc_mtu;
> +       char name[32];
> +       int rc;
> +
> +       dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
> +               acpi_device_hid(acpi_dev));
> +       dev_handle = acpi_device_handle(acpi_dev);
> +       status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
> +                                    &context);
> +       if (!ACPI_SUCCESS(status)) {
> +               dev_err(dev, "FAILURE to lookup PCC indexes from CRS\n");
> +               return -EINVAL;
> +       }
> +
> +       //inbox initialization
> +       snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index);
> +       ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
> +                           mctp_pcc_setup);
> +       if (!ndev)
> +               return -ENOMEM;
> +
> +       mctp_pcc_ndev = netdev_priv(ndev);
> +       rc = devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
> +       if (rc)
> +               goto cleanup_netdev;
> +       spin_lock_init(&mctp_pcc_ndev->lock);
> +
> +       rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
> +                                        context.inbox_index);
> +       if (rc)
> +               goto cleanup_netdev;
> +       mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
> +
> +       //outbox initialization
> +       rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
> +                                        context.outbox_index);
> +       if (rc)
> +               goto cleanup_netdev;
> +
> +       mctp_pcc_ndev->acpi_device = acpi_dev;
> +       mctp_pcc_ndev->inbox.client.dev = dev;
> +       mctp_pcc_ndev->outbox.client.dev = dev;
> +       mctp_pcc_ndev->mdev.dev = ndev;
> +       acpi_dev->driver_data = mctp_pcc_ndev;
> +
> +       /* There is no clean way to pass the MTU to the callback function
> +        * used for registration, so set the values ahead of time.
> +        */
> +       mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
> +               sizeof(struct mctp_pcc_hdr);
> +       ndev->mtu = MCTP_MIN_MTU;
> +       ndev->max_mtu = mctp_pcc_mtu;
> +       ndev->min_mtu = MCTP_MIN_MTU;
> +
> +       /* ndev needs to be freed before the iomemory (mapped above) gets
> +        * unmapped,  devm resources get freed in reverse to the order they
> +        * are added.
> +        */
> +       rc = register_netdev(ndev);

mctp_register_netdev(), so that you're symmetrical with the
mctp_unregister_netdev() in the cleanup path.

Cheers,


Jeremy