* [dpdk-dev] [PATCH v2] net/i40e: fix link_state update for i40e_ethdev_vf drv [not found] ` <20180214115630.136681-1-tmulkar@sandvine.com> @ 2018-02-14 12:00 ` Tushar Mulkar 2018-03-14 14:22 ` Rybalchenko, Kirill 2018-03-27 16:29 ` Zhang, Helin 0 siblings, 2 replies; 4+ messages in thread From: Tushar Mulkar @ 2018-02-14 12:00 UTC (permalink / raw) To: Zhang, Helin, dev; +Cc: Xing, Beilei, Zhang, Qi Z The check for bool was accounting unwanted bits in the calulation of truth value. In dpdk unsingned int is typedefed to bool but all it cares about is Least Significant Bit. But in calculation of condition expression the bits other than LSB was used which doesn't make sense. Some time these bits has values which results in to incorrect expression results. To fix this we just need to account LSB form the bool value . This can be easily done by anding the value with true. Signed-off-by: Tushar Mulkar <tmulkar@sandvine.com> --- drivers/net/i40e/i40e_ethdev_vf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c index b96d77a0c..d23dff044 100644 --- a/drivers/net/i40e/i40e_ethdev_vf.c +++ b/drivers/net/i40e/i40e_ethdev_vf.c @@ -2095,8 +2095,8 @@ i40evf_dev_link_update(struct rte_eth_dev *dev, } /* full duplex only */ new_link.link_duplex = ETH_LINK_FULL_DUPLEX; - new_link.link_status = vf->link_up ? ETH_LINK_UP : - ETH_LINK_DOWN; + new_link.link_status = (vf->link_up & true) ? + ETH_LINK_UP : ETH_LINK_DOWN; new_link.link_autoneg = dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED; -- 2.11.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: fix link_state update for i40e_ethdev_vf drv 2018-02-14 12:00 ` [dpdk-dev] [PATCH v2] net/i40e: fix link_state update for i40e_ethdev_vf drv Tushar Mulkar @ 2018-03-14 14:22 ` Rybalchenko, Kirill 2018-03-27 16:29 ` Zhang, Helin 1 sibling, 0 replies; 4+ messages in thread From: Rybalchenko, Kirill @ 2018-03-14 14:22 UTC (permalink / raw) To: Tushar Mulkar, Zhang, Helin, dev; +Cc: Xing, Beilei, Zhang, Qi Z Hi, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tushar Mulkar > Sent: Wednesday 14 February 2018 12:00 > To: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org > Cc: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com> > Subject: [dpdk-dev] [PATCH v2] net/i40e: fix link_state update for > i40e_ethdev_vf drv > > The check for bool was accounting unwanted bits in the calulation of truth > value. In dpdk unsingned int is typedefed to bool but all it cares about is Least > Significant Bit. But in calculation of condition expression the bits other than > LSB was used which doesn't make sense. Some time these bits has values > which results in to incorrect expression results. To fix this we just need to > account LSB form the bool value . This can be easily done by anding the value > with true. I didn't find any place where link_up is assigned to something other than Boolean. But maybe I'm wrong or I misunderstood what you're saying. And there is another place in this file with similar evaluation in i40evf_read_pfmsg() function. Format of commit message should be modified as well - it is formatted in one long line without line breaks. > > Signed-off-by: Tushar Mulkar <tmulkar@sandvine.com> > --- > drivers/net/i40e/i40e_ethdev_vf.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c > b/drivers/net/i40e/i40e_ethdev_vf.c > index b96d77a0c..d23dff044 100644 > --- a/drivers/net/i40e/i40e_ethdev_vf.c > +++ b/drivers/net/i40e/i40e_ethdev_vf.c > @@ -2095,8 +2095,8 @@ i40evf_dev_link_update(struct rte_eth_dev *dev, > } > /* full duplex only */ > new_link.link_duplex = ETH_LINK_FULL_DUPLEX; > - new_link.link_status = vf->link_up ? ETH_LINK_UP : > - ETH_LINK_DOWN; > + new_link.link_status = (vf->link_up & true) ? > + ETH_LINK_UP : ETH_LINK_DOWN; > new_link.link_autoneg = > dev->data->dev_conf.link_speeds & > ETH_LINK_SPEED_FIXED; > > -- > 2.11.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: fix link_state update for i40e_ethdev_vf drv 2018-02-14 12:00 ` [dpdk-dev] [PATCH v2] net/i40e: fix link_state update for i40e_ethdev_vf drv Tushar Mulkar 2018-03-14 14:22 ` Rybalchenko, Kirill @ 2018-03-27 16:29 ` Zhang, Helin 2018-03-30 3:07 ` Zhang, Helin 1 sibling, 1 reply; 4+ messages in thread From: Zhang, Helin @ 2018-03-27 16:29 UTC (permalink / raw) To: Tushar Mulkar, dev; +Cc: Xing, Beilei, Zhang, Qi Z > -----Original Message----- > From: Tushar Mulkar [mailto:tmulkar@sandvine.com] > Sent: Wednesday, February 14, 2018 8:00 PM > To: Zhang, Helin; dev@dpdk.org > Cc: Xing, Beilei; Zhang, Qi Z > Subject: [PATCH v2] net/i40e: fix link_state update for i40e_ethdev_vf drv > > The check for bool was accounting unwanted bits in the calulation of truth > value. In dpdk unsingned int is typedefed to bool but all it cares about is Least > Significant Bit. But in calculation of condition expression the bits other than > LSB was used which doesn't make sense. Some time these bits has values > which results in to incorrect expression results. To fix this we just need to > account LSB form the bool value . This can be easily done by anding the value > with true. > > Signed-off-by: Tushar Mulkar <tmulkar@sandvine.com> > --- > drivers/net/i40e/i40e_ethdev_vf.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c > b/drivers/net/i40e/i40e_ethdev_vf.c > index b96d77a0c..d23dff044 100644 > --- a/drivers/net/i40e/i40e_ethdev_vf.c > +++ b/drivers/net/i40e/i40e_ethdev_vf.c > @@ -2095,8 +2095,8 @@ i40evf_dev_link_update(struct rte_eth_dev *dev, > } > /* full duplex only */ > new_link.link_duplex = ETH_LINK_FULL_DUPLEX; > - new_link.link_status = vf->link_up ? ETH_LINK_UP : > - ETH_LINK_DOWN; > + new_link.link_status = (vf->link_up & true) ? > + ETH_LINK_UP : ETH_LINK_DOWN; It is a bit strange to do bit manipunation on a bool type of value. Do you see any wrong case with that variable? If yes, I'd suggest correct that one, rather than doing like this. Thanks, Helin > new_link.link_autoneg = > dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED; > > -- > 2.11.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: fix link_state update for i40e_ethdev_vf drv 2018-03-27 16:29 ` Zhang, Helin @ 2018-03-30 3:07 ` Zhang, Helin 0 siblings, 0 replies; 4+ messages in thread From: Zhang, Helin @ 2018-03-30 3:07 UTC (permalink / raw) To: Zhang, Helin, Tushar Mulkar, dev; +Cc: Xing, Beilei, Zhang, Qi Z > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Helin > Sent: Wednesday, March 28, 2018 12:30 AM > To: Tushar Mulkar; dev@dpdk.org > Cc: Xing, Beilei; Zhang, Qi Z > Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix link_state update for > i40e_ethdev_vf drv > > > > > -----Original Message----- > > From: Tushar Mulkar [mailto:tmulkar@sandvine.com] > > Sent: Wednesday, February 14, 2018 8:00 PM > > To: Zhang, Helin; dev@dpdk.org > > Cc: Xing, Beilei; Zhang, Qi Z > > Subject: [PATCH v2] net/i40e: fix link_state update for i40e_ethdev_vf > > drv > > > > The check for bool was accounting unwanted bits in the calulation of > > truth value. In dpdk unsingned int is typedefed to bool but all it > > cares about is Least Significant Bit. But in calculation of condition > > expression the bits other than LSB was used which doesn't make sense. > > Some time these bits has values which results in to incorrect > > expression results. To fix this we just need to account LSB form the > > bool value . This can be easily done by anding the value with true. > > > > Signed-off-by: Tushar Mulkar <tmulkar@sandvine.com> > > --- > > drivers/net/i40e/i40e_ethdev_vf.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c > > b/drivers/net/i40e/i40e_ethdev_vf.c > > index b96d77a0c..d23dff044 100644 > > --- a/drivers/net/i40e/i40e_ethdev_vf.c > > +++ b/drivers/net/i40e/i40e_ethdev_vf.c > > @@ -2095,8 +2095,8 @@ i40evf_dev_link_update(struct rte_eth_dev *dev, > > } > > /* full duplex only */ > > new_link.link_duplex = ETH_LINK_FULL_DUPLEX; > > - new_link.link_status = vf->link_up ? ETH_LINK_UP : > > - ETH_LINK_DOWN; > > + new_link.link_status = (vf->link_up & true) ? > > + ETH_LINK_UP : ETH_LINK_DOWN; > It is a bit strange to do bit manipunation on a bool type of value. > Do you see any wrong case with that variable? If yes, I'd suggest correct that > one, rather than doing like this. I'd set it to change requested for now, if there no clarification for the comments, and no ACK. Feel free to set it back if there is anything new. Thanks! /Helin > > Thanks, > Helin > > > new_link.link_autoneg = > > dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED; > > > > -- > > 2.11.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-30 3:07 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <B100BEBE3931D74C91B0DFE67C1055F9035BC999> [not found] ` <20180214115630.136681-1-tmulkar@sandvine.com> 2018-02-14 12:00 ` [dpdk-dev] [PATCH v2] net/i40e: fix link_state update for i40e_ethdev_vf drv Tushar Mulkar 2018-03-14 14:22 ` Rybalchenko, Kirill 2018-03-27 16:29 ` Zhang, Helin 2018-03-30 3:07 ` Zhang, Helin
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).