Re: [RFC PATCH 1/1] xe/xe_mctp_mailbox: Add support for MCTP transport over mailbox

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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