| Message ID | 20170605091429.16232-1-yuval.shaia@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Jun 5, 2017 at 12:14 PM, Yuval Shaia <yuval.shaia@oracle.com> wrote: > The function get_link_ksettings might return bad status indicating a > failure to retrieve interface atttibutes. > Check return value to cover this case. > > While there, change the zero-initialization to "compiler-helper" instead > of an expensive call to memcpy. > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > --- > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > index 7ba9e69..10c7189 100644 > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > @@ -183,17 +183,19 @@ int bnxt_re_modify_device(struct ib_device *ibdev, > > static void __to_ib_speed_width(struct net_device *netdev, u8 *speed, u8 *width) > { > - struct ethtool_link_ksettings lksettings; > - u32 espeed; > + u32 espeed = SPEED_UNKNOWN; > > if (netdev->ethtool_ops && netdev->ethtool_ops->get_link_ksettings) { > - memset(&lksettings, 0, sizeof(lksettings)); > + struct ethtool_link_ksettings lksettings = {0}; > + int rc; > + > rtnl_lock(); > - netdev->ethtool_ops->get_link_ksettings(netdev, &lksettings); > + rc = netdev->ethtool_ops->get_link_ksettings(netdev, > + &lksettings); > rtnl_unlock(); > - espeed = lksettings.base.speed; > - } else { > - espeed = SPEED_UNKNOWN; > + > + if (!rc) > + espeed = lksettings.base.speed; > } > switch (espeed) { > case SPEED_1000: > -- > 2.9.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html It looks like that bnxt driver also has similar code (function __to_ib_speed_width() in drivers/infiniband/hw/bnxt_re/ib_verbs.c) Maybe you can move this function to IB/core and use it in both places (or more) -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 05, 2017 at 01:42:37PM +0300, Moni Shoua wrote: > On Mon, Jun 5, 2017 at 12:14 PM, Yuval Shaia <yuval.shaia@oracle.com> wrote: > > The function get_link_ksettings might return bad status indicating a > > failure to retrieve interface atttibutes. > > Check return value to cover this case. > > > > While there, change the zero-initialization to "compiler-helper" instead > > of an expensive call to memcpy. > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > --- > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > index 7ba9e69..10c7189 100644 > > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > @@ -183,17 +183,19 @@ int bnxt_re_modify_device(struct ib_device *ibdev, > > > > static void __to_ib_speed_width(struct net_device *netdev, u8 *speed, u8 *width) > > { > > - struct ethtool_link_ksettings lksettings; > > - u32 espeed; > > + u32 espeed = SPEED_UNKNOWN; > > > > if (netdev->ethtool_ops && netdev->ethtool_ops->get_link_ksettings) { > > - memset(&lksettings, 0, sizeof(lksettings)); > > + struct ethtool_link_ksettings lksettings = {0}; > > + int rc; > > + > > rtnl_lock(); > > - netdev->ethtool_ops->get_link_ksettings(netdev, &lksettings); > > + rc = netdev->ethtool_ops->get_link_ksettings(netdev, > > + &lksettings); > > rtnl_unlock(); > > - espeed = lksettings.base.speed; > > - } else { > > - espeed = SPEED_UNKNOWN; > > + > > + if (!rc) > > + espeed = lksettings.base.speed; > > } > > switch (espeed) { > > case SPEED_1000: > > -- > > 2.9.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > It looks like that bnxt driver also has similar code (function > __to_ib_speed_width() in drivers/infiniband/hw/bnxt_re/ib_verbs.c) Sorry, i'm not following, this one fixes bnxt_re > Maybe you can move this function to IB/core and use it in both places (or more) > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 05, 2017 at 12:14:29PM +0300, Yuval Shaia wrote: > The function get_link_ksettings might return bad status indicating a > failure to retrieve interface atttibutes. > Check return value to cover this case. > > While there, change the zero-initialization to "compiler-helper" instead > of an expensive call to memcpy. > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > --- > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > index 7ba9e69..10c7189 100644 > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > @@ -183,17 +183,19 @@ int bnxt_re_modify_device(struct ib_device *ibdev, > > static void __to_ib_speed_width(struct net_device *netdev, u8 *speed, u8 *width) > { > - struct ethtool_link_ksettings lksettings; > - u32 espeed; > + u32 espeed = SPEED_UNKNOWN; > > if (netdev->ethtool_ops && netdev->ethtool_ops->get_link_ksettings) { > - memset(&lksettings, 0, sizeof(lksettings)); > + struct ethtool_link_ksettings lksettings = {0}; > + int rc; > + > rtnl_lock(); > - netdev->ethtool_ops->get_link_ksettings(netdev, &lksettings); > + rc = netdev->ethtool_ops->get_link_ksettings(netdev, > + &lksettings); > rtnl_unlock(); > - espeed = lksettings.base.speed; > - } else { > - espeed = SPEED_UNKNOWN; > + > + if (!rc) Are you sure that it is "if (!rc)" and not "if (rc)"? in commit message you wrote that "The function get_link_ksettings might return bad status indicating". Thanks > + espeed = lksettings.base.speed; > } > switch (espeed) { > case SPEED_1000: > -- > 2.9.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 05, 2017 at 03:52:27PM +0300, Leon Romanovsky wrote: > On Mon, Jun 05, 2017 at 12:14:29PM +0300, Yuval Shaia wrote: > > The function get_link_ksettings might return bad status indicating a > > failure to retrieve interface atttibutes. > > Check return value to cover this case. > > > > While there, change the zero-initialization to "compiler-helper" instead > > of an expensive call to memcpy. > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > --- > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > index 7ba9e69..10c7189 100644 > > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > @@ -183,17 +183,19 @@ int bnxt_re_modify_device(struct ib_device *ibdev, > > > > static void __to_ib_speed_width(struct net_device *netdev, u8 *speed, u8 *width) > > { > > - struct ethtool_link_ksettings lksettings; > > - u32 espeed; > > + u32 espeed = SPEED_UNKNOWN; > > > > if (netdev->ethtool_ops && netdev->ethtool_ops->get_link_ksettings) { > > - memset(&lksettings, 0, sizeof(lksettings)); > > + struct ethtool_link_ksettings lksettings = {0}; > > + int rc; > > + > > rtnl_lock(); > > - netdev->ethtool_ops->get_link_ksettings(netdev, &lksettings); > > + rc = netdev->ethtool_ops->get_link_ksettings(netdev, > > + &lksettings); > > rtnl_unlock(); > > - espeed = lksettings.base.speed; > > - } else { > > - espeed = SPEED_UNKNOWN; > > + > > + if (!rc) > > Are you sure that it is "if (!rc)" and not "if (rc)"? > in commit message you wrote that "The function get_link_ksettings might > return bad status indicating". Sorry, you are right. Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
Hi Yuval,
[auto build test WARNING on rdma/master]
[also build test WARNING on v4.12-rc4 next-20170605]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yuval-Shaia/IB-bnxt_re-Check-return-value-from-get_link_ksettings/20170605-182417
base: https://git.kernel.org/pub/scm/linux/kernel/git/dledford/rdma.git master
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All warnings (new ones prefixed by >>):
drivers/infiniband/hw/bnxt_re/ib_verbs.c: In function '__to_ib_speed_width':
>> drivers/infiniband/hw/bnxt_re/ib_verbs.c:189:10: warning: missing braces around initializer [-Wmissing-braces]
struct ethtool_link_ksettings lksettings = {0};
^
drivers/infiniband/hw/bnxt_re/ib_verbs.c:189:10: warning: (near initialization for 'lksettings.base') [-Wmissing-braces]
vim +189 drivers/infiniband/hw/bnxt_re/ib_verbs.c
173 /* GUID should be made as READ-ONLY */
174 break;
175 case IB_DEVICE_MODIFY_NODE_DESC:
176 /* Node Desc should be made as READ-ONLY */
177 break;
178 default:
179 break;
180 }
181 return 0;
182 }
183
184 static void __to_ib_speed_width(struct net_device *netdev, u8 *speed, u8 *width)
185 {
186 u32 espeed = SPEED_UNKNOWN;
187
188 if (netdev->ethtool_ops && netdev->ethtool_ops->get_link_ksettings) {
> 189 struct ethtool_link_ksettings lksettings = {0};
190 int rc;
191
192 rtnl_lock();
193 rc = netdev->ethtool_ops->get_link_ksettings(netdev,
194 &lksettings);
195 rtnl_unlock();
196
197 if (!rc)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon, Jun 05, 2017 at 03:54:15PM +0300, Leon Romanovsky wrote: > On Mon, Jun 05, 2017 at 03:52:27PM +0300, Leon Romanovsky wrote: > > On Mon, Jun 05, 2017 at 12:14:29PM +0300, Yuval Shaia wrote: > > > The function get_link_ksettings might return bad status indicating a > > > failure to retrieve interface atttibutes. > > > Check return value to cover this case. > > > > > > While there, change the zero-initialization to "compiler-helper" instead > > > of an expensive call to memcpy. > > > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > > --- > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > > index 7ba9e69..10c7189 100644 > > > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > > @@ -183,17 +183,19 @@ int bnxt_re_modify_device(struct ib_device *ibdev, > > > > > > static void __to_ib_speed_width(struct net_device *netdev, u8 *speed, u8 *width) > > > { > > > - struct ethtool_link_ksettings lksettings; > > > - u32 espeed; > > > + u32 espeed = SPEED_UNKNOWN; > > > > > > if (netdev->ethtool_ops && netdev->ethtool_ops->get_link_ksettings) { > > > - memset(&lksettings, 0, sizeof(lksettings)); > > > + struct ethtool_link_ksettings lksettings = {0}; > > > + int rc; > > > + > > > rtnl_lock(); > > > - netdev->ethtool_ops->get_link_ksettings(netdev, &lksettings); > > > + rc = netdev->ethtool_ops->get_link_ksettings(netdev, > > > + &lksettings); > > > rtnl_unlock(); > > > - espeed = lksettings.base.speed; > > > - } else { > > > - espeed = SPEED_UNKNOWN; > > > + > > > + if (!rc) > > > > Are you sure that it is "if (!rc)" and not "if (rc)"? > > in commit message you wrote that "The function get_link_ksettings might > > return bad status indicating". > > Sorry, you are right. Yeah, it is kind of confusing in first glance :) It is the way how diff works. If makes it looks like that "if (!rc) is part of the "else". > > Thanks, > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 05, 2017 at 03:42:14PM +0300, Yuval Shaia wrote: > On Mon, Jun 05, 2017 at 01:42:37PM +0300, Moni Shoua wrote: > > On Mon, Jun 5, 2017 at 12:14 PM, Yuval Shaia <yuval.shaia@oracle.com> wrote: > > > The function get_link_ksettings might return bad status indicating a > > > failure to retrieve interface atttibutes. > > > Check return value to cover this case. > > > > > > While there, change the zero-initialization to "compiler-helper" instead > > > of an expensive call to memcpy. > > > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > > --- > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > > index 7ba9e69..10c7189 100644 > > > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > > @@ -183,17 +183,19 @@ int bnxt_re_modify_device(struct ib_device *ibdev, > > > > > > static void __to_ib_speed_width(struct net_device *netdev, u8 *speed, u8 *width) > > > { > > > - struct ethtool_link_ksettings lksettings; > > > - u32 espeed; > > > + u32 espeed = SPEED_UNKNOWN; > > > > > > if (netdev->ethtool_ops && netdev->ethtool_ops->get_link_ksettings) { > > > - memset(&lksettings, 0, sizeof(lksettings)); > > > + struct ethtool_link_ksettings lksettings = {0}; > > > + int rc; > > > + > > > rtnl_lock(); > > > - netdev->ethtool_ops->get_link_ksettings(netdev, &lksettings); > > > + rc = netdev->ethtool_ops->get_link_ksettings(netdev, > > > + &lksettings); > > > rtnl_unlock(); > > > - espeed = lksettings.base.speed; > > > - } else { > > > - espeed = SPEED_UNKNOWN; > > > + > > > + if (!rc) > > > + espeed = lksettings.base.speed; > > > } > > > switch (espeed) { > > > case SPEED_1000: > > > -- > > > 2.9.4 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > It looks like that bnxt driver also has similar code (function > > __to_ib_speed_width() in drivers/infiniband/hw/bnxt_re/ib_verbs.c) > > Sorry, i'm not following, this one fixes bnxt_re IMHO, he wanted to point your attention that RXE has very similar piece of code. > > > Maybe you can move this function to IB/core and use it in both places (or more) > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 05, 2017 at 04:38:31PM +0300, Leon Romanovsky wrote: > On Mon, Jun 05, 2017 at 03:42:14PM +0300, Yuval Shaia wrote: > > On Mon, Jun 05, 2017 at 01:42:37PM +0300, Moni Shoua wrote: > > > On Mon, Jun 5, 2017 at 12:14 PM, Yuval Shaia <yuval.shaia@oracle.com> wrote: > > > > The function get_link_ksettings might return bad status indicating a > > > > failure to retrieve interface atttibutes. > > > > Check return value to cover this case. > > > > > > > > While there, change the zero-initialization to "compiler-helper" instead > > > > of an expensive call to memcpy. > > > > > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > > > --- > > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- > > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > > > index 7ba9e69..10c7189 100644 > > > > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > > > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > > > @@ -183,17 +183,19 @@ int bnxt_re_modify_device(struct ib_device *ibdev, > > > > > > > > static void __to_ib_speed_width(struct net_device *netdev, u8 *speed, u8 *width) > > > > { > > > > - struct ethtool_link_ksettings lksettings; > > > > - u32 espeed; > > > > + u32 espeed = SPEED_UNKNOWN; > > > > > > > > if (netdev->ethtool_ops && netdev->ethtool_ops->get_link_ksettings) { > > > > - memset(&lksettings, 0, sizeof(lksettings)); > > > > + struct ethtool_link_ksettings lksettings = {0}; > > > > + int rc; > > > > + > > > > rtnl_lock(); > > > > - netdev->ethtool_ops->get_link_ksettings(netdev, &lksettings); > > > > + rc = netdev->ethtool_ops->get_link_ksettings(netdev, > > > > + &lksettings); > > > > rtnl_unlock(); > > > > - espeed = lksettings.base.speed; > > > > - } else { > > > > - espeed = SPEED_UNKNOWN; > > > > + > > > > + if (!rc) > > > > + espeed = lksettings.base.speed; > > > > } > > > > switch (espeed) { > > > > case SPEED_1000: > > > > -- > > > > 2.9.4 > > > > > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > > > > the body of a message to majordomo@vger.kernel.org > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > It looks like that bnxt driver also has similar code (function > > > __to_ib_speed_width() in drivers/infiniband/hw/bnxt_re/ib_verbs.c) > > > > Sorry, i'm not following, this one fixes bnxt_re > > IMHO, he wanted to point your attention that RXE has very similar piece > of code. You mean rxe_query_port? In this case i have two questions. - rxe_query_port translates speeds to ib_speed by range where __to_ib_speed_width translates by fixed speeds. Which one is correct? - rxe_query_port fallback to get_settings where __to_ib_speed_width just take default if get_link_ksettings is not implemented. Which one is correct? > > > > > > Maybe you can move this function to IB/core and use it in both places (or more) > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 linux-rdma" 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 linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > In this case i have two questions. > - rxe_query_port translates speeds to ib_speed by range where > __to_ib_speed_width translates by fixed speeds. Which one is correct? it depends. If lksettings.base.speed is an enum value of any value. only one can be true > - rxe_query_port fallback to get_settings where __to_ib_speed_width just > take default if get_link_ksettings is not implemented. Which one is correct? In this case I would go for rxe implementation. > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 05, 2017 at 06:04:18PM +0300, Moni Shoua wrote: > > > > In this case i have two questions. > > - rxe_query_port translates speeds to ib_speed by range where > > __to_ib_speed_width translates by fixed speeds. Which one is correct? > it depends. If lksettings.base.speed is an enum value of any value. > only one can be true > > - rxe_query_port fallback to get_settings where __to_ib_speed_width just > > take default if get_link_ksettings is not implemented. Which one is correct? > In this case I would go for rxe implementation. > > Ok, i will do my best to merge the two functions correctly. In addition to the above two questions (and thanks for your quick response), i've noticed some conflicts in how bnxt translate to IB speed and how rxe does. For example bnxt.10000=QDR while rxe.10000=FDR10 so will appreciate a careful review here. > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 linux-rdma" 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/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c index 7ba9e69..10c7189 100644 --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c @@ -183,17 +183,19 @@ int bnxt_re_modify_device(struct ib_device *ibdev, static void __to_ib_speed_width(struct net_device *netdev, u8 *speed, u8 *width) { - struct ethtool_link_ksettings lksettings; - u32 espeed; + u32 espeed = SPEED_UNKNOWN; if (netdev->ethtool_ops && netdev->ethtool_ops->get_link_ksettings) { - memset(&lksettings, 0, sizeof(lksettings)); + struct ethtool_link_ksettings lksettings = {0}; + int rc; + rtnl_lock(); - netdev->ethtool_ops->get_link_ksettings(netdev, &lksettings); + rc = netdev->ethtool_ops->get_link_ksettings(netdev, + &lksettings); rtnl_unlock(); - espeed = lksettings.base.speed; - } else { - espeed = SPEED_UNKNOWN; + + if (!rc) + espeed = lksettings.base.speed; } switch (espeed) { case SPEED_1000:
The function get_link_ksettings might return bad status indicating a failure to retrieve interface atttibutes. Check return value to cover this case. While there, change the zero-initialization to "compiler-helper" instead of an expensive call to memcpy. Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> --- drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)