netdev - Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal PHY

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:   Mon, 11 Jan 2021 15:10:20 +0100
From:   Marek Vasut <marex@...x.de>
To:     Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>, Lukas Wunner <lukas@...ner.de>
Subject: Re: [PATCH net-next] net: ks8851: Connect and start/stop the internal
 PHY

On 1/11/21 2:50 PM, Heiner Kallweit wrote:
> On 11.01.2021 14:38, Marek Vasut wrote:
>> On 1/11/21 2:26 PM, Heiner Kallweit wrote:
>> [...]
>>
>>> LGTM. When having a brief look at the driver I stumbled across two things:
>>>
>>> 1. Do MAC/PHY support any pause mode? Then a call to
>>>      phy_support_(a)sym_pause() would be missing.
>>
>> https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8851-16MLL-Single-Port-Ethernet-MAC-Controller-with-8-Bit-or-16-Bit-Non-PCI-Interface-DS00002357B.pdf
>> page 64
>>
>> https://www.mouser.com/datasheet/2/268/ksz8851-16mll_ds-776208.pdf
>> page 65
>>
>> The later is more complete.
>>
>> Apparently it does support pause.

Based on the datasheet, does it support sym or asym pause ?

>>> 2. Don't have the datasheet, but IRQ_LCI seems to be the link change
>>>      interrupt. So far it's ignored by the driver. You could configure
>>>      it and use phy_mac_interrupt() to operate the internal PHY in
>>>      interrupt mode.
>>
>> That's only for link state change, shouldn't the PHY interrupt trigger on other things as well ?
> 
> No, it's sufficient if the interrupt can signal link state change.
> In r8169 I have exactly that case.

I'll do that in a subsequent patch, once I verify it works as it should.