[PATCH ethtool 0/2] fix module info JSON output

[PATCH ethtool 0/2] fix module info JSON output

[Johannes Eigner]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

In one of our products we need to show the SFP diagnostics in a web
interface. Therefore we want to use the JSON output of the ethtool
module information. During integration I found two problems.

When using `ethtool -j -m sfpX` only the basic module information was
JSON formatted, the diagnostics part was not. First patch ensures whole
module information output is JSON formatted for SFP modules.

The same keys were used for both the actual and threshold values in the
diagnostics JSON output, which is not valid JSON. Second patch avoids
this by renaming the threshold keys.
This solution is not backward compatible. I don't see a possibility to
fix this in a backward compatible way. If anyone knows a solution,
please let me know so I can improve the patch.
Another solution for the second patch would be to rename the keys for
the actual values instead of the thresholds. But this is also not
backward compatible. I decided to rename the threshold keys, as this
aligns with the naming used for the warning and alarm flags.
Second bug is definitely affecting SFP modules and maybe also affecting
QSFP and CMIS modules. Possible bug for QSFP and CMIS modules are based
on my understanding of the code only. I have only access to hardware
supporting SFP modules.

Signed-off-by: Johannes Eigner <johannes.eigner@a-eberle.de>
---
Johannes Eigner (2):
      sfpid: Fix JSON output of SFP diagnostics
      sff-common: Fix naming of JSON keys for thresholds

 sff-common.c | 50 +++++++++++++++++++++++++-------------------------
 sfpid.c      |  4 ++--
 2 files changed, 27 insertions(+), 27 deletions(-)
---
base-commit: 422504811c13c245cd627be2718fbaa109bdd6ec
change-id: 20251021-fix-module-info-json-0424107771fe

Best regards,
-- 
Johannes Eigner <johannes.eigner@a-eberle.de>


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4153 bytes --]

[PATCH ethtool 1/2] sfpid: Fix JSON output of SFP diagnostics

[Johannes Eigner]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

Close and delete JSON object only after output of SFP diagnostics so
that it is also JSON formatted. If the JSON object is deleted too early,
some of the output will not be JSON formatted, resulting in mixed output
formats.

Signed-off-by: Johannes Eigner <johannes.eigner@a-eberle.de>
---
 sfpid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sfpid.c b/sfpid.c
index 62acb4f..5216ce3 100644
--- a/sfpid.c
+++ b/sfpid.c
@@ -520,8 +520,6 @@ int sff8079_show_all_nl(struct cmd_context *ctx)
 	new_json_obj(ctx->json);
 	open_json_object(NULL);
 	sff8079_show_all_common(buf);
-	close_json_object();
-	delete_json_obj();
 
 	/* Finish if A2h page is not present */
 	if (!(buf[92] & (1 << 6)))
@@ -537,6 +535,8 @@ int sff8079_show_all_nl(struct cmd_context *ctx)
 
 	sff8472_show_all(buf);
 out:
+	close_json_object();
+	delete_json_obj();
 	free(buf);
 
 	return ret;

-- 
2.43.0


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4153 bytes --]

RE: [PATCH ethtool 1/2] sfpid: Fix JSON output of SFP diagnostics

[Danielle Ratson]

> -----Original Message-----
> From: Johannes Eigner <johannes.eigner@a-eberle.de>
> Sent: Tuesday, 21 October 2025 17:00
> To: netdev@vger.kernel.org
> Cc: Michal Kubecek <mkubecek@suse.cz>; Danielle Ratson
> <danieller@nvidia.com>; Stephan Wurm <stephan.wurm@a-eberle.de>;
> Johannes Eigner <johannes.eigner@a-eberle.de>
> Subject: [PATCH ethtool 1/2] sfpid: Fix JSON output of SFP diagnostics
> 
> Close and delete JSON object only after output of SFP diagnostics so
> that it is also JSON formatted. If the JSON object is deleted too early,
> some of the output will not be JSON formatted, resulting in mixed output
> formats.
> 
> Signed-off-by: Johannes Eigner <johannes.eigner@a-eberle.de>

Hi Johannes,

Please add a fixes tag.

> ---
>  sfpid.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sfpid.c b/sfpid.c
> index 62acb4f..5216ce3 100644
> --- a/sfpid.c
> +++ b/sfpid.c
> @@ -520,8 +520,6 @@ int sff8079_show_all_nl(struct cmd_context *ctx)
>  	new_json_obj(ctx->json);
>  	open_json_object(NULL);
>  	sff8079_show_all_common(buf);
> -	close_json_object();
> -	delete_json_obj();
> 
>  	/* Finish if A2h page is not present */
>  	if (!(buf[92] & (1 << 6)))
> @@ -537,6 +535,8 @@ int sff8079_show_all_nl(struct cmd_context *ctx)
> 
>  	sff8472_show_all(buf);
>  out:
> +	close_json_object();
> +	delete_json_obj();

If sff8079_get_eeprom_page() fails, then close_json_object() and delete_json_object() lines will be called although the object was never opened.
I think those lines should be after the sff8472_show_all(), but outside the out label.

>  	free(buf);
> 
>  	return ret;
> 
> --
> 2.43.0

Re: [PATCH ethtool 1/2] sfpid: Fix JSON output of SFP diagnostics

[Johannes Eigner]

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

Am Wed, Oct 22, 2025 at 12:55:45PM +0000 schrieb Danielle Ratson:
> > -----Original Message-----
> > From: Johannes Eigner <johannes.eigner@a-eberle.de>
> > Sent: Tuesday, 21 October 2025 17:00
> > To: netdev@vger.kernel.org
> > Cc: Michal Kubecek <mkubecek@suse.cz>; Danielle Ratson
> > <danieller@nvidia.com>; Stephan Wurm <stephan.wurm@a-eberle.de>;
> > Johannes Eigner <johannes.eigner@a-eberle.de>
> > Subject: [PATCH ethtool 1/2] sfpid: Fix JSON output of SFP diagnostics
> > 
> > Close and delete JSON object only after output of SFP diagnostics so
> > that it is also JSON formatted. If the JSON object is deleted too early,
> > some of the output will not be JSON formatted, resulting in mixed output
> > formats.
> > 
> > Signed-off-by: Johannes Eigner <johannes.eigner@a-eberle.de>
> 
> Hi Johannes,
> 
> Please add a fixes tag.
> 

Will update patches with fixes tags.

> > ---
> >  sfpid.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sfpid.c b/sfpid.c
> > index 62acb4f..5216ce3 100644
> > --- a/sfpid.c
> > +++ b/sfpid.c
> > @@ -520,8 +520,6 @@ int sff8079_show_all_nl(struct cmd_context *ctx)
> >  	new_json_obj(ctx->json);
> >  	open_json_object(NULL);
> >  	sff8079_show_all_common(buf);
> > -	close_json_object();
> > -	delete_json_obj();
> > 
> >  	/* Finish if A2h page is not present */
> >  	if (!(buf[92] & (1 << 6)))
> > @@ -537,6 +535,8 @@ int sff8079_show_all_nl(struct cmd_context *ctx)
> > 
> >  	sff8472_show_all(buf);
> >  out:
> > +	close_json_object();
> > +	delete_json_obj();
> 
> If sff8079_get_eeprom_page() fails, then close_json_object() and delete_json_object() lines will be called although the object was never opened.
> I think those lines should be after the sff8472_show_all(), but outside the out label.
> 

You are right in case the first sff8079_get_eeprom_page() fails,
close_json_object() and delete_json_object() should not be called. But
for the later goto we need to call close_json_object() and
delete_json_object(). Will add second goto mark to resolve this.

> >  	free(buf);
> > 
> >  	return ret;
> > 
> > --
> > 2.43.0
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4153 bytes --]

[PATCH ethtool 2/2] sff-common: Fix naming of JSON keys for thresholds

[Johannes Eigner]

[-- Attachment #1: Type: text/plain, Size: 3950 bytes --]

Append "_thresholds" to the threshold JSON objects to avoid using the
same key which is not allowed in JSON.
The JSON output for SFP transceivers uses the keys "laser_bias_current",
"laser_output_power", "module_temperature" and "module_voltage" for
both the actual value and the threshold values. This leads to invalid
JSON output as keys in a JSON object must be unique.
For QSPI and CMIS the keys "module_temperature" and "module_voltage" are
also used for both the actual value and the threshold values.

Signed-off-by: Johannes Eigner <johannes.eigner@a-eberle.de>
---
 sff-common.c | 50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/sff-common.c b/sff-common.c
index 0824dfb..6528f5a 100644
--- a/sff-common.c
+++ b/sff-common.c
@@ -104,39 +104,39 @@ void sff8024_show_encoding(const __u8 *id, int encoding_offset, int sff_type)
 
 void sff_show_thresholds_json(struct sff_diags sd)
 {
-	open_json_object("laser_bias_current");
-	PRINT_BIAS_JSON("high_alarm_threshold", sd.bias_cur[HALRM]);
-	PRINT_BIAS_JSON("low_alarm_threshold", sd.bias_cur[LALRM]);
-	PRINT_BIAS_JSON("high_warning_threshold", sd.bias_cur[HWARN]);
-	PRINT_BIAS_JSON("low_warning_threshold", sd.bias_cur[LWARN]);
+	open_json_object("laser_bias_current_thresholds");
+	PRINT_BIAS_JSON("high_alarm", sd.bias_cur[HALRM]);
+	PRINT_BIAS_JSON("low_alarm", sd.bias_cur[LALRM]);
+	PRINT_BIAS_JSON("high_warning", sd.bias_cur[HWARN]);
+	PRINT_BIAS_JSON("low_warning", sd.bias_cur[LWARN]);
 	close_json_object();
 
-	open_json_object("laser_output_power");
-	PRINT_xX_PWR_JSON("high_alarm_threshold", sd.tx_power[HALRM]);
-	PRINT_xX_PWR_JSON("low_alarm_threshold", sd.tx_power[LALRM]);
-	PRINT_xX_PWR_JSON("high_warning_threshold", sd.tx_power[HWARN]);
-	PRINT_xX_PWR_JSON("low_warning_threshold", sd.tx_power[LWARN]);
+	open_json_object("laser_output_power_thresholds");
+	PRINT_xX_PWR_JSON("high_alarm", sd.tx_power[HALRM]);
+	PRINT_xX_PWR_JSON("low_alarm", sd.tx_power[LALRM]);
+	PRINT_xX_PWR_JSON("high_warning", sd.tx_power[HWARN]);
+	PRINT_xX_PWR_JSON("low_warning", sd.tx_power[LWARN]);
 	close_json_object();
 
-	open_json_object("module_temperature");
-	PRINT_TEMP_JSON("high_alarm_threshold", sd.sfp_temp[HALRM]);
-	PRINT_TEMP_JSON("low_alarm_threshold", sd.sfp_temp[LALRM]);
-	PRINT_TEMP_JSON("high_warning_threshold", sd.sfp_temp[HWARN]);
-	PRINT_TEMP_JSON("low_warning_threshold", sd.sfp_temp[LWARN]);
+	open_json_object("module_temperature_thresholds");
+	PRINT_TEMP_JSON("high_alarm", sd.sfp_temp[HALRM]);
+	PRINT_TEMP_JSON("low_alarm", sd.sfp_temp[LALRM]);
+	PRINT_TEMP_JSON("high_warning", sd.sfp_temp[HWARN]);
+	PRINT_TEMP_JSON("low_warning", sd.sfp_temp[LWARN]);
 	close_json_object();
 
-	open_json_object("module_voltage");
-	PRINT_VCC_JSON("high_alarm_threshold", sd.sfp_voltage[HALRM]);
-	PRINT_VCC_JSON("low_alarm_threshold", sd.sfp_voltage[LALRM]);
-	PRINT_VCC_JSON("high_warning_threshold", sd.sfp_voltage[HWARN]);
-	PRINT_VCC_JSON("low_warning_threshold", sd.sfp_voltage[LWARN]);
+	open_json_object("module_voltage_thresholds");
+	PRINT_VCC_JSON("high_alarm", sd.sfp_voltage[HALRM]);
+	PRINT_VCC_JSON("low_alarm", sd.sfp_voltage[LALRM]);
+	PRINT_VCC_JSON("high_warning", sd.sfp_voltage[HWARN]);
+	PRINT_VCC_JSON("low_warning", sd.sfp_voltage[LWARN]);
 	close_json_object();
 
-	open_json_object("laser_rx_power");
-	PRINT_xX_PWR_JSON("high_alarm_threshold", sd.rx_power[HALRM]);
-	PRINT_xX_PWR_JSON("low_alarm_threshold", sd.rx_power[LALRM]);
-	PRINT_xX_PWR_JSON("high_warning_threshold", sd.rx_power[HWARN]);
-	PRINT_xX_PWR_JSON("low_warning_threshold", sd.rx_power[LWARN]);
+	open_json_object("laser_rx_power_thresholds");
+	PRINT_xX_PWR_JSON("high_alarm", sd.rx_power[HALRM]);
+	PRINT_xX_PWR_JSON("low_alarm", sd.rx_power[LALRM]);
+	PRINT_xX_PWR_JSON("high_warning", sd.rx_power[HWARN]);
+	PRINT_xX_PWR_JSON("low_warning", sd.rx_power[LWARN]);
 	close_json_object();
 }
 

-- 
2.43.0


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4153 bytes --]

Re: [PATCH ethtool 2/2] sff-common: Fix naming of JSON keys for thresholds

[Andrew Lunn]

On Tue, Oct 21, 2025 at 04:00:13PM +0200, Johannes Eigner wrote:
> Append "_thresholds" to the threshold JSON objects to avoid using the
> same key which is not allowed in JSON.
> The JSON output for SFP transceivers uses the keys "laser_bias_current",
> "laser_output_power", "module_temperature" and "module_voltage" for
> both the actual value and the threshold values. This leads to invalid
> JSON output as keys in a JSON object must be unique.
> For QSPI and CMIS the keys "module_temperature" and "module_voltage" are
> also used for both the actual value and the threshold values.
> 
> Signed-off-by: Johannes Eigner <johannes.eigner@a-eberle.de>
> ---
>  sff-common.c | 50 +++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/sff-common.c b/sff-common.c
> index 0824dfb..6528f5a 100644
> --- a/sff-common.c
> +++ b/sff-common.c
> @@ -104,39 +104,39 @@ void sff8024_show_encoding(const __u8 *id, int encoding_offset, int sff_type)
>  
>  void sff_show_thresholds_json(struct sff_diags sd)
>  {
> -	open_json_object("laser_bias_current");
> -	PRINT_BIAS_JSON("high_alarm_threshold", sd.bias_cur[HALRM]);
> -	PRINT_BIAS_JSON("low_alarm_threshold", sd.bias_cur[LALRM]);
> -	PRINT_BIAS_JSON("high_warning_threshold", sd.bias_cur[HWARN]);
> -	PRINT_BIAS_JSON("low_warning_threshold", sd.bias_cur[LWARN]);
> +	open_json_object("laser_bias_current_thresholds");
> +	PRINT_BIAS_JSON("high_alarm", sd.bias_cur[HALRM]);
> +	PRINT_BIAS_JSON("low_alarm", sd.bias_cur[LALRM]);
> +	PRINT_BIAS_JSON("high_warning", sd.bias_cur[HWARN]);
> +	PRINT_BIAS_JSON("low_warning", sd.bias_cur[LWARN]);
>  	close_json_object();

I'm struggling understanding the changes here. Maybe give an example
before and after.

The commit message talks about adding _threshold, but you are also
removing _threshold, which is what is confusing me. Is this required?
It makes the ABI breakage bigger.

	Andrew

Re: [PATCH ethtool 2/2] sff-common: Fix naming of JSON keys for thresholds

[Johannes Eigner]

[-- Attachment #1: Type: text/plain, Size: 10120 bytes --]

Am Wed, Oct 22, 2025 at 03:32:24PM +0200 schrieb Andrew Lunn:
> On Tue, Oct 21, 2025 at 04:00:13PM +0200, Johannes Eigner wrote:
> > Append "_thresholds" to the threshold JSON objects to avoid using the
> > same key which is not allowed in JSON.
> > The JSON output for SFP transceivers uses the keys "laser_bias_current",
> > "laser_output_power", "module_temperature" and "module_voltage" for
> > both the actual value and the threshold values. This leads to invalid
> > JSON output as keys in a JSON object must be unique.
> > For QSPI and CMIS the keys "module_temperature" and "module_voltage" are
> > also used for both the actual value and the threshold values.
> > 
> > Signed-off-by: Johannes Eigner <johannes.eigner@a-eberle.de>
> > ---
> >  sff-common.c | 50 +++++++++++++++++++++++++-------------------------
> >  1 file changed, 25 insertions(+), 25 deletions(-)
> > 
> > diff --git a/sff-common.c b/sff-common.c
> > index 0824dfb..6528f5a 100644
> > --- a/sff-common.c
> > +++ b/sff-common.c
> > @@ -104,39 +104,39 @@ void sff8024_show_encoding(const __u8 *id, int encoding_offset, int sff_type)
> >  
> >  void sff_show_thresholds_json(struct sff_diags sd)
> >  {
> > -	open_json_object("laser_bias_current");
> > -	PRINT_BIAS_JSON("high_alarm_threshold", sd.bias_cur[HALRM]);
> > -	PRINT_BIAS_JSON("low_alarm_threshold", sd.bias_cur[LALRM]);
> > -	PRINT_BIAS_JSON("high_warning_threshold", sd.bias_cur[HWARN]);
> > -	PRINT_BIAS_JSON("low_warning_threshold", sd.bias_cur[LWARN]);
> > +	open_json_object("laser_bias_current_thresholds");
> > +	PRINT_BIAS_JSON("high_alarm", sd.bias_cur[HALRM]);
> > +	PRINT_BIAS_JSON("low_alarm", sd.bias_cur[LALRM]);
> > +	PRINT_BIAS_JSON("high_warning", sd.bias_cur[HWARN]);
> > +	PRINT_BIAS_JSON("low_warning", sd.bias_cur[LWARN]);
> >  	close_json_object();
> 
> I'm struggling understanding the changes here. Maybe give an example
> before and after.
>

Shortened example for laser_bias_current, full output at end of mail.

Shortened output without patch
$ ethtool -j -m sfp1
        "laser_bias_current": 15.604,
        "laser_bias_current_high_alarm": false,
        "laser_bias_current_low_alarm": false,
        "laser_bias_current_high_warning": false,
        "laser_bias_current_low_warning": false,
        "laser_bias_current": {
            "high_alarm_threshold": 80,
            "low_alarm_threshold": 2,
            "high_warning_threshold": 70,
            "low_warning_threshold": 3
        },

Shortened output after patch
$ ethtool -j -m sfp1
        "laser_bias_current": 16.168,
        "laser_bias_current_high_alarm": false,
        "laser_bias_current_low_alarm": false,
        "laser_bias_current_high_warning": false,
        "laser_bias_current_low_warning": false,
        "laser_bias_current_threshold": {
            "high_alarm": 80,
            "low_alarm": 2,
            "high_warning": 70,
            "low_warning": 3
        },

> The commit message talks about adding _threshold, but you are also
> removing _threshold, which is what is confusing me. Is this required?
> It makes the ABI breakage bigger.

Removing _threshold from the child objects is not required. I removed
them because it is somehow redundant to have _threshold at the parent and
child. If a smaller ABI breakage is more desirable I can drop the
removal of _threshold in the children.

Johannes


Full output without patch
$ ethtool -j -m sfp1
[ {
        "identifier": 3,
        "identifier_description": "SFP",
        "extended_identifier": 4,
        "extended_identifier_description": "GBIC/SFP defined by 2-wire interface ID",
        "connector": 7,
        "connector_description": "LC",
        "transceiver_codes": [ 0,0,0,2,0,0,0,0,0 ],
        "transceiver_type": "Ethernet: 1000BASE-LX",
        "encoding": 1,
        "encoding_description": "8B/10B",
        "br_nominal": 1300,
        "rate_identifier": 0,
        "rate_identifier_description": "unspecified",
        "length_(smf)": 10,
        "length_(om2)": 0,
        "length_(om1)": 0,
        "length_(copper_or_active_cable)": 0,
        "length_(om3)": 0,
        "laser_wavelength": 1310,
        "vendor_name": "FLEXOPTIX",
        "vendor_oui": [ 56,134,2 ],
        "vendor_pn": "S.1312.2M.DI",
        "vendor_rev": "A",
        "option_values": [ 0,26 ],
        "option": "TX_DISABLE implemented",
        "br_margin_max": 0,
        "br_margin_min": 0,
        "vendor_sn": "F7B0CRQ",
        "date_code": "240618",
        "optical_diagnostics_support": true,
        "laser_bias_current": 15.604,
        "laser_output_power": 0.2461,
        "rx_power": {
            "value": 0.0516,
            "type": "Receiver signal average optical power"
        },
        "module_temperature": 26.5898,
        "module_voltage": 3.3812,
        "alarm/warning_flags_implemented": true,
        "laser_bias_current_high_alarm": false,
        "laser_bias_current_low_alarm": false,
        "laser_bias_current_high_warning": false,
        "laser_bias_current_low_warning": false,
        "laser_output_power_high_alarm": false,
        "laser_output_power_low_alarm": false,
        "laser_output_power_high_warning": false,
        "laser_output_power_low_warning": false,
        "module_temperature_high_alarm": false,
        "module_temperature_low_alarm": false,
        "module_temperature_high_warning": false,
        "module_temperature_low_warning": false,
        "module_voltage_high_alarm": false,
        "module_voltage_low_alarm": false,
        "module_voltage_high_warning": false,
        "module_voltage_low_warning": false,
        "laser_rx_power_high_alarm": false,
        "laser_rx_power_low_alarm": false,
        "laser_rx_power_high_warning": false,
        "laser_rx_power_low_warning": false,
        "laser_bias_current": {
            "high_alarm_threshold": 80,
            "low_alarm_threshold": 2,
            "high_warning_threshold": 70,
            "low_warning_threshold": 3
        },
        "laser_output_power": {
            "high_alarm_threshold": 0.7943,
            "low_alarm_threshold": 0.0794,
            "high_warning_threshold": 0.631,
            "low_warning_threshold": 0.1
        },
        "module_temperature": {
            "high_alarm_threshold": 110,
            "low_alarm_threshold": -45,
            "high_warning_threshold": 95,
            "low_warning_threshold": -42
        },
        "module_voltage": {
            "high_alarm_threshold": 3.6,
            "low_alarm_threshold": 3,
            "high_warning_threshold": 3.5,
            "low_warning_threshold": 3.05
        },
        "laser_rx_power": {
            "high_alarm_threshold": 0.5012,
            "low_alarm_threshold": 0.004,
            "high_warning_threshold": 0.3981,
            "low_warning_threshold": 0.005
        }
    } ]

Full output after patch
$ ethtool -j -m sfp1
[ {
        "identifier": 3,
        "identifier_description": "SFP",
        "extended_identifier": 4,
        "extended_identifier_description": "GBIC/SFP defined by 2-wire interface ID",
        "connector": 7,
        "connector_description": "LC",
        "transceiver_codes": [ 0,0,0,2,0,0,0,0,0 ],
        "transceiver_type": "Ethernet: 1000BASE-LX",
        "encoding": 1,
        "encoding_description": "8B/10B",
        "br_nominal": 1300,
        "rate_identifier": 0,
        "rate_identifier_description": "unspecified",
        "length_(smf)": 10,
        "length_(om2)": 0,
        "length_(om1)": 0,
        "length_(copper_or_active_cable)": 0,
        "length_(om3)": 0,
        "laser_wavelength": 1310,
        "vendor_name": "FLEXOPTIX",
        "vendor_oui": [ 56,134,2 ],
        "vendor_pn": "S.1312.2M.DI",
        "vendor_rev": "A",
        "option_values": [ 0,26 ],
        "option": "TX_DISABLE implemented",
        "br_margin_max": 0,
        "br_margin_min": 0,
        "vendor_sn": "F7B0CRQ",
        "date_code": "240618",
        "optical_diagnostics_support": true,
        "laser_bias_current": 16.168,
        "laser_output_power": 0.2478,
        "rx_power": {
            "value": 0.052,
            "type": "Receiver signal average optical power"
        },
        "module_temperature": 31.918,
        "module_voltage": 3.3678,
        "alarm/warning_flags_implemented": true,
        "laser_bias_current_high_alarm": false,
        "laser_bias_current_low_alarm": false,
        "laser_bias_current_high_warning": false,
        "laser_bias_current_low_warning": false,
        "laser_output_power_high_alarm": false,
        "laser_output_power_low_alarm": false,
        "laser_output_power_high_warning": false,
        "laser_output_power_low_warning": false,
        "module_temperature_high_alarm": false,
        "module_temperature_low_alarm": false,
        "module_temperature_high_warning": false,
        "module_temperature_low_warning": false,
        "module_voltage_high_alarm": false,
        "module_voltage_low_alarm": false,
        "module_voltage_high_warning": false,
        "module_voltage_low_warning": false,
        "laser_rx_power_high_alarm": false,
        "laser_rx_power_low_alarm": false,
        "laser_rx_power_high_warning": false,
        "laser_rx_power_low_warning": false,
        "laser_bias_current_threshold": {
            "high_alarm": 80,
            "low_alarm": 2,
            "high_warning": 70,
            "low_warning": 3
        },
        "laser_output_power_threshold": {
            "high_alarm": 0.7943,
            "low_alarm": 0.0794,
            "high_warning": 0.631,
            "low_warning": 0.1
        },
        "module_temperature_threshold": {
            "high_alarm": 110,
            "low_alarm": -45,
            "high_warning": 95,
            "low_warning": -42
        },
        "module_voltage_threshold": {
            "high_alarm": 3.6,
            "low_alarm": 3,
            "high_warning": 3.5,
            "low_warning": 3.05
        },
        "laser_rx_power_threshold": {
            "high_alarm": 0.5012,
            "low_alarm": 0.004,
            "high_warning": 0.3981,
            "low_warning": 0.005
        }
    } ]


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4153 bytes --]

Re: [PATCH ethtool 2/2] sff-common: Fix naming of JSON keys for thresholds

[Andrew Lunn]

On Wed, Oct 22, 2025 at 04:59:45PM +0200, Johannes Eigner wrote:
> Am Wed, Oct 22, 2025 at 03:32:24PM +0200 schrieb Andrew Lunn:
> > On Tue, Oct 21, 2025 at 04:00:13PM +0200, Johannes Eigner wrote:
> > > Append "_thresholds" to the threshold JSON objects to avoid using the
> > > same key which is not allowed in JSON.
> > > The JSON output for SFP transceivers uses the keys "laser_bias_current",
> > > "laser_output_power", "module_temperature" and "module_voltage" for
> > > both the actual value and the threshold values. This leads to invalid
> > > JSON output as keys in a JSON object must be unique.
> > > For QSPI and CMIS the keys "module_temperature" and "module_voltage" are
> > > also used for both the actual value and the threshold values.
> > > 
> > > Signed-off-by: Johannes Eigner <johannes.eigner@a-eberle.de>
> > > ---
> > >  sff-common.c | 50 +++++++++++++++++++++++++-------------------------
> > >  1 file changed, 25 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/sff-common.c b/sff-common.c
> > > index 0824dfb..6528f5a 100644
> > > --- a/sff-common.c
> > > +++ b/sff-common.c
> > > @@ -104,39 +104,39 @@ void sff8024_show_encoding(const __u8 *id, int encoding_offset, int sff_type)
> > >  
> > >  void sff_show_thresholds_json(struct sff_diags sd)
> > >  {
> > > -	open_json_object("laser_bias_current");
> > > -	PRINT_BIAS_JSON("high_alarm_threshold", sd.bias_cur[HALRM]);
> > > -	PRINT_BIAS_JSON("low_alarm_threshold", sd.bias_cur[LALRM]);
> > > -	PRINT_BIAS_JSON("high_warning_threshold", sd.bias_cur[HWARN]);
> > > -	PRINT_BIAS_JSON("low_warning_threshold", sd.bias_cur[LWARN]);
> > > +	open_json_object("laser_bias_current_thresholds");
> > > +	PRINT_BIAS_JSON("high_alarm", sd.bias_cur[HALRM]);
> > > +	PRINT_BIAS_JSON("low_alarm", sd.bias_cur[LALRM]);
> > > +	PRINT_BIAS_JSON("high_warning", sd.bias_cur[HWARN]);
> > > +	PRINT_BIAS_JSON("low_warning", sd.bias_cur[LWARN]);
> > >  	close_json_object();
> > 
> > I'm struggling understanding the changes here. Maybe give an example
> > before and after.
> >
> 
> Shortened example for laser_bias_current, full output at end of mail.
> 
> Shortened output without patch
> $ ethtool -j -m sfp1
>         "laser_bias_current": 15.604,
>         "laser_bias_current_high_alarm": false,
>         "laser_bias_current_low_alarm": false,
>         "laser_bias_current_high_warning": false,
>         "laser_bias_current_low_warning": false,
>         "laser_bias_current": {
>             "high_alarm_threshold": 80,
>             "low_alarm_threshold": 2,
>             "high_warning_threshold": 70,
>             "low_warning_threshold": 3
>         },
> 
> Shortened output after patch
> $ ethtool -j -m sfp1
>         "laser_bias_current": 16.168,
>         "laser_bias_current_high_alarm": false,
>         "laser_bias_current_low_alarm": false,
>         "laser_bias_current_high_warning": false,
>         "laser_bias_current_low_warning": false,
>         "laser_bias_current_threshold": {
>             "high_alarm": 80,
>             "low_alarm": 2,
>             "high_warning": 70,
>             "low_warning": 3
>         },
> 
> > The commit message talks about adding _threshold, but you are also
> > removing _threshold, which is what is confusing me. Is this required?
> > It makes the ABI breakage bigger.
> 
> Removing _threshold from the child objects is not required. I removed
> them because it is somehow redundant to have _threshold at the parent and
> child. If a smaller ABI breakage is more desirable I can drop the
> removal of _threshold in the children.

Thanks, that makes it clearer. Please expand the commit message.

As to ABI breakage, we have to consider, did this never work, so it
does not matter if we change it?

1) Does the first patch suggest it has always been impossible to get
this part of the module dumped in JSON format?

2) Because the JSON is not valid, how do most parsing libraries handle
that? Do they exit with an error, the JSON is invalid? Are they
typically forgiving and just return one of the two values?

If it never worked, we cannot break anything, so an ABI change is
O.K. You need to state that in the commit message. If it might of
worked, we need to be more cautious, and i would minimise the changes,
keep the redundant _threshold, and again, explain this in the commit
message.

	Andrew

Re: [PATCH ethtool 2/2] sff-common: Fix naming of JSON keys for thresholds

[Johannes Eigner]

[-- Attachment #1: Type: text/plain, Size: 6256 bytes --]

Am Wed, Oct 22, 2025 at 05:34:12PM +0200 schrieb Andrew Lunn:
> On Wed, Oct 22, 2025 at 04:59:45PM +0200, Johannes Eigner wrote:
> > Am Wed, Oct 22, 2025 at 03:32:24PM +0200 schrieb Andrew Lunn:
> > > On Tue, Oct 21, 2025 at 04:00:13PM +0200, Johannes Eigner wrote:
> > > > Append "_thresholds" to the threshold JSON objects to avoid using the
> > > > same key which is not allowed in JSON.
> > > > The JSON output for SFP transceivers uses the keys "laser_bias_current",
> > > > "laser_output_power", "module_temperature" and "module_voltage" for
> > > > both the actual value and the threshold values. This leads to invalid
> > > > JSON output as keys in a JSON object must be unique.
> > > > For QSPI and CMIS the keys "module_temperature" and "module_voltage" are
> > > > also used for both the actual value and the threshold values.
> > > > 
> > > > Signed-off-by: Johannes Eigner <johannes.eigner@a-eberle.de>
> > > > ---
> > > >  sff-common.c | 50 +++++++++++++++++++++++++-------------------------
> > > >  1 file changed, 25 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/sff-common.c b/sff-common.c
> > > > index 0824dfb..6528f5a 100644
> > > > --- a/sff-common.c
> > > > +++ b/sff-common.c
> > > > @@ -104,39 +104,39 @@ void sff8024_show_encoding(const __u8 *id, int encoding_offset, int sff_type)
> > > >  
> > > >  void sff_show_thresholds_json(struct sff_diags sd)
> > > >  {
> > > > -	open_json_object("laser_bias_current");
> > > > -	PRINT_BIAS_JSON("high_alarm_threshold", sd.bias_cur[HALRM]);
> > > > -	PRINT_BIAS_JSON("low_alarm_threshold", sd.bias_cur[LALRM]);
> > > > -	PRINT_BIAS_JSON("high_warning_threshold", sd.bias_cur[HWARN]);
> > > > -	PRINT_BIAS_JSON("low_warning_threshold", sd.bias_cur[LWARN]);
> > > > +	open_json_object("laser_bias_current_thresholds");
> > > > +	PRINT_BIAS_JSON("high_alarm", sd.bias_cur[HALRM]);
> > > > +	PRINT_BIAS_JSON("low_alarm", sd.bias_cur[LALRM]);
> > > > +	PRINT_BIAS_JSON("high_warning", sd.bias_cur[HWARN]);
> > > > +	PRINT_BIAS_JSON("low_warning", sd.bias_cur[LWARN]);
> > > >  	close_json_object();
> > > 
> > > I'm struggling understanding the changes here. Maybe give an example
> > > before and after.
> > >
> > 
> > Shortened example for laser_bias_current, full output at end of mail.
> > 
> > Shortened output without patch
> > $ ethtool -j -m sfp1
> >         "laser_bias_current": 15.604,
> >         "laser_bias_current_high_alarm": false,
> >         "laser_bias_current_low_alarm": false,
> >         "laser_bias_current_high_warning": false,
> >         "laser_bias_current_low_warning": false,
> >         "laser_bias_current": {
> >             "high_alarm_threshold": 80,
> >             "low_alarm_threshold": 2,
> >             "high_warning_threshold": 70,
> >             "low_warning_threshold": 3
> >         },
> > 
> > Shortened output after patch
> > $ ethtool -j -m sfp1
> >         "laser_bias_current": 16.168,
> >         "laser_bias_current_high_alarm": false,
> >         "laser_bias_current_low_alarm": false,
> >         "laser_bias_current_high_warning": false,
> >         "laser_bias_current_low_warning": false,
> >         "laser_bias_current_threshold": {
> >             "high_alarm": 80,
> >             "low_alarm": 2,
> >             "high_warning": 70,
> >             "low_warning": 3
> >         },
> > 
> > > The commit message talks about adding _threshold, but you are also
> > > removing _threshold, which is what is confusing me. Is this required?
> > > It makes the ABI breakage bigger.
> > 
> > Removing _threshold from the child objects is not required. I removed
> > them because it is somehow redundant to have _threshold at the parent and
> > child. If a smaller ABI breakage is more desirable I can drop the
> > removal of _threshold in the children.
> 
> Thanks, that makes it clearer. Please expand the commit message.
> 
> As to ABI breakage, we have to consider, did this never work, so it
> does not matter if we change it?
> 
> 1) Does the first patch suggest it has always been impossible to get
> this part of the module dumped in JSON format?

Yes for SFP modules it was always impossible to get this part. But for
QSFP and CMIS modules it should have worked before.

> 2) Because the JSON is not valid, how do most parsing libraries handle
> that? Do they exit with an error, the JSON is invalid? Are they
> typically forgiving and just return one of the two values?

Regarding RFC different behaviors are possible, see
https://datatracker.ietf.org/doc/html/rfc8259#section-4

But in practice, it seems that most libraries just return the last of
the two values without reporting an error. Observed this behavior with
Boost.JSON, nlohmann json, javascript (running in Firefox and Chromium),
jq, php, python and ruby.

> If it never worked, we cannot break anything, so an ABI change is
> O.K. You need to state that in the commit message. If it might of
> worked, we need to be more cautious, and i would minimise the changes,
> keep the redundant _threshold, and again, explain this in the commit
> message.

For SFP modules it never worked, but for QSFP and CMIS modules it should
have worked before. So I will try to minimize the effect for QSFP and
CMIS modules.

For the biggest possible backward compatibility, I would suggest:
* Keep the function sff_show_thresholds_json() as it is, so threshold
  values remain unchanged in the JSON output
* Only rename the measured values if needed to avoid duplicated keys
  * Keep name "rx_power" for all module types
  * Keep name "laser_tx_bias_current" for QSFP and CMIS modules
  * Rename "laser_bias_current" to "laser_tx_bias_current" for SFP
    modules
  * Keep name "transmit_avg_optical_power" for QSFP and CMIS modules
  * Rename "laser_output_power" to "transmit_avg_optical_power" for SFP
    modules
  * Rename "module_temperature" to "module_temperature_measurement" for
    all modules
  * Rename "module_voltage" to "module_voltage_measurement" for all
    modules

This results in only two keys renamed for QSFP and CMIS modules. As a
side effect this also aligns the key names for the different module
types.

What do you think of this approach? If you agree, I will update the
patch accordingly including an improved commit message.

> 	Andrew

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4153 bytes --]

Re: [PATCH ethtool 2/2] sff-common: Fix naming of JSON keys for thresholds

[Andrew Lunn]

> > As to ABI breakage, we have to consider, did this never work, so it
> > does not matter if we change it?
> > 
> > 1) Does the first patch suggest it has always been impossible to get
> > this part of the module dumped in JSON format?
> 
> Yes for SFP modules it was always impossible to get this part. But for
> QSFP and CMIS modules it should have worked before.

So we are potentially causing regressions, if anybody has an
application using these types of modules, and a forgiving JSON parser.

But the JSON is clearly broken, so we have to do something. The commit
message is important here, because if somebody does have a regression,
they find the commit which makes the change, we want to convince them
the consequences were considered, and they should just accept it,
rather than ask for a revert.

But we also have more flexibility for SFPs, if they never worked, we
can rename properties which are specific to them without issues. But
again, this should be explained in the commit message.

> For SFP modules it never worked, but for QSFP and CMIS modules it should
> have worked before. So I will try to minimize the effect for QSFP and
> CMIS modules.

Agreed

> For the biggest possible backward compatibility, I would suggest:
> * Keep the function sff_show_thresholds_json() as it is, so threshold
>   values remain unchanged in the JSON output
> * Only rename the measured values if needed to avoid duplicated keys
>   * Keep name "rx_power" for all module types
>   * Keep name "laser_tx_bias_current" for QSFP and CMIS modules
>   * Rename "laser_bias_current" to "laser_tx_bias_current" for SFP
>     modules
>   * Keep name "transmit_avg_optical_power" for QSFP and CMIS modules
>   * Rename "laser_output_power" to "transmit_avg_optical_power" for SFP
>     modules
>   * Rename "module_temperature" to "module_temperature_measurement" for
>     all modules
>   * Rename "module_voltage" to "module_voltage_measurement" for all
>     modules
> 
> This results in only two keys renamed for QSFP and CMIS modules. As a
> side effect this also aligns the key names for the different module
> types.

This is good.

Thanks for the research you did into this, looking at difference JSON
libraries, etc.

	Andrew