[PATCH net-next 0/2] Rework SFP A2 access conditionals

[PATCH net-next 0/2] Rework SFP A2 access conditionals

[Russell King (Oracle)]

Hi,

This series reworks the SFP A2 (diagnostics and control) access so we
don't end up testing a variable number of conditions in several places.

This also resolves a minor issue where we may have a module indicating
that it is not SFF8472 compliant, doesn't implement A2, but fails to
set the enhanced option byte to zero, leading to accesses to the A2
page that fail.

The first patch adds a new flag "have_a2" which indicates whether we
should be accessing the A2 page, and uses this for hwmon. The
conditions are kept the same.

The second patch extends the check for soft-state polling and control
by using this "have_a2" flag (which effectively augments the check to
include some level of SFF8472 compliance.)

 drivers/net/phy/sfp.c | 48 +++++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 19 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 1/2] net: sfp: add A2h presence flag

[Russell King (Oracle)]

The hwmon code wants to know when it is safe to access the A2h data
stored in a separate address. We indicate that this is present when
we have SFF-8472 compliance and the lack of an address-change
sequence.,

The same conditions are also true if we want to access other controls
and status in the A2h address. So let's make a flag to indicate whether
we can access it, instead of repeating the conditions throughout the
code.

For now, only convert the hwmon code.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index c02cad6478a8..4ff07b5a5590 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -255,6 +255,8 @@ struct sfp {
 	unsigned int module_power_mW;
 	unsigned int module_t_start_up;
 	unsigned int module_t_wait;
+
+	bool have_a2;
 	bool tx_fault_ignore;
 
 	const struct sfp_quirk *quirk;
@@ -1453,20 +1455,10 @@ static void sfp_hwmon_probe(struct work_struct *work)
 
 static int sfp_hwmon_insert(struct sfp *sfp)
 {
-	if (sfp->id.ext.sff8472_compliance == SFP_SFF8472_COMPLIANCE_NONE)
-		return 0;
-
-	if (!(sfp->id.ext.diagmon & SFP_DIAGMON_DDM))
-		return 0;
-
-	if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE)
-		/* This driver in general does not support address
-		 * change.
-		 */
-		return 0;
-
-	mod_delayed_work(system_wq, &sfp->hwmon_probe, 1);
-	sfp->hwmon_tries = R_PROBE_RETRY_SLOW;
+	if (sfp->have_a2 && sfp->id.ext.diagmon & SFP_DIAGMON_DDM) {
+		mod_delayed_work(system_wq, &sfp->hwmon_probe, 1);
+		sfp->hwmon_tries = R_PROBE_RETRY_SLOW;
+	}
 
 	return 0;
 }
@@ -1916,6 +1908,18 @@ static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct sfp_eeprom_id *id)
 	return 0;
 }
 
+static int sfp_module_parse_sff8472(struct sfp *sfp)
+{
+	/* If the module requires address swap mode, warn about it */
+	if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE)
+		dev_warn(sfp->dev,
+			 "module address swap to access page 0xA2 is not supported.\n");
+	else
+		sfp->have_a2 = true;
+
+	return 0;
+}
+
 static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 {
 	/* SFP module inserted - read I2C data */
@@ -2053,10 +2057,11 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 		return -EINVAL;
 	}
 
-	/* If the module requires address swap mode, warn about it */
-	if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE)
-		dev_warn(sfp->dev,
-			 "module address swap to access page 0xA2 is not supported.\n");
+	if (sfp->id.ext.sff8472_compliance != SFP_SFF8472_COMPLIANCE_NONE) {
+		ret = sfp_module_parse_sff8472(sfp);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* Parse the module power requirement */
 	ret = sfp_module_parse_power(sfp);
@@ -2103,6 +2108,7 @@ static void sfp_sm_mod_remove(struct sfp *sfp)
 
 	memset(&sfp->id, 0, sizeof(sfp->id));
 	sfp->module_power_mW = 0;
+	sfp->have_a2 = false;
 
 	dev_info(sfp->dev, "module removed\n");
 }
-- 
2.30.2

[PATCH net-next 2/2] net: sfp: only use soft polling if we have A2h access

[Russell King (Oracle)]

The soft state bits are stored in the A2h memory space, and require
SFF-8472 compliance. This is what our have_a2 flag tells us, so use
this to indicate whether we should attempt to use the soft signals.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 4ff07b5a5590..39e3095796d0 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2284,7 +2284,11 @@ static void sfp_sm_main(struct sfp *sfp, unsigned int event)
 		    sfp->sm_dev_state != SFP_DEV_UP)
 			break;
 
-		if (!(sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE))
+		/* Only use the soft state bits if we have access to the A2h
+		 * memory, which implies that we have some level of SFF-8472
+		 * compliance.
+		 */
+		if (sfp->have_a2)
 			sfp_soft_start_poll(sfp);
 
 		sfp_module_tx_enable(sfp);
-- 
2.30.2

Re: [PATCH net-next 1/2] net: sfp: add A2h presence flag

[Andrew Lunn]

On Thu, Mar 09, 2023 at 03:57:11PM +0000, Russell King (Oracle) wrote:
> The hwmon code wants to know when it is safe to access the A2h data
> stored in a separate address. We indicate that this is present when
> we have SFF-8472 compliance and the lack of an address-change
> sequence.,
> 
> The same conditions are also true if we want to access other controls
> and status in the A2h address. So let's make a flag to indicate whether
> we can access it, instead of repeating the conditions throughout the
> code.
> 
> For now, only convert the hwmon code.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 2/2] net: sfp: only use soft polling if we have A2h access

[Andrew Lunn]

On Thu, Mar 09, 2023 at 03:57:16PM +0000, Russell King (Oracle) wrote:
> The soft state bits are stored in the A2h memory space, and require
> SFF-8472 compliance. This is what our have_a2 flag tells us, so use
> this to indicate whether we should attempt to use the soft signals.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 0/2] Rework SFP A2 access conditionals

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 9 Mar 2023 15:56:14 +0000 you wrote:
> Hi,
> 
> This series reworks the SFP A2 (diagnostics and control) access so we
> don't end up testing a variable number of conditions in several places.
> 
> This also resolves a minor issue where we may have a module indicating
> that it is not SFF8472 compliant, doesn't implement A2, but fails to
> set the enhanced option byte to zero, leading to accesses to the A2
> page that fail.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: sfp: add A2h presence flag
    https://git.kernel.org/netdev/net-next/c/f94b9bed12e8
  - [net-next,2/2] net: sfp: only use soft polling if we have A2h access
    https://git.kernel.org/netdev/net-next/c/5daed426f012

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html