netdev - 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]

Date:   Tue, 18 Feb 2020 11:12:37 -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 10:49 AM, Russell King - ARM Linux admin wrote:
> On Tue, Feb 18, 2020 at 10:36:47AM -0600, Dan Murphy wrote:
>> Russell
>>
>> On 2/18/20 10:25 AM, Russell King - ARM Linux admin wrote:
>>> On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote:
>>>> Grygorii
>>>>
>>>> On 2/14/20 12:32 PM, Grygorii Strashko wrote:
>>>>> I think it's good idea to have this message as just wrong cable might be
>>>>> used.
>>>>>
>>>>> But this notifier make no sense in it current form - it will produce
>>>>> noise in case of forced 100m/10M.
>>>>>
>>>>> FYI. PHY sequence to update link:
>>>>> phy_state_machine()
>>>>> |-phy_check_link_status()
>>>>>     |-phy_link_down/up()
>>>>>       |- .phy_link_change()->phy_link_change()
>>>>>       |-adjust_link() ----> netdev callback
>>>>> |-phydev->drv->link_change_notify(phydev);
>>>>>
>>>>> So, log output has to be done or in .read_status() or
>>>>> some info has to be saved in .read_status() and then re-used in
>>>>> .link_change_notify().
>>>>>
>>>> OK I will try to find a way to give some sort of message.
>>> How do you know the speed that the PHY downshifted to?
>> The DP83867 has a register PHYSTS where BIT 15:14 indicate the speed that
>> the PHY negotiated.
>>
>> In the same register BIT 13 indicates the duplex mode.
>>
>>> If the speed and duplex are available in some PHY specific status
>>> register, then one way you can detect downshift is to decode the
>>> negotiated speed/duplex from the advertisements (specifically the LPA
>>> read from the registers and the advertisement that we should be
>>> advertising - some PHYs modify their registers when downshifting) and
>>> check whether it matches the negotiated parameters in the PHY
>>> specific status register.
>>>
>>> Alternatively, if the PHY modifies the advertisement register on
>>> downshift, comparing the advertisement register with what it should
>>> be will tell you if downshift has occurred.
>> The ISR register BIT 5 indicates if a downshift occurred or not. So we can
>> indicate that the PHY downshifted but there is no cause in the registers bit
>> field.  My concern for this bit though is the register is clear on read so
>> all other interrupts are lost if we only read to check downshift.  And the
>> link_change_notifier is called before the interrupt ACK call back.  We could
>> call the interrupt function and get the downshift status but again it will
>> clear the interrupt register and any other statuses may be lost.
> What's wrong with having an ack_interrupt() method that reads the
> PHY ISR register, and records in a driver private flag that bit 5
> has been set?  The read_status() method can clear the flag if link
> goes down, or check the flag if link is up and report that a
> downshift event occurred.
>
> If IRQs are not in use, then read_status() would have to read the
> ISR itself.
>
> It may be better to move ack_interrupt() to did_interrupt(), which
> will ensure that it gets executed before the PHY state machine is
> triggered by phy_interrupt().
>
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.

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.


Dan