Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The patch moves the RX frame-bound drop into a new helper,
ibmveth_poll_drop_oversize(), and changes its log text.
--
ibmveth: Split RX poll into per-slot helpers for MQ
This commit splits the ibmveth_poll() loop body into per-slot helpers
(stopping, recycle_invalid, skip_bad_correlator, drop_oversize,
deliver_frame, bump_invalid), so the NAPI callback becomes a thin budget
loop. It also adds checks that stop polling once the device is down or a
napi_disable() is pending, and a WARN for a NAPI index past the queue
count, in preparation for multi-queue RX.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index a1f286b37effa..b12bfa1e485eb 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> +static void ibmveth_poll_drop_oversize(struct net_device *netdev,
> + struct ibmveth_adapter *adapter,
> + int queue_index, unsigned int off,
> + unsigned int len, unsigned int room,
> + u64 correlator)
> +{
> + if (net_ratelimit())
> + netdev_err(netdev,
> + "RX frame %u+%u exceeds buffer %u on queue %d,
> dropping\n",
> + off, len, room, queue_index);
> + netdev->stats.rx_dropped++;
> + ibmveth_rxq_harvest_correlator(adapter, queue_index, true, correlator);
> +}
[ ... ]
> + pool = correlator >> 32;
> + bpool = &adapter->rx_buff_pool[queue_index][pool];
> + room = min_t(unsigned int, skb_tailroom(skb),
> + bpool->buff_size);
> + off = offset;
> + len = length;
> + if (unlikely(len < ETH_HLEN || off >= room ||
> + len > room - off)) {
> + ibmveth_poll_drop_oversize(netdev, adapter, queue_index,
> + off, len, room, correlator);
> + return 0;
> + }
[Severity: Low]
Does the new message in ibmveth_poll_drop_oversize() describe the runt
case correctly?
The guard in ibmveth_poll_deliver_frame() sends all three conditions to
the same helper, including len < ETH_HLEN. A 10-byte frame at offset 0
in a 2048-byte buffer would be logged as:
RX frame 0+10 exceeds buffer 2048 on queue 0, dropping
That frame was too short, not too large.
The old text this replaces was neutral and matched every condition:
- "bad RX frame offset %u
length %u (buffer %u), dropping\n",
This is the only log line on this path, so it is what someone looking
into PHYP descriptor problems from dmesg would see.
The commit message calls this moving existing checks into helpers, and
only says "a PHYP offset+length beyond min(skb_tailroom,
pool->buff_size) is dropped". It doesn't mention that the message
changed or that runt frames now go through the oversize helper. The
wording is the same at the end of the series.
Could the neutral wording be kept, the log say which check failed, or
the len < ETH_HLEN case be handled on its own?
[ ... ]
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235556.60604-1-mmc%40linux.ibm.com
