Message-ID: <aPn9HkM-mjQHEo9v@PC-LX-JohEi>
Date: Thu, 23 Oct 2025 12:02:06 +0200
From: Johannes Eigner <johannes.eigner@...berle.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, Michal Kubecek <mkubecek@...e.cz>,
	Danielle Ratson <danieller@...dia.com>,
	Stephan Wurm <stephan.wurm@...berle.de>
Subject: Re: [PATCH ethtool 2/2] sff-common: Fix naming of JSON keys for
 thresholds

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@...berle.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

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (4153 bytes)

Powered by blists - more mailing lists