[PATCH 0/2]: fixes to port query and sysfs in 3.4-rc1

[PATCH 0/2]: fixes to port query and sysfs in 3.4-rc1

[Or Gerlitz]

Or Gerlitz (2):
  IB/mlx4: fix the case of invalid speed value returned when the port is down
  IB/core: add missing string for the display of SDR rates in sysfs

 drivers/infiniband/core/sysfs.c   |    1 +
 drivers/infiniband/hw/mlx4/main.c |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

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

[PATCH 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Or Gerlitz]

When the IB port is down, the active_speed value returned by the MAD_IFC
command equals seven (7) which isn't among the IB speeds defined by the
ib_port_speed enum. This results in invalid speed value seen by higher
layers or applications who do port query. Fix that by setting the speed
to be SDR - the lowest possible when the port is down.
---
 drivers/infiniband/hw/mlx4/main.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 75d3056..b727094 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -253,6 +253,10 @@ static int ib_link_query_port(struct ib_device *ibdev, u8 port,
 		if (out_mad->data[15] & 0x1)
 			props->active_speed = IB_SPEED_FDR10;
 	}
+
+	/* avoid wrong speed value returned by FW if the IB link is down */
+	if (props->state == IB_PORT_DOWN)
+		 props->active_speed = IB_SPEED_SDR;
 out:
 	kfree(in_mad);
 	kfree(out_mad);
-- 
1.7.1

--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Hal Rosenstock]

On 4/2/2012 10:45 AM, Or Gerlitz wrote:
> When the IB port is down, the active_speed value returned by the MAD_IFC
> command equals seven (7) which isn't among the IB speeds defined by the
> ib_port_speed enum. This results in invalid speed value seen by higher
> layers or applications who do port query. Fix that by setting the speed
> to be SDR - the lowest possible when the port is down.
> ---
>  drivers/infiniband/hw/mlx4/main.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 75d3056..b727094 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -253,6 +253,10 @@ static int ib_link_query_port(struct ib_device *ibdev, u8 port,
>  		if (out_mad->data[15] & 0x1)
>  			props->active_speed = IB_SPEED_FDR10;
>  	}
> +
> +	/* avoid wrong speed value returned by FW if the IB link is down */
> +	if (props->state == IB_PORT_DOWN)
> +		 props->active_speed = IB_SPEED_SDR;

Rather than always overwriting active_speed in this case, wouldn't it be
better to only do that for invalid values ?

-- Hal

>  out:
>  	kfree(in_mad);
>  	kfree(out_mad);

--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Or Gerlitz]

On 4/2/2012 7:35 PM, Hal Rosenstock wrote:
> Rather than always overwriting active_speed in this case, wouldn't it 
> be better to only do that for invalid values?

Yes, I have thought about that, however, spotting invalid values would 
make the code a bit ugly, so I took this approach, Roland?

Or.
--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Hal Rosenstock]

On 4/2/2012 12:47 PM, Or Gerlitz wrote:
> On 4/2/2012 7:35 PM, Hal Rosenstock wrote:
>> Rather than always overwriting active_speed in this case, wouldn't it
>> be better to only do that for invalid values?
> 
> Yes, I have thought about that, however, spotting invalid values would
> make the code a bit ugly, 

The code for this already exists inside of sysfs.c:rate_show anyhow and
just needs to be made into a global validation routine so mlx4/main.c
can access it. Only a slightly larger patch...

-- Hal

> so I took this approach, Roland?
> 
> Or.
> 

--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Hefty, Sean]

> On 4/2/2012 7:35 PM, Hal Rosenstock wrote:
> > Rather than always overwriting active_speed in this case, wouldn't it
> > be better to only do that for invalid values?
> 
> Yes, I have thought about that, however, spotting invalid values would
> make the code a bit ugly, so I took this approach, Roland?

Does the active speed even make sense if the port is down?  If not, should we assume that any value returned by the FW is correct?
--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Hal Rosenstock]

On 4/2/2012 2:39 PM, Hefty, Sean wrote:
>> On 4/2/2012 7:35 PM, Hal Rosenstock wrote:
>>> Rather than always overwriting active_speed in this case, wouldn't it
>>> be better to only do that for invalid values?
>>
>> Yes, I have thought about that, however, spotting invalid values would
>> make the code a bit ugly, so I took this approach, Roland?
> 
> Does the active speed even make sense if the port is down?  

When a port is DOWN, the only PortInfo components that are not
vendor dependent are PortState and PortPhysicalState:

IBA 1.2.1 vol 1 p.830 line 16 states:
C14-24.2.1: If PortInfo:Portstate=Down, then
• a SubnGet(PortInfo) shall produce valid data for PortInfo:PortState
and PortInfo:PortPhysicalState; whether any other component has
valid data is vendor-dependent

> If not, should we assume that any value returned by the FW is correct?

A reserved value should never be returned though so not all values are
"correct". Of course, it has limited meaning in this state.

-- Hal

> --
> 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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Jason Gunthorpe]

On Mon, Apr 02, 2012 at 06:39:36PM +0000, Hefty, Sean wrote:
> > On 4/2/2012 7:35 PM, Hal Rosenstock wrote:
> > > Rather than always overwriting active_speed in this case, wouldn't it
> > > be better to only do that for invalid values?
> > 
> > Yes, I have thought about that, however, spotting invalid values would
> > make the code a bit ugly, so I took this approach, Roland?
> 
> Does the active speed even make sense if the port is down?  If not,
> should we assume that any value returned by the FW is correct?

Force returning SDR makes sense to me. The link width probably has a
similar problem too... Returning EINVAL from any sysfs query is really
unfriendly.

Jason
--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Roland Dreier]

On Mon, Apr 2, 2012 at 7:45 AM, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> When the IB port is down, the active_speed value returned by the MAD_IFC
> command equals seven (7) which isn't among the IB speeds defined by the
> ib_port_speed enum. This results in invalid speed value seen by higher
> layers or applications who do port query. Fix that by setting the speed
> to be SDR - the lowest possible when the port is down.

This looks reasonable, applied.
--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Hal Rosenstock]

On 4/2/2012 3:27 PM, Roland Dreier wrote:
> On Mon, Apr 2, 2012 at 7:45 AM, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>> When the IB port is down, the active_speed value returned by the MAD_IFC
>> command equals seven (7) which isn't among the IB speeds defined by the
>> ib_port_speed enum. This results in invalid speed value seen by higher
>> layers or applications who do port query. Fix that by setting the speed
>> to be SDR - the lowest possible when the port is down.
> 
> This looks reasonable, applied.

How about validating the speed in mlx4 before overwriting it ? Would you
take such a patch ?

-- Hal

> --
> 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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Roland Dreier]

On Mon, Apr 2, 2012 at 12:39 PM, Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> How about validating the speed in mlx4 before overwriting it ? Would you
> take such a patch ?

I don't think so... what does speed even mean when we're reporting the
link is down?
Do we gain anything from that check?

 - R.
--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Hal Rosenstock]

On 4/2/2012 3:41 PM, Roland Dreier wrote:
> On Mon, Apr 2, 2012 at 12:39 PM, Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
>> How about validating the speed in mlx4 before overwriting it ? Would you
>> take such a patch ?
> 
> I don't think so... what does speed even mean when we're reporting the
> link is down?

It has no meaning but should be a valid not reserved speed.

> Do we gain anything from that check?

It's a small gain. We preserve the original FW value when possible which
means we don't need to remember that this might have be changed by the
kernel if some question comes up in the field. Many mlx4
devices/versions do the right thing here and don't return a reserved value.

-- Hal

>  - R.
> 

--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Or Gerlitz]

On 4/2/2012 10:27 PM, Roland Dreier wrote:
> This looks reasonable, applied

Thanks, so we're in a situation where there are / will be fixes for 3.4 
and new code for 3.5 is coming, how about that suggestion I made lately 
-  for-next to contain code for the next kernel (e.g 3.5 at this point) 
and have a different branch (maybe rc-fixes) to track fixes for the 
current kernel (e.g 3.4)?

Or.

--
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 1/2] IB/mlx4: fix the case of invalid speed value returned when the port is down

[Roland Dreier]

On Tue, Apr 3, 2012 at 11:38 PM, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> On 4/2/2012 10:27 PM, Roland Dreier wrote:
>>
>> This looks reasonable, applied
>
>
> Thanks, so we're in a situation where there are / will be fixes for 3.4 and
> new code for 3.5 is coming, how about that suggestion I made lately -
>  for-next to contain code for the next kernel (e.g 3.5 at this point) and
> have a different branch (maybe rc-fixes) to track fixes for the current
> kernel (e.g 3.4)?

Yes, I am planning on doing that, and also never rebasing my for-next branch.

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

[PATCH 2/2] IB/core: add missing string for the display of SDR rates in sysfs

[Or Gerlitz]

commits 2e96691c "IB: Use central enum for speed instead of hard-coded values"
and  e9319b0cb "IB/core: Fix SDR rates in sysfs" still didn't fill in the "SDR"
string in the SDR switch case, fix that.
---
 drivers/infiniband/core/sysfs.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index 83b720e..1611569 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -188,6 +188,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 
 	switch (attr.active_speed) {
 	case IB_SPEED_SDR:
+		speed = " SDR";
 		rate = 25;
 		break;
 	case IB_SPEED_DDR:
-- 
1.7.1

--
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 2/2] IB/core: add missing string for the display of SDR rates in sysfs

[Roland Dreier]

On Mon, Apr 2, 2012 at 7:45 AM, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>        switch (attr.active_speed) {
>        case IB_SPEED_SDR:
> +               speed = " SDR";
>                rate = 25;
>                break;
>        case IB_SPEED_DDR:

I don't think we want this -- old kernels didn't print "SDR" so I think
we shouldn't change the interface at this point.

 - R.
--
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 2/2] IB/core: add missing string for the display of SDR rates in sysfs

[Or Gerlitz]

On Mon, Apr 2, 2012 at 8:42 PM, Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> On Mon, Apr 2, 2012 at 7:45 AM, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> >        switch (attr.active_speed) {
> >        case IB_SPEED_SDR:
> > +               speed = " SDR";
> >                rate = 25;
> >                break;
> >        case IB_SPEED_DDR:
>
> I don't think we want this -- old kernels didn't print "SDR" so I think
> we shouldn't change the interface at this point.

In other words, you want this fix for 3.5 and not 3.4 as this being
non-regression? its obviously a fix, since for SDR you just see emtpy
string.

Or.
--
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 2/2] IB/core: add missing string for the display of SDR rates in sysfs

[Or Gerlitz]

On Mon, Apr 2, 2012 at 8:42 PM, Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Apr 2, 2012 at 7:45 AM, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> >        switch (attr.active_speed) {
> >        case IB_SPEED_SDR:
> > +               speed = " SDR";
> >                rate = 25;
> >                break;
> >        case IB_SPEED_DDR:
>
> I don't think we want this -- old kernels didn't print "SDR" so I think
> we shouldn't change the interface at this point.

Oh, I see what you mean - and I don't think we need to consider this
as interface change - these is an interface - namely "nGbs (mX DDD)
where n is number  m is number and DDD is string (SDR, DDR, QDR,
FDR10, etc) and this interface was buggy to wrong print certain cases,
which we fixed in the two commits I mentioned. Now we found another
bug ("SDR" not printed) and we fix it (hopefully the last one in this
function) - that's all. I don't think we should assume there are
apps/libs up their who set their code to match the exact bugs we had
in sysfs, it was just not important enough the report on, or they
didn't hit it (e.g SDR is pretty not common now), thoughts?

Or.
--
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 2/2] IB/core: add missing string for the display of SDR rates in sysfs

[Roland Dreier]

On Mon, Apr 2, 2012 at 11:50 AM, Or Gerlitz <or.gerlitz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Oh, I see what you mean - and I don't think we need to consider this
> as interface change - these is an interface - namely "nGbs (mX DDD)
> where n is number  m is number and DDD is string (SDR, DDR, QDR,
> FDR10, etc) and this interface was buggy to wrong print certain cases,
> which we fixed in the two commits I mentioned. Now we found another
> bug ("SDR" not printed) and we fix it (hopefully the last one in this
> function) - that's all. I don't think we should assume there are
> apps/libs up their who set their code to match the exact bugs we had
> in sysfs, it was just not important enough the report on, or they
> didn't hit it (e.g SDR is pretty not common now), thoughts?

I really don't think there's a good reason to change this output (which
we had for many years).  Maybe nothing cares either way but the
benefit is so minimal that I don't think the risk is worth it.
--
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

[PATCH] IB/core: Don't return EINVAL from sysfs rate attribute for invalid speeds

[Roland Dreier]

From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>

Commit e9319b0cb00d ("IB/core: Fix SDR rates in sysfs") changed our
sysfs rate attribute to return EINVAL to userspace if the underlying
device driver returns an invalid rate.  Apparently some drivers do this
when the link is down and some userspace pukes if it gets an error when
reading this attribute, so avoid a regression by not return an error to
match the old code.

Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
I think I'd rather just add this, to get back closer to the original
behavior even for non-fixed drivers (but I'll still merge the mlx4
patch, since that makes sense too).

 drivers/infiniband/core/sysfs.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index 83b720e..246fdc1 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -179,7 +179,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 {
 	struct ib_port_attr attr;
 	char *speed = "";
-	int rate = -1;		/* in deci-Gb/sec */
+	int rate;		/* in deci-Gb/sec */
 	ssize_t ret;
 
 	ret = ib_query_port(p->ibdev, p->port_num, &attr);
@@ -187,9 +187,6 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		return ret;
 
 	switch (attr.active_speed) {
-	case IB_SPEED_SDR:
-		rate = 25;
-		break;
 	case IB_SPEED_DDR:
 		speed = " DDR";
 		rate = 50;
@@ -210,6 +207,10 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		speed = " EDR";
 		rate = 250;
 		break;
+	case IB_SPEED_SDR:
+	default:		/* default to SDR for invalid rates */
+		rate = 25;
+		break;
 	}
 
 	rate *= ib_width_enum_to_int(attr.active_width);
-- 
1.7.9.1

--
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/core: Don't return EINVAL from sysfs rate attribute for invalid speeds

[Or Gerlitz]

On Mon, Apr 2, 2012 at 10:35 PM, Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> I think I'd rather just add this, to get back closer to the original
> behavior even for non-fixed drivers (but I'll still merge the mlx4
> patch, since that makes sense too).

okay, makes sense
--
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/core: Don't return EINVAL from sysfs rate attribute for invalid speeds

[Hal Rosenstock]

On 4/2/2012 3:35 PM, Roland Dreier wrote:
> From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> 
> Commit e9319b0cb00d ("IB/core: Fix SDR rates in sysfs") changed our
> sysfs rate attribute to return EINVAL to userspace if the underlying
> device driver returns an invalid rate.  Apparently some drivers do this
> when the link is down and some userspace pukes if it gets an error when
> reading this attribute, so avoid a regression by not return an error to
> match the old code.
> 
> Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> ---
> I think I'd rather just add this, to get back closer to the original
> behavior even for non-fixed drivers (but I'll still merge the mlx4
> patch, since that makes sense too).
> 
>  drivers/infiniband/core/sysfs.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> index 83b720e..246fdc1 100644
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -179,7 +179,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>  {
>  	struct ib_port_attr attr;
>  	char *speed = "";
> -	int rate = -1;		/* in deci-Gb/sec */
> +	int rate;		/* in deci-Gb/sec */
>  	ssize_t ret;
>  
>  	ret = ib_query_port(p->ibdev, p->port_num, &attr);
> @@ -187,9 +187,6 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>  		return ret;
>  
>  	switch (attr.active_speed) {
> -	case IB_SPEED_SDR:
> -		rate = 25;
> -		break;
>  	case IB_SPEED_DDR:
>  		speed = " DDR";
>  		rate = 50;
> @@ -210,6 +207,10 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>  		speed = " EDR";
>  		rate = 250;
>  		break;
> +	case IB_SPEED_SDR:
> +	default:		/* default to SDR for invalid rates */
> +		rate = 25;
> +		break;
>  	}
>  
>  	rate *= ib_width_enum_to_int(attr.active_width);

Should there be a similar change to ib_width_enum_to_int implementation
in ib_verbs.h and then rate < 0 returning -EINVAL here can be removed ?

-- Hal
--
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/core: Don't return EINVAL from sysfs rate attribute for invalid speeds

[Bart Van Assche]

> From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> 
> Commit e9319b0cb00d ("IB/core: Fix SDR rates in sysfs") changed our
> sysfs rate attribute to return EINVAL to userspace if the underlying
> device driver returns an invalid rate.  Apparently some drivers do this
> when the link is down and some userspace pukes if it gets an error when
> reading this attribute, so avoid a regression by not return an error to
> match the old code.
> 
> Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> ---
> I think I'd rather just add this, to get back closer to the original
> behavior even for non-fixed drivers (but I'll still merge the mlx4
> patch, since that makes sense too).
> 
>  drivers/infiniband/core/sysfs.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> index 83b720e..246fdc1 100644
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -179,7 +179,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>  {
>  	struct ib_port_attr attr;
>  	char *speed = "";
> -	int rate = -1;		/* in deci-Gb/sec */
> +	int rate;		/* in deci-Gb/sec */
>  	ssize_t ret;
>  
>  	ret = ib_query_port(p->ibdev, p->port_num, &attr);
> @@ -187,9 +187,6 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>  		return ret;
>  
>  	switch (attr.active_speed) {
> -	case IB_SPEED_SDR:
> -		rate = 25;
> -		break;
>  	case IB_SPEED_DDR:
>  		speed = " DDR";
>  		rate = 50;
> @@ -210,6 +207,10 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
>  		speed = " EDR";
>  		rate = 250;
>  		break;
> +	case IB_SPEED_SDR:
> +	default:		/* default to SDR for invalid rates */
> +		rate = 25;
> +		break;
>  	}
>  
>  	rate *= ib_width_enum_to_int(attr.active_width);
> -- 
> 1.7.9.1

Tested-by: Bart Van Assche <bvanassche-HInyCGIudOg@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