netdev - Re: [PATCH net 1/2] s390/qeth: update cached link

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: <7735d444-5041-ccde-accc-5a69af2f2731@linux.ibm.com>
Date:   Fri, 5 Aug 2022 09:05:47 +0200
From:   Alexandra Winter <wintera@...ux.ibm.com>
To:     Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-s390@...r.kernel.org, Heiko Carstens <hca@...ux.ibm.com>,
        Thorsten Winkler <twinkler@...ux.ibm.com>
Subject: Re: [PATCH net 1/2] s390/qeth: update cached link_info for ethtool



On 04.08.22 22:27, Jakub Kicinski wrote:
> On Thu, 4 Aug 2022 15:08:11 +0200 Andrew Lunn wrote:
>> On Thu, Aug 04, 2022 at 10:53:33AM +0200, Alexandra Winter wrote:
>>> Thank you Andrew for the review. I fully understand your point.
>>> I would like to propose that I put that on my ToDo list and fix
>>> that in a follow-on patch to net-next.
>>>
>>> The fields in the link_info control blocks are used today to generate
>>> other values (e.g. supported speed) which will not work with *_UNKNOWN,
>>> so the follow-on patch will be more than just 2 lines.  
>>
>> So it sounds like your code is all backwards around. If you know what
>> the hardware is, you know the supported link modes are, assuming its
>> not an SFP and the SFP module is not plugged in. Those link modes
>> should be independent of if the link is up or not. speed/duplex is
>> only valid when the link is up and negotiation has finished.
> 
> To make sure I understand - the code depends on the speed and duplex
> being set to something specific when the device is _down_? Can this be
> spelled out more clearly in the commit message?
This was a discussion about existing code. We display default speed and
duplex even when the device is down. And this patch does not change that
behaviour. Andrew rightfully pointed out that this should (eventually) be
changed.
> 
>> Since this is for net, than yes, maybe it would be best to go with a
>> minimal patch to make your backwards around code work. But for
>> net-next, you really should fix this properly. 
> 
> Then again this patch doesn't look like a regression fix (and does not
> have a fixes tag). Channeling my inner Greg I'd say - fix this right and
> then worry about backports later. 
This patch is a pre-req for [PATCH net 2/2] s390/qeth: use cached link_info for ethtool
2/2 is the regression fix.
Guidance is welcome. Should I merge them into a single commit?
Or clarify in the commit message of 1/1 that this is a preparation for 2/2?