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

[Jason Wang]

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
>