On Tue, May 5, 2009 at 10:45 AM, Eli Dorfman (Voltaire) <[email protected]> wrote: > Hal Rosenstock wrote: >> On Tue, May 5, 2009 at 9:48 AM, Eli Dorfman (Voltaire) >> <[email protected]> wrote: >>> Hal Rosenstock wrote: >>>> On Tue, May 5, 2009 at 9:00 AM, Doron Shoham <[email protected]> wrote: >>>>> when setting max_op_vls = 0 >>>>> do not force it to 1. >>>>> 0 is valid value which means "No change" >>>>> >>>>> Signed-off-by: Doron Shoham <[email protected]> >>>>> --- >>>>> opensm/opensm/osm_port.c | 6 ------ >>>>> opensm/opensm/osm_subnet.c | 8 ++++++++ >>>>> 2 files changed, 8 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/opensm/opensm/osm_port.c b/opensm/opensm/osm_port.c >>>>> index 2e6c642..db0c27e 100644 >>>>> --- a/opensm/opensm/osm_port.c >>>>> +++ b/opensm/opensm/osm_port.c >>>>> @@ -380,12 +380,6 @@ uint8_t osm_physp_calc_link_op_vls(IN osm_log_t * >>>>> p_log, >>>>> if (op_vls > p_subn->opt.max_op_vls) >>>>> op_vls = p_subn->opt.max_op_vls; >>>>> >>>>> - if (op_vls == 0) { >>>>> - OSM_LOG(p_log, OSM_LOG_DEBUG, "ERR 4102: " >>>>> - "Invalid OP_VLS = 0. Forcing correction to 1 >>>>> (VL0)\n"); >>>>> - op_vls = 1; >>>>> - } >>>>> - >>>> Should that only be done when max_op_vls is 0 ? >>>> >>>> Something like: >>>> if (op_vls > p_subn->opt.max_op_vls) >>>> op_vls = p_subn->opt.max_op_vls; >>>> else if (op_vls == 0) { >>>> OSM_LOG(p_log, OSM_LOG_DEBUG, "ERR 4102: " >>>> "Invalid OP_VLS = 0. Forcing correction to 1 >>>> (VL0)\n"); >>>> op_vls = 1; >>>> } >>> why do you suggest a special case for op_vls=0 (and not for other portinfo >>> fields)? >> >>> is there a firmware bug that reports op_vls=0? >> >> There were (still are ?) implementations which returned op_vls 0 which >> is why the words "valid on Set()" were added to the IBA spec and why I >> don't feel safe removing the code as originally proposed but think my >> alternative is safe and accomplishes the stated goal. Is there a >> problem with my alternative proposal ? > > no, but there are other fields in portinfo that are not validated.
Yes, there's some inconsistency here but it's based on field experience. > for example link_speed_enabled (which allows 0 value only on Set as well). Yes, but this field had the specific issue I noted and 0 being returned on get was never observed on any of the other fields where 0 is valid on set (added there as well). > also if a node returns op_vl=0 how do you know it supports op_vl=1? op_vls 1 is always safe as at least 1 data VL must be supported. It's just that possibly more op vls could have been supported if things had been compliant. -- Hal > Eli _______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
