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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <82ab2f16-b471-4d60-850d-ee4b83712cdc@lunn.ch>
Date: Wed, 22 Oct 2025 17:34:12 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Johannes Eigner <johannes.eigner@...berle.de>
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

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?

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