On 9/4/25 02:16, Jeremy Kerr wrote:

Hi Adam,

You seem to have missed any fixes from Alok's review here.

Yes, I missed that message.  I will make them in my next version.  
However, something more significant in this one might delay that....

Further comments inline:

quoted

+static int mctp_pcc_ndo_open(struct net_device *ndev)
+{
+       struct mctp_pcc_ndev *mctp_pcc_ndev =
+           netdev_priv(ndev);
+       struct mctp_pcc_mailbox *outbox =
+           &mctp_pcc_ndev->outbox;
+       struct mctp_pcc_mailbox *inbox =
+           &mctp_pcc_ndev->inbox;
+
+       outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
+       if (IS_ERR(outbox->chan))
+               return PTR_ERR(outbox->chan);
+
+       inbox->chan = pcc_mbox_request_channel(&inbox->client, inbox->index);
+       if (IS_ERR(inbox->chan)) {
+               pcc_mbox_free_channel(outbox->chan);
+               return PTR_ERR(inbox->chan);
+       }
+
+       mctp_pcc_ndev->inbox.chan->rx_alloc = mctp_pcc_rx_alloc;
+       mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
+       mctp_pcc_ndev->outbox.chan->manage_writes = true;

 From v25:

quoted

Minor: you have the convenience vars for ->inbox and ->outbox, may as
well use them.

Also: you're setting the client rx_callback *after* having set up the
PCC channel. Won't this race with RX on the inbox?

I think this might be a deal breaker.  I was trying to avoid making a 
change to the mailbox API, but I think I need a new client scoped  
callback to replace the one that I added here: there is no way to avoid 
the race condition without an atomic request_channel.  I added this to 
the channel (there is no PCC specific client) to try and minimize churn 
on this set, but I think I need to do it the right way.

And, since Sudeep is unhappy with the changes to the PCC mailbox anyway, 
I would expect a rewrite of that layer to happen before I get to address 
this.

quoted

+static int initialize_MTU(struct net_device *ndev)
+{
+       struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
+       struct mctp_pcc_mailbox *outbox;
+       int mctp_pcc_mtu;
+
+       outbox = &mctp_pcc_ndev->outbox;
+       outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
+       mctp_pcc_mtu = outbox->chan->shmem_size - sizeof(struct pcc_header);
+       if (IS_ERR(outbox->chan))
+               return PTR_ERR(outbox->chan);

You have already dereferenced outbox->chan before this confitional. Move
the usage to after this check.

Will do.

quoted

+
+       pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
+
+       mctp_pcc_ndev = netdev_priv(ndev);
+       ndev->mtu = MCTP_MIN_MTU;
+       ndev->max_mtu = mctp_pcc_mtu;
+       ndev->min_mtu = MCTP_MIN_MTU;
+
+       return 0;
+}
+
+static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
+{
+       struct mctp_pcc_lookup_context context = {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;
+       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;
+       }
+
+       snprintf(name, sizeof(name), "mctppcc%d", context.inbox_index);
+       ndev = alloc_netdev(sizeof(*mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
+                           mctp_pcc_setup);
+       if (!ndev)
+               return -ENOMEM;
+
+       mctp_pcc_ndev = netdev_priv(ndev);
+
+       mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
+                                   context.inbox_index);
+       mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
+                                   context.outbox_index);
+       if (rc)
+               goto free_netdev;

'rc' has never been set at this point.

Artifact of old code  that has been moved.  I will remove the RC. 
mctp_pcc_initialize_mailbox is simpler now, and has no failure case.

quoted

+
+       mctp_pcc_ndev->outbox.client.tx_done = mctp_pcc_tx_done;
+       mctp_pcc_ndev->acpi_device = acpi_dev;
+       mctp_pcc_ndev->ndev = ndev;
+       acpi_dev->driver_data = mctp_pcc_ndev;
+
+       initialize_MTU(ndev);

initialize_MTU() has an int return value; either make that void, or
handle the error here.

I will handle it here.

Cheers,


Jeremy