linux-kernel - Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 28 Oct 2017 07:59:01 -0400
From:   Hal Rosenstock <hal@....mellanox.co.il>
To:     Thomas Bogendoerfer <tbogendoerfer@...e.de>
Cc:     Ghazale Hosseinabadi <ghazale.hosseinabadi@...cle.com>,
        linux-rdma@...r.kernel.org, matanb@...lanox.com,
        leonro@...lanox.com, parav@...lanox.com,
        linux-kernel@...r.kernel.org, dledford@...hat.com
Subject: Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged
 in SFP module

On 10/28/2017 6:42 AM, Thomas Bogendoerfer wrote:
>> I must be missing something as to what is going on in this scenario.

I think I see what's going on now. The -EINVAL in kernel sysfs/rate_show
causes error to either open or read of file. I had checked that an empty
file is parsed correctly but didn't consider an error on open or read.

>> sysfs.c:rate_show is inconsistent as it paves over an invalid speed
>> setting that to SDR but does not pave over invalid width returning
>> -EINVAL but this comment is in another "direction".
> I thought about going in the other direction by making the speed/width unknown case
> somehow visible in rate_show(). When I checked all other drivers only mlx5 stand out
> so I decided to only change mlx5. But I make a change to let rate_show() print out, 
> that is currently has no rate for the port.

I wasn't proposing to make that visible in rate_show() but rather
pointing out the inconsistency in changing unknown speeds to SDR but
unknown widths are error. There were comments on not having to set
"random" numbers for speed/width. If so, shouldn't these be handled
consistently here ? Would setting -EINVAL when not one of recognized
speeds cause an issue (rather than setting to SDR) ?

IMO a more future proof implementation is not to have error for
ib_width_enum_to_int but have unknown widths default to 1x similar to
how unknown speeds default to SDR:

include/rdma/ib_verbs.h:
static inline int ib_width_enum_to_int(enum ib_port_width width)
{
        switch (width) {
        case IB_WIDTH_1X:  return  1;
        case IB_WIDTH_4X:  return  4;
        case IB_WIDTH_8X:  return  8;
        case IB_WIDTH_12X: return 12;
        default:          return 1;
        }
}

For example, 2x link widths have been added to IBA spec.

This is alternative approach to libibumad/ibstat patches for invalid
link width as it's not set when QSFP is not plugged into port.

-- Hal