Message ID | 20100622085415.5566.22523.sendpatchset@localhost.localdomain |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, 2010-06-22 at 04:50 -0400, Amerigo Wang wrote: > This patch adds dynamic LRO diable support for s2io net driver. > > (I don't have s2io card, so only did compiling test. Anyone who wants > to test this is more than welcome.) > > This is based on Neil's initial work, and heavily modified > based on Ramkrishna's suggestions. [...] > +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data) > +{ > + struct s2io_nic *sp = netdev_priv(dev); > + int rc = 0; > + int changed = 0; > + > + if (data & ~ETH_FLAG_LRO) > + return -EOPNOTSUPP; > + > + if (data & ETH_FLAG_LRO) { > + if (lro_enable) { > + if (!(dev->features & NETIF_F_LRO)) { > + dev->features |= NETIF_F_LRO; > + changed = 1; > + } > + } else > + rc = -EOPNOTSUPP; Should lro_enable=0 really prevent enabling it later? This seems unusual. > + } else if (dev->features & NETIF_F_LRO) { > + dev->features &= ~NETIF_F_LRO; > + changed = 1; > + } > + > + if (changed && netif_running(dev)) { > + s2io_stop_all_tx_queue(sp); > + s2io_card_down(sp); > + sp->lro = dev->features & NETIF_F_LRO; [...] This means s2io_nic::lro and ring_info::lro can have the value NETIF_F_LRO, where previously they would only have the value 0 or 1. I don't know whether this could be a problem, but the safe thing to do is to coerce the value by writing !!(dev->features & NETIF_F_LRO). Ben.
On Tue, 22 Jun 2010 12:44:40 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote: > On Tue, 2010-06-22 at 04:50 -0400, Amerigo Wang wrote: > > This patch adds dynamic LRO diable support for s2io net driver. > > > > (I don't have s2io card, so only did compiling test. Anyone who wants > > to test this is more than welcome.) > > > > This is based on Neil's initial work, and heavily modified > > based on Ramkrishna's suggestions. > [...] > > +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data) > > +{ > > + struct s2io_nic *sp = netdev_priv(dev); > > + int rc = 0; > > + int changed = 0; > > + > > + if (data & ~ETH_FLAG_LRO) > > + return -EOPNOTSUPP; > > + > > + if (data & ETH_FLAG_LRO) { > > + if (lro_enable) { > > + if (!(dev->features & NETIF_F_LRO)) { > > + dev->features |= NETIF_F_LRO; > > + changed = 1; > > + } > > + } else > > + rc = -EOPNOTSUPP; > > Should lro_enable=0 really prevent enabling it later? This seems > unusual. We are doing this in bnx2x. Current Amerigo patch change to the same behavior mlx4. For me that have sense - if you want to disallow LRO - use module option. In this case however lro_enable variable looks obsolete from times where there was no ethtool possibility to dynamic set/unset LRO. Perhaps it should be removed at all, but maybe not as part of that patch. Stanislaw -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 22, 2010 at 01:50:07AM -0700, Amerigo Wang wrote: > > This patch adds dynamic LRO diable support for s2io net driver. > > (I don't have s2io card, so only did compiling test. Anyone who wants > to test this is more than welcome.) Unfortunately the patch did not quite work, mostly due to NETIF_F_LRO needing to be added at probe time to the device features. I was able to modify the patch to get it working, and will respond seperately with the working patch. Thanks, Jon > > This is based on Neil's initial work, and heavily modified > based on Ramkrishna's suggestions. > > Signed-off-by: WANG Cong <amwang@redhat.com> > Signed-off-by: Neil Horman <nhorman@redhat.com> > Acked-by: Neil Horman <nhorman@redhat.com> > Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com> > Cc: Ramkrishna Vepa <Ramkrishna.Vepa@exar.com> > > --- > > diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c > index 668327c..2e6e3b3 100644 > --- a/drivers/net/s2io.c > +++ b/drivers/net/s2io.c > @@ -795,7 +795,6 @@ static int init_shared_mem(struct s2io_nic *nic) > ring->rx_curr_put_info.ring_len = rx_cfg->num_rxd - 1; > ring->nic = nic; > ring->ring_no = i; > - ring->lro = lro_enable; > > blk_cnt = rx_cfg->num_rxd / (rxd_count[nic->rxd_mode] + 1); > /* Allocating all the Rx blocks */ > @@ -6684,6 +6683,41 @@ static int s2io_ethtool_op_set_tso(struct net_device *dev, u32 data) > > return 0; > } > +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data) > +{ > + struct s2io_nic *sp = netdev_priv(dev); > + int rc = 0; > + int changed = 0; > + > + if (data & ~ETH_FLAG_LRO) > + return -EOPNOTSUPP; > + > + if (data & ETH_FLAG_LRO) { > + if (lro_enable) { > + if (!(dev->features & NETIF_F_LRO)) { > + dev->features |= NETIF_F_LRO; > + changed = 1; > + } > + } else > + rc = -EOPNOTSUPP; > + } else if (dev->features & NETIF_F_LRO) { > + dev->features &= ~NETIF_F_LRO; > + changed = 1; > + } > + > + if (changed && netif_running(dev)) { > + s2io_stop_all_tx_queue(sp); > + s2io_card_down(sp); > + sp->lro = dev->features & NETIF_F_LRO; > + rc = s2io_card_up(sp); > + if (rc) > + s2io_reset(sp); > + else > + s2io_start_all_tx_queue(sp); > + } > + > + return rc; > +} > > static const struct ethtool_ops netdev_ethtool_ops = { > .get_settings = s2io_ethtool_gset, > @@ -6701,6 +6735,8 @@ static const struct ethtool_ops netdev_ethtool_ops = { > .get_rx_csum = s2io_ethtool_get_rx_csum, > .set_rx_csum = s2io_ethtool_set_rx_csum, > .set_tx_csum = s2io_ethtool_op_set_tx_csum, > + .set_flags = s2io_ethtool_set_flags, > + .get_flags = ethtool_op_get_flags, > .set_sg = ethtool_op_set_sg, > .get_tso = s2io_ethtool_op_get_tso, > .set_tso = s2io_ethtool_op_set_tso, > @@ -7229,6 +7265,7 @@ static int s2io_card_up(struct s2io_nic *sp) > struct ring_info *ring = &mac_control->rings[i]; > > ring->mtu = dev->mtu; > + ring->lro = sp->lro; > ret = fill_rx_buffers(sp, ring, 1); > if (ret) { > DBG_PRINT(ERR_DBG, "%s: Out of memory in Open\n", > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/22/10 19:44, Ben Hutchings wrote: > On Tue, 2010-06-22 at 04:50 -0400, Amerigo Wang wrote: >> This patch adds dynamic LRO diable support for s2io net driver. >> >> (I don't have s2io card, so only did compiling test. Anyone who wants >> to test this is more than welcome.) >> >> This is based on Neil's initial work, and heavily modified >> based on Ramkrishna's suggestions. > [...] >> +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data) >> +{ >> + struct s2io_nic *sp = netdev_priv(dev); >> + int rc = 0; >> + int changed = 0; >> + >> + if (data& ~ETH_FLAG_LRO) >> + return -EOPNOTSUPP; >> + >> + if (data& ETH_FLAG_LRO) { >> + if (lro_enable) { >> + if (!(dev->features& NETIF_F_LRO)) { >> + dev->features |= NETIF_F_LRO; >> + changed = 1; >> + } >> + } else >> + rc = -EOPNOTSUPP; > > Should lro_enable=0 really prevent enabling it later? This seems > unusual. > I agree with Stanislaw on this point. >> + } else if (dev->features& NETIF_F_LRO) { >> + dev->features&= ~NETIF_F_LRO; >> + changed = 1; >> + } >> + >> + if (changed&& netif_running(dev)) { >> + s2io_stop_all_tx_queue(sp); >> + s2io_card_down(sp); >> + sp->lro = dev->features& NETIF_F_LRO; > [...] > > This means s2io_nic::lro and ring_info::lro can have the value > NETIF_F_LRO, where previously they would only have the value 0 or 1. I > don't know whether this could be a problem, but the safe thing to do is > to coerce the value by writing !!(dev->features& NETIF_F_LRO). > Yeah, agreed. It seems that Jon already fixed this when he updated the patch. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/25/10 12:33, Jon Mason wrote: > On Tue, Jun 22, 2010 at 01:50:07AM -0700, Amerigo Wang wrote: >> >> This patch adds dynamic LRO diable support for s2io net driver. >> >> (I don't have s2io card, so only did compiling test. Anyone who wants >> to test this is more than welcome.) > > Unfortunately the patch did not quite work, mostly due to NETIF_F_LRO > needing to be added at probe time to the device features. I was able > to modify the patch to get it working, and will respond seperately > with the working patch. > Thanks very much, Jon! I am very pleased to see this patch gets tested. :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c index 668327c..2e6e3b3 100644 --- a/drivers/net/s2io.c +++ b/drivers/net/s2io.c @@ -795,7 +795,6 @@ static int init_shared_mem(struct s2io_nic *nic) ring->rx_curr_put_info.ring_len = rx_cfg->num_rxd - 1; ring->nic = nic; ring->ring_no = i; - ring->lro = lro_enable; blk_cnt = rx_cfg->num_rxd / (rxd_count[nic->rxd_mode] + 1); /* Allocating all the Rx blocks */ @@ -6684,6 +6683,41 @@ static int s2io_ethtool_op_set_tso(struct net_device *dev, u32 data) return 0; } +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data) +{ + struct s2io_nic *sp = netdev_priv(dev); + int rc = 0; + int changed = 0; + + if (data & ~ETH_FLAG_LRO) + return -EOPNOTSUPP; + + if (data & ETH_FLAG_LRO) { + if (lro_enable) { + if (!(dev->features & NETIF_F_LRO)) { + dev->features |= NETIF_F_LRO; + changed = 1; + } + } else + rc = -EOPNOTSUPP; + } else if (dev->features & NETIF_F_LRO) { + dev->features &= ~NETIF_F_LRO; + changed = 1; + } + + if (changed && netif_running(dev)) { + s2io_stop_all_tx_queue(sp); + s2io_card_down(sp); + sp->lro = dev->features & NETIF_F_LRO; + rc = s2io_card_up(sp); + if (rc) + s2io_reset(sp); + else + s2io_start_all_tx_queue(sp); + } + + return rc; +} static const struct ethtool_ops netdev_ethtool_ops = { .get_settings = s2io_ethtool_gset, @@ -6701,6 +6735,8 @@ static const struct ethtool_ops netdev_ethtool_ops = { .get_rx_csum = s2io_ethtool_get_rx_csum, .set_rx_csum = s2io_ethtool_set_rx_csum, .set_tx_csum = s2io_ethtool_op_set_tx_csum, + .set_flags = s2io_ethtool_set_flags, + .get_flags = ethtool_op_get_flags, .set_sg = ethtool_op_set_sg, .get_tso = s2io_ethtool_op_get_tso, .set_tso = s2io_ethtool_op_set_tso, @@ -7229,6 +7265,7 @@ static int s2io_card_up(struct s2io_nic *sp) struct ring_info *ring = &mac_control->rings[i]; ring->mtu = dev->mtu; + ring->lro = sp->lro; ret = fill_rx_buffers(sp, ring, 1); if (ret) { DBG_PRINT(ERR_DBG, "%s: Out of memory in Open\n",