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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 4 Apr 2016 17:58:15 -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
Subject: Re: [PATCH net-next 3/3] net: bcmgenet: fix dmadesc_set()

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!

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