Messages in this thread |  | | Subject | Re: [RFC PATCH 1/1] xe/xe_mctp_mailbox: Add support for MCTP transport over mailbox | | From | Jeremy Kerr <> | | Date | Tue, 05 May 2026 10:15:34 +0800 |
| |
Hi Badal,
> Add support for MCTP transport over the Intel vendor-specific mailbox > protocol to enable in-band firmware updates for GPU/AMC via PLDM
I know this is an RFC, but you're missing the distinctive part, around the hardware interactions on the transport side. The skeleton of a MCTP driver is mostly the same as existing transport drivers, so there's not a lot to comment on at present.
Some items on the parts here though:
I'd suggest not using the term "MCTP over mailbox" / mctp_mailbox in general; there are lots of "mailbox" implementations around, including the proposed PCC mailbox driver. Perhaps "MCTP over Xe mailbox"?
> diff --git a/drivers/gpu/drm/xe/xe_mctp_mailbox.c b/drivers/gpu/drm/xe/xe_mctp_mailbox.c > new file mode 100644 > index 000000000000..f1a81208a9a1 > --- /dev/null > +++ b/drivers/gpu/drm/xe/xe_mctp_mailbox.c > @@ -0,0 +1,186 @@ > +// SPDX-License-Identifier: MIT
This will need to include GPLv2.
> +/* > + * MCTP-over-MAILBOX transport for Xe. > + * > + * Copyright 2026 Intel Corporation > + */ > + > +#include "xe_device_types.h" > + > +#include <linux/netdevice.h> > +#include <linux/jiffies.h> > +#include <linux/workqueue.h> > + > +#include <net/mctp.h> > +#include <net/mctpdevice.h> > +#include <net/pkt_sched.h> > + > +#include <uapi/linux/if_arp.h> > + > +#include "xe_mctp_mailbox.h" > + > +#define XE_MCTP_MAILBOX_RX_POLL_MS 100 > + > +/** @mctp_mailbox: Struct for mctp over mailbox */ > +struct xe_mctp_mailbox { > + /** @mctp_mailbox.netdev: network device */ > + struct net_device *netdev; > + /** @running: true while netdev is opened */ > + bool running; > + /** @work: worker to handle mctp requests from firmware */ > + struct delayed_work work; > + /** @wq: workqueue to schecdule mctp rx worker */ > + struct workqueue_struct *wq; > +}; > + > +/* > + * mailbox protocol is interrupt free so for receive path i.e. endpoint to host > + * there is no irq available so rx handler need to be polled in worker periodically. > + */
This is unfortunate - there's no facility you can use to trigger RX?
> +static void mctp_mailbox_rx_handler(struct work_struct *work) > +{ > + struct xe_mctp_mailbox *mctp_mailbox = > + container_of(work, struct xe_mctp_mailbox, work.work); > + struct net_device *netdev = mctp_mailbox->netdev; > + > + if (!netdev) > + return; > + > + dev_hold(netdev); > + > + /* > + * if (mctp_mailbox_rx_ready()) { > + * Get data over MAILBOX > + * Allocate skb and copy rx data to skb > + * Queue skb to upper layer > + * netif_rx(skb); > + } > + */ > + dev_put(netdev); > + > + if (mctp_mailbox->running) > + queue_delayed_work(mctp_mailbox->wq, &mctp_mailbox->work, > + msecs_to_jiffies(XE_MCTP_MAILBOX_RX_POLL_MS)); > +}
One packet per 100ms will mean you will very likely miss a retry timeout on fragmented messages. You probably want to schedule the next RX immediately until the mailbox is empty, assuming that will work with the semantics of the Xe mailbox.
Even then, a single-packet (worst-case) transmission delay of 100ms is getting a bit large. It may be necessary to increase the polling frequency, but that has downsides. Hence the question about an RX notification facility.
> + > +static netdev_tx_t mctp_mailbox_start_xmit(struct sk_buff *skb, > + struct net_device *dev) > +{ > + /* RFC stub: send skb over MAILBOX */ > + dev_dstats_tx_dropped(dev); > + kfree_skb(skb); > + > + return NETDEV_TX_OK; > +} > + > +static int mctp_mailbox_open(struct net_device *dev) > +{ > + struct xe_mctp_mailbox *mctp_mailbox = netdev_priv(dev); > + > + mctp_mailbox->running = true; > + netif_start_queue(dev); > + > + queue_delayed_work(mctp_mailbox->wq, &mctp_mailbox->work, 0); > + > + return 0; > +} > + > +static int mctp_mailbox_stop(struct net_device *dev) > +{ > + struct xe_mctp_mailbox *mctp_mailbox = netdev_priv(dev); > + > + mctp_mailbox->running = false; > + netif_stop_queue(dev); > + > + cancel_delayed_work_sync(&mctp_mailbox->work); > + flush_workqueue(mctp_mailbox->wq); > + > + return 0; > +} > + > +static const struct net_device_ops mctp_mailbox_netdev_ops = { > + .ndo_start_xmit = mctp_mailbox_start_xmit, > + .ndo_open = mctp_mailbox_open, > + .ndo_stop = mctp_mailbox_stop, > +}; > + > +static void mctp_mailbox_netdev_setup(struct net_device *dev) > +{ > + /* Populate netdev structure */ > + dev->type = ARPHRD_MCTP; > + /* > + * dev->mtu = MCTP_MAILBOX_MTU_MIN; > + * dev->min_mtu = MCTP_MAILBOX_MTU_MIN; > + * dev->max_mtu = MCTP_MAILBOX_MTU_MAX; > + * > + * dev->hard_header_len = sizeof(struct mctp_mailbox_hdr);
What's in struct mctp_mailbox_hdr? I assume you don't need to handle any physical addressing, but can you confirm?
> + * dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN; > + */ > + dev->flags = IFF_NOARP; > + dev->netdev_ops = &mctp_mailbox_netdev_ops; > + dev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS; > +} > + > +static void xe_mctp_mailbox_fini(void *arg) > +{ > + struct xe_device *xe = arg; > + struct xe_mctp_mailbox *mctp_mailbox = xe->mctp_mailbox; > + struct net_device *netdev; > + > + if (!mctp_mailbox) > + return; > + > + netdev = mctp_mailbox->netdev; > + if (!netdev) { > + xe->mctp_mailbox = NULL; > + return; > + } > + > + if (mctp_mailbox->wq) { > + mctp_mailbox->running = false; > + cancel_delayed_work_sync(&mctp_mailbox->work); > + destroy_workqueue(mctp_mailbox->wq); > + mctp_mailbox->wq = NULL; > + } > + > + xe->mctp_mailbox = NULL; > + mctp_unregister_netdev(netdev); > + free_netdev(netdev); > +} > + > +int xe_mctp_mailbox_init(struct xe_device *xe) > +{ > + struct xe_mctp_mailbox *mctp_mailbox; > + struct net_device *netdev; > + int ret, err; > + > + netdev = alloc_netdev(sizeof(*mctp_mailbox), "mctp_mailbox%d", NET_NAME_ENUM, > + mctp_mailbox_netdev_setup);
I'd suggest mctpxe%d here. We don't tend to have underscores in netdev names, and 'mailbox' is quite generic.
Can you tie the name to the instance of the GPU (and then use NET_NAME_PREDICTABLE) perhaps?
> + if (!netdev) > + return -ENOMEM; > + > + SET_NETDEV_DEV(netdev, xe->drm.dev); > + mctp_mailbox = netdev_priv(netdev); > + mctp_mailbox->netdev = netdev; > + > + ret = mctp_register_netdev(netdev, NULL, MCTP_PHYS_BINDING_VENDOR); > + if (ret) { > + free_netdev(netdev); > + return ret; > + } > + > + INIT_DELAYED_WORK(&mctp_mailbox->work, mctp_mailbox_rx_handler); > + mctp_mailbox->wq = alloc_ordered_workqueue("mctp-mailbox-ordered-wq", 0); > + if (!mctp_mailbox->wq) { > + mctp_unregister_netdev(netdev); > + free_netdev(netdev); > + return -ENOMEM; > + } > + > + xe->mctp_mailbox = mctp_mailbox; > + err = devm_add_action_or_reset(xe->drm.dev, xe_mctp_mailbox_fini, xe); > + if (err) > + return err; > + > + return 0; > +}
Cheers,
Jeremy
|  |