netdev - RE: [RFC net-next 1/1] ethtool: Add support for set eeprom metadata.

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CO2PR11MB00889DA9200C03CA3F940D21975B0@CO2PR11MB0088.namprd11.prod.outlook.com>
Date:	Sun, 5 Jun 2016 13:29:03 +0000
From:	Yuval Mintz <Yuval.Mintz@...gic.com>
To:	Ben Hutchings <ben@...adent.org.uk>,
	David Miller <davem@...emloft.net>
CC:	netdev <netdev@...r.kernel.org>,
	Sudarsana Kalluru <Sudarsana.Kalluru@...gic.com>
Subject: RE: [RFC net-next 1/1] ethtool: Add support for set eeprom metadata.

> > > Currently ethtool implementation does not have a way to pass the
> > > metadata for eeprom related operations. Some adapters have a
> > > complicated non-volatile memory implementation that requires
> > > additional information – there are drivers [bnx2x and bnxt] that use
> > > the ‘magic’ field in the {G,S}EEPROM  for that purpose, although that’s not
> its intended usage.
> > >
> > > This patch adds a provision to pass the eeprom metadata for
> > > %ETHTOOL_SEEPROM/%ETHTOOL_GEEPROM implementations. User
> provided
> > > metadata will be cached by the driver and assigns a magic value
> > > which the application need to use for the subsequent {G,S}EEPROM
> command.
> >
> > Hi Dave,
> >
> > This got no comments at all.
> > What do you want us to with it next? Should we re-send it as a patch?
> 
> Here's a comment: I really dislike this.
:-)

> - It doesn't specify any semantics for the 'metadata'.  The comment hints that
> they are driver-specific identifiers for different NVRAM partitions.
Not exactly, but close [in our use case there are 2 'methods' of accessing the
flash - either according to addresses or logical 'files'].

> - It doesn't provide a way for userland to enumerate the valid metadata values.
I agree; I can't think of any good way of enumerating device-specific values.

> - It's not clear whether the driver is supposed to maintain just one
> metadata:magic mapping, or more than that.
Theoretically, I guess it could maintain multiple, but that wasn't the intention.
One should do.

> Is the ethtool API really the right interface for access to flash?  The sfc driver
> exposes a large number of flash partitions using MTD instead.  These can be
> enumerated (through /proc/mtd or sysfs) and they can be read and written
> through block devices.

I think the better question then is 'what's the purpose of this ethtool API at all'?
I agree we can go and do everything via MTD; The reason we've tried using this
API was mainly... because it was there. And thus we thought this is the RIGHT
method for providing users the way of reading their flash.

I don't think that 'how complicated it is to read from the device's flash' should
be the determining factor on whether to use this API or not [nor should 'have
we managed to go under the rader' as is the case for bnx2x & bnxt]. 

I think it's a bit odd that our network drivers are offering a dedicated API
for reading flash when we have perfectly reasonable infrastructure [MTD] that
allows exposing them as their own devices.
But I think we need to either decide that this API is deprecated, or else find
some way to extend it to support slightly more complicated use cases.

[Not claiming this RFC is the best possible avenue; But I think we first need
to make a decision whether it's even worth the effort of trying to revise parts
of it]

Thanks,
Yuval