| Message ID | 20180212200033.1802-1-niklas.soderlund+renesas@ragnatech.se |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | ravb: add support for changing MTU | expand |
Hello! 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. :-) > 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@ragnatech.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'? [...] > @@ -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... And why add 16? [...] MBR, Sergei
Hi Sergei, Thanks for your feedback. On 2018-02-13 13:01:04 +0300, Sergei Shtylyov wrote: > Hello! > > 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@ragnatech.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. 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 :-) > > [...] > > @@ -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... And why add 16? And +16 is added as the comment above states, to leave from the descriptor status appended by the hardware. 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 agree? If in deed the FSC is not added I think we should fix this for both drivers in a follow up commit. > > [...] > > MBR, Sergei
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@ragnatech.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... >> [...] >>> @@ -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). > 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? > 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
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@ragnatech.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
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index 96a27b00c90e212a..b81f4faf7b10114d 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -1018,6 +1018,7 @@ struct ravb_private { u32 dirty_rx[NUM_RX_QUEUE]; /* Producer ring indices */ u32 cur_tx[NUM_TX_QUEUE]; u32 dirty_tx[NUM_TX_QUEUE]; + u32 rx_buf_sz; /* Based on MTU+slack. */ struct napi_struct napi[NUM_RX_QUEUE]; struct work_struct work; /* MII transceiver section. */ 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 @@ -238,7 +238,7 @@ static void ravb_ring_free(struct net_device *ndev, int q) le32_to_cpu(desc->dptr))) dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr), - PKT_BUF_SZ, + priv->rx_buf_sz, DMA_FROM_DEVICE); } ring_size = sizeof(struct ravb_ex_rx_desc) * @@ -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), DMA_FROM_DEVICE); /* We just set the data size to 0 for a failed mapping which * should prevent DMA from happening... @@ -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; + /* Allocate RX and TX skb rings */ priv->rx_skb[q] = kcalloc(priv->num_rx_ring[q], sizeof(*priv->rx_skb[q]), GFP_KERNEL); @@ -355,7 +359,7 @@ static int ravb_ring_init(struct net_device *ndev, int q) goto error; for (i = 0; i < priv->num_rx_ring[q]; i++) { - skb = netdev_alloc_skb(ndev, PKT_BUF_SZ + RAVB_ALIGN - 1); + skb = netdev_alloc_skb(ndev, priv->rx_buf_sz + RAVB_ALIGN - 1); if (!skb) goto error; ravb_set_buffer_align(skb); @@ -586,7 +590,7 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int q) skb = priv->rx_skb[q][entry]; priv->rx_skb[q][entry] = NULL; dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr), - PKT_BUF_SZ, + priv->rx_buf_sz, DMA_FROM_DEVICE); get_ts &= (q == RAVB_NC) ? RAVB_RXTSTAMP_TYPE_V2_L2_EVENT : @@ -619,11 +623,12 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int q) for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) { entry = priv->dirty_rx[q] % priv->num_rx_ring[q]; desc = &priv->rx_ring[q][entry]; - desc->ds_cc = cpu_to_le16(PKT_BUF_SZ); + desc->ds_cc = cpu_to_le16(priv->rx_buf_sz); if (!priv->rx_skb[q][entry]) { skb = netdev_alloc_skb(ndev, - PKT_BUF_SZ + RAVB_ALIGN - 1); + priv->rx_buf_sz + + RAVB_ALIGN - 1); if (!skb) break; /* Better luck next round. */ ravb_set_buffer_align(skb); @@ -1854,6 +1859,17 @@ static int ravb_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd) return phy_mii_ioctl(phydev, req, cmd); } +static int ravb_change_mtu(struct net_device *ndev, int new_mtu) +{ + if (netif_running(ndev)) + return -EBUSY; + + ndev->mtu = new_mtu; + netdev_update_features(ndev); + + return 0; +} + static void ravb_set_rx_csum(struct net_device *ndev, bool enable) { struct ravb_private *priv = netdev_priv(ndev); @@ -1895,6 +1911,7 @@ static const struct net_device_ops ravb_netdev_ops = { .ndo_set_rx_mode = ravb_set_rx_mode, .ndo_tx_timeout = ravb_tx_timeout, .ndo_do_ioctl = ravb_do_ioctl, + .ndo_change_mtu = ravb_change_mtu, .ndo_validate_addr = eth_validate_addr, .ndo_set_mac_address = eth_mac_addr, .ndo_set_features = ravb_set_features, @@ -2117,6 +2134,9 @@ static int ravb_probe(struct platform_device *pdev) goto out_release; } + ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); + ndev->min_mtu = ETH_MIN_MTU; + /* Set function */ ndev->netdev_ops = &ravb_netdev_ops; ndev->ethtool_ops = &ravb_ethtool_ops;
Allow for chancing the MTU within the limit of the maximum size of a 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@ragnatech.se> --- drivers/net/ethernet/renesas/ravb.h | 1 + drivers/net/ethernet/renesas/ravb_main.c | 34 +++++++++++++++++++++++++------- 2 files changed, 28 insertions(+), 7 deletions(-)