Bug 256482

Summary:

[genet] gen_encap() dma loads into active map when 'tx_queue' is full

Product:

Base System

Reporter:

ghuckriede

Component:

arm

Assignee:

Mitchell Horne <mhorne>

Status:

Closed FIXED

Severity:

Affects Only Me

CC:

grahamperrin, markj, mhorne, tod.jackson

Priority:

---

Version:

CURRENT

Hardware:

arm64

OS:

Any

Attachments:

Description	Flags
Potential Fix	none

Description ghuckriede 2021-06-08 14:48:32 UTC

Created attachment 225638 [details]
Potential Fix

In gen_encap() #n1053 it always calls bus_dmamap_load_mbuf_sg() into 'map' (which is the current tx_queue).  If the tx_queue is full, it will load with a 'map' that already has a currently active mapping.

https://www.freebsd.org/cgi/man.cgi?query=busdma&sektion=9
bus_dmamap_load: "map     A DMA map without a currently active mapping."

Also "(if nsegs == 0)" #n1077 shouldn't bus_dmamap_unload() be called as bus_dmamap_load_mbuf_sq() was successful?

See https://cgit.freebsd.org/src/tree/sys/arm64/broadcom/genet/if_genet.c?id=f66a1f40740c63741b6ebe48cb0cbce9e52ef34e for line number references.

Attached is diff with a potential fix.

Checking for a full queue and returning ENOMEM will allow gen_start_locked() to set the IFF_DRV_OACTIVE faster without having to needlessly check if the mbuf will fit (it won't).

Comment 1 commit-hook freebsd_committer

2024-06-27 17:45:35 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=a35f66510917f5ac21c11e9642174cda7718fbc6

commit a35f66510917f5ac21c11e9642174cda7718fbc6
Author:     Mitchell Horne <mhorne@FreeBSD.org>
AuthorDate: 2024-06-27 17:26:54 +0000
Commit:     Mitchell Horne <mhorne@FreeBSD.org>
CommitDate: 2024-06-27 17:44:36 +0000

    if_genet: don't load DMA mapping when tx_queue is full

    gen_encap() always calls bus_dmamap_load_mbuf_sg() into 'map' (which is
    the current tx_queue). If the tx_queue is full, it will load with a
    'map' that already has a currently active mapping. This violates the
    busdma(9) KPI.

    Checking for a full queue and returning ENOBUFS will allow
    gen_start_locked() to set the IFF_DRV_OACTIVE faster without having to
    needlessly check if the mbuf will fit (it won't).

    PR:             256482
    Reviewed by:    mhorne
    MFC after:      1 week
    Submitted by:   ghuckriede@blackberry.com

 sys/arm64/broadcom/genet/if_genet.c | 4 ++++
 1 file changed, 4 insertions(+)

Comment 2 Mitchell Horne freebsd_committer

2024-06-27 17:51:26 UTC

Hi, thanks for the submission.

I considered the part about calling bus_dmamap_unload(). This seems to be a grey area of the busdma API, but the majority of other ethernet drivers following this pattern do not call bus_dmamap_unload() when nsegs == 0. 

Looking at the busdma implementations, it is not clear when it is possible to return 0 with nsegs == 0; most paths will return some kind of error. The most likely meaning of this scenario (nsegs == 0) is that the call was successful but nothing was actually mapped. Therefore the call to bus_dmamap_unload() is unnecessary.

Comment 3 commit-hook freebsd_committer

2024-07-04 13:32:30 UTC

A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=8daa60945080c602f2a65ed2a0f2333c15bd947e

commit 8daa60945080c602f2a65ed2a0f2333c15bd947e
Author:     Mitchell Horne <mhorne@FreeBSD.org>
AuthorDate: 2024-06-27 17:26:54 +0000
Commit:     Mitchell Horne <mhorne@FreeBSD.org>
CommitDate: 2024-07-04 13:31:54 +0000

    if_genet: don't load DMA mapping when tx_queue is full

    gen_encap() always calls bus_dmamap_load_mbuf_sg() into 'map' (which is
    the current tx_queue). If the tx_queue is full, it will load with a
    'map' that already has a currently active mapping. This violates the
    busdma(9) KPI.

    Checking for a full queue and returning ENOBUFS will allow
    gen_start_locked() to set the IFF_DRV_OACTIVE faster without having to
    needlessly check if the mbuf will fit (it won't).

    PR:             256482
    Reviewed by:    mhorne
    MFC after:      1 week
    Submitted by:   ghuckriede@blackberry.com

    (cherry picked from commit a35f66510917f5ac21c11e9642174cda7718fbc6)

 sys/arm64/broadcom/genet/if_genet.c | 4 ++++
 1 file changed, 4 insertions(+)