netdev - Re: [PATCH net-next v2 3/3] net: phy: marvell10g: add SFP+ support

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: <20191117195957.GV25745@shell.armlinux.org.uk>
Date:   Sun, 17 Nov 2019 19:59:57 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/3] net: phy: marvell10g: add SFP+ support

On Sun, Nov 17, 2019 at 08:25:29PM +0100, Andrew Lunn wrote:
> > The answer is... it depends.
> 
> Hi Russell
> 
> One issue we have had with phylink is people using the interfaces
> wrongly. When asking this question, i was thinking about
> documentation. Your answer suggests this method is not simply about
> the validation you are doing here, it could also be about
> configuration of the PHY to fit the module.

Err.

Is that not what phylink_sfp_module_insert() is doing - validating that
the module can be supported by the MAC and setting the MAC up for it.

The implementation for marvell10g is no different - I'm not sure why
you're thinking something else is going on here.

> Maybe it would be good to add documentation somewhere about the range
> of things this call can do?
> 
> > So, this patch reflects what can be done with the SFP+ slots on
> > Macchiatobin boards today.
> 
> This all sounds very hardware dependent. So we are going to need some
> more DT properties i guess. Have you thought about this?

I don't see how DT properties help.

DT properties can't stop you plugging in a SFP module into a SFP+ slot
with too strong pullups and corrupting the EEPROM on the SFP module.

There's nothing that really tells you that the module is SFP or SFP+,
and if we wanted to guess, we'd need to read the EEPROM... but reading
the EEPROM might corrupt it - catch-22.

> Maybe we need to add compatibles sff,sfp+ and sff,sff+ ? Indicate the
> board is capable of the higher speeds? And when sfp+/sff+ is used,
> maybe a boolean to indicate it is also sff/sfp compatible?

I've had a patch like that hanging around for a few years.  I've yet to
find anything that could benefit from it or use it to make some kind of
decision in the code.

> sfp_select_interface() can then look at these properties and return
> PHY_INTERFACE_MODE_NA if the board is not capable of supporting the
> module?
> 
> Would it even make sense to make the PHY interface more like the MAC
> interface? A validate function to indicate what it is capable of? A
> configure function to configure its mode towards the SFP?

I don't see how any of that helps.  The problem is not whether the
PHY interface mode is supported it not - on the Macchiatobin DS, the
88x3310 PHY certainly supports 10GBASE-R and 1000BASE-X via the fiber
port.  So it's entirely possible to configure the 3310 to drive a
1G fiber SFP.

The problem on the Macchiatobin is the I2C pull-up resistors, which
either prevent a SFP module being readable or end up corrupting the
SFP module EEPROM in the process of trying (and probably failing) to
read it.  There is nothing the kernel can do to work around that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up