[ofa-general] [PATCH] osm_port.c: do not force max_op_vls = 0 to 1

[Hal Rosenstock]

On Tue, May 5, 2009 at 10:45 AM, Eli Dorfman (Voltaire)
<dorfman.eli at gmail.com> wrote:
> Hal Rosenstock wrote:
>> On Tue, May 5, 2009 at 9:48 AM, Eli Dorfman (Voltaire)
>> <dorfman.eli at gmail.com> wrote:
>>> Hal Rosenstock wrote:
>>>> On Tue, May 5, 2009 at 9:00 AM, Doron Shoham <dorons at voltaire.com> 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 <dorons at voltaire.com>
>>>>> ---
>>>>>  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