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 think it's a bit of an accident - MTD was designed for flash in embedded
> systems, and it used to have a static limit on the number of partitions. The
> ethtool API was then rather better suited to plug-in cards that would have a
> single small EEPROM.
>
> > 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 think that MTD makes more sense for flash partitions, especially when there
> are several of them. I already did the work of removing the static limit on
> the
> number of partitions, and convincing distributions to enable the MTD core
> drivers. (That said, you will still find some users who need to change their
> custom kernel
> configurations.)
Notice we don't really have multiple partitions. We have a single partition
pseudo file-system, but problem is device has no direct access to the
controller;
Every access has to go via the management firmware.
And in order to work with it we need some additional metadata.
I think MTD would eventually look just as 'magical'.
Re: [RFC net-next 1/1] ethtool: Add support for set eeprom metadata.
On Sun, 2016-06-05 at 13:29 +, Yuval Mintz wrote:
> >
> > > > 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 think it's a bit of an accident - MTD was designed for flash in
embedded systems, and it used to have a static limit on the number of
partitions. The ethtool API was then rather better suited to plug-in
cards that would have a single small EEPROM.
> 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 think that MTD makes more sense for flash partitions, especially when
there are several of them. I already did the work of removing
the static limit on the number of partitions, and convincing
distributions to enable the MTD core drivers. (That said, you will
still find some users who need to change their custom kernel
configurations.)
Ben.
--
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
- Albert
Einstein
signature.asc
Description: This is a digitally signed message part
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
Re: [RFC net-next 1/1] ethtool: Add support for set eeprom metadata.
On Sun, 2016-06-05 at 10:16 +, Yuval Mintz wrote:
> >
> > 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.
- It doesn't provide a way for userland to enumerate the valid metadata
values.
- It's not clear whether the driver is supposed to maintain just one
metadata:magic mapping, or more than that.
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.
Ben.
--
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
- Albert
Einstein
signature.asc
Description: This is a digitally signed message part
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?
Thanks,
Yuval
[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.
---
include/linux/ethtool.h | 1 +
include/uapi/linux/ethtool.h | 29 +
net/core/ethtool.c | 25 +
3 files changed, 55 insertions(+)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9ded8c6..6f98c2a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -316,6 +316,7 @@ struct ethtool_ops {
struct ethtool_eeprom *, u8 *);
int (*set_eeprom)(struct net_device *,
struct ethtool_eeprom *, u8 *);
+ int (*set_meeprom)(struct net_device *, struct ethtool_meeprom *);
int (*get_coalesce)(struct net_device *, struct ethtool_coalesce *);
int (*set_coalesce)(struct net_device *, struct ethtool_coalesce *);
void(*get_ringparam)(struct net_device *,
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 9222db8..f0f28a8 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -279,6 +279,13 @@ struct ethtool_regs {
* The value passed in to %ETHTOOL_SEEPROM must match the value
* returned by %ETHTOOL_GEEPROM for the same device. This is
* unused when @cmd is %ETHTOOL_GMODULEEEPROM.
+ * Depending on @cmd, the value passed must match:
+ * - For %ETHTOOL_GMODULEEEPROM, no requirements.
+ * - For %ETHTOOL_GEEPROM, must match the value returned by the
+ * previous %ETHTOOL_SEEPROMMETA in case device supports it.
+ * - For %ETHTOOL_SEEPROM, must match value returned by the previous
+ * %ETHTOOL_SEEPROMMETA in case device supports it, or by
+ * previous %ETHTOOL_GEEPROM otherwise.
* @offset: Offset within the EEPROM to begin reading/writing, in bytes
* @len: On entry, number of bytes to read/write. On successful
* return, number of bytes actually read/written. In case of
@@ -298,6 +305,27 @@ struct ethtool_eeprom {
};
/**
+ * struct ethtool_meeprom - Set EEPROM metadata
+ * @cmd: Command number = %ETHTOOL_SEEPROMMETA
+ * @metadata: eeprom metadata corresponds to the successive %ETHTOOL_SEEPROM or
+ * %ETHTOOL_GEEPROM command.
+ * @magic: A 'magic cookie' value to be used by the successive
%ETHTOOL_SEEPROM/
+ * %ETHTOOL_GEEPROM command. User provided eeprom metadata will be
+ * assoicated with a magic value. The value passed in to %ETHTOOL_SEEPROM
+ * or %ETHTOOL_GEEPROM must match with the value returned by
+ * %ETHTOOL_SEEPROMMETA for the same device.
+ * Essentialy user will send eeprom meta data, followed by %ETHTOOL_SEEPROM
+ * or %ETHTOOL_GEEPROM command. It's driver's responsibility to map the
+ * metadata to the subsequent eeprom command, using the magic to validate
+ * correctness.
+ */
+struct ethtool_meeprom {
+ __u32 cmd;
+ __u32 metadata;
+ __u32 magic;
+};
+
+/**
* struct ethtool_eee - Energy Efficient Ethernet information
* @cmd: ETHTOOL_{G,S}EEE
* @supported: Mask of %SUPPORTED_* flags for the speed/duplex combinations
@@ -1315,6 +1343,7 @@ struct ethtool_per_queue_op {
#define ETHTOOL_GLINKSETTINGS 0x004c /* Get ethtool_link_settings */
#define ETHTOOL_SLINKSETTINGS 0x004d /* Set ethtool_link_settings */
+#define ETHTOOL_SEEPROMMETA0x004e /* Set eeprom metadata */
/* compatibility with older code */
#define SPARC_ETH_GSET ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index bdb4013..c5efadd 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -2421,6 +2421,28 @@ static int ethtool_set_per_queue(struct net_device *dev,
void __user *useraddr)
};
}
+static int ethtool_set_meeprom(struct net_device *dev, void __user *useraddr)
+{
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+ struct ethtool_meeprom meeprom;
+ int ret;
+
+ if (!ops->set_meeprom)
+ return -EOPNOTSUPP;
+
+ if (copy_from_user(, useraddr, sizeof(meeprom)))
+ return -EFAULT;
+
+ ret = ops->set_meeprom(dev, );
+ if (ret)
+ return ret;
+
+ if (copy_to_user(useraddr, , sizeof(meeprom)))
+ return -EFAULT;
+
+ return 0;
+}
+
/* The main entry point in this file. Called from net/core/dev_ioctl.c */
int dev_ethtool(struct net *net, struct ifreq *ifr)
@@
