Re: [PATCH net-next v2 6/8] eth: mpnic: add a netdevice and basic Tx handling

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Sun Sep 27, 2026 at 8:01 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 2 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The netdev registered by this patch has no statistics
>   reporting, and Tx drops are not counted anywhere.
> - [Medium] mpnic_open() undoes the netif_device_detach() that this patch
>   adds to mpnic_mmio_err(), and reports success on a dead device.
> - [Low] Concern: mpnic_netdev_alloc() calls
>   alloc_etherdev_mq(sizeof(*mpn), MPNIC_MAX_RXQS), which leaves…
>
> --
>
> eth: mpnic: add a netdevice and basic Tx handling
>
> This commit registers the mpnic network interface and adds a transmit
> path. Each skb gets a metadata descriptor followed by address/length
> descriptors. A completion is requested only when xmit_more is not set. The
> index of the deferred metadata descriptor is latched, so that dropping a
> packet can still ring the doorbell for packets already queued.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
>> new file mode 100644
>> index 0000000000000..6d5007be6cadb
>> --- /dev/null
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
>> @@ -0,0 +1,180 @@
>
> [ ... ]
>
>> +static int mpnic_open(struct net_device *netdev)
>> +{
>> +	struct mpnic_net *mpn = netdev_priv(netdev);
>> +	int err;
>> +
>
> [ ... ]
>
>> +	mpnic_enable(mpn);
>> +	mpnic_napi_enable(mpn);
>> +
>> +	netif_tx_wake_all_queues(netdev);
>> +	netif_carrier_on(netdev);
>> +
>> +	return 0;
>
> [Severity: Medium]
> Can this undo the netif_device_detach() that this patch adds to
> mpnic_mmio_err(), and report a successful open on a dead device?
>
> Suppose the device goes away while the interface is down. mpnic_enable()
> ends with mpnic_wrfl(), which reads MPNIC_BDQ_SPARE. If that read returns
> all ones, mpnic_mmio_err() clears uc_addr0 and calls
> netif_device_detach().
>
> __dev_open() has already set __LINK_STATE_START before calling ndo_open.
> So the detach clears __LINK_STATE_PRESENT and stops the Tx queues:
>
> __dev_open()
>   set_bit(__LINK_STATE_START, &dev->state)
>   mpnic_open()
>     mpnic_enable()
>       mpnic_wrfl()
>         mpnic_rd64(mpd, MPNIC_BDQ_SPARE)
>           mpnic_mmio_err()
>             netif_device_detach()
>     mpnic_napi_enable()
>     netif_tx_wake_all_queues()
>     netif_carrier_on()
>     return 0
>
> mpnic_open() then wakes every queue again, turns the carrier on and
> returns 0. The interface ends up IFF_UP, with carrier and awake Tx queues,
> while it is marked not present.
>
> Would mpnic_xmit_frame() then keep mapping skbs and writing to the cached
> ring->doorbell until the ring fills, with nothing ever completing? The dev
> watchdog would not fire either, because it requires
> netif_device_present().
>
> mpnic_netdev_register() checks mpnic_present() before register_netdev()
> for this same reason. Does mpnic_open() need a similar check after enabling
> the device, so it can unwind and return an error?
>
> The version of mpnic_open() at the end of the series only adds
> mpnic_fill(), so it seems to behave the same way.
>
> [ ... ]

See my reply in v1: https://lore.kernel.org/all/DLNQX7U9QRFW.2G0KYKS5W3X3H@gmail.com/

>
>> +struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
>> +{
>> +	struct net_device *netdev;
>> +	struct mpnic_net *mpn;
>> +	unsigned int queues;
>> +
>> +	netdev = alloc_etherdev_mq(sizeof(*mpn), MPNIC_MAX_RXQS);
>> +	if (!netdev)
>> +		return NULL;
>
> [Severity: Low]
> alloc_etherdev_mq() uses MPNIC_MAX_RXQS for both the Tx and the Rx queue
> count. Only the Tx count is trimmed later, in mpnic_netdev_register():
>
>     err = netif_set_real_num_tx_queues(netdev, mpn->num_tx_queues);
>
> Doesn't that leave real_num_rx_queues at 1024 at this commit? Sysfs and
> netlink would then list 1024 rx-N queues that have nothing behind them.
>
> This looks resolved later in the series by "eth: mpnic: add basic Rx
> handling". There, mpnic_netdev_register() and mpnic_set_netif_queues()
> switch to netif_set_real_num_queues() with num_rx_queues.
>
> There is also a smaller point. The Tx queue count comes from
> MPNIC_MAX_RXQS, but mpn->tx[] in struct mpnic_net is sized by
> MPNIC_MAX_TXQS. Both are 1024u today, so the
> mpn->tx[skb_get_queue_mapping(skb)] lookup in mpnic_xmit_frame() cannot
> overflow.
>
> Would alloc_etherdev_mqs() with MPNIC_MAX_TXQS and MPNIC_MAX_RXQS express
> the intent more precisely?
>

Like you said the real rx queues half is set in the commit that
allocates rx queues. Don't think this needs to change here.

1024 is the right count to use for both tx and rx. The Tx triads have a
second submission queue for future use with xdp, but that should not be
registered as a real tx queue.

> [ ... ]
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index aaa3a3e99a138..b1fb0d3902a7a 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>
> [ ... ]
>
>> +static netdev_tx_t mpnic_xmit_frame_ring(struct sk_buff *skb,
>> +					 struct mpnic_ring *ring)
>> +{
>> +	__le64 *meta = &ring->desc[ring->tail];
>> +	u32 tail = ring->tail;
>> +
>> +	if (skb_put_padto(skb, MPNIC_MIN_FRAME_LEN))
>> +		goto err_drop;
>
> [ ... ]
>
>> +	if (mpnic_tx_map(ring, skb, meta))
>> +		goto err_free;
>> +
>> +	return NETDEV_TX_OK;
>> +
>> +err_free:
>> +	dev_kfree_skb_any(skb);
>> +	ring->tx_buf[tail] = NULL;
>> +	ring->tail = tail;
>> +err_drop:
>> +	mpnic_tx_flush_doorbell(ring);
>> +
>> +	return NETDEV_TX_OK;
>> +}
>
> [Severity: Medium]
> Should these drop paths count the dropped skb somewhere?
>
> - A skb_put_padto() failure goes to err_drop with no log and no counter.
> - An mpnic_tx_map() failure (oversized head or frag, or
>   dma_mapping_error()) only logs a rate-limited netdev_err() before
>   dev_kfree_skb_any().
>
> Neither path calls dev_core_stats_tx_dropped_inc() or updates any counter.
>
> More broadly, mpnic_netdev_ops only provides ndo_open, ndo_stop,
> ndo_validate_addr and ndo_start_xmit:
>
> - There is no ndo_get_stats64.
> - pcpu_stat_type is not set.
> - dev->stats is never updated.
>
> The per-packet totals in mpnic_clean_twq0() only go to
> netif_txq_completed_wake() for BQL.
>
> Won't dev_get_stats() report zero for tx_packets, tx_bytes and tx_dropped
> while traffic is flowing? The last patch in the series, "eth: mpnic: add
> basic Rx handling", still doesn't add a stats callback.

Stats coming in subsequent series.