netdev - Re: FWD: [PATCH v2] Marvell phy: add fiber status check for some components

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:	Wed, 13 Apr 2016 11:27:21 +0200
From:	Charles-Antoine Couret <charles-antoine.couret@...vision.fr>
To:	Florian Fainelli <f.fainelli@...il.com>,
	Andrew Lunn <andrew@...n.ch>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: FWD: [PATCH v2] Marvell phy: add fiber status check for some
 components

Le 11/04/2016 21:47, Florian Fainelli a écrit :
> On 04/04/16 06:25, Andrew Lunn wrote:
>>> >From 564b767163d19355a3b5efaad195e93796570c71 Mon Sep 17 00:00:00 2001
>>> From: Charles-Antoine Couret <charles-antoine.couret@...vision.fr>
>>> Date: Fri, 1 Apr 2016 16:16:35 +0200
>>> Subject: [PATCH] Marvell phy: add fiber status check for some components
>>>
>>> Marvell's phy could have two modes: fiber and copper. Currently, the driver
>>> checks only the copper mode registers to get the status link which could be
>>> wrong.
>>>
>>> This commit add a handler to check fiber then copper status link.
>>> If the fiber link is activated, the driver would use this information.
>>> Else, it would use the copper status.
>>
>> Hi Florian
>>
>> What do you think about this?
>>
>> This works for basic status information. But what about other ethtool
>> options? Setting the speed and duplex, turning pause on/off, etc.
> 
> Agreed, it seems like a PHY configured for fiber will need to provide
> these informations differently, or does the standard BMSR register
> provide accurate information already?


The BSMR is similar between fiber and copper mode (but, it's the same values for duplex, speed...) in register 17.
To force speed, duplex, etc. it's the same way too (register 0).

The register's numbers are the same, only the page change.

>> Do we actually need to stay on page 1 if fibre is in use? How do we
>> initially change to page 1 when the fibre link is still down?
> 
> I also do not feel very comfortable with reading the fiber status first,
> and then copper and then combine these two. At the very best, could we
> do something like:
> 
> - identify if the PHY is configured for fiber in drv->probe or
> drv->config_init, retain that information
> - have two paths in drv->read_status which take care of reading one
> status or the other?

It should be a great idea.
Because, we could select the preferred link (copper or fiber). This option could select that during the init process.

I could save the state into marvell_priv structure, but how configure this choice ? Set copper by default and the user change that
by ethtool, driver loading option or the driver code ? I don't know what is the correct way to configure that properly. 


> Are there other side effects for other register accesses (say,
> statistics, or auto-negotiation) that need to be fiber vs. copper aware?

Auto-negociation are separated. But the statistics are globally common, there are some specific registers about that too. But I think these registers are not relevant.

I will improve my patch.
Thanks.

Regards.
Charles-Antoine Couret