Messages in this thread |  | | Date | Tue, 5 May 2026 11:23:29 +0300 | | From | Andy Shevchenko <> | | Subject | Re: [RFC PATCH 1/1] xe/xe_mctp_mailbox: Add support for MCTP transport over mailbox |
| |
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
|  |