linux-kernel - Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

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: <f5c42936-98a6-8221-a244-ed61840c9c81@ti.com>
Date:   Tue, 18 Feb 2020 11:38:33 -0600
From:   Dan Murphy <dmurphy@...com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     Grygorii Strashko <grygorii.strashko@...com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>, <andrew@...n.ch>,
        <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization
 feature

Russell

On 2/18/20 11:33 AM, Russell King - ARM Linux admin wrote:
> On Tue, Feb 18, 2020 at 11:12:37AM -0600, Dan Murphy wrote:
>> Well now the read_status is becoming a lot more complex.  It would be better
>> to remove the ack_interrupt call back and just have read_status call the
>> interrupt function regardless of interrupts or not.  Because all the
>> interrupt function would be doing is clearing all the interrupts in the ISR
>> register on a link up/down event.  And as you pointed out we can check and
>> set a flag that indicates if a downshift has happened on link up status and
>> clear it on link down. We would need to set the downshift interrupt mask to
>> always report that bit.  As opposed to not setting any interrupts if
>> interrupts are not enabled.  If the user wants to track WoL interrupt or any
>> other feature interrupt we would have to add that flag to the read_status as
>> well seems like it could get a bit out of control.
> To be honest, I don't like phylib's interrupt implementation; it
> imposes a fixed way of dealing with interrupts on phylib drivers,
> and it sounds like its ideas don't match what modern PHYs want.
>
> For example, the Marvell 88x3310 can produce interrupts for GPIOs
> that are on-board, or temperature alarms, or other stuff... but
> phylib's idea is that a PHY interrupt is for signalling that the
> link changed somehow, and imposes a did_interrupt(), handle
> interrupt, clear_interrupt() structure whether or not it's
> suitable for the PHY.  If you don't provide the functions phylib
> wants, then phydev->irq gets killed.  Plus, phylib claims the
> interrupt for you at certain points depending on whether the
> PHY is bound to a network interface or not.
>
> So, my suggestion to move ack_interrupt to did_interrupt will
> result in phylib forcing phydev->irq to PHY_POLL, killing any
> support for interrupts what so ever...

Does it really make sense to do all of this for a downshift message in 
the kernel log?

>> Again this is a lot of error prone complex changes and tracking just to
>> populate a message in the kernel log.  There is no guarantee that the LP did
>> not force the downshift or advertise that it supports 1Gbps.  So what
>> condition is really being reported here?  There seems like there are so many
>> different scenarios why the PHY could not negotiate to its advertised 1Gbps.
> Note that when a PHY wants to downshift, it does that by changing its
> advertisement that is sent to the other PHY.
>
> So, if both ends support 1G, 100M and 10M, and the remote end decides
> to downshift to 100M, the remote end basically stops advertising 1G
> and restarts negotiation afresh with the new advertisement.
>
> In that case, if you look at ethtool's output, it will indicate that
> the link partner is not advertising 1G, and the link is operating at
> 100M.
>
> If 100M doesn't work (and there are cases where connections become
> quite flakey) then 100M may also be omitted as well, negotiation
> restarted, causing a downshift to 10M.
>
> That's basically how downshift works - a PHY will attempt to
> establish link a number of times before deciding to restart
> negotiation with some of the advertisement omitted, and the
> reduced negotiation advertisement appears in the remote PHY's
> link partner registers.
This is the way I understand it too.
> As I mentioned, the PHY on either end of the link can be the one
> which decides to downshift, and the partner PHY has no idea that
> a downshift has happened.
>
Exactly so we can only report that if the PHY on our end caused the 
downshift.  If this PHY does not cause the downshift then the message 
will not be presented even though a downshift occurred. So what is the 
value in presenting this message?