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] [day] [month] [year] [list]
Message-ID: <ebbb0690-13d1-452d-95d7-5ce61bb8a0e8@ans.pl>
Date: Sun, 7 Jul 2024 20:42:48 -0700
From: Krzysztof Olędzki <ole@....pl>
To: Ido Schimmel <idosch@...sch.org>
Cc: Dan Merillat <git@....merillat.org>, Michal Kubecek <mkubecek@...e.cz>,
        netdev <netdev@...r.kernel.org>
Subject: Re: ethtool fails to read some QSFP+ modules.

On 07.07.2024 at 01:35, Ido Schimmel wrote:
> On Thu, Jul 04, 2024 at 05:27:49AM -0700, Krzysztof Olędzki wrote:
>> 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.
>> +	 */
> 
> Looks fine to me although I believe an error in this case will always be
> returned because of a driver bug. Reading a page that does not exist
> should not result in an error, but in the module returning Upper Page
> 00h. Yet to encounter a module that works in a different way. From
> SFF-8636 Section 6.1:
> 
> "Writing the value of a non-supported page shall not be accepted by the
> slave. The Page Select byte shall revert to 0h and read/write operations
> shall be to Upper Page 00h. Because Upper Page 00h is read-only, this
> scheme prevents the inadvertent corruption of module memory by a host
> attempting to write to a non-supported location."

Fair, I updated the comment based on your feedback.

>>
>> 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");
> 
> I think it's better to print it in sff8636_memory_map_init_pages() as
> that way the user can more easily understand the reason for "netlink
> error: Invalid argument":
> 
> # ./ethtool -m swp13                                                                                                                                                                                                                                                                 
> netlink error: Invalid argument
> Failed to read Upper Page 03h
>         Identifier                                : 0x11 (QSFP28)
>         Extended identifier                       : 0xcf
> [...]

Great idea, done.


> BTW, just to be sure, you are going to post patches for both ethtool and
> mlx4, right?

Yes, I just sent both.
 
Krzysztof