netdev - Re: [PATCH/RFC net-next] ravb: Add dma_unmap_single in ravb_ring

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] [day] [month] [year] [list]

Message-ID: <7e499a0f-3c98-420e-de8a-b29bd54a989d@cogentembedded.com>
Date:   Wed, 9 Nov 2016 21:00:25 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     Simon Horman <horms+renesas@...ge.net.au>
Cc:     Magnus Damm <magnus.damm@...il.com>, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH/RFC net-next] ravb: Add dma_unmap_single in ravb_ring_free

Hello.

On 11/07/2016 06:27 PM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@...esas.com>
>
> The kernel panic occurs with "swiotlb buffer is full" message
> after repeating suspend and resume, because dma_map_single of
> ravb_ring_format and ravb_start_xmit is not released.

    The same issue must occur after several ifconfig up/down I think...
this is quite embarrassing actually, and I think this bug was inherited from 
sh_eth. Perhaps it was made more visible with adding PM support. :-/

> This patch adds dma_unmap_single in ravb_ring_free, and fixes
> its problem.

    Well, actually ravb_ring_free() was meant to undo what ravb_ring_init() 
does...

> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@...esas.com>
> Signed-off-by: Simon Horman <horms+renesas@...ge.net.au>
> ---
> Sergei, this is a patch from the Gen3 3.3.2 BSP.

    I suspect the ravb driver there is greatly different from the upstream one...

> Please consider if it is appropriate for mainline.
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 27cfec3154c8..e44629b75c83 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -185,6 +185,9 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	int ring_size;
>  	int i;
> +	struct ravb_ex_rx_desc *rx_desc;
> +	struct ravb_tx_desc *tx_desc;
> +	u32 size;

    DaveM prefers the local declarations in the reverse Xmas tree order; me 
too. :-)

[...]
> @@ -207,6 +210,16 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>  	priv->tx_align[q] = NULL;
>
>  	if (priv->rx_ring[q]) {
> +		for (i = 0; i < priv->num_rx_ring[q]; i++) {
> +			rx_desc = &priv->rx_ring[q][i];
> +			if (rx_desc->dptr != 0) {

    This seems wrong. This driver uses RX descriptors with zero data size to 
indicate the failed DMA mapping. And anyway, I think you should have used 
dma_mapping_error() instead of a zero check... and '!= 0' was unnecessary.

> +				dma_unmap_single(ndev->dev.parent,
> +						 le32_to_cpu(rx_desc->dptr),
> +						 PKT_BUF_SZ,
> +						 DMA_FROM_DEVICE);
> +				rx_desc->dptr = 0;

    Hence I'd prefer:

				rx_desc->ds_cc = cpu_to_le16(0);

[...]
> @@ -215,6 +228,16 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>  	}
>
>  	if (priv->tx_ring[q]) {
> +		for (i = 0; i < priv->num_tx_ring[q]; i++) {

    I'm afraid this is wrong. TX ring contains NUM_TX_DESC (2) descriptors per 
skb, so this loop only cleans the 1st half of the TX ring.

> +			tx_desc = &priv->tx_ring[q][i];
> +			size = le16_to_cpu(tx_desc->ds_tagl) & TX_DS;
> +			if (tx_desc->dptr != 0) {
> +				dma_unmap_single(ndev->dev.parent,
> +						 le32_to_cpu(tx_desc->dptr),
> +						 size, DMA_TO_DEVICE);
> +				tx_desc->dptr = 0;

    Again, I'm not fond of this... are you sure 0 is incorrect DMA address?

> +			}
> +		}

    BTW, can't we use ravb_tx_free() instead of this loop?

MBR, Sergei