On Tue, May 05, 2026 at 01:04:22AM +0530, Badal Nilawar wrote:
> Add support for MCTP transport over the Intel vendor-specific mailbox
> protocol to enable in-band firmware updates for GPU/AMC via PLDM
...
> +#include "xe_device_types.h"
Why is the location of this inclusion is here?
> +#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>
> +
The above doesn't sound like more generic than linux/* ones. Move it here.
> +#include "xe_mctp_mailbox.h"
...
> +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;
This is bad style from maintenance perspective. Use
struct net_device *netdev;
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);
> + }
> + */
What is this?! If you want to put a nice comment, format it accordingly.
> + 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));
> +}
...
> +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);
> + * dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> + */
Even for RFC these should not exist in this form. Always add the respective
FIXME/TODO/et cetera to explain the commented out code.
> + dev->flags = IFF_NOARP;
> + dev->netdev_ops = &mctp_mailbox_netdev_ops;
> + dev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
> +}
--
With Best Regards,
Andy Shevchenko
