netdev - Re: ethtool fails to read some QSFP+ modules.

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8711f66b-9461-4e3c-8881-5bda2379b926@ans.pl>
Date: Thu, 4 Jul 2024 05:27:49 -0700
From: Krzysztof Olędzki <ole@....pl>
To: Ido Schimmel <idosch@...sch.org>, Dan Merillat <git@....merillat.org>
Cc: Michal Kubecek <mkubecek@...e.cz>, netdev <netdev@...r.kernel.org>
Subject: Re: ethtool fails to read some QSFP+ modules.

On 03.07.2024 at 07:36, Krzysztof Olędzki wrote:
> Good morning,
> 
> On 01.07.2024 at 00:41, Ido Schimmel wrote:
>> Forgot to add Krzysztof :p
>>
>> On Mon, Jul 01, 2024 at 10:28:39AM +0300, Ido Schimmel wrote:
>>> On Sun, Jun 30, 2024 at 01:27:07PM -0400, Dan Merillat wrote:
>>>>
>>>> I was testing an older Kaiam XQX2502 40G-LR4 and ethtool -m failed with netlink error.  It's treating a failure to read
>>>> the optional page3 data as a hard failure.
>>>>
>>>> This patch allows ethtool to read qsfp modules that don't implement the voltage/temperature alarm data.
>>>
>>> Thanks for the report and the patch. Krzysztof Olędzki reported the same
>>> issue earlier this year:
>>> https://lore.kernel.org/netdev/9e757616-0396-4573-9ea9-3cb5ef5c901a@ans.pl/
>>>
>>> Krzysztof, are you going to submit the ethtool and mlx4 patches?
> 
> Yes, and I apologize for the delay - I have been traveling with my family and was unable to get into it.
> 
> I should be able to work on the patches later this week, so please expect something from me around the weekend.
> 
>>>> From 3144fbfc08fbfb90ecda4848fc9356bde8933d4a Mon Sep 17 00:00:00 2001
>>>> From: Dan Merillat <git@....eginity.com>
>>>> Date: Sun, 30 Jun 2024 13:11:51 -0400
>>>> Subject: [PATCH] Some qsfp modules do not support page 3
>>>>
>>>> Tested on an older Kaiam XQX2502 40G-LR4 module.
>>>> ethtool -m aborts with netlink error due to page 3
>>>> not existing on the module. Ignore the error and
>>>> leave map->page_03h NULL.

BTW - the code change does what we have discussed, but I think the comment
may be incorrect? Before we call nl_get_eeprom_page, there is a check to
verify that Page 03h is present:

        /* Page 03h is only present when the module memory model is paged and
         * not flat.
         */
        if (map->lower_memory[SFF8636_STATUS_2_OFFSET] &
            SFF8636_STATUS_PAGE_3_PRESENT)
                return 0;

In this case, it seems to me that the failure can only be caused by either
HW issues (NIC or SFP) *or* a bug in the driver. Assuming we want to provide
some details in the code, maybe something like this may be better?

+	/* Page 03h is not available due to either HW issue or a bug
+	 * in the driver. This is a non-fatal error and sff8636_dom_parse()
+	 * handles this correctly.
+	 */

We were also discussing if printing a warning in such situation may make sense.
As I was thinking about this more, I wonder if we can just use the same check
in sff8636_show_dom() and if map->page_03h is NULL print for
 "Alarm/warning flags implemented"
something like:
 "Failed (Page 03h access error, HW issue or kernel driver bug?)"

We would get it in addition to "netlink error: Invalid argument" that comes from:
./netlink/nlsock.c:             perror("netlink error");

>>> User space only tries to read this page because the module advertised it
>>> as supported. It is more likely that the NIC driver does not return all
>>> the pages. Which driver is it?
>>

Krzysztof