linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
@ 2020-02-18  9:59 Kamal Heib
  2020-02-18 15:06 ` Bernard Metzler
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Kamal Heib @ 2020-02-18  9:59 UTC (permalink / raw)
  To: linux-rdma; +Cc: Jason Gunthorpe, Doug Ledford, Bernard Metzler, Kamal Heib

Make sure to set the active_{speed, width} attributes to avoid reporting
the same values regardless of the underlying device.

Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
---
V2: Change rc to rv.
---
 drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
index 73485d0da907..d5390d498c61 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -165,11 +165,12 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
 		   struct ib_port_attr *attr)
 {
 	struct siw_device *sdev = to_siw_dev(base_dev);
+	int rv;
 
 	memset(attr, 0, sizeof(*attr));
 
-	attr->active_speed = 2;
-	attr->active_width = 2;
+	rv = ib_get_eth_speed(base_dev, port, &attr->active_speed,
+			 &attr->active_width);
 	attr->gid_tbl_len = 1;
 	attr->max_msg_sz = -1;
 	attr->max_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
@@ -192,7 +193,7 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
 	 * attr->subnet_timeout = 0;
 	 * attr->init_type_repy = 0;
 	 */
-	return 0;
+	return rv;
 }
 
 int siw_get_port_immutable(struct ib_device *base_dev, u8 port,
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re:  [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
  2020-02-18  9:59 [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes Kamal Heib
@ 2020-02-18 15:06 ` Bernard Metzler
  2020-02-18 16:58 ` Leon Romanovsky
  2020-02-27 20:40 ` Jason Gunthorpe
  2 siblings, 0 replies; 10+ messages in thread
From: Bernard Metzler @ 2020-02-18 15:06 UTC (permalink / raw)
  To: Kamal Heib; +Cc: linux-rdma, Jason Gunthorpe, Doug Ledford

-----"Kamal Heib" <kamalheib1@gmail.com> wrote: -----

>To: linux-rdma@vger.kernel.org
>From: "Kamal Heib" <kamalheib1@gmail.com>
>Date: 02/18/2020 10:59AM
>Cc: "Jason Gunthorpe" <jgg@ziepe.ca>, "Doug Ledford"
><dledford@redhat.com>, "Bernard Metzler" <bmt@zurich.ibm.com>, "Kamal
>Heib" <kamalheib1@gmail.com>
>Subject: [EXTERNAL] [PATCH for-next v2] RDMA/siw: Fix setting
>active_{speed, width} attributes
>
>Make sure to set the active_{speed, width} attributes to avoid
>reporting
>the same values regardless of the underlying device.
>
>Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
>Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
>---
>V2: Change rc to rv.
>---
> drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
>b/drivers/infiniband/sw/siw/siw_verbs.c
>index 73485d0da907..d5390d498c61 100644
>--- a/drivers/infiniband/sw/siw/siw_verbs.c
>+++ b/drivers/infiniband/sw/siw/siw_verbs.c
>@@ -165,11 +165,12 @@ int siw_query_port(struct ib_device *base_dev,
>u8 port,
> 		   struct ib_port_attr *attr)
> {
> 	struct siw_device *sdev = to_siw_dev(base_dev);
>+	int rv;
> 
> 	memset(attr, 0, sizeof(*attr));
> 
>-	attr->active_speed = 2;
>-	attr->active_width = 2;
>+	rv = ib_get_eth_speed(base_dev, port, &attr->active_speed,
>+			 &attr->active_width);
> 	attr->gid_tbl_len = 1;
> 	attr->max_msg_sz = -1;
> 	attr->max_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
>@@ -192,7 +193,7 @@ int siw_query_port(struct ib_device *base_dev, u8
>port,
> 	 * attr->subnet_timeout = 0;
> 	 * attr->init_type_repy = 0;
> 	 */
>-	return 0;
>+	return rv;
> }
> 
> int siw_get_port_immutable(struct ib_device *base_dev, u8 port,
>-- 
>2.21.1
>
>
Looks good, and does what it should do, thanks!

Tested-by: Bernard Metzler <bmt@zurich.ibm.com>
Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
  2020-02-18  9:59 [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes Kamal Heib
  2020-02-18 15:06 ` Bernard Metzler
@ 2020-02-18 16:58 ` Leon Romanovsky
  2020-02-19  8:43   ` Kamal Heib
  2020-02-27 20:40 ` Jason Gunthorpe
  2 siblings, 1 reply; 10+ messages in thread
From: Leon Romanovsky @ 2020-02-18 16:58 UTC (permalink / raw)
  To: Kamal Heib; +Cc: linux-rdma, Jason Gunthorpe, Doug Ledford, Bernard Metzler

On Tue, Feb 18, 2020 at 11:59:11AM +0200, Kamal Heib wrote:
> Make sure to set the active_{speed, width} attributes to avoid reporting
> the same values regardless of the underlying device.
>
> Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
> Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> ---
> V2: Change rc to rv.
> ---
>  drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> index 73485d0da907..d5390d498c61 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -165,11 +165,12 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
>  		   struct ib_port_attr *attr)
>  {
>  	struct siw_device *sdev = to_siw_dev(base_dev);
> +	int rv;
>
>  	memset(attr, 0, sizeof(*attr));

This line should go too. IB/core clears attr prior to call driver.

Thanks

>
> -	attr->active_speed = 2;
> -	attr->active_width = 2;
> +	rv = ib_get_eth_speed(base_dev, port, &attr->active_speed,
> +			 &attr->active_width);
>  	attr->gid_tbl_len = 1;
>  	attr->max_msg_sz = -1;
>  	attr->max_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> @@ -192,7 +193,7 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
>  	 * attr->subnet_timeout = 0;
>  	 * attr->init_type_repy = 0;
>  	 */
> -	return 0;
> +	return rv;
>  }
>
>  int siw_get_port_immutable(struct ib_device *base_dev, u8 port,
> --
> 2.21.1
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
  2020-02-18 16:58 ` Leon Romanovsky
@ 2020-02-19  8:43   ` Kamal Heib
  2020-02-19  9:33     ` Leon Romanovsky
  0 siblings, 1 reply; 10+ messages in thread
From: Kamal Heib @ 2020-02-19  8:43 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: linux-rdma, Jason Gunthorpe, Doug Ledford, Bernard Metzler

On Tue, Feb 18, 2020 at 06:58:47PM +0200, Leon Romanovsky wrote:
> On Tue, Feb 18, 2020 at 11:59:11AM +0200, Kamal Heib wrote:
> > Make sure to set the active_{speed, width} attributes to avoid reporting
> > the same values regardless of the underlying device.
> >
> > Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
> > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> > ---
> > V2: Change rc to rv.
> > ---
> >  drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> > index 73485d0da907..d5390d498c61 100644
> > --- a/drivers/infiniband/sw/siw/siw_verbs.c
> > +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> > @@ -165,11 +165,12 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
> >  		   struct ib_port_attr *attr)
> >  {
> >  	struct siw_device *sdev = to_siw_dev(base_dev);
> > +	int rv;
> >
> >  	memset(attr, 0, sizeof(*attr));
> 
> This line should go too. IB/core clears attr prior to call driver.
> 
> Thanks
>

This can be done in a separate patch as this patch fixes a specific issue.

Thanks,
Kamal

> >
> > -	attr->active_speed = 2;
> > -	attr->active_width = 2;
> > +	rv = ib_get_eth_speed(base_dev, port, &attr->active_speed,
> > +			 &attr->active_width);
> >  	attr->gid_tbl_len = 1;
> >  	attr->max_msg_sz = -1;
> >  	attr->max_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> > @@ -192,7 +193,7 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
> >  	 * attr->subnet_timeout = 0;
> >  	 * attr->init_type_repy = 0;
> >  	 */
> > -	return 0;
> > +	return rv;
> >  }
> >
> >  int siw_get_port_immutable(struct ib_device *base_dev, u8 port,
> > --
> > 2.21.1
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
  2020-02-19  8:43   ` Kamal Heib
@ 2020-02-19  9:33     ` Leon Romanovsky
  2020-02-19 10:21       ` Kamal Heib
  0 siblings, 1 reply; 10+ messages in thread
From: Leon Romanovsky @ 2020-02-19  9:33 UTC (permalink / raw)
  To: Kamal Heib; +Cc: linux-rdma, Jason Gunthorpe, Doug Ledford, Bernard Metzler

On Wed, Feb 19, 2020 at 10:43:59AM +0200, Kamal Heib wrote:
> On Tue, Feb 18, 2020 at 06:58:47PM +0200, Leon Romanovsky wrote:
> > On Tue, Feb 18, 2020 at 11:59:11AM +0200, Kamal Heib wrote:
> > > Make sure to set the active_{speed, width} attributes to avoid reporting
> > > the same values regardless of the underlying device.
> > >
> > > Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
> > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> > > ---
> > > V2: Change rc to rv.
> > > ---
> > >  drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> > > index 73485d0da907..d5390d498c61 100644
> > > --- a/drivers/infiniband/sw/siw/siw_verbs.c
> > > +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> > > @@ -165,11 +165,12 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
> > >  		   struct ib_port_attr *attr)
> > >  {
> > >  	struct siw_device *sdev = to_siw_dev(base_dev);
> > > +	int rv;
> > >
> > >  	memset(attr, 0, sizeof(*attr));
> >
> > This line should go too. IB/core clears attr prior to call driver.
> >
> > Thanks
> >
>
> This can be done in a separate patch as this patch fixes a specific issue.

Whatever works for you, if you don't value your own time, go for it,
do separate patch for every line you are changing. It just looks crazy
to see changes like this:
 * changed line
 * line to be changed, but not changed
 * another changed line

Thanks

>
> Thanks,
> Kamal
>
> > >
> > > -	attr->active_speed = 2;
> > > -	attr->active_width = 2;
> > > +	rv = ib_get_eth_speed(base_dev, port, &attr->active_speed,
> > > +			 &attr->active_width);
> > >  	attr->gid_tbl_len = 1;
> > >  	attr->max_msg_sz = -1;
> > >  	attr->max_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> > > @@ -192,7 +193,7 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
> > >  	 * attr->subnet_timeout = 0;
> > >  	 * attr->init_type_repy = 0;
> > >  	 */
> > > -	return 0;
> > > +	return rv;
> > >  }
> > >
> > >  int siw_get_port_immutable(struct ib_device *base_dev, u8 port,
> > > --
> > > 2.21.1
> > >

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
  2020-02-19  9:33     ` Leon Romanovsky
@ 2020-02-19 10:21       ` Kamal Heib
  2020-02-19 10:53         ` Leon Romanovsky
  0 siblings, 1 reply; 10+ messages in thread
From: Kamal Heib @ 2020-02-19 10:21 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: linux-rdma, Jason Gunthorpe, Doug Ledford, Bernard Metzler

On Wed, Feb 19, 2020 at 11:33:21AM +0200, Leon Romanovsky wrote:
> On Wed, Feb 19, 2020 at 10:43:59AM +0200, Kamal Heib wrote:
> > On Tue, Feb 18, 2020 at 06:58:47PM +0200, Leon Romanovsky wrote:
> > > On Tue, Feb 18, 2020 at 11:59:11AM +0200, Kamal Heib wrote:
> > > > Make sure to set the active_{speed, width} attributes to avoid reporting
> > > > the same values regardless of the underlying device.
> > > >
> > > > Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
> > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> > > > ---
> > > > V2: Change rc to rv.
> > > > ---
> > > >  drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> > > > index 73485d0da907..d5390d498c61 100644
> > > > --- a/drivers/infiniband/sw/siw/siw_verbs.c
> > > > +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> > > > @@ -165,11 +165,12 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
> > > >  		   struct ib_port_attr *attr)
> > > >  {
> > > >  	struct siw_device *sdev = to_siw_dev(base_dev);
> > > > +	int rv;
> > > >
> > > >  	memset(attr, 0, sizeof(*attr));
> > >
> > > This line should go too. IB/core clears attr prior to call driver.
> > >
> > > Thanks
> > >
> >
> > This can be done in a separate patch as this patch fixes a specific issue.
> 
> Whatever works for you, if you don't value your own time, go for it,
> do separate patch for every line you are changing. It just looks crazy
> to see changes like this:
>  * changed line
>  * line to be changed, but not changed
>  * another changed line
> 
> Thanks
>

Leon, With all my respect, This isn't your decision what I do and when.

W.R.T your suggestion for removing the memset() line, there are multiple
places in the siw drivers that do memset in the ib_device_ops, with that
said, I think that this need to be done in a separate patch. BTW you can
do the change too ;)

Thanks,
Kamal

> >
> > Thanks,
> > Kamal
> >
> > > >
> > > > -	attr->active_speed = 2;
> > > > -	attr->active_width = 2;
> > > > +	rv = ib_get_eth_speed(base_dev, port, &attr->active_speed,
> > > > +			 &attr->active_width);
> > > >  	attr->gid_tbl_len = 1;
> > > >  	attr->max_msg_sz = -1;
> > > >  	attr->max_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> > > > @@ -192,7 +193,7 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
> > > >  	 * attr->subnet_timeout = 0;
> > > >  	 * attr->init_type_repy = 0;
> > > >  	 */
> > > > -	return 0;
> > > > +	return rv;
> > > >  }
> > > >
> > > >  int siw_get_port_immutable(struct ib_device *base_dev, u8 port,
> > > > --
> > > > 2.21.1
> > > >

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
  2020-02-19 10:21       ` Kamal Heib
@ 2020-02-19 10:53         ` Leon Romanovsky
  2020-02-19 11:07           ` Gal Pressman
  2020-02-19 11:13           ` Kamal Heib
  0 siblings, 2 replies; 10+ messages in thread
From: Leon Romanovsky @ 2020-02-19 10:53 UTC (permalink / raw)
  To: Kamal Heib; +Cc: linux-rdma, Jason Gunthorpe, Doug Ledford, Bernard Metzler

On Wed, Feb 19, 2020 at 12:21:10PM +0200, Kamal Heib wrote:
> On Wed, Feb 19, 2020 at 11:33:21AM +0200, Leon Romanovsky wrote:
> > On Wed, Feb 19, 2020 at 10:43:59AM +0200, Kamal Heib wrote:
> > > On Tue, Feb 18, 2020 at 06:58:47PM +0200, Leon Romanovsky wrote:
> > > > On Tue, Feb 18, 2020 at 11:59:11AM +0200, Kamal Heib wrote:
> > > > > Make sure to set the active_{speed, width} attributes to avoid reporting
> > > > > the same values regardless of the underlying device.
> > > > >
> > > > > Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
> > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> > > > > ---
> > > > > V2: Change rc to rv.
> > > > > ---
> > > > >  drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
> > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> > > > > index 73485d0da907..d5390d498c61 100644
> > > > > --- a/drivers/infiniband/sw/siw/siw_verbs.c
> > > > > +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> > > > > @@ -165,11 +165,12 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
> > > > >  		   struct ib_port_attr *attr)
> > > > >  {
> > > > >  	struct siw_device *sdev = to_siw_dev(base_dev);
> > > > > +	int rv;
> > > > >
> > > > >  	memset(attr, 0, sizeof(*attr));
> > > >
> > > > This line should go too. IB/core clears attr prior to call driver.
> > > >
> > > > Thanks
> > > >
> > >
> > > This can be done in a separate patch as this patch fixes a specific issue.
> >
> > Whatever works for you, if you don't value your own time, go for it,
> > do separate patch for every line you are changing. It just looks crazy
> > to see changes like this:
> >  * changed line
> >  * line to be changed, but not changed
> >  * another changed line
> >
> > Thanks
> >
>
> Leon, With all my respect, This isn't your decision what I do and when.

Please carefully reread my answer, I didn't say what and when you should
do, simply gave to you an explanation why request remove useless memeset
makes sense in the context of proposed patch.

Thanks

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
  2020-02-19 10:53         ` Leon Romanovsky
@ 2020-02-19 11:07           ` Gal Pressman
  2020-02-19 11:13           ` Kamal Heib
  1 sibling, 0 replies; 10+ messages in thread
From: Gal Pressman @ 2020-02-19 11:07 UTC (permalink / raw)
  To: Leon Romanovsky, Kamal Heib
  Cc: linux-rdma, Jason Gunthorpe, Doug Ledford, Bernard Metzler

On 19/02/2020 12:53, Leon Romanovsky wrote:
> On Wed, Feb 19, 2020 at 12:21:10PM +0200, Kamal Heib wrote:
>> On Wed, Feb 19, 2020 at 11:33:21AM +0200, Leon Romanovsky wrote:
>>> On Wed, Feb 19, 2020 at 10:43:59AM +0200, Kamal Heib wrote:
>>>> On Tue, Feb 18, 2020 at 06:58:47PM +0200, Leon Romanovsky wrote:
>>>>> On Tue, Feb 18, 2020 at 11:59:11AM +0200, Kamal Heib wrote:
>>>>>> Make sure to set the active_{speed, width} attributes to avoid reporting
>>>>>> the same values regardless of the underlying device.
>>>>>>
>>>>>> Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
>>>>>> Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
>>>>>> ---
>>>>>> V2: Change rc to rv.
>>>>>> ---
>>>>>>  drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
>>>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
>>>>>> index 73485d0da907..d5390d498c61 100644
>>>>>> --- a/drivers/infiniband/sw/siw/siw_verbs.c
>>>>>> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
>>>>>> @@ -165,11 +165,12 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
>>>>>>  		   struct ib_port_attr *attr)
>>>>>>  {
>>>>>>  	struct siw_device *sdev = to_siw_dev(base_dev);
>>>>>> +	int rv;
>>>>>>
>>>>>>  	memset(attr, 0, sizeof(*attr));
>>>>>
>>>>> This line should go too. IB/core clears attr prior to call driver.
>>>>>
>>>>> Thanks
>>>>>
>>>>
>>>> This can be done in a separate patch as this patch fixes a specific issue.
>>>
>>> Whatever works for you, if you don't value your own time, go for it,
>>> do separate patch for every line you are changing. It just looks crazy
>>> to see changes like this:
>>>  * changed line
>>>  * line to be changed, but not changed
>>>  * another changed line
>>>
>>> Thanks
>>>
>>
>> Leon, With all my respect, This isn't your decision what I do and when.
> 
> Please carefully reread my answer, I didn't say what and when you should
> do, simply gave to you an explanation why request remove useless memeset
> makes sense in the context of proposed patch.

I tend to agree with Kamal..
If the patch contained the memset removal I would argue it should be dropped as
it has nothing to do with the purpose of this commit. Just adds noise to the review.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
  2020-02-19 10:53         ` Leon Romanovsky
  2020-02-19 11:07           ` Gal Pressman
@ 2020-02-19 11:13           ` Kamal Heib
  1 sibling, 0 replies; 10+ messages in thread
From: Kamal Heib @ 2020-02-19 11:13 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: linux-rdma, Jason Gunthorpe, Doug Ledford, Bernard Metzler

On Wed, Feb 19, 2020 at 12:53:20PM +0200, Leon Romanovsky wrote:
> On Wed, Feb 19, 2020 at 12:21:10PM +0200, Kamal Heib wrote:
> > On Wed, Feb 19, 2020 at 11:33:21AM +0200, Leon Romanovsky wrote:
> > > On Wed, Feb 19, 2020 at 10:43:59AM +0200, Kamal Heib wrote:
> > > > On Tue, Feb 18, 2020 at 06:58:47PM +0200, Leon Romanovsky wrote:
> > > > > On Tue, Feb 18, 2020 at 11:59:11AM +0200, Kamal Heib wrote:
> > > > > > Make sure to set the active_{speed, width} attributes to avoid reporting
> > > > > > the same values regardless of the underlying device.
> > > > > >
> > > > > > Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
> > > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> > > > > > ---
> > > > > > V2: Change rc to rv.
> > > > > > ---
> > > > > >  drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
> > > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> > > > > > index 73485d0da907..d5390d498c61 100644
> > > > > > --- a/drivers/infiniband/sw/siw/siw_verbs.c
> > > > > > +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> > > > > > @@ -165,11 +165,12 @@ int siw_query_port(struct ib_device *base_dev, u8 port,
> > > > > >  		   struct ib_port_attr *attr)
> > > > > >  {
> > > > > >  	struct siw_device *sdev = to_siw_dev(base_dev);
> > > > > > +	int rv;
> > > > > >
> > > > > >  	memset(attr, 0, sizeof(*attr));
> > > > >
> > > > > This line should go too. IB/core clears attr prior to call driver.
> > > > >
> > > > > Thanks
> > > > >
> > > >
> > > > This can be done in a separate patch as this patch fixes a specific issue.
> > >
> > > Whatever works for you, if you don't value your own time, go for it,
> > > do separate patch for every line you are changing. It just looks crazy
> > > to see changes like this:
> > >  * changed line
> > >  * line to be changed, but not changed
> > >  * another changed line
> > >
> > > Thanks
> > >
> >
> > Leon, With all my respect, This isn't your decision what I do and when.
> 
> Please carefully reread my answer, I didn't say what and when you should
> do, simply gave to you an explanation why request remove useless memeset
> makes sense in the context of proposed patch.
> 
> Thanks

I simply give an answer to why I think otherwise, That doesn't mean that
I don't value my own time...

Thanks,
Kamal


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes
  2020-02-18  9:59 [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes Kamal Heib
  2020-02-18 15:06 ` Bernard Metzler
  2020-02-18 16:58 ` Leon Romanovsky
@ 2020-02-27 20:40 ` Jason Gunthorpe
  2 siblings, 0 replies; 10+ messages in thread
From: Jason Gunthorpe @ 2020-02-27 20:40 UTC (permalink / raw)
  To: Kamal Heib; +Cc: linux-rdma, Doug Ledford, Bernard Metzler

On Tue, Feb 18, 2020 at 11:59:11AM +0200, Kamal Heib wrote:
> Make sure to set the active_{speed, width} attributes to avoid reporting
> the same values regardless of the underlying device.
> 
> Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
> Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> Tested-by: Bernard Metzler <bmt@zurich.ibm.com>
> Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>
> ---
> V2: Change rc to rv.
> ---
>  drivers/infiniband/sw/siw/siw_verbs.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Applied to for-next, thanks

Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-02-27 20:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18  9:59 [PATCH for-next v2] RDMA/siw: Fix setting active_{speed, width} attributes Kamal Heib
2020-02-18 15:06 ` Bernard Metzler
2020-02-18 16:58 ` Leon Romanovsky
2020-02-19  8:43   ` Kamal Heib
2020-02-19  9:33     ` Leon Romanovsky
2020-02-19 10:21       ` Kamal Heib
2020-02-19 10:53         ` Leon Romanovsky
2020-02-19 11:07           ` Gal Pressman
2020-02-19 11:13           ` Kamal Heib
2020-02-27 20:40 ` Jason Gunthorpe

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).