[PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Yuval Shaia]

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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Moni Shoua]

On Mon, Jun 5, 2017 at 12:14 PM, Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> 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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Yuval Shaia]

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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> 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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]

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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2107 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50834 bytes --]

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Yuval Shaia]

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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3281 bytes --]

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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> 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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Yuval Shaia]

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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> 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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Moni Shoua]

>
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/bnxt_re: Check return value from get_link_ksettings

[Yuval Shaia]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html