[PATCH net-next 0/2] net: sfp: small improvements

[PATCH net-next 0/2] net: sfp: small improvements

[Antoine Tenart]

Hi Russell,

This series was part of the mvpp2 phylink one but as we reworked it to
use fixed-link on the DB boards, the SFP commits weren't needed
anymore for our use case. Two of the three patches still are needed I
believe (I ditched the one about non-wired SFP cages), so they are sent
here in a separate series.

Thanks!
Antoine

Since last time:
  - s/-EOPNOTSUPP/-ENODEV/ in patch 1/2.
  - I added the acked-by tag in patch 2/2.

Antoine Tenart (2):
  net: phy: sfp: make the i2c-bus property really optional
  net: phy: sfp: warn the user when no tx_disable pin is available

 drivers/net/phy/sfp.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

-- 
2.17.0

[PATCH net-next 1/2] net: phy: sfp: make the i2c-bus property really optional

[Antoine Tenart]

The SFF,SFP documentation is clear about making all the DT properties,
with the exception of the compatible, optional. In practice this is not
the case and without an i2c-bus property provided the SFP code will
throw NULL pointer exceptions.

This patch is an attempt to fix this.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/sfp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 4ab6e9a50bbe..0fd2a92a6f7b 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -298,11 +298,17 @@ static void sfp_set_state(struct sfp *sfp, unsigned int state)
 
 static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
 {
+	if (!sfp->read)
+		return -ENODEV;
+
 	return sfp->read(sfp, a2, addr, buf, len);
 }
 
 static int sfp_write(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
 {
+	if (!sfp->write)
+		return -ENODEV;
+
 	return sfp->write(sfp, a2, addr, buf, len);
 }
 
@@ -533,6 +539,8 @@ static int sfp_sm_mod_hpower(struct sfp *sfp)
 		return 0;
 
 	err = sfp_read(sfp, true, SFP_EXT_STATUS, &val, sizeof(val));
+	if (err == -ENODEV)
+		goto err;
 	if (err != sizeof(val)) {
 		dev_err(sfp->dev, "Failed to read EEPROM: %d\n", err);
 		err = -EAGAIN;
@@ -542,6 +550,8 @@ static int sfp_sm_mod_hpower(struct sfp *sfp)
 	val |= BIT(0);
 
 	err = sfp_write(sfp, true, SFP_EXT_STATUS, &val, sizeof(val));
+	if (err == -ENODEV)
+		goto err;
 	if (err != sizeof(val)) {
 		dev_err(sfp->dev, "Failed to write EEPROM: %d\n", err);
 		err = -EAGAIN;
@@ -565,6 +575,8 @@ static int sfp_sm_mod_probe(struct sfp *sfp)
 	int ret;
 
 	ret = sfp_read(sfp, false, 0, &id, sizeof(id));
+	if (ret == -ENODEV)
+		return ret;
 	if (ret < 0) {
 		dev_err(sfp->dev, "failed to read EEPROM: %d\n", ret);
 		return -EAGAIN;
-- 
2.17.0

Re: [PATCH net-next 1/2] net: phy: sfp: make the i2c-bus property really optional

[Andrew Lunn]

On Thu, May 17, 2018 at 10:29:06AM +0200, Antoine Tenart wrote:
> The SFF,SFP documentation is clear about making all the DT properties,
> with the exception of the compatible, optional. In practice this is not
> the case and without an i2c-bus property provided the SFP code will
> throw NULL pointer exceptions.
> 
> This patch is an attempt to fix this.

Hi Antoine, Russell

How usable is an SFF/SFP module without access to the i2c EEPROM? I
guess this comes down to link speed. Can it be manually configured?

I'm just wondering if we want to make this mandatory? Fail the probe
if it is not listed?

	Andrew

Re: [PATCH net-next 1/2] net: phy: sfp: make the i2c-bus property really optional

[Antoine Tenart]

Hi Andrew,

On Thu, May 17, 2018 at 02:41:28PM +0200, Andrew Lunn wrote:
> On Thu, May 17, 2018 at 10:29:06AM +0200, Antoine Tenart wrote:
> > The SFF,SFP documentation is clear about making all the DT properties,
> > with the exception of the compatible, optional. In practice this is not
> > the case and without an i2c-bus property provided the SFP code will
> > throw NULL pointer exceptions.
> > 
> > This patch is an attempt to fix this.
> 
> How usable is an SFF/SFP module without access to the i2c EEPROM? I
> guess this comes down to link speed. Can it be manually configured?
>
> I'm just wondering if we want to make this mandatory? Fail the probe
> if it is not listed?

Yes, the other option would be to fail when probing a cage missing the
i2c description. I'd say a passive module can work without the i2c
EEPROM accessible as it does not need to be configured. I don't know
what would happen with active ones.

So the question is, do we want to enable partially working SFP cages
(ie. probably working with only a subset of SFP modules)?

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 1/2] net: phy: sfp: make the i2c-bus property really optional

[Andrew Lunn]

On Thu, May 17, 2018 at 02:56:48PM +0200, Antoine Tenart wrote:
> Hi Andrew,
> 
> On Thu, May 17, 2018 at 02:41:28PM +0200, Andrew Lunn wrote:
> > On Thu, May 17, 2018 at 10:29:06AM +0200, Antoine Tenart wrote:
> > > The SFF,SFP documentation is clear about making all the DT properties,
> > > with the exception of the compatible, optional. In practice this is not
> > > the case and without an i2c-bus property provided the SFP code will
> > > throw NULL pointer exceptions.
> > > 
> > > This patch is an attempt to fix this.
> > 
> > How usable is an SFF/SFP module without access to the i2c EEPROM? I
> > guess this comes down to link speed. Can it be manually configured?
> >
> > I'm just wondering if we want to make this mandatory? Fail the probe
> > if it is not listed?
> 
> Yes, the other option would be to fail when probing a cage missing the
> i2c description. I'd say a passive module can work without the i2c
> EEPROM accessible as it does not need to be configured. I don't know
> what would happen with active ones.

Hi Antoine

I was thinking about how it reads the bit rate from the EEPROM. From
that it determines what mode the MAC could use, 1000-Base-X,
2500-Base-X, etc. Can you still configure this correctly via ethtool,
if you don't have the bitrate information?

   Andrew

Re: [PATCH net-next 1/2] net: phy: sfp: make the i2c-bus property really optional

[Antoine Tenart]

On Thu, May 17, 2018 at 03:04:06PM +0200, Andrew Lunn wrote:
> 
> I was thinking about how it reads the bit rate from the EEPROM. From
> that it determines what mode the MAC could use, 1000-Base-X,
> 2500-Base-X, etc. Can you still configure this correctly via ethtool,
> if you don't have the bitrate information?

I see. That's a very good question. When testing this, I used SFP cages
which were not wired *at all*. So it worked because the SFP module
injection never was seen by the kernel, which was then not calling
phylink_sfp_module_insert() and thus not calling sfp_parse_support().

But in cases where the module insertion can be detected, as you pointed
out, I'm not so sure it can work then. I'll wait for other answers, but
we may want to fail when probing such modules as you suggested.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 1/2] net: phy: sfp: make the i2c-bus property really optional

[Russell King - ARM Linux]

On Thu, May 17, 2018 at 03:04:06PM +0200, Andrew Lunn wrote:
> On Thu, May 17, 2018 at 02:56:48PM +0200, Antoine Tenart wrote:
> > Hi Andrew,
> > 
> > On Thu, May 17, 2018 at 02:41:28PM +0200, Andrew Lunn wrote:
> > > On Thu, May 17, 2018 at 10:29:06AM +0200, Antoine Tenart wrote:
> > > > The SFF,SFP documentation is clear about making all the DT properties,
> > > > with the exception of the compatible, optional. In practice this is not
> > > > the case and without an i2c-bus property provided the SFP code will
> > > > throw NULL pointer exceptions.
> > > > 
> > > > This patch is an attempt to fix this.
> > > 
> > > How usable is an SFF/SFP module without access to the i2c EEPROM? I
> > > guess this comes down to link speed. Can it be manually configured?
> > >
> > > I'm just wondering if we want to make this mandatory? Fail the probe
> > > if it is not listed?
> > 
> > Yes, the other option would be to fail when probing a cage missing the
> > i2c description. I'd say a passive module can work without the i2c
> > EEPROM accessible as it does not need to be configured. I don't know
> > what would happen with active ones.
> 
> Hi Antoine
> 
> I was thinking about how it reads the bit rate from the EEPROM. From
> that it determines what mode the MAC could use, 1000-Base-X,
> 2500-Base-X, etc. Can you still configure this correctly via ethtool,
> if you don't have the bitrate information?

Determining the protocol is kind of guess work even with the EEPROM
available - see comments above sfp_parse_interface().

Without knowing the contents of the EEPROM, you can't even guess what
protocol should be used for a particular module.

For example, there are 10/100/1000 modules from one vendor that use an
88e1111, which are configured for SGMII on the MAC side.  There is
another variant of that module which has the same hardware, but the
88e1111 is programmed for 1G only mode, and uses 1000base-X on the MAC
side.  For both modules, the 88e1111 is accessible, the host side
protocol can be reconfigured and the manufacturer includes the 88e1111
register access instructions for doing so.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH net-next 1/2] net: phy: sfp: make the i2c-bus property really optional

[Russell King - ARM Linux]

On Thu, May 17, 2018 at 02:41:28PM +0200, Andrew Lunn wrote:
> On Thu, May 17, 2018 at 10:29:06AM +0200, Antoine Tenart wrote:
> > The SFF,SFP documentation is clear about making all the DT properties,
> > with the exception of the compatible, optional. In practice this is not
> > the case and without an i2c-bus property provided the SFP code will
> > throw NULL pointer exceptions.
> > 
> > This patch is an attempt to fix this.
> 
> Hi Antoine, Russell
> 
> How usable is an SFF/SFP module without access to the i2c EEPROM? I
> guess this comes down to link speed. Can it be manually configured?

While we can support forcing the speed/duplex through ethtool, there
is no obvious facility via ethtool to set the protocol currently (eg,
SGMII vs 1000base-X).  There is, however, the possibility to use the
advertise mask to determine which mode we should be using, but that's
awkward to deal with via ethtool as ethtool wants a hex mask, which
really isn't user friendly.

The protocol matters - for example, a copper SFP module will typically
want to use SGMII, and if it encounters 1000base-X on the other side,
the PHY won't pass data because it will believe that the link to the
MAC is down.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH net-next 2/2] net: phy: sfp: warn the user when no tx_disable pin is available

[Antoine Tenart]

In case no Tx disable pin is available the SFP modules will always be
emitting. This could be an issue when using modules using laser as their
light source as we would have no way to disable it when the fiber is
removed. This patch adds a warning when registering an SFP cage which do
not have its tx_disable pin wired or available.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 0fd2a92a6f7b..4e62769b3e00 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1077,6 +1077,15 @@ static int sfp_probe(struct platform_device *pdev)
 	if (poll)
 		mod_delayed_work(system_wq, &sfp->poll, poll_jiffies);
 
+	/* We could have an issue in cases no Tx disable pin is available or
+	 * wired as modules using a laser as their light source will continue to
+	 * be active when the fiber is removed. This could be a safety issue and
+	 * we should at least warn the user about that.
+	 */
+	if (!sfp->gpio[GPIO_TX_DISABLE])
+		dev_warn(sfp->dev,
+			 "No tx_disable pin: SFP modules will always be emitting.\n");
+
 	return 0;
 }
 
-- 
2.17.0

Re: [PATCH net-next 0/2] net: sfp: small improvements

[David Miller]

From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Thu, 17 May 2018 10:29:05 +0200

> This series was part of the mvpp2 phylink one but as we reworked it to
> use fixed-link on the DB boards, the SFP commits weren't needed
> anymore for our use case. Two of the three patches still are needed I
> believe (I ditched the one about non-wired SFP cages), so they are sent
> here in a separate series.

Based upon the discussion of patch #1, it seems there is a desire to make
the i2c-bus property mandatory since it isn't clear if access to the SFP
module without it really all that doable.

Re: [PATCH net-next 0/2] net: sfp: small improvements

[Antoine Tenart]

Hi David,

On Mon, May 21, 2018 at 11:51:15AM -0400, David Miller wrote:
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Date: Thu, 17 May 2018 10:29:05 +0200
> 
> > This series was part of the mvpp2 phylink one but as we reworked it to
> > use fixed-link on the DB boards, the SFP commits weren't needed
> > anymore for our use case. Two of the three patches still are needed I
> > believe (I ditched the one about non-wired SFP cages), so they are sent
> > here in a separate series.
> 
> Based upon the discussion of patch #1, it seems there is a desire to make
> the i2c-bus property mandatory since it isn't clear if access to the SFP
> module without it really all that doable.

Thanks for the clarification, I was about to ask for it. I'll make a v2
making i2c-bus mandatory then.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com