On Thu, 19 Nov 2020 23:52:55 +0000 Ramsay, Lincoln wrote:
> When performing IPv6 forwarding, there is an expectation that SKBs
> will have some headroom. When forwarding a packet from the aquantia
> driver, this does not always happen, triggering a kernel warning.
>
> aq_ring.c has this code (edited slightly for brevity):
>
> if (buff->is_eop && buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
> skb = build_skb(aq_buf_vaddr(&buff->rxdata), AQ_CFG_RX_FRAME_MAX);
> } else {
> skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
>
> There is a significant difference between the SKB produced by these
> 2 code paths. When napi_alloc_skb creates an SKB, there is a certain
> amount of headroom reserved. However, this is not done in the
> build_skb codepath.
>
> As the hardware buffer that build_skb is built around does not
> handle the presence of the SKB header, this code path is being
> removed and the napi_alloc_skb path will always be used. This code
> path does have to copy the packet header into the SKB, but it adds
> the packet data as a frag.
>
> Signed-off-by: Lincoln Ramsay <lincoln.ram...@opengear.com>
I was going to apply as a fix to net and stable but too many small nits
here to pass. First of all please add a From: line at the beginning of
the mail which matches the signoff (or use git-send-email, it'll get it
right).
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 4f913658eea4..425e8e5afec7 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -413,85 +413,64 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
> buff->rxdata.pg_off,
> buff->len, DMA_FROM_DEVICE);
>
> - /* for single fragment packets use build_skb() */
> - if (buff->is_eop &&
> - buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
> - skb = build_skb(aq_buf_vaddr(&buff->rxdata),
> + skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
> + if (unlikely(!skb)) {
> + u64_stats_update_begin(&self->stats.rx.syncp);
> + self->stats.rx.skb_alloc_fails++;
> + u64_stats_update_end(&self->stats.rx.syncp);
> + err = -ENOMEM;
> + goto err_exit;
> + }
> + if (is_ptp_ring)
> + buff->len -=
> + aq_ptp_extract_ts(self->aq_nic, skb,
> + aq_buf_vaddr(&buff->rxdata),
> + buff->len);
Align continuations of the lines under '(' like:
./scripts/checkpatch.pl --max-line-length=80 --strict
is asking you to.
> + hdr_len = buff->len;
> + if (hdr_len > AQ_CFG_RX_HDR_SIZE)
> + hdr_len = eth_get_headlen(skb->dev,
> +
> aq_buf_vaddr(&buff->rxdata),
> + AQ_CFG_RX_HDR_SIZE);
ditto
> + memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
> + ALIGN(hdr_len, sizeof(long)));
And here, etc.
> + if (buff->len - hdr_len > 0) {
> + skb_add_rx_frag(skb, 0, buff->rxdata.page,
> + buff->rxdata.pg_off + hdr_len,
> + buff->len - hdr_len,
> + if (!buff->is_eop) {
> + buff_ = buff;
> + i = 1U;
> + do {
> + next_ = buff_->next,
The end of this line should be a semicolon.
> + buff_ = &self->buff_ring[next_];
Thanks!
