* [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument @ 2017-09-18 18:47 Ferruh Yigit 2017-09-18 18:47 ` [dpdk-dev] [PATCH 2/2] net/tap: fix unregistering callback with invalid fd Ferruh Yigit 2017-09-19 12:45 ` [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument Pascal Mazon 0 siblings, 2 replies; 7+ messages in thread From: Ferruh Yigit @ 2017-09-18 18:47 UTC (permalink / raw) To: Pascal Mazon; +Cc: dev, Keith Wiles, Ferruh Yigit, Vipin Varghese, stable From: Vipin Varghese <vipin.varghese@intel.com> tap speed argument is not working without generating any error. This patch sets the configured speed during device start. Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD") Cc: stable@dpdk.org Signed-off-by: Vipin Varghese <vipin.varghese@intel.com> --- drivers/net/tap/rte_eth_tap.c | 27 +++++++++++++++++++++++++++ drivers/net/tap/rte_eth_tap.h | 2 ++ 2 files changed, 29 insertions(+) diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c index 01cba0fa1..00dad167f 100644 --- a/drivers/net/tap/rte_eth_tap.c +++ b/drivers/net/tap/rte_eth_tap.c @@ -545,6 +545,7 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request, case SIOCGIFHWADDR: case SIOCSIFHWADDR: case SIOCSIFMTU: + case SIOCETHTOOL: break; default: RTE_ASSERT(!"unsupported request type: must not happen"); @@ -585,6 +586,32 @@ static int tap_dev_start(struct rte_eth_dev *dev) { int err; + struct ifreq ifr; + struct ethtool_cmd edata = {0}; + struct pmd_internals *pmd = dev->data->dev_private; + + /*set & get speed to device*/ + edata.speed = pmd_link.link_speed; + edata.cmd = ETHTOOL_SSET; + ifr.ifr_data = (caddr_t)&edata; + if (tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0, LOCAL_ONLY) < 0) { + RTE_LOG(WARNING, PMD, + "Could not set param speed %d for ifindex for %s.\n", + pmd_link.link_speed, + pmd->name); + + /* fetch current speed of created device */ + edata.cmd = ETHTOOL_GSET; + ifr.ifr_data = (caddr_t)&edata; + if (tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0, LOCAL_ONLY) < 0) + return 0; + + pmd_link.link_speed = edata.speed; + RTE_LOG(INFO, PMD, "get speed %d for ifindex for %s.\n", + pmd_link.link_speed, + pmd->name); + } + dev->data->dev_link = pmd_link; err = tap_intr_handle_set(dev, 1); if (err) diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h index 928a0454e..3e91db4ae 100644 --- a/drivers/net/tap/rte_eth_tap.h +++ b/drivers/net/tap/rte_eth_tap.h @@ -40,6 +40,8 @@ #include <net/if.h> #include <linux/if_tun.h> +#include <linux/ethtool.h> +#include <linux/sockios.h> #include <rte_ethdev.h> #include <rte_ether.h> -- 2.13.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [dpdk-dev] [PATCH 2/2] net/tap: fix unregistering callback with invalid fd 2017-09-18 18:47 [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument Ferruh Yigit @ 2017-09-18 18:47 ` Ferruh Yigit 2017-09-19 12:46 ` Pascal Mazon 2017-09-19 12:45 ` [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument Pascal Mazon 1 sibling, 1 reply; 7+ messages in thread From: Ferruh Yigit @ 2017-09-18 18:47 UTC (permalink / raw) To: Pascal Mazon; +Cc: dev, Keith Wiles, Ferruh Yigit, Vipin Varghese, stable From: Vipin Varghese <vipin.varghese@intel.com> tap_intr_handle_set() called by tap_dev_start(), and if LSC is disabled (dev_conf.intr_conf.lsc == 0), it tries to unregister interrupt callback without checking the interrupt file descriptor. Fixes: c0bddd3a057f ("net/tap: add link status notification") Cc: stable@dpdk.org Signed-off-by: Vipin Varghese <vipin.varghese@intel.com> --- drivers/net/tap/rte_eth_tap.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c index 00dad167f..fcfd4215e 100644 --- a/drivers/net/tap/rte_eth_tap.c +++ b/drivers/net/tap/rte_eth_tap.c @@ -1098,10 +1098,11 @@ tap_intr_handle_set(struct rte_eth_dev *dev, int set) /* In any case, disable interrupt if the conf is no longer there. */ if (!dev->data->dev_conf.intr_conf.lsc) { - if (pmd->intr_handle.fd != -1) + if (pmd->intr_handle.fd != -1) { nl_final(pmd->intr_handle.fd); - rte_intr_callback_unregister( - &pmd->intr_handle, tap_dev_intr_handler, dev); + rte_intr_callback_unregister(&pmd->intr_handle, + tap_dev_intr_handler, dev); + } return 0; } if (set) { -- 2.13.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] net/tap: fix unregistering callback with invalid fd 2017-09-18 18:47 ` [dpdk-dev] [PATCH 2/2] net/tap: fix unregistering callback with invalid fd Ferruh Yigit @ 2017-09-19 12:46 ` Pascal Mazon 2017-09-20 16:05 ` Ferruh Yigit 0 siblings, 1 reply; 7+ messages in thread From: Pascal Mazon @ 2017-09-19 12:46 UTC (permalink / raw) To: Ferruh Yigit; +Cc: dev, Keith Wiles, Vipin Varghese, stable Looks good. Best regards, Pascal Acked-by: Pascal Mazon <pascal.mazon@6wind.com> On 18/09/2017 20:47, Ferruh Yigit wrote: > From: Vipin Varghese <vipin.varghese@intel.com> > > tap_intr_handle_set() called by tap_dev_start(), and if LSC is disabled > (dev_conf.intr_conf.lsc == 0), it tries to unregister interrupt > callback without checking the interrupt file descriptor. > > Fixes: c0bddd3a057f ("net/tap: add link status notification") > Cc: stable@dpdk.org > > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com> > --- > drivers/net/tap/rte_eth_tap.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c > index 00dad167f..fcfd4215e 100644 > --- a/drivers/net/tap/rte_eth_tap.c > +++ b/drivers/net/tap/rte_eth_tap.c > @@ -1098,10 +1098,11 @@ tap_intr_handle_set(struct rte_eth_dev *dev, int set) > > /* In any case, disable interrupt if the conf is no longer there. */ > if (!dev->data->dev_conf.intr_conf.lsc) { > - if (pmd->intr_handle.fd != -1) > + if (pmd->intr_handle.fd != -1) { > nl_final(pmd->intr_handle.fd); > - rte_intr_callback_unregister( > - &pmd->intr_handle, tap_dev_intr_handler, dev); > + rte_intr_callback_unregister(&pmd->intr_handle, > + tap_dev_intr_handler, dev); > + } > return 0; > } > if (set) { ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] net/tap: fix unregistering callback with invalid fd 2017-09-19 12:46 ` Pascal Mazon @ 2017-09-20 16:05 ` Ferruh Yigit 0 siblings, 0 replies; 7+ messages in thread From: Ferruh Yigit @ 2017-09-20 16:05 UTC (permalink / raw) To: Pascal Mazon; +Cc: dev, Keith Wiles, Vipin Varghese, stable On 9/19/2017 1:46 PM, Pascal Mazon wrote: <...> > On 18/09/2017 20:47, Ferruh Yigit wrote: >> From: Vipin Varghese <vipin.varghese@intel.com> >> >> tap_intr_handle_set() called by tap_dev_start(), and if LSC is disabled >> (dev_conf.intr_conf.lsc == 0), it tries to unregister interrupt >> callback without checking the interrupt file descriptor. >> >> Fixes: c0bddd3a057f ("net/tap: add link status notification") >> Cc: stable@dpdk.org >> >> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com> > Acked-by: Pascal Mazon <pascal.mazon@6wind.com> Applied to dpdk-next-net/master, thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument 2017-09-18 18:47 [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument Ferruh Yigit 2017-09-18 18:47 ` [dpdk-dev] [PATCH 2/2] net/tap: fix unregistering callback with invalid fd Ferruh Yigit @ 2017-09-19 12:45 ` Pascal Mazon 2017-10-25 1:24 ` Ferruh Yigit 1 sibling, 1 reply; 7+ messages in thread From: Pascal Mazon @ 2017-09-19 12:45 UTC (permalink / raw) To: Ferruh Yigit; +Cc: dev, Keith Wiles, Vipin Varghese, stable Hi, The patch looks mainly ok to me. I'll put some comments inline. On 18/09/2017 20:47, Ferruh Yigit wrote: > From: Vipin Varghese <vipin.varghese@intel.com> > > tap speed argument is not working without generating any error. Can you describe the error, paste the log? > > This patch sets the configured speed during device start. > > Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD") > Cc: stable@dpdk.org > > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com> > --- > drivers/net/tap/rte_eth_tap.c | 27 +++++++++++++++++++++++++++ > drivers/net/tap/rte_eth_tap.h | 2 ++ > 2 files changed, 29 insertions(+) > > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c > index 01cba0fa1..00dad167f 100644 > --- a/drivers/net/tap/rte_eth_tap.c > +++ b/drivers/net/tap/rte_eth_tap.c > @@ -545,6 +545,7 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request, > case SIOCGIFHWADDR: > case SIOCSIFHWADDR: > case SIOCSIFMTU: > + case SIOCETHTOOL: > break; > default: > RTE_ASSERT(!"unsupported request type: must not happen"); > @@ -585,6 +586,32 @@ static int > tap_dev_start(struct rte_eth_dev *dev) > { > int err; > + struct ifreq ifr; > + struct ethtool_cmd edata = {0}; > + struct pmd_internals *pmd = dev->data->dev_private; > + > + /*set & get speed to device*/ > + edata.speed = pmd_link.link_speed; > + edata.cmd = ETHTOOL_SSET; > + ifr.ifr_data = (caddr_t)&edata; > + if (tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0, LOCAL_ONLY) < 0) { I think it would be best to also try setting the speed on the remote (tap_ioctl(REMOTE_ONLY)), but continue execution even if that fails. > + RTE_LOG(WARNING, PMD, > + "Could not set param speed %d for ifindex for %s.\n", > + pmd_link.link_speed, > + pmd->name); > + > + /* fetch current speed of created device */ > + edata.cmd = ETHTOOL_GSET; > + ifr.ifr_data = (caddr_t)&edata; > + if (tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0, LOCAL_ONLY) < 0) > + return 0; > + > + pmd_link.link_speed = edata.speed; > + RTE_LOG(INFO, PMD, "get speed %d for ifindex for %s.\n", I would say "got". > + pmd_link.link_speed, > + pmd->name); > + } > + dev->data->dev_link = pmd_link; > > err = tap_intr_handle_set(dev, 1); > if (err) > diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h > index 928a0454e..3e91db4ae 100644 > --- a/drivers/net/tap/rte_eth_tap.h > +++ b/drivers/net/tap/rte_eth_tap.h > @@ -40,6 +40,8 @@ > #include <net/if.h> > > #include <linux/if_tun.h> > +#include <linux/ethtool.h> > +#include <linux/sockios.h> > > #include <rte_ethdev.h> > #include <rte_ether.h> These includes are only useful inside rte_eth_tap.c, I would put them there only. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument 2017-09-19 12:45 ` [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument Pascal Mazon @ 2017-10-25 1:24 ` Ferruh Yigit 2017-10-25 6:27 ` Pascal Mazon 0 siblings, 1 reply; 7+ messages in thread From: Ferruh Yigit @ 2017-10-25 1:24 UTC (permalink / raw) To: Pascal Mazon; +Cc: dev, Keith Wiles, Vipin Varghese, stable On 9/19/2017 5:45 AM, Pascal Mazon wrote: > Hi, > > The patch looks mainly ok to me. > > I'll put some comments inline. > > On 18/09/2017 20:47, Ferruh Yigit wrote: >> From: Vipin Varghese <vipin.varghese@intel.com> >> >> tap speed argument is not working without generating any error. > Can you describe the error, paste the log? >> >> This patch sets the configured speed during device start. >> >> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD") >> Cc: stable@dpdk.org >> >> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com> >> --- >> drivers/net/tap/rte_eth_tap.c | 27 +++++++++++++++++++++++++++ >> drivers/net/tap/rte_eth_tap.h | 2 ++ >> 2 files changed, 29 insertions(+) >> >> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c >> index 01cba0fa1..00dad167f 100644 >> --- a/drivers/net/tap/rte_eth_tap.c >> +++ b/drivers/net/tap/rte_eth_tap.c >> @@ -545,6 +545,7 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request, >> case SIOCGIFHWADDR: >> case SIOCSIFHWADDR: >> case SIOCSIFMTU: >> + case SIOCETHTOOL: >> break; >> default: >> RTE_ASSERT(!"unsupported request type: must not happen"); >> @@ -585,6 +586,32 @@ static int >> tap_dev_start(struct rte_eth_dev *dev) >> { >> int err; >> + struct ifreq ifr; >> + struct ethtool_cmd edata = {0}; >> + struct pmd_internals *pmd = dev->data->dev_private; >> + >> + /*set & get speed to device*/ >> + edata.speed = pmd_link.link_speed; >> + edata.cmd = ETHTOOL_SSET; >> + ifr.ifr_data = (caddr_t)&edata; >> + if (tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0, LOCAL_ONLY) < 0) { > I think it would be best to also try setting the speed on the remote > (tap_ioctl(REMOTE_ONLY)), but continue execution even if that fails. Hi Pascal, It looks like this ioctl fails to set tap speed. And as far as I can see speed is hardcoded in kernel [1], do you know if it is possible to set speed of tap interface? Thanks, ferruh [1] http://elixir.free-electrons.com/linux/v4.13.9/source/drivers/net/tun.c#L2460 >> + RTE_LOG(WARNING, PMD, >> + "Could not set param speed %d for ifindex for %s.\n", >> + pmd_link.link_speed, >> + pmd->name); >> + >> + /* fetch current speed of created device */ >> + edata.cmd = ETHTOOL_GSET; >> + ifr.ifr_data = (caddr_t)&edata; >> + if (tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0, LOCAL_ONLY) < 0) >> + return 0; >> + >> + pmd_link.link_speed = edata.speed; >> + RTE_LOG(INFO, PMD, "get speed %d for ifindex for %s.\n", > I would say "got". > >> + pmd_link.link_speed, >> + pmd->name); >> + } >> + dev->data->dev_link = pmd_link; >> >> err = tap_intr_handle_set(dev, 1); >> if (err) >> diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h >> index 928a0454e..3e91db4ae 100644 >> --- a/drivers/net/tap/rte_eth_tap.h >> +++ b/drivers/net/tap/rte_eth_tap.h >> @@ -40,6 +40,8 @@ >> #include <net/if.h> >> >> #include <linux/if_tun.h> >> +#include <linux/ethtool.h> >> +#include <linux/sockios.h> >> >> #include <rte_ethdev.h> >> #include <rte_ether.h> > These includes are only useful inside rte_eth_tap.c, I would put them > there only. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument 2017-10-25 1:24 ` Ferruh Yigit @ 2017-10-25 6:27 ` Pascal Mazon 0 siblings, 0 replies; 7+ messages in thread From: Pascal Mazon @ 2017-10-25 6:27 UTC (permalink / raw) To: Ferruh Yigit; +Cc: dev, Keith Wiles, Vipin Varghese, stable On 25/10/2017 03:24, Ferruh Yigit wrote: > On 9/19/2017 5:45 AM, Pascal Mazon wrote: >> Hi, >> >> The patch looks mainly ok to me. >> >> I'll put some comments inline. >> >> On 18/09/2017 20:47, Ferruh Yigit wrote: >>> From: Vipin Varghese <vipin.varghese@intel.com> >>> >>> tap speed argument is not working without generating any error. >> Can you describe the error, paste the log? >>> This patch sets the configured speed during device start. >>> >>> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com> >>> --- >>> drivers/net/tap/rte_eth_tap.c | 27 +++++++++++++++++++++++++++ >>> drivers/net/tap/rte_eth_tap.h | 2 ++ >>> 2 files changed, 29 insertions(+) >>> >>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c >>> index 01cba0fa1..00dad167f 100644 >>> --- a/drivers/net/tap/rte_eth_tap.c >>> +++ b/drivers/net/tap/rte_eth_tap.c >>> @@ -545,6 +545,7 @@ tap_ioctl(struct pmd_internals *pmd, unsigned long request, >>> case SIOCGIFHWADDR: >>> case SIOCSIFHWADDR: >>> case SIOCSIFMTU: >>> + case SIOCETHTOOL: >>> break; >>> default: >>> RTE_ASSERT(!"unsupported request type: must not happen"); >>> @@ -585,6 +586,32 @@ static int >>> tap_dev_start(struct rte_eth_dev *dev) >>> { >>> int err; >>> + struct ifreq ifr; >>> + struct ethtool_cmd edata = {0}; >>> + struct pmd_internals *pmd = dev->data->dev_private; >>> + >>> + /*set & get speed to device*/ >>> + edata.speed = pmd_link.link_speed; >>> + edata.cmd = ETHTOOL_SSET; >>> + ifr.ifr_data = (caddr_t)&edata; >>> + if (tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0, LOCAL_ONLY) < 0) { >> I think it would be best to also try setting the speed on the remote >> (tap_ioctl(REMOTE_ONLY)), but continue execution even if that fails. > Hi Pascal, > > It looks like this ioctl fails to set tap speed. And as far as I can see speed > is hardcoded in kernel [1], do you know if it is possible to set speed of tap > interface? > > Thanks, > ferruh > > [1] > http://elixir.free-electrons.com/linux/v4.13.9/source/drivers/net/tun.c#L2460 Hi Ferruh, I concur with you, it definitely looks like speed is hardcoded in the kernel. And there is no set_settings() callback defined either, so it won't be possible to set anything using ethtool. I haven't seen the error and I don't think Vipin Varghese sent a log for that. I'm still not sure what the patch was fixing; it would be good to have that feedback to improve the patch. Best regards, Pascal > >>> + RTE_LOG(WARNING, PMD, >>> + "Could not set param speed %d for ifindex for %s.\n", >>> + pmd_link.link_speed, >>> + pmd->name); >>> + >>> + /* fetch current speed of created device */ >>> + edata.cmd = ETHTOOL_GSET; >>> + ifr.ifr_data = (caddr_t)&edata; >>> + if (tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0, LOCAL_ONLY) < 0) >>> + return 0; >>> + >>> + pmd_link.link_speed = edata.speed; >>> + RTE_LOG(INFO, PMD, "get speed %d for ifindex for %s.\n", >> I would say "got". >> >>> + pmd_link.link_speed, >>> + pmd->name); >>> + } >>> + dev->data->dev_link = pmd_link; >>> >>> err = tap_intr_handle_set(dev, 1); >>> if (err) >>> diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h >>> index 928a0454e..3e91db4ae 100644 >>> --- a/drivers/net/tap/rte_eth_tap.h >>> +++ b/drivers/net/tap/rte_eth_tap.h >>> @@ -40,6 +40,8 @@ >>> #include <net/if.h> >>> >>> #include <linux/if_tun.h> >>> +#include <linux/ethtool.h> >>> +#include <linux/sockios.h> >>> >>> #include <rte_ethdev.h> >>> #include <rte_ether.h> >> These includes are only useful inside rte_eth_tap.c, I would put them >> there only. >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-10-25 6:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-18 18:47 [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument Ferruh Yigit 2017-09-18 18:47 ` [dpdk-dev] [PATCH 2/2] net/tap: fix unregistering callback with invalid fd Ferruh Yigit 2017-09-19 12:46 ` Pascal Mazon 2017-09-20 16:05 ` Ferruh Yigit 2017-09-19 12:45 ` [dpdk-dev] [PATCH 1/2] net/tap: fix setting speed by argument Pascal Mazon 2017-10-25 1:24 ` Ferruh Yigit 2017-10-25 6:27 ` Pascal Mazon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).