Re: [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits

[Stefan Hajnoczi]

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
> >
>
>