| Message ID | 20221117165554.1773409-2-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtl8139: honor large send MSS value | expand |
On 17/11/22 17:55, Stefan Hajnoczi wrote: > The device turns the Tx Descriptor into a Tx Status descriptor after > fully reading the descriptor. This involves clearing Tx Own (bit 31) to > indicate that the driver has ownership of the descriptor again as well > as several other bits. > > The code keeps the first dword of the Tx Descriptor in the txdw0 local > variable. txdw0 is reused to build the first word of the Tx Status > descriptor. Later on the code uses txdw0 again, incorrectly assuming > that it still contains the first dword of the Tx Descriptor. The tx > offloading code misbehaves because it sees bogus bits in txdw0. > > Use a separate local variable for Tx Status and preserve Tx Descriptor > in txdw0. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/net/rtl8139.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Fri, Nov 18, 2022 at 12:56 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > The device turns the Tx Descriptor into a Tx Status descriptor after > fully reading the descriptor. This involves clearing Tx Own (bit 31) to > indicate that the driver has ownership of the descriptor again as well > as several other bits. > > The code keeps the first dword of the Tx Descriptor in the txdw0 local > variable. txdw0 is reused to build the first word of the Tx Status > descriptor. Later on the code uses txdw0 again, incorrectly assuming > that it still contains the first dword of the Tx Descriptor. The tx > offloading code misbehaves because it sees bogus bits in txdw0. (This is only noticeable with patch 2). > > Use a separate local variable for Tx Status and preserve Tx Descriptor > in txdw0. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Acked-by: Jason Wang <jasowang@redhat.com> > --- > hw/net/rtl8139.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c > index e6643e3c9d..ffef3789b5 100644 > --- a/hw/net/rtl8139.c > +++ b/hw/net/rtl8139.c > @@ -2027,18 +2027,21 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) > s->currCPlusTxDesc = 0; > } > > + /* Build the Tx Status Descriptor */ > + uint32_t tx_status = txdw0; > + > /* transfer ownership to target */ > - txdw0 &= ~CP_TX_OWN; > + tx_status &= ~CP_TX_OWN; > > /* reset error indicator bits */ > - txdw0 &= ~CP_TX_STATUS_UNF; > - txdw0 &= ~CP_TX_STATUS_TES; > - txdw0 &= ~CP_TX_STATUS_OWC; > - txdw0 &= ~CP_TX_STATUS_LNKF; > - txdw0 &= ~CP_TX_STATUS_EXC; > + tx_status &= ~CP_TX_STATUS_UNF; > + tx_status &= ~CP_TX_STATUS_TES; > + tx_status &= ~CP_TX_STATUS_OWC; > + tx_status &= ~CP_TX_STATUS_LNKF; > + tx_status &= ~CP_TX_STATUS_EXC; > > /* update ring data */ > - val = cpu_to_le32(txdw0); > + val = cpu_to_le32(tx_status); > pci_dma_write(d, cplus_tx_ring_desc, (uint8_t *)&val, 4); > > /* Now decide if descriptor being processed is holding the last segment of packet */ > -- > 2.38.1 >
On Sun, 20 Nov 2022 at 23:17, Jason Wang <jasowang@redhat.com> wrote: > > On Fri, Nov 18, 2022 at 12:56 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > The device turns the Tx Descriptor into a Tx Status descriptor after > > fully reading the descriptor. This involves clearing Tx Own (bit 31) to > > indicate that the driver has ownership of the descriptor again as well > > as several other bits. > > > > The code keeps the first dword of the Tx Descriptor in the txdw0 local > > variable. txdw0 is reused to build the first word of the Tx Status > > descriptor. Later on the code uses txdw0 again, incorrectly assuming > > that it still contains the first dword of the Tx Descriptor. The tx > > offloading code misbehaves because it sees bogus bits in txdw0. > > (This is only noticeable with patch 2). Yes, although the large_send_mss variable is already junk because some bits have been cleared: int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK; Luckily it's not used yet, aside from DPRINTF(). > > > > > Use a separate local variable for Tx Status and preserve Tx Descriptor > > in txdw0. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > Acked-by: Jason Wang <jasowang@redhat.com> > > > --- > > hw/net/rtl8139.c | 17 ++++++++++------- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c > > index e6643e3c9d..ffef3789b5 100644 > > --- a/hw/net/rtl8139.c > > +++ b/hw/net/rtl8139.c > > @@ -2027,18 +2027,21 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) > > s->currCPlusTxDesc = 0; > > } > > > > + /* Build the Tx Status Descriptor */ > > + uint32_t tx_status = txdw0; > > + > > /* transfer ownership to target */ > > - txdw0 &= ~CP_TX_OWN; > > + tx_status &= ~CP_TX_OWN; > > > > /* reset error indicator bits */ > > - txdw0 &= ~CP_TX_STATUS_UNF; > > - txdw0 &= ~CP_TX_STATUS_TES; > > - txdw0 &= ~CP_TX_STATUS_OWC; > > - txdw0 &= ~CP_TX_STATUS_LNKF; > > - txdw0 &= ~CP_TX_STATUS_EXC; > > + tx_status &= ~CP_TX_STATUS_UNF; > > + tx_status &= ~CP_TX_STATUS_TES; > > + tx_status &= ~CP_TX_STATUS_OWC; > > + tx_status &= ~CP_TX_STATUS_LNKF; > > + tx_status &= ~CP_TX_STATUS_EXC; > > > > /* update ring data */ > > - val = cpu_to_le32(txdw0); > > + val = cpu_to_le32(tx_status); > > pci_dma_write(d, cplus_tx_ring_desc, (uint8_t *)&val, 4); > > > > /* Now decide if descriptor being processed is holding the last segment of packet */ > > -- > > 2.38.1 > > > >
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index e6643e3c9d..ffef3789b5 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -2027,18 +2027,21 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) s->currCPlusTxDesc = 0; } + /* Build the Tx Status Descriptor */ + uint32_t tx_status = txdw0; + /* transfer ownership to target */ - txdw0 &= ~CP_TX_OWN; + tx_status &= ~CP_TX_OWN; /* reset error indicator bits */ - txdw0 &= ~CP_TX_STATUS_UNF; - txdw0 &= ~CP_TX_STATUS_TES; - txdw0 &= ~CP_TX_STATUS_OWC; - txdw0 &= ~CP_TX_STATUS_LNKF; - txdw0 &= ~CP_TX_STATUS_EXC; + tx_status &= ~CP_TX_STATUS_UNF; + tx_status &= ~CP_TX_STATUS_TES; + tx_status &= ~CP_TX_STATUS_OWC; + tx_status &= ~CP_TX_STATUS_LNKF; + tx_status &= ~CP_TX_STATUS_EXC; /* update ring data */ - val = cpu_to_le32(txdw0); + val = cpu_to_le32(tx_status); pci_dma_write(d, cplus_tx_ring_desc, (uint8_t *)&val, 4); /* Now decide if descriptor being processed is holding the last segment of packet */
The device turns the Tx Descriptor into a Tx Status descriptor after fully reading the descriptor. This involves clearing Tx Own (bit 31) to indicate that the driver has ownership of the descriptor again as well as several other bits. The code keeps the first dword of the Tx Descriptor in the txdw0 local variable. txdw0 is reused to build the first word of the Tx Status descriptor. Later on the code uses txdw0 again, incorrectly assuming that it still contains the first dword of the Tx Descriptor. The tx offloading code misbehaves because it sees bogus bits in txdw0. Use a separate local variable for Tx Status and preserve Tx Descriptor in txdw0. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/net/rtl8139.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)