On Fri, Feb 20, 2026 at 09:19:45AM -0800, Stephen Hemminger wrote: > On Fri, 20 Feb 2026 21:15:13 +0000 > Talluri Chaitanyababu <[email protected]> wrote: > > > Update e1000 driver docs to cover all Intel 1G devices, > > including emulated e1000 and real hardware. > > > > v3: Resolved apply patch failure. > > > > Signed-off-by: Talluri Chaitanyababu <[email protected]> > > Pointed AI review at this patch, and the overall e1000 driver documentation. >
I think the review is overly negative, but it does highlight some stuff that should be fixed either in this patch or a different one. > Here's my review of the patch against the existing documentation: > > --- > > **Patch:** `[PATCH v3] doc: improve e1000 driver documentation` > **Author:** Talluri Chaitanyababu <[email protected]> > > **Summary:** The patch prepends a new top-level section "E1000 Poll Mode > Driver" with an "Overview" subsection to `e1000em.rst`, but there are several > issues. > > --- > > **Structural / RST Issues** > > The patch inserts a new top-level title ("E1000 Poll Mode Driver", underlined > with `=`) before the existing top-level title ("Driver for VM Emulated > Devices", also underlined with `=`). RST doesn't allow two sections at the > same heading level in a document without one being nested under the other. > This creates a document with two competing top-level headings, which is > likely to confuse Sphinx and render incorrectly. The patch should either > replace the existing title or restructure so "Driver for VM Emulated Devices" > becomes a subsection beneath the new top-level heading. > This needs to be fixed in this patch. > **Content Accuracy** > > The note in the new section states: *"The `igb` and `igc` PMDs are built from > the same driver binary but are documented in separate sections for clarity."* > This is misleading. `igb` is a Linux kernel driver, not a DPDK PMD, and `igc` > similarly refers to a kernel driver (for I225/I226). DPDK's 1G PMD for real > hardware uses the `e1000` driver. The note should be removed or corrected — > as written it introduces inaccurate information. > I disagree with this. I think the content here is correct so far as it goes, but maybe we need to expand and clarify a little? > **Scope vs. Commit Message** > > The commit message says "Update e1000 driver docs to cover all Intel 1G > devices, including emulated e1000 and real hardware," but the patch only adds > 16 lines of overview text and doesn't actually document real hardware (PCI > IDs, configuration, feature support, etc.). The commit message overclaims > what the patch delivers. > > **Content of the Overview** > > The bullet point for `e1000e` lists an extensive range of adapters > (82563/6/7, 82571/2/3/4/7/8/9, 82583, I217/I218/I219) but the existing file > is entirely focused on emulated/VM devices. Mixing in details about `e1000e` > physical hardware here, without corresponding documentation for those > devices, leaves the overview incomplete and potentially confusing to readers. > Yes, this needs to be fixed, but not necessarily in this patch. > **Minor Style Issues** > > The note uses a Unicode non-breaking hyphen (`‑`) in "PCI‑Express" rather > than a regular hyphen or the conventional "PCIe" abbreviation. This can cause > rendering or copy-paste issues and should use standard ASCII. > Needs fixing. > **Existing Document Issues (pre-patch)** > > Since you asked for a documentation review of the file as well, a few > pre-existing issues worth noting: > > - The document is severely outdated — it references Fedora 14/18, QEMU > 0.14.0/0.15.1, KVM setup instructions from around 2011–2012, and kernel > version 2.6.25. These sections should either be updated or removed. > - Line 60 has a broken code block: `tar xzf qemu-kvm-release.tar.gz cd > qemu-kvm-release` — the `cd` command is on the same line as `tar`, missing a > newline. > - The hyperlinks on lines 53, 81, 95–99 use the old `http://` scheme and some > point to domains that are likely stale (sourceforge KVM project, > `qemu.weilnetz.de`). > - Line 90: "The Qemu" should be "QEMU" (it's an acronym, always uppercase in > DPDK docs). > - Lines 113, 114, 120: em-dashes used as list bullets (`—`) are non-standard > RST; they should use standard `*` bullets or proper RST definition lists. > > --- These issue do need to be fixed, but again, not necessarily in this patch. It would be good after this patch is respun to have a follow-up patch or patchset cleaning up the rest of the doc. Marking patch as "changes requested" in patchwork, because of the things I highlight above that need fixing. /Bruce
