netdev - Re: [PATCH net-next 3/3] net: bcmgenet: fix dmadesc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGVrzcYbX0FAu9oyrmCfHafyjuQdBKvAfj+iArsjpcO-z1PPDQ@mail.gmail.com>
Date:	Mon, 4 Apr 2016 18:27:16 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Petri Gynther <pgynther@...gle.com>
Cc:	netdev <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Jaedon Shin <jaedon.shin@...il.com>, opendmb@...il.com,
	Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next 3/3] net: bcmgenet: fix dmadesc_set()

2016-04-04 18:11 GMT-07:00 Petri Gynther <pgynther@...gle.com>:
> Hi Florian,
>
> On Mon, Apr 4, 2016 at 5:58 PM, Florian Fainelli <f.fainelli@...il.com> wrote:
>>
>> 2016-04-04 17:10 GMT-07:00 Petri Gynther <pgynther@...gle.com>:
>> > dmadesc_set() is used for setting the Tx buffer DMA address, length,
>> > and status bits on a Tx ring descriptor when a frame is being Tx'ed.
>> >
>> > Always set the Tx buffer DMA address first, before updating the length
>> > and status bits, i.e. giving the Tx descriptor to the hardware.
>>
>> Does this fix any real bug you have observed? The hardware won't
>> transmit anything until you start writing the correct TDMA producer
>> index. Also, dmadesc_set_length_status and dmadesc_set_addr both use
>> I/O accessors which use a volatile, so they should not be re-ordered
>> relative to each other.
>>
>> I do agree that the change looks like how it should be done, I am just
>> questioning the qualification of this as a fix or not.
>>
>> Thanks!
>
> You are right. Nothing is transmitted until TDMA producer index is
> incremented, and that happens after dmadesc_set() has been called.
>
> So, this is really just a cleanup. Perhaps not worth it, after all.

OK, this is fine as a cleanup, and the typical logic is to write the
address first and the length/status second in case of descriptor-based
DMA-backed NICs, so why not.

>
> But I do have a question -- do we need wmb() in bcmgenet_xmit(), just
> before we kick the TDMA producer index, to ensure Tx descriptor writes
> have been completed?

Humm, that is a good question. We definitively need to make sure that
the writes to prod_index completes, since that variable is stored in
DRAM/normal memory, so subject to pre-fetching based on the memory
attributes (at least on ARM). It seems to me like there is a direct
dependency though, so by the time we use ring->prod_index, the CPU
should have ensured that all writes are complete.

>
> @@ -1606,6 +1606,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff
> *skb, struct net_device *dev)
>
>         if (!skb->xmit_more || netif_xmit_stopped(txq)) {
>                 /* Packets are ready, update producer index */
> +               wmb();
>                 bcmgenet_tdma_ring_writel(priv, ring->index,
>                                           ring->prod_index, TDMA_PROD_INDEX);
>
>>
>>
>> >
>> > Signed-off-by: Petri Gynther <pgynther@...gle.com>
>> > ---
>> >  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> > index d77cd6d..f7b42b9 100644
>> > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> > @@ -104,8 +104,8 @@ static inline void dmadesc_set_addr(struct bcmgenet_priv *priv,
>> >  static inline void dmadesc_set(struct bcmgenet_priv *priv,
>> >                                void __iomem *d, dma_addr_t addr, u32 val)
>> >  {
>> > -       dmadesc_set_length_status(priv, d, val);
>> >         dmadesc_set_addr(priv, d, addr);
>> > +       dmadesc_set_length_status(priv, d, val);
>> >  }
>> >
>> >  static inline dma_addr_t dmadesc_get_addr(struct bcmgenet_priv *priv,
>> > --
>> > 2.8.0.rc3.226.g39d4020
>> >
>>
>>
>>
>> --
>> Florian



-- 
Florian