Re: [PATCH 1/3] net: macb: fix random memory corruption on RX with 64-bit DMA

[<Claudiu.Beznea@microchip.com>]

Hi Anssi,

Few comments... Otherwise I tested this series on a SAMA5D2 Xplained and
SAMA5D4 Xplained under heavy traffic and it seems to behave OK.

Thank you,
Claudiu Beznea

On 30.11.2018 20:21, Anssi Hannula wrote:
> 64-bit DMA addresses are split in upper and lower halves that are
> written in separate fields on GEM. For RX, bit 0 of the address is used
> as the ownership bit (RX_USED). When the RX_USED bit is unset the
> controller is allowed to write data to the buffer.
> 
> The driver does not guarantee that the controller already sees the upper
> half when the RX_USED bit is cleared, possibly resulting in the
> controller writing an incoming frame to an address with an incorrect
> upper half and therefore possibly corrupting unrelated system memory.
> 
> Fix that by adding the necessary DMA memory barrier between the writes.>
> This corruption was observed on a ZynqMP based system.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> Fixes: fff8019a08b6 ("net: macb: Add 64 bit addressing support for GEM")
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Harini Katakam <harini.katakam@xilinx.com>
> Cc: Michal Simek <michal.simek@xilinx.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d8c7ca037ae3..0bc2aab7be40 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -682,6 +682,11 @@ static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_
>  	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
>  		desc_64 = macb_64b_desc(bp, desc);
>  		desc_64->addrh = upper_32_bits(addr);
> +		/* The low bits of RX address contain the RX_USED bit, clearing
> +		 * of which allows packet RX. Make sure the high bits are also
> +		 * visible to HW at that point.
> +		 */
> +		dma_wmb();

I think a wmb() would fit better here so that on ARM to also force the
flushing of caches not affected by dmb() by calling arm_heavy_mb().

Thank you,
Claudiu Beznea

>  	}
>  #endif
>  	desc->addr = lower_32_bits(addr);
>