* [PATCH 0/2] IB/core: Set invalid active widths to 1X when port is not active
@ 2018-03-15 9:02 Honggang LI
2018-03-15 9:02 ` [PATCH 1/2] IB/core: Set speed string to SDR for invalid active rates Honggang LI
2018-03-15 9:02 ` [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down Honggang LI
0 siblings, 2 replies; 10+ messages in thread
From: Honggang LI @ 2018-03-15 9:02 UTC (permalink / raw)
To: dledford, jgg, linux-rdma; +Cc: noaos, linux-kernel, honli
From: Honggang Li <honli@redhat.com>
Before the mlx5 RoCE port negotiate the active rate and width
with remote side, the mlx5_ib driver set them to zero.
The tool 'ibstat' of 'infiniband-diags' package will panic as it
read invalid width from the sys file. Set the active widths to
lowest valid vaule as what ib_core module did for invalid rates.
Honggang Li (2):
IB/core: Set speed string to SDR for invalid active rates
IB/core: Set width to 1X for invalid active widths when port is down
drivers/infiniband/core/sysfs.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
--
2.14.GIT
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] IB/core: Set speed string to SDR for invalid active rates
2018-03-15 9:02 [PATCH 0/2] IB/core: Set invalid active widths to 1X when port is not active Honggang LI
@ 2018-03-15 9:02 ` Honggang LI
2018-03-19 17:53 ` Jason Gunthorpe
2018-03-15 9:02 ` [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down Honggang LI
1 sibling, 1 reply; 10+ messages in thread
From: Honggang LI @ 2018-03-15 9:02 UTC (permalink / raw)
To: dledford, jgg, linux-rdma; +Cc: noaos, linux-kernel, honli
From: Honggang Li <honli@redhat.com>
commit f1b65df5a232 ("IB/mlx5: Add support for active_width and
active_speed in RoCE"). Before this patch applied, the mlx5_ib
driver set default active_width and active_speed to IB_WIDTH_4X
and IB_SPEED_QDR.
Now, the active_width and active_speed are zeros if the RoCE port
is in DOWN state. The speed string should be set to " SDR" instead of
a blank string when active_speed is zero.
Signed-off-by: Honggang Li <honli@redhat.com>
---
drivers/infiniband/core/sysfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index 8ae1308eecc7..cf36ff1f0068 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -273,6 +273,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
break;
case IB_SPEED_SDR:
default: /* default to SDR for invalid rates */
+ speed = " SDR";
rate = 25;
break;
}
--
2.14.GIT
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down
2018-03-15 9:02 [PATCH 0/2] IB/core: Set invalid active widths to 1X when port is not active Honggang LI
2018-03-15 9:02 ` [PATCH 1/2] IB/core: Set speed string to SDR for invalid active rates Honggang LI
@ 2018-03-15 9:02 ` Honggang LI
2018-03-15 12:01 ` Hal Rosenstock
1 sibling, 1 reply; 10+ messages in thread
From: Honggang LI @ 2018-03-15 9:02 UTC (permalink / raw)
To: dledford, jgg, linux-rdma; +Cc: noaos, linux-kernel, honli
From: Honggang Li <honli@redhat.com>
commit f1b65df5a232 ("IB/mlx5: Add support for active_width and
active_speed in RoCE"). Before this patch applied, the mlx5_ib
driver set default active_width and active_speed to IB_WIDTH_4X
and IB_SPEED_QDR.
When the RoCE port is down, the RoCE port did not negotiate the
active width with remote side. The active width is zero. If run
ibstat to require the port status, ibstat will panic as it read
invalid width from sys file.
Signed-off-by: Honggang Li <honli@redhat.com>
---
drivers/infiniband/core/sysfs.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index cf36ff1f0068..722e4571f4d2 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
struct ib_port_attr attr;
char *speed = "";
int rate; /* in deci-Gb/sec */
+ int width;
ssize_t ret;
ret = ib_query_port(p->ibdev, p->port_num, &attr);
@@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
break;
}
- rate *= ib_width_enum_to_int(attr.active_width);
- if (rate < 0)
- return -EINVAL;
+ width = ib_width_enum_to_int(attr.active_width);
+ if (width < 0) {
+ if (attr.state != IB_PORT_ACTIVE)
+ width = 1; /* default to 1X for invalid widths */
+ else
+ return -EINVAL;
+ }
+
+ rate *= width;
return sprintf(buf, "%d%s Gb/sec (%dX%s)\n",
rate / 10, rate % 10 ? ".5" : "",
- ib_width_enum_to_int(attr.active_width), speed);
+ width, speed);
}
static ssize_t phys_state_show(struct ib_port *p, struct port_attribute *unused,
--
2.14.GIT
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down
2018-03-15 9:02 ` [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down Honggang LI
@ 2018-03-15 12:01 ` Hal Rosenstock
2018-03-15 12:27 ` Honggang LI
2018-03-15 12:32 ` Hal Rosenstock
0 siblings, 2 replies; 10+ messages in thread
From: Hal Rosenstock @ 2018-03-15 12:01 UTC (permalink / raw)
To: Honggang LI, dledford, jgg, linux-rdma; +Cc: noaos, linux-kernel
On 3/15/2018 5:02 AM, Honggang LI wrote:
> From: Honggang Li <honli@redhat.com>
>
> commit f1b65df5a232 ("IB/mlx5: Add support for active_width and
> active_speed in RoCE"). Before this patch applied, the mlx5_ib
> driver set default active_width and active_speed to IB_WIDTH_4X
> and IB_SPEED_QDR.
>
> When the RoCE port is down, the RoCE port did not negotiate the
> active width with remote side. The active width is zero. If run
> ibstat to require the port status, ibstat will panic as it read
> invalid width from sys file.
>
> Signed-off-by: Honggang Li <honli@redhat.com>
> ---
> drivers/infiniband/core/sysfs.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> index cf36ff1f0068..722e4571f4d2 100644
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
> struct ib_port_attr attr;
> char *speed = "";
> int rate; /* in deci-Gb/sec */
> + int width;
> ssize_t ret;
>
> ret = ib_query_port(p->ibdev, p->port_num, &attr);
> @@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
> break;
> }
>
> - rate *= ib_width_enum_to_int(attr.active_width);
> - if (rate < 0)
> - return -EINVAL;
> + width = ib_width_enum_to_int(attr.active_width);
> + if (width < 0) {
> + if (attr.state != IB_PORT_ACTIVE)
Link width is valid in any PortState other than Down so I think that
this check should be:
if (attr.state != IB_PORT_DOWN)
However, I don't think overriding width should be needed for this case
and just returning -EINVAL should be fine regardless of port state.
AFAIK it's the driver responsibility to populate acceptable defaults for
such parameters. What driver(s) have this issue ? Shouldn't it be fixed
there rather than here ?
-- Hal
> + width = 1; /* default to 1X for invalid widths */
> + else
> + return -EINVAL;
> + }
> +
> + rate *= width;
>
> return sprintf(buf, "%d%s Gb/sec (%dX%s)\n",
> rate / 10, rate % 10 ? ".5" : "",
> - ib_width_enum_to_int(attr.active_width), speed);
> + width, speed);
> }
>
> static ssize_t phys_state_show(struct ib_port *p, struct port_attribute *unused,
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down
2018-03-15 12:01 ` Hal Rosenstock
@ 2018-03-15 12:27 ` Honggang LI
2018-03-15 12:32 ` Hal Rosenstock
1 sibling, 0 replies; 10+ messages in thread
From: Honggang LI @ 2018-03-15 12:27 UTC (permalink / raw)
To: Hal Rosenstock; +Cc: dledford, jgg, linux-rdma, noaos, linux-kernel
On Thu, Mar 15, 2018 at 08:01:08AM -0400, Hal Rosenstock wrote:
> On 3/15/2018 5:02 AM, Honggang LI wrote:
> > From: Honggang Li <honli@redhat.com>
> >
> > commit f1b65df5a232 ("IB/mlx5: Add support for active_width and
> > active_speed in RoCE"). Before this patch applied, the mlx5_ib
> > driver set default active_width and active_speed to IB_WIDTH_4X
> > and IB_SPEED_QDR.
> >
> > When the RoCE port is down, the RoCE port did not negotiate the
> > active width with remote side. The active width is zero. If run
> > ibstat to require the port status, ibstat will panic as it read
> > invalid width from sys file.
> >
> > Signed-off-by: Honggang Li <honli@redhat.com>
> > ---
> > drivers/infiniband/core/sysfs.c | 15 +++++++++++----
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> > index cf36ff1f0068..722e4571f4d2 100644
> > --- a/drivers/infiniband/core/sysfs.c
> > +++ b/drivers/infiniband/core/sysfs.c
> > @@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
> > struct ib_port_attr attr;
> > char *speed = "";
> > int rate; /* in deci-Gb/sec */
> > + int width;
> > ssize_t ret;
> >
> > ret = ib_query_port(p->ibdev, p->port_num, &attr);
> > @@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
> > break;
> > }
> >
> > - rate *= ib_width_enum_to_int(attr.active_width);
> > - if (rate < 0)
> > - return -EINVAL;
> > + width = ib_width_enum_to_int(attr.active_width);
> > + if (width < 0) {
> > + if (attr.state != IB_PORT_ACTIVE)
>
> Link width is valid in any PortState other than Down so I think that
> this check should be:
> if (attr.state != IB_PORT_DOWN)
OK, thanks for correct this.
> However, I don't think overriding width should be needed for this case
> and just returning -EINVAL should be fine regardless of port state.
> AFAIK it's the driver responsibility to populate acceptable defaults for
> such parameters. What driver(s) have this issue ? Shouldn't it be fixed
mlx5_ib, commit f1b65df5a232 changed the default active width and rate.
> there rather than here ?
Yes, I think so, as only mlx5 RoCE impacted by this issue. So, fix the
driver is also acceptable.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down
2018-03-15 12:01 ` Hal Rosenstock
2018-03-15 12:27 ` Honggang LI
@ 2018-03-15 12:32 ` Hal Rosenstock
2018-03-15 12:43 ` Honggang LI
1 sibling, 1 reply; 10+ messages in thread
From: Hal Rosenstock @ 2018-03-15 12:32 UTC (permalink / raw)
To: Honggang LI, dledford, jgg, linux-rdma; +Cc: noaos, linux-kernel
On 3/15/2018 8:01 AM, Hal Rosenstock wrote:
> On 3/15/2018 5:02 AM, Honggang LI wrote:
>> From: Honggang Li <honli@redhat.com>
>>
>> commit f1b65df5a232 ("IB/mlx5: Add support for active_width and
>> active_speed in RoCE"). Before this patch applied, the mlx5_ib
>> driver set default active_width and active_speed to IB_WIDTH_4X
>> and IB_SPEED_QDR.
>>
>> When the RoCE port is down, the RoCE port did not negotiate the
>> active width with remote side. The active width is zero. If run
>> ibstat to require the port status, ibstat will panic as it read
>> invalid width from sys file.
>>
>> Signed-off-by: Honggang Li <honli@redhat.com>
>> ---
>> drivers/infiniband/core/sysfs.c | 15 +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
>> index cf36ff1f0068..722e4571f4d2 100644
>> --- a/drivers/infiniband/core/sysfs.c
>> +++ b/drivers/infiniband/core/sysfs.c
>> @@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>> struct ib_port_attr attr;
>> char *speed = "";
>> int rate; /* in deci-Gb/sec */
>> + int width;
>> ssize_t ret;
>>
>> ret = ib_query_port(p->ibdev, p->port_num, &attr);
>> @@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>> break;
>> }
>>
>> - rate *= ib_width_enum_to_int(attr.active_width);
>> - if (rate < 0)
>> - return -EINVAL;
>> + width = ib_width_enum_to_int(attr.active_width);
>> + if (width < 0) {
>> + if (attr.state != IB_PORT_ACTIVE)
>
> Link width is valid in any PortState other than Down so I think that
> this check should be:
> if (attr.state != IB_PORT_DOWN)
>
> However, I don't think overriding width should be needed for this case
> and just returning -EINVAL should be fine regardless of port state.
> AFAIK it's the driver responsibility to populate acceptable defaults for
> such parameters. What driver(s) have this issue ? Shouldn't it be fixed
> there rather than here ?
I just noticed that you reference commit f1b65df5a232 ("IB/mlx5: Add
support for active_width and active_speed in RoCE"). Before this patch
applied, the mlx5_ib driver set default active_width and active_speed to
IB_WIDTH_4X and IB_SPEED_QDR.
Should the fix be to hw/mlx5/main.c:translate_eth_proto_oper which now has:
switch (eth_proto_oper) {
...
default:
return -EINVAL;
}
return 0;
and change default case to:
*active_width = IB_WIDTH_1X;
?
> -- Hal
>
>> + width = 1; /* default to 1X for invalid widths */
>> + else
>> + return -EINVAL;
>> + }
>> +
>> + rate *= width;
>>
>> return sprintf(buf, "%d%s Gb/sec (%dX%s)\n",
>> rate / 10, rate % 10 ? ".5" : "",
>> - ib_width_enum_to_int(attr.active_width), speed);
>> + width, speed);
>> }
>>
>> static ssize_t phys_state_show(struct ib_port *p, struct port_attribute *unused,
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down
2018-03-15 12:32 ` Hal Rosenstock
@ 2018-03-15 12:43 ` Honggang LI
2018-03-15 12:47 ` Hal Rosenstock
0 siblings, 1 reply; 10+ messages in thread
From: Honggang LI @ 2018-03-15 12:43 UTC (permalink / raw)
To: Hal Rosenstock; +Cc: dledford, jgg, linux-rdma, noaos, linux-kernel
On Thu, Mar 15, 2018 at 08:32:02AM -0400, Hal Rosenstock wrote:
> On 3/15/2018 8:01 AM, Hal Rosenstock wrote:
> > On 3/15/2018 5:02 AM, Honggang LI wrote:
> >> From: Honggang Li <honli@redhat.com>
> >>
> >> commit f1b65df5a232 ("IB/mlx5: Add support for active_width and
> >> active_speed in RoCE"). Before this patch applied, the mlx5_ib
> >> driver set default active_width and active_speed to IB_WIDTH_4X
> >> and IB_SPEED_QDR.
> >>
> >> When the RoCE port is down, the RoCE port did not negotiate the
> >> active width with remote side. The active width is zero. If run
> >> ibstat to require the port status, ibstat will panic as it read
> >> invalid width from sys file.
> >>
> >> Signed-off-by: Honggang Li <honli@redhat.com>
> >> ---
> >> drivers/infiniband/core/sysfs.c | 15 +++++++++++----
> >> 1 file changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> >> index cf36ff1f0068..722e4571f4d2 100644
> >> --- a/drivers/infiniband/core/sysfs.c
> >> +++ b/drivers/infiniband/core/sysfs.c
> >> @@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
> >> struct ib_port_attr attr;
> >> char *speed = "";
> >> int rate; /* in deci-Gb/sec */
> >> + int width;
> >> ssize_t ret;
> >>
> >> ret = ib_query_port(p->ibdev, p->port_num, &attr);
> >> @@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
> >> break;
> >> }
> >>
> >> - rate *= ib_width_enum_to_int(attr.active_width);
> >> - if (rate < 0)
> >> - return -EINVAL;
> >> + width = ib_width_enum_to_int(attr.active_width);
> >> + if (width < 0) {
> >> + if (attr.state != IB_PORT_ACTIVE)
> >
> > Link width is valid in any PortState other than Down so I think that
> > this check should be:
> > if (attr.state != IB_PORT_DOWN)
> >
> > However, I don't think overriding width should be needed for this case
> > and just returning -EINVAL should be fine regardless of port state.
> > AFAIK it's the driver responsibility to populate acceptable defaults for
> > such parameters. What driver(s) have this issue ? Shouldn't it be fixed
> > there rather than here ?
>
> I just noticed that you reference commit f1b65df5a232 ("IB/mlx5: Add
> support for active_width and active_speed in RoCE"). Before this patch
> applied, the mlx5_ib driver set default active_width and active_speed to
> IB_WIDTH_4X and IB_SPEED_QDR.
>
> Should the fix be to hw/mlx5/main.c:translate_eth_proto_oper which now has:
>
> switch (eth_proto_oper) {
> ...
> default:
> return -EINVAL;
> }
>
> return 0;
>
> and change default case to:
> *active_width = IB_WIDTH_1X;
>
> ?
I suggest to restore previous behavior before apply f1b65df5a232.
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 033b6af90de9..0d73d2772d9b 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -388,6 +388,9 @@ static int mlx5_query_port_roce(struct ib_device *device, u8 port_num,
if (err)
goto out;
+ props->active_width = IB_WIDTH_4X;
+ props->active_speed = IB_SPEED_QDR;
+
translate_eth_proto_oper(eth_prot_oper, &props->active_speed,
&props->active_width);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down
2018-03-15 12:43 ` Honggang LI
@ 2018-03-15 12:47 ` Hal Rosenstock
2018-03-16 2:38 ` Honggang LI
0 siblings, 1 reply; 10+ messages in thread
From: Hal Rosenstock @ 2018-03-15 12:47 UTC (permalink / raw)
To: Honggang LI; +Cc: dledford, jgg, linux-rdma, noaos, linux-kernel
On 3/15/2018 8:43 AM, Honggang LI wrote:
> On Thu, Mar 15, 2018 at 08:32:02AM -0400, Hal Rosenstock wrote:
>> On 3/15/2018 8:01 AM, Hal Rosenstock wrote:
>>> On 3/15/2018 5:02 AM, Honggang LI wrote:
>>>> From: Honggang Li <honli@redhat.com>
>>>>
>>>> commit f1b65df5a232 ("IB/mlx5: Add support for active_width and
>>>> active_speed in RoCE"). Before this patch applied, the mlx5_ib
>>>> driver set default active_width and active_speed to IB_WIDTH_4X
>>>> and IB_SPEED_QDR.
>>>>
>>>> When the RoCE port is down, the RoCE port did not negotiate the
>>>> active width with remote side. The active width is zero. If run
>>>> ibstat to require the port status, ibstat will panic as it read
>>>> invalid width from sys file.
>>>>
>>>> Signed-off-by: Honggang Li <honli@redhat.com>
>>>> ---
>>>> drivers/infiniband/core/sysfs.c | 15 +++++++++++----
>>>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
>>>> index cf36ff1f0068..722e4571f4d2 100644
>>>> --- a/drivers/infiniband/core/sysfs.c
>>>> +++ b/drivers/infiniband/core/sysfs.c
>>>> @@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>>>> struct ib_port_attr attr;
>>>> char *speed = "";
>>>> int rate; /* in deci-Gb/sec */
>>>> + int width;
>>>> ssize_t ret;
>>>>
>>>> ret = ib_query_port(p->ibdev, p->port_num, &attr);
>>>> @@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>>>> break;
>>>> }
>>>>
>>>> - rate *= ib_width_enum_to_int(attr.active_width);
>>>> - if (rate < 0)
>>>> - return -EINVAL;
>>>> + width = ib_width_enum_to_int(attr.active_width);
>>>> + if (width < 0) {
>>>> + if (attr.state != IB_PORT_ACTIVE)
>>>
>>> Link width is valid in any PortState other than Down so I think that
>>> this check should be:
>>> if (attr.state != IB_PORT_DOWN)
>>>
>>> However, I don't think overriding width should be needed for this case
>>> and just returning -EINVAL should be fine regardless of port state.
>>> AFAIK it's the driver responsibility to populate acceptable defaults for
>>> such parameters. What driver(s) have this issue ? Shouldn't it be fixed
>>> there rather than here ?
>>
>> I just noticed that you reference commit f1b65df5a232 ("IB/mlx5: Add
>> support for active_width and active_speed in RoCE"). Before this patch
>> applied, the mlx5_ib driver set default active_width and active_speed to
>> IB_WIDTH_4X and IB_SPEED_QDR.
>>
>> Should the fix be to hw/mlx5/main.c:translate_eth_proto_oper which now has:
>>
>> switch (eth_proto_oper) {
>> ...
>> default:
>> return -EINVAL;
>> }
>>
>> return 0;
>>
>> and change default case to:
>> *active_width = IB_WIDTH_1X;
>>
>> ?
>
> I suggest to restore previous behavior before apply f1b65df5a232.
>
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 033b6af90de9..0d73d2772d9b 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -388,6 +388,9 @@ static int mlx5_query_port_roce(struct ib_device *device, u8 port_num,
> if (err)
> goto out;
>
> + props->active_width = IB_WIDTH_4X;
> + props->active_speed = IB_SPEED_QDR;
> +
> translate_eth_proto_oper(eth_prot_oper, &props->active_speed,
> &props->active_width);
>
Sure; makes sense that it should preserve the original behavior for this
case.
Thanks.
-- Hal
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down
2018-03-15 12:47 ` Hal Rosenstock
@ 2018-03-16 2:38 ` Honggang LI
0 siblings, 0 replies; 10+ messages in thread
From: Honggang LI @ 2018-03-16 2:38 UTC (permalink / raw)
To: Hal Rosenstock; +Cc: dledford, jgg, linux-rdma, noaos, linux-kernel
On Thu, Mar 15, 2018 at 08:47:44AM -0400, Hal Rosenstock wrote:
> On 3/15/2018 8:43 AM, Honggang LI wrote:
> > On Thu, Mar 15, 2018 at 08:32:02AM -0400, Hal Rosenstock wrote:
> >> On 3/15/2018 8:01 AM, Hal Rosenstock wrote:
> >>> On 3/15/2018 5:02 AM, Honggang LI wrote:
> >>>> From: Honggang Li <honli@redhat.com>
> >>>>
> >>>> commit f1b65df5a232 ("IB/mlx5: Add support for active_width and
> >>>> active_speed in RoCE"). Before this patch applied, the mlx5_ib
> >>>> driver set default active_width and active_speed to IB_WIDTH_4X
> >>>> and IB_SPEED_QDR.
> >>>>
> >>>> When the RoCE port is down, the RoCE port did not negotiate the
> >>>> active width with remote side. The active width is zero. If run
> >>>> ibstat to require the port status, ibstat will panic as it read
> >>>> invalid width from sys file.
> >>>>
> >>>> Signed-off-by: Honggang Li <honli@redhat.com>
> >>>> ---
> >>>> drivers/infiniband/core/sysfs.c | 15 +++++++++++----
> >>>> 1 file changed, 11 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> >>>> index cf36ff1f0068..722e4571f4d2 100644
> >>>> --- a/drivers/infiniband/core/sysfs.c
> >>>> +++ b/drivers/infiniband/core/sysfs.c
> >>>> @@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
> >>>> struct ib_port_attr attr;
> >>>> char *speed = "";
> >>>> int rate; /* in deci-Gb/sec */
> >>>> + int width;
> >>>> ssize_t ret;
> >>>>
> >>>> ret = ib_query_port(p->ibdev, p->port_num, &attr);
> >>>> @@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
> >>>> break;
> >>>> }
> >>>>
> >>>> - rate *= ib_width_enum_to_int(attr.active_width);
> >>>> - if (rate < 0)
> >>>> - return -EINVAL;
> >>>> + width = ib_width_enum_to_int(attr.active_width);
> >>>> + if (width < 0) {
> >>>> + if (attr.state != IB_PORT_ACTIVE)
> >>>
> >>> Link width is valid in any PortState other than Down so I think that
> >>> this check should be:
> >>> if (attr.state != IB_PORT_DOWN)
> >>>
> >>> However, I don't think overriding width should be needed for this case
> >>> and just returning -EINVAL should be fine regardless of port state.
> >>> AFAIK it's the driver responsibility to populate acceptable defaults for
> >>> such parameters. What driver(s) have this issue ? Shouldn't it be fixed
> >>> there rather than here ?
> >>
> >> I just noticed that you reference commit f1b65df5a232 ("IB/mlx5: Add
> >> support for active_width and active_speed in RoCE"). Before this patch
> >> applied, the mlx5_ib driver set default active_width and active_speed to
> >> IB_WIDTH_4X and IB_SPEED_QDR.
> >>
> >> Should the fix be to hw/mlx5/main.c:translate_eth_proto_oper which now has:
> >>
> >> switch (eth_proto_oper) {
> >> ...
> >> default:
> >> return -EINVAL;
> >> }
> >>
> >> return 0;
> >>
> >> and change default case to:
> >> *active_width = IB_WIDTH_1X;
> >>
> >> ?
> >
> > I suggest to restore previous behavior before apply f1b65df5a232.
> >
> > diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> > index 033b6af90de9..0d73d2772d9b 100644
> > --- a/drivers/infiniband/hw/mlx5/main.c
> > +++ b/drivers/infiniband/hw/mlx5/main.c
> > @@ -388,6 +388,9 @@ static int mlx5_query_port_roce(struct ib_device *device, u8 port_num,
> > if (err)
> > goto out;
> >
> > + props->active_width = IB_WIDTH_4X;
> > + props->active_speed = IB_SPEED_QDR;
> > +
> > translate_eth_proto_oper(eth_prot_oper, &props->active_speed,
> > &props->active_width);
> >
>
> Sure; makes sense that it should preserve the original behavior for this
> case.
[PATCH] IB/mlx5: Set the default active rate and width to QDR and 4X
This new patch had been sent to upstream mailing list for review.
thanks
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] IB/core: Set speed string to SDR for invalid active rates
2018-03-15 9:02 ` [PATCH 1/2] IB/core: Set speed string to SDR for invalid active rates Honggang LI
@ 2018-03-19 17:53 ` Jason Gunthorpe
0 siblings, 0 replies; 10+ messages in thread
From: Jason Gunthorpe @ 2018-03-19 17:53 UTC (permalink / raw)
To: Honggang LI; +Cc: dledford, linux-rdma, noaos, linux-kernel
On Thu, Mar 15, 2018 at 05:02:13PM +0800, Honggang LI wrote:
> From: Honggang Li <honli@redhat.com>
>
> commit f1b65df5a232 ("IB/mlx5: Add support for active_width and
> active_speed in RoCE"). Before this patch applied, the mlx5_ib
> driver set default active_width and active_speed to IB_WIDTH_4X
> and IB_SPEED_QDR.
>
> Now, the active_width and active_speed are zeros if the RoCE port
> is in DOWN state. The speed string should be set to " SDR" instead of
> a blank string when active_speed is zero.
>
> Signed-off-by: Honggang Li <honli@redhat.com>
> drivers/infiniband/core/sysfs.c | 1 +
> 1 file changed, 1 insertion(+)
Applied to for-next, thanks
Jason
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-19 17:53 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-15 9:02 [PATCH 0/2] IB/core: Set invalid active widths to 1X when port is not active Honggang LI
2018-03-15 9:02 ` [PATCH 1/2] IB/core: Set speed string to SDR for invalid active rates Honggang LI
2018-03-19 17:53 ` Jason Gunthorpe
2018-03-15 9:02 ` [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down Honggang LI
2018-03-15 12:01 ` Hal Rosenstock
2018-03-15 12:27 ` Honggang LI
2018-03-15 12:32 ` Hal Rosenstock
2018-03-15 12:43 ` Honggang LI
2018-03-15 12:47 ` Hal Rosenstock
2018-03-16 2:38 ` Honggang LI
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).