netdev - Re: [PATCH] ravb: add support for changing MTU

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: <20180214140649.GB8442@bigcity.dyn.berto.se>
Date:   Wed, 14 Feb 2018 15:06:49 +0100
From:   Niklas Söderlund 
        <niklas.soderlund@...natech.se>
To:     Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Cc:     "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH] ravb: add support for changing MTU

Hi Sergei,

Thanks for your feedback.

On 2018-02-14 14:34:09 +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 02/13/2018 04:12 PM, Niklas Söderlund wrote:
> 
> >> On 02/12/2018 11:00 PM, Niklas Söderlund wrote:
> >>
> >>> Allow for chancing the MTU within the limit of the maximum size of a
> >>
> >>    Changing. :-)
> > 
> > Yes :-)
> > 
> >>> descriptor (2048 bytes). Add the callback to change MTU from user-space
> >>> and take the configurable MTU into account when configuring the
> >>> hardware.
> >>>
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> >> [...]
> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >>> index c87f57ca44371586..a4870c9e42195802 100644
> >>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>> @@ -300,9 +300,9 @@ static void ravb_ring_format(struct net_device *ndev, int q)
> >>>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> >>>  		/* RX descriptor */
> >>>  		rx_desc = &priv->rx_ring[q][i];
> >>> -		rx_desc->ds_cc = cpu_to_le16(PKT_BUF_SZ);
> >>> +		rx_desc->ds_cc = cpu_to_le16(priv->rx_buf_sz);
> >>>  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> >>> -					  PKT_BUF_SZ,
> >>> +					  le16_to_cpu(rx_desc->ds_cc),
> >>
> >>   Why not 'priv->rx_buf_sz'?
> > 
> > To align the arguments used with the one in ravb_rx() which uses 
> > le16_to_cpu(rx_desc->ds_cc) already before this patch.
> 
>    Why?
> 
> > 	static bool ravb_rx(struct net_device *ndev, int *quota, int q)
> > 	{
> > 	...
> > 	    /* Refill the RX ring buffers. */
> > 	    for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
> > 		    ...
> > 		    desc->ds_cc = cpu_to_le16(priv->rx_buf_sz);
> > 
> > 		    if (!priv->rx_skb[q][entry]) {
> > 			    ...
> > 			    dma_addr = dma_map_single(ndev->dev.parent, skb->data,
> > 						      le16_to_cpu(desc->ds_cc),
> > 						      DMA_FROM_DEVICE);
> > 			    ...
> > 		    }    
> > 		    ...
> > 	    }
> > 	    ...
> > 	}
> > 
> > I have no preference one way or the other but I think both call sites 
> > should look the same :-)
> 
>    Why? I don't like this idea at all...

OK, I will use 'priv->rx_buf_sz' in next version. But I still think it's 
confusing to not align the call sites :-)

> 
> >> [...]
> >>> @@ -346,6 +346,10 @@ static int ravb_ring_init(struct net_device *ndev, int q)
> >>>  	int ring_size;
> >>>  	int i;
> >>>  
> >>> +	/* +16 gets room from the status from the card. */
> >>> +	priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
> >>> +		ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN + 16;
> >>
> >>    Mhm, I don't think FCS gets added to the frame buffer...
> 
>    It certainly isn't included, judging by the manuals... Instead 2-byte checksum is
> included after the frame data (if checksumming is enabled).

OK, I will drop ETH_FCS_LEN from v2. Would you like a similar patch for 
sh_eth ?

> 
> > And why add 16?
> > 
> > And +16 is added as the comment above states, to leave from the 
> > descriptor status appended by the hardware.
> 
>    I don't see any appended status in the manuals, do you?

You are correct, looks like I misunderstood the docs, I was thinking of 
the descriptor described in 50.4.4 (7) but I now see that is handled 
differently, will drop the +16 for v2. Thanks for spotting this!

> 
> > This is already the case 
> > with PKT_BUF_SZ which for ravb is is set to 1538. MTU (1500) + ETH_HLEN 
> > (14) + VLAN_HLEN(4) + ETH_FCS_LEN(4) + ravb status (16) == 1538.
> 
> > This is also what the sh_eth driver do and I think it's value to keep 
> > these to driver as similar as possible in this regard, would you not 
> 
>   The DMA hardware is totally different, so I don't see any value in mirroring what sh_eth does...
> 
> > agree? If in deed the FSC is not added I think we should fix this for 
> > both drivers in a follow up commit.
> 
>    Probably a good idea... :-)
> 
> [...]
> 
> MBR, Sergei

-- 
Regards,
Niklas Söderlund