netdev - RE: [net-next 03/17] i40e: allow various base numbers in debugfs aq commands

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: <FC41C24E35F18A40888AACA1A36F3E418AD9D22F@fmsmsx115.amr.corp.intel.com>
Date:	Mon, 24 Nov 2014 17:26:30 +0000
From:	"Nelson, Shannon" <shannon.nelson@...el.com>
To:	David Laight <David.Laight@...LAB.COM>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"nhorman@...hat.com" <nhorman@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>,
	"jogreene@...hat.com" <jogreene@...hat.com>
Subject: RE: [net-next 03/17] i40e: allow various base numbers in debugfs aq
 commands

> -----Original Message-----
> From: David Laight [mailto:David.Laight@...LAB.COM]
> Sent: Monday, November 24, 2014 8:51 AM
> To: Kirsher, Jeffrey T; davem@...emloft.net
> Cc: Nelson, Shannon; netdev@...r.kernel.org; nhorman@...hat.com;
> sassmann@...hat.com; jogreene@...hat.com
> Subject: RE: [net-next 03/17] i40e: allow various base numbers in
> debugfs aq commands
> 
> From: Jeff Kirsher
> > From: Shannon Nelson <shannon.nelson@...el.com>
> >
> > Use the 'i' rather than the more restrictive 'x' or 'd' in the aq_cmd
> > arguments.  This makes the user interface much more forgiving and user
> > friendly.
> >
> > Change-ID: I5dcd57b9befc047e06b74cf1152a25a3fa9e1309
> > Signed-off-by: Shannon Nelson <shannon.nelson@...el.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> > ---
> >  drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > index 3a3c237..16ac3f8 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > @@ -1493,7 +1493,7 @@ static ssize_t i40e_dbg_command_write(struct
> file *filp,
> >  		if (!desc)
> >  			goto command_write_done;
> >  		cnt = sscanf(&cmd_buf[11],
> > -			     "%hx %hx %hx %hx %x %x %x %x %x %x",
> > +			     "%hi %hi %hi %hi %i %i %i %i %i %i",
> 
> Isn't that an API change?
> Anything that used to specify "10" will now get 10 instead of 16.
> 
> So if this has been in a release kernel you probably shouldn't change
> it.
> 
> 	David

Thanks, David, for looking through our code.

If this were in any other part of the kernel, I would agree with you.  However, this is in our debugfs module, which is not intended for general use.  Following the original definition, there is no expectation of a stable ABI - of course, as discussed in http://lwn.net/Articles/309298/ this is a bit of a pipe dream, but the concept does remain.  As it is, we have already removed a couple of commands, and will likely remove more in the near future as more features are exposed in ethtool and other more standard tools.

The other mitigating factors in my mind are that (a) this is still a very new device, only been in customer hands for a short time, and (b) this particular command is painfully obtuse.  Probably not many have figured out that it is there or how to use it, and the input specs were inconsistent (note the %hd at the end of the second sscanf()).  This patch gives the UI more consistency with standard UI concepts for the aspect of least-surprise, eg forces express base-16 and base-8 format, and it gets this fix in before others likely have run into it.

sln

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html