netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Arlie Davis <arlied@google.com>
To: netdev@vger.kernel.org, linux-parisc@vger.kernel.org
Subject: Bug report (with fix) for DEC Tulip driver (de2104x.c)
Date: Mon, 16 Sep 2019 14:50:53 -0700	[thread overview]
Message-ID: <CAK-9enMxA68mRYFG=2zD02guvCqe-aa3NO0YZuJcTdBWn5MPqg@mail.gmail.com> (raw)

Hello. I'm a developer on GCE, Google's virtual machine platform. As
part of my work, we needed to emulate a DEC Tulip 2104x NIC, so I
implemented a basic virtual device for it.

While doing so, I believe I found a bug in the Linux driver for this
device, in de2104x.c. I see in MAINTAINERS that this is an orphaned
device driver, but I was wondering if the kernel would still accept a
patch for it.  Should I submit this patch, and if so, where should I
submit it?

Below is the commit text from my local repo, and the patch diffs
(they're quite short).

    Fix a bug in DEC Tulip driver (de2104x.c)

    The DEC Tulip Ethernet controller uses a 16-byte transfer descriptor for
    both its transmit (tx) and receive (rx) rings. Each descriptor has a
    "status" uint32 field (called opts1 in de2104x.c, and called TDES0 /
    Status in the DEC hardware specifications) and a "control" field (called
    opts2 in de2104x.c and called TDES1 / Control in the DEC
    specifications). In the "control" field, bit 30 is the LastSegment bit,
    which indicates that this is the last transfer descriptor in a sequence
    of descriptors (in case a single Ethernet frame spans more than one
    descriptor).

    The de2104x driver correctly sets LastSegment, in the de_start_xmit
    function. (The code calls it LastFrag, not LastSegment). However, in the
    interrupt handler (in function de_tx), the driver incorrectly checks for
    this bit in the status field, not the control field. This means that the
    driver is reading bits that are undefined in the specification; the
    spec does not make any guarantees at all about the contents of bits 29
    and bits 30 in the "status" field.

    The effect of the bug is that the driver may think that a TX ring entry
    is never finished, even though a compliant DEC Tulip hardware device (or
    a virtualized device, in a VM) actually did finish sending the Ethernet
    frame.

    The fix is to read the correct "control" field from the TX descriptor.

    DEC Tulip programming specification:

    https://web.archive.org/web/20050805091751/http://www.intel.com/design/network/manuals/21140ahm.pdf

    See section 4.2.2 for the specs on the transfer descriptor.

Here's my patch that fixes it:

diff --git a/drivers/net/ethernet/dec/tulip/de2104x.c
b/drivers/net/ethernet/dec/tulip/de2104x.c
index f1a2da15dd0a..3a420ceb52e5 100644
--- a/drivers/net/ethernet/dec/tulip/de2104x.c
+++ b/drivers/net/ethernet/dec/tulip/de2104x.c
@@ -545,6 +545,7 @@ static void de_tx (struct de_private *de)
        while (tx_tail != tx_head) {
                struct sk_buff *skb;
                u32 status;
+               u32 control;

                rmb();
                status = le32_to_cpu(de->tx_ring[tx_tail].opts1);
@@ -565,7 +566,8 @@ static void de_tx (struct de_private *de)
                pci_unmap_single(de->pdev, de->tx_skb[tx_tail].mapping,
                                 skb->len, PCI_DMA_TODEVICE);

-               if (status & LastFrag) {
+               control = le32_to_cpu(de->tx_ring[tx_tail].opts2);
+               if (control & LastFrag) {
                        if (status & TxError) {
                                netif_dbg(de, tx_err, de->dev,
                                          "tx err, status 0x%x\n",

             reply	other threads:[~2019-09-16 21:51 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-16 21:50 Arlie Davis [this message]
2019-09-17 21:28 ` Bug report (with fix) for DEC Tulip driver (de2104x.c) Andrew Lunn
2019-09-17 21:36   ` Arlie Davis
2019-09-17 22:51     ` John David Anglin
2019-09-18  5:56       ` Helge Deller
2019-09-18 13:27         ` Thomas Bogendoerfer
2019-10-03  1:29           ` Maciej W. Rozycki
2019-09-19 20:31         ` Sven Schnelle
2019-09-20 10:43 ` Thomas Bogendoerfer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAK-9enMxA68mRYFG=2zD02guvCqe-aa3NO0YZuJcTdBWn5MPqg@mail.gmail.com' \
    --to=arlied@google.com \
    --cc=linux-parisc@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).