[PATCH net-next 0/4] net: phy: sfp: Fixes and debugging improvements

Hi all,

This patch series contains two non critical fixes and also slightly improves
the debuggability/readability of the state machine by using human readable
strings.

Russell, please review.

Florian Fainelli (4):
  net: phy: sfp: Do not reject soldered down modules
  net: phy: sfp: Use correct endian for sfp->id.ext.options
  net: phy: sfp: Separate enumerations and states
  net: phy: sfp: Pretty print state machine events

 drivers/net/phy/sfp.c | 75 ++++++++++++++++++++++++++++++++++++++++++---------
 include/linux/sfp.h   |  1 +
 2 files changed, 64 insertions(+), 12 deletions(-)

-- 
2.14.1

The SFP module identification code in sfp_sm_mod_probe() will reject SFF
modules soldered down because they have an identified of 0x2, while the code
currently checks for 0x3 only (SFP_PHYS_ID_SFP), update that.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/sfp.c | 5 +++--
 include/linux/sfp.h   | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index e381811e5f11..942288aa9cdb 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c

@@ -463,8 +463,9 @@ static int sfp_sm_mod_probe(struct sfp *sfp)
 		 vendor, part, rev, sn, date);
 
 	/* We only support SFP modules, not the legacy GBIC modules. */
-	if (sfp->id.base.phys_id != SFP_PHYS_ID_SFP ||
-	    sfp->id.base.phys_ext_id != SFP_PHYS_EXT_ID_SFP) {
+	if ((sfp->id.base.phys_id != SFP_PHYS_ID_SFP &&
+	     sfp->id.base.phys_id != SFP_PHYS_ID_SFF) ||
+	     sfp->id.base.phys_ext_id != SFP_PHYS_EXT_ID_SFP) {
 		dev_err(sfp->dev, "module is not SFP - phys id 0x%02x 0x%02x\n",
 			sfp->id.base.phys_id, sfp->id.base.phys_ext_id);
 		return -EINVAL;

diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index 4a906f560817..ee65831fe2c5 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h

@@ -222,6 +222,7 @@ enum {
 	SFP_SFF8472_COMPLIANCE		= 0x5e,
 	SFP_CC_EXT			= 0x5f,
 
+	SFP_PHYS_ID_SFF			= 0x02,
 	SFP_PHYS_ID_SFP			= 0x03,
 	SFP_PHYS_EXT_ID_SFP		= 0x04,
 	SFP_CONNECTOR_UNSPEC		= 0x00,

-- 
2.14.1

The extended ID options 16-bit value is big-endian (and actually annotated as
such), but we would be accessing it with our CPU endian, which would not
allow the correct detection of whether the LOS signal is inverted or not.

Fixes: 73970055450e ("sfp: add SFP module support")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/sfp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 942288aa9cdb..dfb28b269687 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c

@@ -355,7 +355,7 @@ static void sfp_sm_link_check_los(struct sfp *sfp)
 	 * SFP_OPTIONS_LOS_NORMAL are set?  For now, we assume
 	 * the same as SFP_OPTIONS_LOS_NORMAL set.
 	 */
-	if (sfp->id.ext.options & SFP_OPTIONS_LOS_INVERTED)
+	if (be16_to_cpu(sfp->id.ext.options) & SFP_OPTIONS_LOS_INVERTED)
 		los ^= SFP_F_LOS;
 
 	if (los)

@@ -583,7 +583,8 @@ static void sfp_sm_event(struct sfp *sfp, unsigned int event)
 		if (event == SFP_E_TX_FAULT)
 			sfp_sm_fault(sfp, true);
 		else if (event ==
-			 (sfp->id.ext.options & SFP_OPTIONS_LOS_INVERTED ?
+			 (be16_to_cpu(sfp->id.ext.options) &
+			  SFP_OPTIONS_LOS_INVERTED ?
 			  SFP_E_LOS_HIGH : SFP_E_LOS_LOW))
 			sfp_sm_link_up(sfp);
 		break;

@@ -593,7 +594,8 @@ static void sfp_sm_event(struct sfp *sfp, unsigned int event)
 			sfp_sm_link_down(sfp);
 			sfp_sm_fault(sfp, true);
 		} else if (event ==
-			   (sfp->id.ext.options & SFP_OPTIONS_LOS_INVERTED ?
+			   (be16_to_cpu(sfp->id.ext.options) &
+			    SFP_OPTIONS_LOS_INVERTED ?
 			    SFP_E_LOS_LOW : SFP_E_LOS_HIGH)) {
 			sfp_sm_link_down(sfp);
 			sfp_sm_next(sfp, SFP_S_WAIT_LOS, 0);

-- 
2.14.1

Create separate enumerations for the SFP physical state (computed from GPIOs),
device state, module state, and actual state machine. This will make it easier
to make sure the correct states are used, and also pretty print those to help
debugging.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/sfp.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index dfb28b269687..e4662bc58c01 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c

@@ -23,13 +23,17 @@ enum {
 	GPIO_TX_DISABLE,
 	GPIO_RATE_SELECT,
 	GPIO_MAX,
+};
 
+enum sfp_f_state {
 	SFP_F_PRESENT = BIT(GPIO_MODDEF0),
 	SFP_F_LOS = BIT(GPIO_LOS),
 	SFP_F_TX_FAULT = BIT(GPIO_TX_FAULT),
 	SFP_F_TX_DISABLE = BIT(GPIO_TX_DISABLE),
 	SFP_F_RATE_SELECT = BIT(GPIO_RATE_SELECT),
+};
 
+enum sfp_event_state {
 	SFP_E_INSERT = 0,
 	SFP_E_REMOVE,
 	SFP_E_DEV_DOWN,

@@ -39,15 +43,21 @@ enum {
 	SFP_E_LOS_HIGH,
 	SFP_E_LOS_LOW,
 	SFP_E_TIMEOUT,
+};
 
+enum sfp_mod_state {
 	SFP_MOD_EMPTY = 0,
 	SFP_MOD_PROBE,
 	SFP_MOD_PRESENT,
 	SFP_MOD_ERROR,
+};
 
+enum sfp_dev_state {
 	SFP_DEV_DOWN = 0,
 	SFP_DEV_UP,
+};
 
+enum sfp_sm_state {
 	SFP_S_DOWN = 0,
 	SFP_S_INIT,
 	SFP_S_WAIT_LOS,

@@ -115,9 +125,9 @@ struct sfp {
 	struct delayed_work poll;
 	struct delayed_work timeout;
 	struct mutex sm_mutex;
-	unsigned char sm_mod_state;
-	unsigned char sm_dev_state;
-	unsigned short sm_state;
+	enum sfp_mod_state sm_mod_state;
+	enum sfp_dev_state sm_dev_state;
+	enum sfp_sm_state sm_state;
 	unsigned int sm_retries;
 
 	struct sfp_eeprom_id id;

-- 
2.14.1

Pretty print the entry and exit of the state machine by using human readable
strings.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/sfp.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index e4662bc58c01..8f938404e8ba 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c

@@ -45,6 +45,18 @@ enum sfp_event_state {
 	SFP_E_TIMEOUT,
 };
 
+static const char *sfp_even_to_str[] = {
+	"insert",
+	"remove",
+	"device down",
+	"device up",
+	"TX fault",
+	"TX clear",
+	"LOS high",
+	"LOS low",
+	"timeout"
+};
+
 enum sfp_mod_state {
 	SFP_MOD_EMPTY = 0,
 	SFP_MOD_PROBE,

@@ -52,11 +64,23 @@ enum sfp_mod_state {
 	SFP_MOD_ERROR,
 };
 
+static const char *sfp_mod_state_to_str[] = {
+	"empty",
+	"probe",
+	"present",
+	"error",
+};
+
 enum sfp_dev_state {
 	SFP_DEV_DOWN = 0,
 	SFP_DEV_UP,
 };
 
+static const char *sfp_dev_state_to_str[] = {
+	"UP",
+	"DOWN",
+};
+
 enum sfp_sm_state {
 	SFP_S_DOWN = 0,
 	SFP_S_INIT,

@@ -67,6 +91,16 @@ enum sfp_sm_state {
 	SFP_S_TX_DISABLE,
 };
 
+static const char *sfp_state_to_str[] = {
+	"down",
+	"init",
+	"wait LOS",
+	"link UP",
+	"TX fault",
+	"re-initialization",
+	"TX disable",
+};
+
 static const char *gpio_of_names[] = {
 	"mod-def0",
 	"los",

@@ -502,8 +536,10 @@ static void sfp_sm_event(struct sfp *sfp, unsigned int event)
 {
 	mutex_lock(&sfp->sm_mutex);
 
-	dev_dbg(sfp->dev, "SM: enter %u:%u:%u event %u\n",
-		sfp->sm_mod_state, sfp->sm_dev_state, sfp->sm_state, event);
+	dev_dbg(sfp->dev, "SM: enter %s:%s:%s event: %s\n",
+		sfp_mod_state_to_str[sfp->sm_mod_state],
+		sfp_dev_state_to_str[sfp->sm_dev_state],
+		sfp_state_to_str[sfp->sm_state], sfp_even_to_str[event]);
 
 	/* This state machine tracks the insert/remove state of
 	 * the module, and handles probing the on-board EEPROM.

@@ -632,8 +668,10 @@ static void sfp_sm_event(struct sfp *sfp, unsigned int event)
 		break;
 	}
 
-	dev_dbg(sfp->dev, "SM: exit %u:%u:%u\n",
-		sfp->sm_mod_state, sfp->sm_dev_state, sfp->sm_state);
+	dev_dbg(sfp->dev, "SM: exit %s:%s:%s\n",
+		sfp_mod_state_to_str[sfp->sm_mod_state],
+		sfp_dev_state_to_str[sfp->sm_dev_state],
+		sfp_state_to_str[sfp->sm_state]);
 
 	mutex_unlock(&sfp->sm_mutex);
 }

-- 
2.14.1

On Tue, Nov 07, 2017 at 07:49:09PM -0800, Florian Fainelli wrote:

It would be more efficient to convert the constants to BE16 rather
than an indeterminant number to CPU endian.  The compiler can optimise
the constant.  Same for the other two hunks.

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

On Tue, Nov 07, 2017 at 07:49:10PM -0800, Florian Fainelli wrote:

Create separate enumerations for the SFP physical state (computed from GPIOs),
device state, module state, and actual state machine. This will make it easier
to make sure the correct states are used, and also pretty print those to help
debugging.

The compiler does no type checking of these, so I don't see how it
makes it any "easier to make sure the correct states are used".

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

On Tue, Nov 07, 2017 at 07:49:08PM -0800, Florian Fainelli wrote:

I'd prefer that we do something like the patch I sent a couple of nights
ago, having a separate compatible for the SFF modules (since they have
no insert signal as SFF is soldered in place) and use that to decide
which phys_id we accept here.

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

On 11/08/2017 03:15 AM, Russell King - ARM Linux wrote:

On Tue, Nov 07, 2017 at 07:49:08PM -0800, Florian Fainelli wrote:

quoted

The SFP module identification code in sfp_sm_mod_probe() will reject SFF
modules soldered down because they have an identified of 0x2, while the code
currently checks for 0x3 only (SFP_PHYS_ID_SFP), update that.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/sfp.c | 5 +++--
 include/linux/sfp.h   | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index e381811e5f11..942288aa9cdb 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c

@@ -463,8 +463,9 @@ static int sfp_sm_mod_probe(struct sfp *sfp)
 		 vendor, part, rev, sn, date);
 
 	/* We only support SFP modules, not the legacy GBIC modules. */
-	if (sfp->id.base.phys_id != SFP_PHYS_ID_SFP ||
-	    sfp->id.base.phys_ext_id != SFP_PHYS_EXT_ID_SFP) {
+	if ((sfp->id.base.phys_id != SFP_PHYS_ID_SFP &&
+	     sfp->id.base.phys_id != SFP_PHYS_ID_SFF) ||
+	     sfp->id.base.phys_ext_id != SFP_PHYS_EXT_ID_SFP) {

I'd prefer that we do something like the patch I sent a couple of nights
ago, having a separate compatible for the SFF modules (since they have
no insert signal as SFF is soldered in place) and use that to decide
which phys_id we accept here.

Fair enough.
-- 
Florian

On 11/08/2017 12:56 AM, Russell King - ARM Linux wrote:

On Tue, Nov 07, 2017 at 07:49:09PM -0800, Florian Fainelli wrote:

quoted

The extended ID options 16-bit value is big-endian (and actually annotated as
such), but we would be accessing it with our CPU endian, which would not
allow the correct detection of whether the LOS signal is inverted or not.

Fixes: 73970055450e ("sfp: add SFP module support")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/sfp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 942288aa9cdb..dfb28b269687 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c

@@ -355,7 +355,7 @@ static void sfp_sm_link_check_los(struct sfp *sfp)
 	 * SFP_OPTIONS_LOS_NORMAL are set?  For now, we assume
 	 * the same as SFP_OPTIONS_LOS_NORMAL set.
 	 */
-	if (sfp->id.ext.options & SFP_OPTIONS_LOS_INVERTED)
+	if (be16_to_cpu(sfp->id.ext.options) & SFP_OPTIONS_LOS_INVERTED)

It would be more efficient to convert the constants to BE16 rather
than an indeterminant number to CPU endian.  The compiler can optimise
the constant.  Same for the other two hunks.

Sure, I can do that.
-- 
Florian

On 11/08/2017 12:58 AM, Russell King - ARM Linux wrote:

On Tue, Nov 07, 2017 at 07:49:10PM -0800, Florian Fainelli wrote:

quoted

Create separate enumerations for the SFP physical state (computed from GPIOs),
device state, module state, and actual state machine. This will make it easier
to make sure the correct states are used, and also pretty print those to help
debugging.

The compiler does no type checking of these, so I don't see how it
makes it any "easier to make sure the correct states are used".

The types currently used (unsigned char, unsigned short) do not make it
easy to spot what the enumeration is about and what values could be
valid. Overall it seems to me like this improves code readability if
nothing else.
-- 
Florian