2016-04-04 18:11 GMT-07:00 Petri Gynther <pgynt...@google.com>:
> Hi Florian,
>
> On Mon, Apr 4, 2016 at 5:58 PM, Florian Fainelli <f.faine...@gmail.com> wrote:
>>
>> 2016-04-04 17:10 GMT-07:00 Petri Gynther <pgynt...@google.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 <pgynt...@google.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
