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

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

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