* Re: [PATCH net-next v3 1/2] RDS: TCP: Add sysctl tunables for sndbuf/rcvbuf on rds-tcp socket
From: santosh shilimkar @ 2016-03-15 19:21 UTC (permalink / raw)
To: Sowmini Varadhan, netdev; +Cc: davem, eric.dumazet
In-Reply-To: <5c18f6bdde99628e486d63a071c256d1be036c28.1458064902.git.sowmini.varadhan@oracle.com>
On 3/15/2016 11:53 AM, Sowmini Varadhan wrote:
> Add per-net sysctl tunables to set the size of sndbuf and
> rcvbuf on the kernel tcp socket.
>
> The tunables are added at /proc/sys/net/rds/tcp/rds_tcp_sndbuf
> and /proc/sys/net/rds/tcp/rds_tcp_rcvbuf.
>
> Since these values must be set before accept() or connect(),
> and there may be an arbitrary number of existing rds-tcp
> sockets when the tunable is modified. To make sure that all
> connections in the netns pick up the same value for the tunable,
> we reset existing rds-tcp connections in the netns, so that
> they can reconnect with the new parameters.
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
> v2; use sysctl instead of module param. Tunabes are now per netns,
> and can be dynamically modified without restarting all namespaces.
> v3: review comments from Santosh Shilimkar, Eric Dumazet
>
> net/rds/tcp.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 134 insertions(+), 9 deletions(-)
>
This version looks fine to me. Thanks !!
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
^ permalink raw reply
* Re: [PATCH net-next v3 2/2] RDS: TCP: Remove unused constant
From: santosh shilimkar @ 2016-03-15 19:22 UTC (permalink / raw)
To: Sowmini Varadhan, netdev; +Cc: davem, eric.dumazet
In-Reply-To: <46e6ca62f49240021c7b91da101edaa1a5e978f9.1458064902.git.sowmini.varadhan@oracle.com>
On 3/15/2016 11:53 AM, Sowmini Varadhan wrote:
> RDS_TCP_DEFAULT_BUFSIZE has been unused since commit 1edd6a14d24f
> ("RDS-TCP: Do not bloat sndbuf/rcvbuf in rds_tcp_tune").
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
> v3: review comments from Santosh Shilimkar
>
> net/rds/tcp.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
^ permalink raw reply
* Re: [PATCH v2 net-next] rds-tcp: Add sysctl tunables for sndbuf/rcvbuf on rds-tcp socket
From: Sowmini Varadhan @ 2016-03-15 19:57 UTC (permalink / raw)
To: Tom Herbert
Cc: Linux Kernel Network Developers, santosh.shilimkar,
David S. Miller
In-Reply-To: <CALx6S35f+DFTXCQgHPrMgdkAxEZKepWgZN2BowOp_Nt4nTpLgg@mail.gmail.com>
On (03/15/16 10:47), Tom Herbert wrote:
> Both sshd and nfsd and allow configurable listener port numbers. Any
> listener service will allow a configurable port number. An IANA port
> number is good as a default, but there are many reasons why people
> want or need to use a different port number. I don't see what makes
> RDS special in this regard.
Tom,
TCP is supposed to be the transparent infra for RDS-TCP.
If the server listens at something other than the well-known port,
the client kernel tcp socket has to know the value of the port to
connect to. That means you also have to push down configuration
at each node, saying "client IP address X is to be connected at port Y"
etc. That is a lot of configuration information to manage in a cluster.
In addition to the volume of information that needs to be managed,
it also makes connectivity problems hard to trouble-shoot.
It has the danger of being a solution looking for a problem, because
we have not seen the use-case for this so far.
Thus to repeat what I said earlier,
*if* some such need arises
and
*if* there is absolutely no way to solve it with the existing infra,
there is nothing preventing a design extension to the daemon approach
in the future.
By itself, the sysctl support adds value and can co-exist with those
extensions.
--Sowmini
^ permalink raw reply
* Re: [RFC -next 2/2] virtio_net: Read and use the advised MTU
From: Aaron Conole @ 2016-03-15 20:52 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <1457621848.9753.44.camel@redhat.com>
Paolo Abeni <pabeni@redhat.com> writes:
> On Thu, 2016-03-10 at 09:28 -0500, Aaron Conole wrote:
>> This patch checks the feature bit for the VIRTIO_NET_F_MTU feature. If it
>> exists, read the advised MTU and use it.
>>
>> No proper error handling is provided for the case where a user changes the
>> negotiated MTU. A future commit will add proper error handling. Instead, a
>> warning is emitted if the guest changes the device MTU after previously being
>> given advice.
>>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>> drivers/net/virtio_net.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 767ab11..7175563 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -146,6 +146,7 @@ struct virtnet_info {
>> virtio_net_ctrl_ack ctrl_status;
>> u8 ctrl_promisc;
>> u8 ctrl_allmulti;
>> + bool negotiated_mtu;
>> };
>>
>> struct padded_vnet_hdr {
>> @@ -1390,8 +1391,12 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
>>
>> static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
>> {
>> + struct virtnet_info *vi = netdev_priv(dev);
>> if (new_mtu < MIN_MTU || new_mtu > MAX_MTU)
>> return -EINVAL;
>> + if (vi->negotiated_mtu == true) {
>
> why don't:
>
> if ((vi->negotiated_mtu == true) && (dev->mtu != new_mtu))
>
> ?
Okay, I'll put this test in.
>> + pr_warn("changing mtu from negotiated mtu.");
>> + }
>> dev->mtu = new_mtu;
>> return 0;
>> }
>> @@ -1836,6 +1841,13 @@ static int virtnet_probe(struct virtio_device *vdev)
>> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>> vi->has_cvq = true;
>>
>> + if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
>> + vi->negotiated_mtu = true;
>> + dev->mtu = virtio_cread16(vdev,
>> + offsetof(struct virtio_net_config,
>> + mtu));
>> + }
>> +
>> if (vi->any_header_sg)
>> dev->needed_headroom = vi->hdr_len;
>>
>> @@ -2017,8 +2029,9 @@ static unsigned int features[] = {
>> VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
>> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
>> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
>> - VIRTIO_NET_F_CTRL_MAC_ADDR,
>> + VIRTIO_NET_F_CTRL_MAC_ADDR,
>
> Here a trailing white space slipped-in.
>
> Otherwise LGTM.
>
> Paolo
D'oh! Okay, v2 will have this fixed.
>> VIRTIO_F_ANY_LAYOUT,
>> + VIRTIO_NET_F_MTU,
>> };
>>
>> static struct virtio_driver virtio_net_driver = {
Thanks so much for the review, Paolo!
-Aaron
^ permalink raw reply
* Re: [RFC -next 2/2] virtio_net: Read and use the advised MTU
From: Aaron Conole @ 2016-03-15 20:53 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <56E1C372.60204@cogentembedded.com>
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:
> Hello.
Hi Sergei,
> On 03/10/2016 05:28 PM, Aaron Conole wrote:
>
>> This patch checks the feature bit for the VIRTIO_NET_F_MTU feature. If it
>> exists, read the advised MTU and use it.
>>
>> No proper error handling is provided for the case where a user changes the
>> negotiated MTU. A future commit will add proper error handling. Instead, a
>> warning is emitted if the guest changes the device MTU after previously being
>> given advice.
>>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>> drivers/net/virtio_net.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 767ab11..7175563 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
> [...]
>> @@ -1390,8 +1391,12 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
>>
>> static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
>> {
>> + struct virtnet_info *vi = netdev_priv(dev);
>> if (new_mtu < MIN_MTU || new_mtu > MAX_MTU)
>> return -EINVAL;
>> + if (vi->negotiated_mtu == true) {
>> + pr_warn("changing mtu from negotiated mtu.");
>> + }
>
> {} not needed, see Documentation/CodingStyle.
Okay, I'll make sure to fix this with v2.
> [...]
>
> MBR, Sergei
Thanks so much for the review!
-Aaron
^ permalink raw reply
* [RFC v2 -next 0/2] virtio-net: Advised MTU feature
From: Aaron Conole @ 2016-03-15 21:04 UTC (permalink / raw)
To: netdev, Michael S. Tsirkin, virtualization, linux-kernel
The following series adds the ability for a hypervisor to set an MTU on the
guest during feature negotiation phase. This is useful for VM orchestration
when, for instance, tunneling is involved and the MTU of the various systems
should be homogenous.
The first patch adds the feature bit as described in the proposed VFIO spec
addition found at
https://lists.oasis-open.org/archives/virtio-dev/201603/msg00001.html
The second patch adds a user of the bit, and a warning when the guest changes
the MTU from the hypervisor advised MTU. Future patches may add more thorough
error handling.
v2:
* Whitespace and code style cleanups from Sergei Shtylyov and Paolo Abeni
* Additional test before printing a warning
Aaron Conole (2):
virtio: Start feature MTU support
virtio_net: Read the advised MTU
drivers/net/virtio_net.c | 12 ++++++++++++
include/uapi/linux/virtio_net.h | 3 +++
2 files changed, 15 insertions(+)
--
2.5.0
^ permalink raw reply
* [RFC v2 -next 1/2] virtio: Start feature MTU support
From: Aaron Conole @ 2016-03-15 21:04 UTC (permalink / raw)
To: netdev, Michael S. Tsirkin, virtualization, linux-kernel
In-Reply-To: <1458075853-14789-1-git-send-email-aconole@redhat.com>
This commit adds the feature bit and associated mtu device entry for the
virtio network device. Future commits will make use of these bits to support
negotiated MTU.
Signed-off-by: Aaron Conole <aconole@bytheb.org>
---
v2:
* No change
include/uapi/linux/virtio_net.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index ec32293..41a6a01 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -55,6 +55,7 @@
#define VIRTIO_NET_F_MQ 22 /* Device supports Receive Flow
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_MTU 25 /* Device supports Default MTU Negotiation */
#ifndef VIRTIO_NET_NO_LEGACY
#define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
@@ -73,6 +74,8 @@ struct virtio_net_config {
* Legal values are between 1 and 0x8000
*/
__u16 max_virtqueue_pairs;
+ /* Default maximum transmit unit advice */
+ __u16 mtu;
} __attribute__((packed));
/*
--
2.5.0
^ permalink raw reply related
* [RFC v2 -next 2/2] virtio_net: Read the advised MTU
From: Aaron Conole @ 2016-03-15 21:04 UTC (permalink / raw)
To: netdev, Michael S. Tsirkin, virtualization, linux-kernel
In-Reply-To: <1458075853-14789-1-git-send-email-aconole@redhat.com>
This patch checks the feature bit for the VIRTIO_NET_F_MTU feature. If it
exists, read the advised MTU and use it.
No proper error handling is provided for the case where a user changes the
negotiated MTU. A future commit will add proper error handling. Instead, a
warning is emitted if the guest changes the device MTU after previously
being given advice.
Signed-off-by: Aaron Conole <aconole@bytheb.org>
---
v2:
* Whitespace cleanup in the last hunk
* Code style change around the pr_warn
* Additional test for mtu change before printing warning
drivers/net/virtio_net.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 767ab11..429fe01 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -146,6 +146,7 @@ struct virtnet_info {
virtio_net_ctrl_ack ctrl_status;
u8 ctrl_promisc;
u8 ctrl_allmulti;
+ bool negotiated_mtu;
};
struct padded_vnet_hdr {
@@ -1390,8 +1391,11 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
{
+ struct virtnet_info *vi = netdev_priv(dev);
if (new_mtu < MIN_MTU || new_mtu > MAX_MTU)
return -EINVAL;
+ if ((vi->negotiated_mtu) && (dev->mtu != new_mtu))
+ pr_warn("changing mtu while the advised mtu bit exists.");
dev->mtu = new_mtu;
return 0;
}
@@ -1836,6 +1840,13 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
vi->has_cvq = true;
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
+ vi->negotiated_mtu = true;
+ dev->mtu = virtio_cread16(vdev,
+ offsetof(struct virtio_net_config,
+ mtu));
+ }
+
if (vi->any_header_sg)
dev->needed_headroom = vi->hdr_len;
@@ -2019,6 +2030,7 @@ static unsigned int features[] = {
VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
VIRTIO_NET_F_CTRL_MAC_ADDR,
VIRTIO_F_ANY_LAYOUT,
+ VIRTIO_NET_F_MTU,
};
static struct virtio_driver virtio_net_driver = {
--
2.5.0
^ permalink raw reply related
* Re: [RFC v2 -next 0/2] virtio-net: Advised MTU feature
From: Rick Jones @ 2016-03-15 21:34 UTC (permalink / raw)
To: Aaron Conole, netdev, Michael S. Tsirkin, virtualization,
linux-kernel
In-Reply-To: <1458075853-14789-1-git-send-email-aconole@redhat.com>
On 03/15/2016 02:04 PM, Aaron Conole wrote:
> The following series adds the ability for a hypervisor to set an MTU on the
> guest during feature negotiation phase. This is useful for VM orchestration
> when, for instance, tunneling is involved and the MTU of the various systems
> should be homogenous.
>
> The first patch adds the feature bit as described in the proposed VFIO spec
> addition found at
> https://lists.oasis-open.org/archives/virtio-dev/201603/msg00001.html
>
> The second patch adds a user of the bit, and a warning when the guest changes
> the MTU from the hypervisor advised MTU. Future patches may add more thorough
> error handling.
How do you see this interacting with VMs getting MTU settings via DHCP?
rick jones
>
> v2:
> * Whitespace and code style cleanups from Sergei Shtylyov and Paolo Abeni
> * Additional test before printing a warning
>
> Aaron Conole (2):
> virtio: Start feature MTU support
> virtio_net: Read the advised MTU
>
> drivers/net/virtio_net.c | 12 ++++++++++++
> include/uapi/linux/virtio_net.h | 3 +++
> 2 files changed, 15 insertions(+)
>
^ permalink raw reply
* [PATCH net-next] cls_bpf: reset class and reuse major in da
From: Daniel Borkmann @ 2016-03-15 21:41 UTC (permalink / raw)
To: davem; +Cc: alexei.starovoitov, netdev, Daniel Borkmann
There are two issues with the current code. First one is that we need
to set res->class to 0 in case we use non-default classid matching.
This is important for the case where cls_bpf was initially set up with
an optional binding to a default class with tcf_bind_filter(), where
the underlying qdisc implements bind_tcf() that fills res->class and
tests for it later on when doing the classification. Convention for
these cases is that after tc_classify() was called, such qdiscs (atm,
drr, qfq, cbq, hfsc, htb) first test class, and if 0, then they lookup
based on classid.
Second, there's a bug with da mode, where res->classid is only assigned
a 16 bit minor, but it needs to expand to the full 32 bit major/minor
combination instead, therefore we need to expand with the bound major.
This is fine as classes belonging to a classful qdisc must share the
same major.
Fixes: 045efa82ff56 ("cls_bpf: introduce integrated actions")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/sched/cls_bpf.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 8dc8430..425fe6a 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -103,8 +103,9 @@ static int cls_bpf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
}
if (prog->exts_integrated) {
- res->class = prog->res.class;
- res->classid = qdisc_skb_cb(skb)->tc_classid;
+ res->class = 0;
+ res->classid = TC_H_MAJ(prog->res.classid) |
+ qdisc_skb_cb(skb)->tc_classid;
ret = cls_bpf_exec_opcode(filter_res);
if (ret == TC_ACT_UNSPEC)
@@ -114,10 +115,12 @@ static int cls_bpf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
if (filter_res == 0)
continue;
-
- *res = prog->res;
- if (filter_res != -1)
+ if (filter_res != -1) {
+ res->class = 0;
res->classid = filter_res;
+ } else {
+ *res = prog->res;
+ }
ret = tcf_exts_exec(skb, &prog->exts, res);
if (ret < 0)
--
1.9.3
^ permalink raw reply related
* [PATCH] net: mvneta: bm: clarify dependencies
From: Arnd Bergmann @ 2016-03-15 21:47 UTC (permalink / raw)
To: netdev
Cc: Arnd Bergmann, David S. Miller, Gregory CLEMENT, Marcin Wojtas,
linux-kernel
MVNETA_BM has a dependency on MVNETA, so we can only select the former
if the latter is enabled. However, the code dependency is the reverse:
The mvneta module can call into the mvneta_bm module, so mvneta cannot
be a built-in if mvneta_bm is a module, or we get a link error:
drivers/net/built-in.o: In function `mvneta_remove':
drivers/net/ethernet/marvell/mvneta.c:4211: undefined reference to `mvneta_bm_pool_destroy'
drivers/net/built-in.o: In function `mvneta_bm_update_mtu':
drivers/net/ethernet/marvell/mvneta.c:1034: undefined reference to `mvneta_bm_bufs_free'
This avoids the problem by further clarifying the dependency so that
MVNETA_BM is a silent Kconfig option that gets turned on by the
new MVNETA_BM_ENABLE option. This way both the core HWBM module and
the MVNETA_BM code are always built-in when needed.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: dc35a10f68d3 ("net: mvneta: bm: add support for hardware buffer management")
---
drivers/net/ethernet/marvell/Kconfig | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index 62d80fddbe34..b5c6d42daa12 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -40,10 +40,9 @@ config MVMDIO
This driver is used by the MV643XX_ETH and MVNETA drivers.
-config MVNETA_BM
+config MVNETA_BM_ENABLE
tristate "Marvell Armada 38x/XP network interface BM support"
depends on MVNETA
- select HWBM
---help---
This driver supports auxiliary block of the network
interface units in the Marvell ARMADA XP and ARMADA 38x SoC
@@ -67,6 +66,15 @@ config MVNETA
driver, which should be used for the older Marvell SoCs
(Dove, Orion, Discovery, Kirkwood).
+config MVNETA_BM
+ tristate
+ default y if MVNETA=y && MVNETA_BM_ENABLE
+ default MVNETA_BM_ENABLE
+ select HWBM
+ help
+ MVNETA_BM must not be 'm' if MVNETA=y, so this symbol ensures
+ that all dependencies are met.
+
config MVPP2
tristate "Marvell Armada 375 network interface support"
depends on MACH_ARMADA_375
--
2.7.0
^ permalink raw reply related
* Re: [PATCH] qmi_wwan: Added support for Gemalto's Cinterion PHxx WWAN interface
From: David Miller @ 2016-03-15 21:52 UTC (permalink / raw)
To: bjorn; +Cc: Hans-Christoph.Schemmel, netdev, linux-usb
In-Reply-To: <871t7cq63s.fsf@nemi.mork.no>
From: Bjørn Mork <bjorn@mork.no>
Date: Tue, 15 Mar 2016 13:27:03 +0100
> Schemmel Hans-Christoph <Hans-Christoph.Schemmel@gemalto.com> writes:
>
>> If you are not the intended recipient of this message, please delete it and notify the sender.
>
> OK. Deleted.
I've also deleted it and therefore won't look at this patch.
^ permalink raw reply
* [PATCH 0/2] lan78xx: patch series
From: Woojung.Huh @ 2016-03-15 22:52 UTC (permalink / raw)
To: davem, netdev; +Cc: UNGLinuxDriver
From: Woojung Huh <woojung.huh@microchip.com>
Woojung Huh (2):
lan78xx: handle statistics counter rollover
lan78xx: add ndo_get_stats64
drivers/net/usb/lan78xx.c | 301 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 288 insertions(+), 13 deletions(-)
--
2.7.0
^ permalink raw reply
* [PATCH 1/2] lan78xx: handle statistics counter rollover
From: Woojung.Huh @ 2016-03-15 22:52 UTC (permalink / raw)
To: davem, netdev; +Cc: UNGLinuxDriver
From: Woojung Huh <woojung.huh@microchip.com>
Update to handle statistics counter rollover.
Check statistics counter periodically and compensate it when
counter value rolls over at max (20 or 32bits).
Simple mechanism adjusts monitoring timer to allow USB auto suspend.
Signed-off-by: Woojung Huh <woojung.huh@microchip.com>
---
drivers/net/usb/lan78xx.c | 252 +++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 239 insertions(+), 13 deletions(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 705c180..f20890e 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -36,7 +36,7 @@
#define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@microchip.com>"
#define DRIVER_DESC "LAN78XX USB 3.0 Gigabit Ethernet Devices"
#define DRIVER_NAME "lan78xx"
-#define DRIVER_VERSION "1.0.3"
+#define DRIVER_VERSION "1.0.4"
#define TX_TIMEOUT_JIFFIES (5 * HZ)
#define THROTTLE_JIFFIES (HZ / 8)
@@ -86,6 +86,9 @@
/* default autosuspend delay (mSec)*/
#define DEFAULT_AUTOSUSPEND_DELAY (10 * 1000)
+/* statistic update interval (mSec) */
+#define STAT_UPDATE_TIMER (1 * 1000)
+
static const char lan78xx_gstrings[][ETH_GSTRING_LEN] = {
"RX FCS Errors",
"RX Alignment Errors",
@@ -186,6 +189,56 @@ struct lan78xx_statstage {
u32 eee_tx_lpi_time;
};
+struct lan78xx_statstage64 {
+ u64 rx_fcs_errors;
+ u64 rx_alignment_errors;
+ u64 rx_fragment_errors;
+ u64 rx_jabber_errors;
+ u64 rx_undersize_frame_errors;
+ u64 rx_oversize_frame_errors;
+ u64 rx_dropped_frames;
+ u64 rx_unicast_byte_count;
+ u64 rx_broadcast_byte_count;
+ u64 rx_multicast_byte_count;
+ u64 rx_unicast_frames;
+ u64 rx_broadcast_frames;
+ u64 rx_multicast_frames;
+ u64 rx_pause_frames;
+ u64 rx_64_byte_frames;
+ u64 rx_65_127_byte_frames;
+ u64 rx_128_255_byte_frames;
+ u64 rx_256_511_bytes_frames;
+ u64 rx_512_1023_byte_frames;
+ u64 rx_1024_1518_byte_frames;
+ u64 rx_greater_1518_byte_frames;
+ u64 eee_rx_lpi_transitions;
+ u64 eee_rx_lpi_time;
+ u64 tx_fcs_errors;
+ u64 tx_excess_deferral_errors;
+ u64 tx_carrier_errors;
+ u64 tx_bad_byte_count;
+ u64 tx_single_collisions;
+ u64 tx_multiple_collisions;
+ u64 tx_excessive_collision;
+ u64 tx_late_collisions;
+ u64 tx_unicast_byte_count;
+ u64 tx_broadcast_byte_count;
+ u64 tx_multicast_byte_count;
+ u64 tx_unicast_frames;
+ u64 tx_broadcast_frames;
+ u64 tx_multicast_frames;
+ u64 tx_pause_frames;
+ u64 tx_64_byte_frames;
+ u64 tx_65_127_byte_frames;
+ u64 tx_128_255_byte_frames;
+ u64 tx_256_511_bytes_frames;
+ u64 tx_512_1023_byte_frames;
+ u64 tx_1024_1518_byte_frames;
+ u64 tx_greater_1518_byte_frames;
+ u64 eee_tx_lpi_transitions;
+ u64 eee_tx_lpi_time;
+};
+
struct lan78xx_net;
struct lan78xx_priv {
@@ -232,6 +285,15 @@ struct usb_context {
#define EVENT_DEV_WAKING 6
#define EVENT_DEV_ASLEEP 7
#define EVENT_DEV_OPEN 8
+#define EVENT_STAT_UPDATE 9
+
+struct statstage {
+ struct mutex access_lock; /* for stats access */
+ struct lan78xx_statstage saved;
+ struct lan78xx_statstage rollover_count;
+ struct lan78xx_statstage rollover_max;
+ struct lan78xx_statstage64 curr_stat;
+};
struct lan78xx_net {
struct net_device *net;
@@ -272,6 +334,7 @@ struct lan78xx_net {
unsigned maxpacket;
struct timer_list delay;
+ struct timer_list stat_monitor;
unsigned long data[5];
@@ -284,6 +347,9 @@ struct lan78xx_net {
int fc_autoneg;
u8 fc_request_control;
+
+ int delta;
+ struct statstage stats;
};
/* use ethtool to change the level for any given device */
@@ -382,6 +448,93 @@ static int lan78xx_read_stats(struct lan78xx_net *dev,
return ret;
}
+#define check_counter_rollover(struct1, dev_stats, member) { \
+ if (struct1->member < dev_stats.saved.member) \
+ dev_stats.rollover_count.member++; \
+ }
+
+static void lan78xx_check_stat_rollover(struct lan78xx_net *dev,
+ struct lan78xx_statstage *stats)
+{
+ check_counter_rollover(stats, dev->stats, rx_fcs_errors);
+ check_counter_rollover(stats, dev->stats, rx_alignment_errors);
+ check_counter_rollover(stats, dev->stats, rx_fragment_errors);
+ check_counter_rollover(stats, dev->stats, rx_jabber_errors);
+ check_counter_rollover(stats, dev->stats, rx_undersize_frame_errors);
+ check_counter_rollover(stats, dev->stats, rx_oversize_frame_errors);
+ check_counter_rollover(stats, dev->stats, rx_dropped_frames);
+ check_counter_rollover(stats, dev->stats, rx_unicast_byte_count);
+ check_counter_rollover(stats, dev->stats, rx_broadcast_byte_count);
+ check_counter_rollover(stats, dev->stats, rx_multicast_byte_count);
+ check_counter_rollover(stats, dev->stats, rx_unicast_frames);
+ check_counter_rollover(stats, dev->stats, rx_broadcast_frames);
+ check_counter_rollover(stats, dev->stats, rx_multicast_frames);
+ check_counter_rollover(stats, dev->stats, rx_pause_frames);
+ check_counter_rollover(stats, dev->stats, rx_64_byte_frames);
+ check_counter_rollover(stats, dev->stats, rx_65_127_byte_frames);
+ check_counter_rollover(stats, dev->stats, rx_128_255_byte_frames);
+ check_counter_rollover(stats, dev->stats, rx_256_511_bytes_frames);
+ check_counter_rollover(stats, dev->stats, rx_512_1023_byte_frames);
+ check_counter_rollover(stats, dev->stats, rx_1024_1518_byte_frames);
+ check_counter_rollover(stats, dev->stats, rx_greater_1518_byte_frames);
+ check_counter_rollover(stats, dev->stats, eee_rx_lpi_transitions);
+ check_counter_rollover(stats, dev->stats, eee_rx_lpi_time);
+ check_counter_rollover(stats, dev->stats, tx_fcs_errors);
+ check_counter_rollover(stats, dev->stats, tx_excess_deferral_errors);
+ check_counter_rollover(stats, dev->stats, tx_carrier_errors);
+ check_counter_rollover(stats, dev->stats, tx_bad_byte_count);
+ check_counter_rollover(stats, dev->stats, tx_single_collisions);
+ check_counter_rollover(stats, dev->stats, tx_multiple_collisions);
+ check_counter_rollover(stats, dev->stats, tx_excessive_collision);
+ check_counter_rollover(stats, dev->stats, tx_late_collisions);
+ check_counter_rollover(stats, dev->stats, tx_unicast_byte_count);
+ check_counter_rollover(stats, dev->stats, tx_broadcast_byte_count);
+ check_counter_rollover(stats, dev->stats, tx_multicast_byte_count);
+ check_counter_rollover(stats, dev->stats, tx_unicast_frames);
+ check_counter_rollover(stats, dev->stats, tx_broadcast_frames);
+ check_counter_rollover(stats, dev->stats, tx_multicast_frames);
+ check_counter_rollover(stats, dev->stats, tx_pause_frames);
+ check_counter_rollover(stats, dev->stats, tx_64_byte_frames);
+ check_counter_rollover(stats, dev->stats, tx_65_127_byte_frames);
+ check_counter_rollover(stats, dev->stats, tx_128_255_byte_frames);
+ check_counter_rollover(stats, dev->stats, tx_256_511_bytes_frames);
+ check_counter_rollover(stats, dev->stats, tx_512_1023_byte_frames);
+ check_counter_rollover(stats, dev->stats, tx_1024_1518_byte_frames);
+ check_counter_rollover(stats, dev->stats, tx_greater_1518_byte_frames);
+ check_counter_rollover(stats, dev->stats, eee_tx_lpi_transitions);
+ check_counter_rollover(stats, dev->stats, eee_tx_lpi_time);
+
+ memcpy(&dev->stats.saved, stats, sizeof(struct lan78xx_statstage));
+}
+
+static void lan78xx_update_stats(struct lan78xx_net *dev)
+{
+ u32 *p, *count, *max;
+ u64 *data;
+ int i;
+ struct lan78xx_statstage lan78xx_stats;
+
+ if (usb_autopm_get_interface(dev->intf) < 0)
+ return;
+
+ p = (u32 *)&lan78xx_stats;
+ count = (u32 *)&dev->stats.rollover_count;
+ max = (u32 *)&dev->stats.rollover_max;
+ data = (u64 *)&dev->stats.curr_stat;
+
+ mutex_lock(&dev->stats.access_lock);
+
+ if (lan78xx_read_stats(dev, &lan78xx_stats) > 0)
+ lan78xx_check_stat_rollover(dev, &lan78xx_stats);
+
+ for (i = 0; i < (sizeof(lan78xx_stats) / (sizeof(u32))); i++)
+ data[i] = (u64)p[i] + ((u64)count[i] * ((u64)max[i] + 1));
+
+ mutex_unlock(&dev->stats.access_lock);
+
+ usb_autopm_put_interface(dev->intf);
+}
+
/* Loop until the read is completed with timeout called with phy_mutex held */
static int lan78xx_phy_wait_not_busy(struct lan78xx_net *dev)
{
@@ -967,6 +1120,8 @@ static int lan78xx_link_reset(struct lan78xx_net *dev)
return -EIO;
phy_mac_interrupt(phydev, 0);
+
+ del_timer(&dev->stat_monitor);
} else if (phydev->link && !dev->link_on) {
dev->link_on = true;
@@ -1007,6 +1162,12 @@ static int lan78xx_link_reset(struct lan78xx_net *dev)
ret = lan78xx_update_flowcontrol(dev, ecmd.duplex, ladv, radv);
phy_mac_interrupt(phydev, 1);
+
+ if (!timer_pending(&dev->stat_monitor)) {
+ dev->delta = 1;
+ mod_timer(&dev->stat_monitor,
+ jiffies + STAT_UPDATE_TIMER);
+ }
}
return ret;
@@ -1099,20 +1260,12 @@ static void lan78xx_get_stats(struct net_device *netdev,
struct ethtool_stats *stats, u64 *data)
{
struct lan78xx_net *dev = netdev_priv(netdev);
- struct lan78xx_statstage lan78xx_stat;
- u32 *p;
- int i;
- if (usb_autopm_get_interface(dev->intf) < 0)
- return;
+ lan78xx_update_stats(dev);
- if (lan78xx_read_stats(dev, &lan78xx_stat) > 0) {
- p = (u32 *)&lan78xx_stat;
- for (i = 0; i < (sizeof(lan78xx_stat) / (sizeof(u32))); i++)
- data[i] = p[i];
- }
-
- usb_autopm_put_interface(dev->intf);
+ mutex_lock(&dev->stats.access_lock);
+ memcpy(data, &dev->stats.curr_stat, sizeof(dev->stats.curr_stat));
+ mutex_unlock(&dev->stats.access_lock);
}
static void lan78xx_get_wol(struct net_device *netdev,
@@ -2095,6 +2248,32 @@ static int lan78xx_reset(struct lan78xx_net *dev)
return 0;
}
+static void lan78xx_init_stats(struct lan78xx_net *dev)
+{
+ u32 *p;
+ int i;
+
+ /* initialize for stats update
+ * some counters are 20bits and some are 32bits
+ */
+ p = (u32 *)&dev->stats.rollover_max;
+ for (i = 0; i < (sizeof(dev->stats.rollover_max) / (sizeof(u32))); i++)
+ p[i] = 0xFFFFF;
+
+ dev->stats.rollover_max.rx_unicast_byte_count = 0xFFFFFFFF;
+ dev->stats.rollover_max.rx_broadcast_byte_count = 0xFFFFFFFF;
+ dev->stats.rollover_max.rx_multicast_byte_count = 0xFFFFFFFF;
+ dev->stats.rollover_max.eee_rx_lpi_transitions = 0xFFFFFFFF;
+ dev->stats.rollover_max.eee_rx_lpi_time = 0xFFFFFFFF;
+ dev->stats.rollover_max.tx_unicast_byte_count = 0xFFFFFFFF;
+ dev->stats.rollover_max.tx_broadcast_byte_count = 0xFFFFFFFF;
+ dev->stats.rollover_max.tx_multicast_byte_count = 0xFFFFFFFF;
+ dev->stats.rollover_max.eee_tx_lpi_transitions = 0xFFFFFFFF;
+ dev->stats.rollover_max.eee_tx_lpi_time = 0xFFFFFFFF;
+
+ lan78xx_defer_kevent(dev, EVENT_STAT_UPDATE);
+}
+
static int lan78xx_open(struct net_device *net)
{
struct lan78xx_net *dev = netdev_priv(net);
@@ -2122,6 +2301,8 @@ static int lan78xx_open(struct net_device *net)
}
}
+ lan78xx_init_stats(dev);
+
set_bit(EVENT_DEV_OPEN, &dev->flags);
netif_start_queue(net);
@@ -2166,6 +2347,9 @@ int lan78xx_stop(struct net_device *net)
{
struct lan78xx_net *dev = netdev_priv(net);
+ if (timer_pending(&dev->stat_monitor))
+ del_timer_sync(&dev->stat_monitor);
+
phy_stop(net->phydev);
phy_disconnect(net->phydev);
net->phydev = NULL;
@@ -2910,6 +3094,13 @@ static void lan78xx_bh(unsigned long param)
}
if (netif_device_present(dev->net) && netif_running(dev->net)) {
+ /* reset update timer delta */
+ if (timer_pending(&dev->stat_monitor) && (dev->delta != 1)) {
+ dev->delta = 1;
+ mod_timer(&dev->stat_monitor,
+ jiffies + STAT_UPDATE_TIMER);
+ }
+
if (!skb_queue_empty(&dev->txq_pend))
lan78xx_tx_bh(dev);
@@ -2984,6 +3175,17 @@ skip_reset:
usb_autopm_put_interface(dev->intf);
}
}
+
+ if (test_bit(EVENT_STAT_UPDATE, &dev->flags)) {
+ lan78xx_update_stats(dev);
+
+ clear_bit(EVENT_STAT_UPDATE, &dev->flags);
+
+ mod_timer(&dev->stat_monitor,
+ jiffies + (STAT_UPDATE_TIMER * dev->delta));
+
+ dev->delta = min((dev->delta * 2), 50);
+ }
}
static void intr_complete(struct urb *urb)
@@ -3074,6 +3276,15 @@ static const struct net_device_ops lan78xx_netdev_ops = {
.ndo_vlan_rx_kill_vid = lan78xx_vlan_rx_kill_vid,
};
+static void lan78xx_stat_monitor(unsigned long param)
+{
+ struct lan78xx_net *dev;
+
+ dev = (struct lan78xx_net *)param;
+
+ lan78xx_defer_kevent(dev, EVENT_STAT_UPDATE);
+}
+
static int lan78xx_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -3120,6 +3331,13 @@ static int lan78xx_probe(struct usb_interface *intf,
netdev->watchdog_timeo = TX_TIMEOUT_JIFFIES;
netdev->ethtool_ops = &lan78xx_ethtool_ops;
+ dev->stat_monitor.function = lan78xx_stat_monitor;
+ dev->stat_monitor.data = (unsigned long)dev;
+ dev->delta = 1;
+ init_timer(&dev->stat_monitor);
+
+ mutex_init(&dev->stats.access_lock);
+
ret = lan78xx_bind(dev, intf);
if (ret < 0)
goto out2;
@@ -3397,6 +3615,8 @@ int lan78xx_suspend(struct usb_interface *intf, pm_message_t message)
}
if (test_bit(EVENT_DEV_ASLEEP, &dev->flags)) {
+ del_timer(&dev->stat_monitor);
+
if (PMSG_IS_AUTO(message)) {
/* auto suspend (selective suspend) */
ret = lan78xx_read_reg(dev, MAC_TX, &buf);
@@ -3457,6 +3677,12 @@ int lan78xx_resume(struct usb_interface *intf)
int ret;
u32 buf;
+ if (!timer_pending(&dev->stat_monitor)) {
+ dev->delta = 1;
+ mod_timer(&dev->stat_monitor,
+ jiffies + STAT_UPDATE_TIMER);
+ }
+
if (!--dev->suspend_count) {
/* resume interrupt URBs */
if (dev->urb_intr && test_bit(EVENT_DEV_OPEN, &dev->flags))
--
2.7.0
^ permalink raw reply related
* [PATCH 2/2] lan78xx: add ndo_get_stats64
From: Woojung.Huh @ 2016-03-15 22:52 UTC (permalink / raw)
To: davem, netdev; +Cc: UNGLinuxDriver
From: Woojung Huh <woojung.huh@microchip.com>
Add lan78xx_get_stats64 of ndo_get_stats64 to report
all statistics counters including errors from HW statistics.
Read from HW when auto suspend is disabled, use saved counter when
auto suspend is enabled because periodic call to ndo_get_stats64
prevents USB auto suspend.
Signed-off-by: Woojung Huh <woojung.huh@microchip.com>
---
drivers/net/usb/lan78xx.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index f20890e..f4a9708f 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3261,6 +3261,54 @@ void lan78xx_tx_timeout(struct net_device *net)
tasklet_schedule(&dev->bh);
}
+struct rtnl_link_stats64 *lan78xx_get_stats64(struct net_device *netdev,
+ struct rtnl_link_stats64 *storage)
+{
+ struct lan78xx_net *dev = netdev_priv(netdev);
+ struct lan78xx_statstage64 stats;
+
+ /* curr_stat is updated by timer.
+ * periodic reading from HW will prevent from entering USB auto suspend.
+ * if autosuspend is disabled, read from HW.
+ */
+ if (!dev->udev->dev.power.runtime_auto)
+ lan78xx_update_stats(dev);
+
+ mutex_lock(&dev->stats.access_lock);
+ memcpy(&stats, &dev->stats.curr_stat, sizeof(stats));
+ mutex_unlock(&dev->stats.access_lock);
+
+ /* calc by driver */
+ storage->rx_packets = (__u64)netdev->stats.rx_packets;
+ storage->tx_packets = (__u64)netdev->stats.tx_packets;
+ storage->rx_bytes = (__u64)netdev->stats.rx_bytes;
+ storage->tx_bytes = (__u64)netdev->stats.tx_bytes;
+
+ /* use counter */
+ storage->rx_length_errors = (stats.rx_undersize_frame_errors +
+ stats.rx_oversize_frame_errors);
+ storage->rx_crc_errors = stats.rx_fcs_errors;
+ storage->rx_frame_errors = stats.rx_alignment_errors;
+ storage->rx_fifo_errors = stats.rx_dropped_frames;
+ storage->rx_over_errors = stats.rx_oversize_frame_errors;
+ storage->rx_errors = (stats.rx_fcs_errors +
+ stats.rx_alignment_errors +
+ stats.rx_fragment_errors +
+ stats.rx_jabber_errors +
+ stats.rx_undersize_frame_errors +
+ stats.rx_oversize_frame_errors +
+ stats.rx_dropped_frames);
+
+ storage->tx_carrier_errors = stats.tx_carrier_errors;
+ storage->tx_errors = (stats.tx_fcs_errors +
+ stats.tx_excess_deferral_errors +
+ stats.tx_carrier_errors);
+
+ storage->multicast = stats.rx_multicast_frames;
+
+ return storage;
+}
+
static const struct net_device_ops lan78xx_netdev_ops = {
.ndo_open = lan78xx_open,
.ndo_stop = lan78xx_stop,
@@ -3274,6 +3322,7 @@ static const struct net_device_ops lan78xx_netdev_ops = {
.ndo_set_features = lan78xx_set_features,
.ndo_vlan_rx_add_vid = lan78xx_vlan_rx_add_vid,
.ndo_vlan_rx_kill_vid = lan78xx_vlan_rx_kill_vid,
+ .ndo_get_stats64 = lan78xx_get_stats64,
};
static void lan78xx_stat_monitor(unsigned long param)
--
2.7.0
^ permalink raw reply related
* Re: [PATCH] net: smc911x: convert pxa dma to dmaengine
From: Robert Jarzmik @ 2016-03-15 22:56 UTC (permalink / raw)
To: David Miller
Cc: Fabio Estevam, Guennadi Liakhovetski, Hitoshi Mitake,
Fabio Estevam, Hitoshi Mitake, netdev@vger.kernel.org, LKML
In-Reply-To: <87lh5zpbaw.fsf@belgarion.home>
Robert Jarzmik <robert.jarzmik@free.fr> writes:
> Fabio Estevam <festevam@gmail.com> writes:
>> Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
>
> Thanks Fabio for the test.
>
> I also tried with a different different approach.
>
> I built the different platforms (imx v6, etc ...) :
> - one version (a) on a vanilla v4.5-rc2
> - one version (b) on a vaniall v4.5-rc2 + this patch
>
> For each of the builds, I took the preprocessed output of smc911x.c, and made a
> diff between (a) and (b). The commands used were :
> - building in [1]
> - diffing in [2]
>
> The result is [3]. I conclude from [3] that the driver cannot break for non PXA
> platforms.
>
> With Fabio's test and this demonstration, is enough to convince you to apply
> this patch David ?
Hi David,
Can you consider this patch now it is tested please ?
^ permalink raw reply
* Re: [ethtool PATCH v4 11/11] ethtool.c: support absence of v4 sockets
From: David Decotigny @ 2016-03-15 23:19 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev@vger.kernel.org, Jeff Garzik, David Miller,
Vidya Sagar Ravipati, Joe Perches
In-Reply-To: <1457889843.3331.36.camel@decadent.org.uk>
will send a v5 shortly without this patch: question below.
about this patch: this is in prevision of a world where INET can be
compiled out. So it is not something that matters today with current
kernels.
Now, as you mentioned in another patch, the only socket that survives
various reasonable CONFIG_* gymnastics is netlink. So even though
non-IPv4 kernels with IP support is not feasible today, I believe
there is some logic to using netlink sockets for ethtool purposes,
instead of IPv4 or IPv6 sockets. Shall I propose a patch to add
ethtool support on AF_NETLINK sockets, and update the tool to try
AF_INET first for backward compatibility reasons, then fallback to
AF_NETLINK?
On Sun, Mar 13, 2016 at 10:24 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Fri, 2016-03-11 at 09:58 -0800, David Decotigny wrote:
>> From: David Decotigny <decot@googlers.com>
>>
>>
>> Signed-off-by: David Decotigny <decot@googlers.com>
>> ---
>> ethtool.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/ethtool.c b/ethtool.c
>> index 761252f..f9336e3 100644
>> --- a/ethtool.c
>> +++ b/ethtool.c
>> @@ -4615,6 +4615,9 @@ opt_found:
>> /* Open control socket. */
>> ctx.fd = socket(AF_INET, SOCK_DGRAM, 0);
>> if (ctx.fd < 0) {
>> + ctx.fd = socket(AF_UNIX, SOCK_DGRAM, 0);
>> + }
>
> You still haven't answered whether this is a real problem on Linux.
>
> Ben.
>
>> + if (ctx.fd < 0) {
>> perror("Cannot get control socket");
>> return 70;
>> }
> --
> Ben Hutchings
> If at first you don't succeed, you're doing about average.
^ permalink raw reply
* Re: [ethtool PATCH v4 11/11] ethtool.c: support absence of v4 sockets
From: Ben Hutchings @ 2016-03-15 23:23 UTC (permalink / raw)
To: David Decotigny
Cc: netdev@vger.kernel.org, Jeff Garzik, David Miller,
Vidya Sagar Ravipati, Joe Perches
In-Reply-To: <CAG88wWbKagbn7yUVVi2GW8t1amMWZOsCdNqmB_-mZuozXQfkVA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 953 bytes --]
On Tue, 2016-03-15 at 16:19 -0700, David Decotigny wrote:
> will send a v5 shortly without this patch: question below.
>
> about this patch: this is in prevision of a world where INET can be
> compiled out. So it is not something that matters today with current
> kernels.
OK.
> Now, as you mentioned in another patch, the only socket that survives
> various reasonable CONFIG_* gymnastics is netlink. So even though
> non-IPv4 kernels with IP support is not feasible today, I believe
> there is some logic to using netlink sockets for ethtool purposes,
> instead of IPv4 or IPv6 sockets. Shall I propose a patch to add
> ethtool support on AF_NETLINK sockets, and update the tool to try
> AF_INET first for backward compatibility reasons, then fallback to
> AF_NETLINK?
Yes, please do that.
Ben.
--
Ben Hutchings
Hoare's Law of Large Problems:
Inside every large problem is a small problem struggling to get out.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [ethtool PATCH v5] ethtool.c: add support for ETHTOOL_xLINKSETTINGS ioctls
From: David Decotigny @ 2016-03-15 23:39 UTC (permalink / raw)
To: netdev
Cc: Jeff Garzik, Ben Hutchings, David Miller, Vidya Sagar Ravipati,
Joe Perches, David Decotigny
From: David Decotigny <decot@googlers.com>
More info with kernel commit 8d3f2806f8fb ("Merge branch
'ethtool-ksettings'").
Signed-off-by: David Decotigny <decot@googlers.com>
---
ethtool.c | 681 ++++++++++++++++++++++++++++++++++++++++++++-------------
internal.h | 67 ++++++
test-cmdline.c | 13 ++
3 files changed, 603 insertions(+), 158 deletions(-)
diff --git a/ethtool.c b/ethtool.c
index 0cd0d4f..dbe4a6a 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -47,42 +47,6 @@
#define MAX_ADDR_LEN 32
#endif
-#define ALL_ADVERTISED_MODES \
- (ADVERTISED_10baseT_Half | \
- ADVERTISED_10baseT_Full | \
- ADVERTISED_100baseT_Half | \
- ADVERTISED_100baseT_Full | \
- ADVERTISED_1000baseT_Half | \
- ADVERTISED_1000baseT_Full | \
- ADVERTISED_1000baseKX_Full| \
- ADVERTISED_2500baseX_Full | \
- ADVERTISED_10000baseT_Full | \
- ADVERTISED_10000baseKX4_Full | \
- ADVERTISED_10000baseKR_Full | \
- ADVERTISED_10000baseR_FEC | \
- ADVERTISED_20000baseMLD2_Full | \
- ADVERTISED_20000baseKR2_Full | \
- ADVERTISED_40000baseKR4_Full | \
- ADVERTISED_40000baseCR4_Full | \
- ADVERTISED_40000baseSR4_Full | \
- ADVERTISED_40000baseLR4_Full | \
- ADVERTISED_56000baseKR4_Full | \
- ADVERTISED_56000baseCR4_Full | \
- ADVERTISED_56000baseSR4_Full | \
- ADVERTISED_56000baseLR4_Full)
-
-#define ALL_ADVERTISED_FLAGS \
- (ADVERTISED_Autoneg | \
- ADVERTISED_TP | \
- ADVERTISED_AUI | \
- ADVERTISED_MII | \
- ADVERTISED_FIBRE | \
- ADVERTISED_BNC | \
- ADVERTISED_Pause | \
- ADVERTISED_Asym_Pause | \
- ADVERTISED_Backplane | \
- ALL_ADVERTISED_MODES)
-
#ifndef HAVE_NETIF_MSG
enum {
NETIF_MSG_DRV = 0x0001,
@@ -293,6 +257,43 @@ static void get_mac_addr(char *src, unsigned char *dest)
}
}
+static int parse_hex_u32_bitmap(const char *s,
+ unsigned int nbits, u32 *result)
+{
+ const unsigned nwords = __KERNEL_DIV_ROUND_UP(nbits, 32);
+ size_t slen = strlen(s);
+ size_t i;
+
+ /* ignore optional '0x' prefix */
+ if ((slen > 2) && (strncasecmp(s, "0x", 2) == 0)) {
+ slen -= 2;
+ s += 2;
+ }
+
+ if (slen > 8 * nwords) /* up to 2 digits per byte */
+ return -1;
+
+ memset(result, 0, 4 * nwords);
+ for (i = 0 ; i < slen ; ++i) {
+ const unsigned shift = (slen - 1 - i) * 4;
+ u32 *dest = &result[shift / 32];
+ u32 nibble;
+
+ if ('a' <= s[i] && s[i] <= 'f')
+ nibble = 0xa + (s[i] - 'a');
+ else if ('A' <= s[i] && s[i] <= 'F')
+ nibble = 0xa + (s[i] - 'A');
+ else if ('0' <= s[i] && s[i] <= '9')
+ nibble = (s[i] - '0');
+ else
+ return -1;
+
+ *dest |= (nibble << (shift % 32));
+ }
+
+ return 0;
+}
+
static void parse_generic_cmdline(struct cmd_context *ctx,
int *changed,
struct cmdline_info *info,
@@ -472,64 +473,157 @@ static int do_version(struct cmd_context *ctx)
return 0;
}
-static void dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
- int link_mode_only);
+/* link mode routines */
-static void dump_supported(struct ethtool_cmd *ep)
+static __ETHTOOL_DECLARE_LINK_MODE_MASK(all_advertised_modes);
+static __ETHTOOL_DECLARE_LINK_MODE_MASK(all_advertised_flags);
+
+static void init_global_link_mode_masks(void)
{
- u32 mask = ep->supported;
+ static const enum ethtool_link_mode_bit_indices
+ all_advertised_modes_bits[] = {
+ ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+ ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+ ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+ ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
+ ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
+ ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
+ ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
+ ETHTOOL_LINK_MODE_10000baseR_FEC_BIT,
+ ETHTOOL_LINK_MODE_20000baseMLD2_Full_BIT,
+ ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT,
+ ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT,
+ ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT,
+ ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT,
+ ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT,
+ ETHTOOL_LINK_MODE_56000baseKR4_Full_BIT,
+ ETHTOOL_LINK_MODE_56000baseCR4_Full_BIT,
+ ETHTOOL_LINK_MODE_56000baseSR4_Full_BIT,
+ ETHTOOL_LINK_MODE_56000baseLR4_Full_BIT,
+ };
+ static const enum ethtool_link_mode_bit_indices
+ additional_advertised_flags_bits[] = {
+ ETHTOOL_LINK_MODE_Autoneg_BIT,
+ ETHTOOL_LINK_MODE_TP_BIT,
+ ETHTOOL_LINK_MODE_AUI_BIT,
+ ETHTOOL_LINK_MODE_MII_BIT,
+ ETHTOOL_LINK_MODE_FIBRE_BIT,
+ ETHTOOL_LINK_MODE_BNC_BIT,
+ ETHTOOL_LINK_MODE_Pause_BIT,
+ ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ ETHTOOL_LINK_MODE_Backplane_BIT,
+ };
+ unsigned i;
+
+ ethtool_link_mode_zero(all_advertised_modes);
+ ethtool_link_mode_zero(all_advertised_flags);
+ for (i = 0 ; i < ARRAY_SIZE(all_advertised_modes_bits) ; ++i) {
+ ethtool_link_mode_set_bit(all_advertised_modes_bits[i],
+ all_advertised_modes);
+ ethtool_link_mode_set_bit(all_advertised_modes_bits[i],
+ all_advertised_flags);
+ }
+ for (i = 0 ; i < ARRAY_SIZE(additional_advertised_flags_bits) ; ++i) {
+ ethtool_link_mode_set_bit(
+ additional_advertised_flags_bits[i],
+ all_advertised_flags);
+ }
+}
+
+static void dump_link_caps(const char *prefix, const char *an_prefix,
+ const u32 *mask, int link_mode_only);
+
+static void dump_supported(const struct ethtool_link_usettings *link_usettings)
+{
fprintf(stdout, " Supported ports: [ ");
- if (mask & SUPPORTED_TP)
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_TP_BIT,
+ link_usettings->link_modes.supported))
fprintf(stdout, "TP ");
- if (mask & SUPPORTED_AUI)
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_AUI_BIT,
+ link_usettings->link_modes.supported))
fprintf(stdout, "AUI ");
- if (mask & SUPPORTED_BNC)
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_BNC_BIT,
+ link_usettings->link_modes.supported))
fprintf(stdout, "BNC ");
- if (mask & SUPPORTED_MII)
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_MII_BIT,
+ link_usettings->link_modes.supported))
fprintf(stdout, "MII ");
- if (mask & SUPPORTED_FIBRE)
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_FIBRE_BIT,
+ link_usettings->link_modes.supported))
fprintf(stdout, "FIBRE ");
- if (mask & SUPPORTED_Backplane)
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_Backplane_BIT,
+ link_usettings->link_modes.supported))
fprintf(stdout, "Backplane ");
fprintf(stdout, "]\n");
- dump_link_caps("Supported", "Supports", mask, 0);
+ dump_link_caps("Supported", "Supports",
+ link_usettings->link_modes.supported, 0);
}
/* Print link capability flags (supported, advertised or lp_advertised).
* Assumes that the corresponding SUPPORTED and ADVERTISED flags are equal.
*/
-static void
-dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
- int link_mode_only)
+static void dump_link_caps(const char *prefix, const char *an_prefix,
+ const u32 *mask, int link_mode_only)
{
static const struct {
int same_line; /* print on same line as previous */
- u32 value;
+ unsigned bit_index;
const char *name;
} mode_defs[] = {
- { 0, ADVERTISED_10baseT_Half, "10baseT/Half" },
- { 1, ADVERTISED_10baseT_Full, "10baseT/Full" },
- { 0, ADVERTISED_100baseT_Half, "100baseT/Half" },
- { 1, ADVERTISED_100baseT_Full, "100baseT/Full" },
- { 0, ADVERTISED_1000baseT_Half, "1000baseT/Half" },
- { 1, ADVERTISED_1000baseT_Full, "1000baseT/Full" },
- { 0, ADVERTISED_1000baseKX_Full, "1000baseKX/Full" },
- { 0, ADVERTISED_2500baseX_Full, "2500baseX/Full" },
- { 0, ADVERTISED_10000baseT_Full, "10000baseT/Full" },
- { 0, ADVERTISED_10000baseKX4_Full, "10000baseKX4/Full" },
- { 0, ADVERTISED_10000baseKR_Full, "10000baseKR/Full" },
- { 0, ADVERTISED_20000baseMLD2_Full, "20000baseMLD2/Full" },
- { 0, ADVERTISED_20000baseKR2_Full, "20000baseKR2/Full" },
- { 0, ADVERTISED_40000baseKR4_Full, "40000baseKR4/Full" },
- { 0, ADVERTISED_40000baseCR4_Full, "40000baseCR4/Full" },
- { 0, ADVERTISED_40000baseSR4_Full, "40000baseSR4/Full" },
- { 0, ADVERTISED_40000baseLR4_Full, "40000baseLR4/Full" },
- { 0, ADVERTISED_56000baseKR4_Full, "56000baseKR4/Full" },
- { 0, ADVERTISED_56000baseCR4_Full, "56000baseCR4/Full" },
- { 0, ADVERTISED_56000baseSR4_Full, "56000baseSR4/Full" },
- { 0, ADVERTISED_56000baseLR4_Full, "56000baseLR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+ "10baseT/Half" },
+ { 1, ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+ "10baseT/Full" },
+ { 0, ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+ "100baseT/Half" },
+ { 1, ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+ "100baseT/Full" },
+ { 0, ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+ "1000baseT/Half" },
+ { 1, ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ "1000baseT/Full" },
+ { 0, ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
+ "1000baseKX/Full" },
+ { 0, ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
+ "2500baseX/Full" },
+ { 0, ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+ "10000baseT/Full" },
+ { 0, ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
+ "10000baseKX4/Full" },
+ { 0, ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
+ "10000baseKR/Full" },
+ { 0, ETHTOOL_LINK_MODE_20000baseMLD2_Full_BIT,
+ "20000baseMLD2/Full" },
+ { 0, ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT,
+ "20000baseKR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT,
+ "40000baseKR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT,
+ "40000baseCR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT,
+ "40000baseSR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT,
+ "40000baseLR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_56000baseKR4_Full_BIT,
+ "56000baseKR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_56000baseCR4_Full_BIT,
+ "56000baseCR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_56000baseSR4_Full_BIT,
+ "56000baseSR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_56000baseLR4_Full_BIT,
+ "56000baseLR4/Full" },
};
int indent;
int did1, new_line_pend, i;
@@ -546,7 +640,8 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
for (i = 0; i < ARRAY_SIZE(mode_defs); i++) {
if (did1 && !mode_defs[i].same_line)
new_line_pend = 1;
- if (mask & mode_defs[i].value) {
+ if (ethtool_link_mode_test_bit(mode_defs[i].bit_index,
+ mask)) {
if (new_line_pend) {
fprintf(stdout, "\n");
fprintf(stdout, " %*s", indent, "");
@@ -562,46 +657,52 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
if (!link_mode_only) {
fprintf(stdout, " %s pause frame use: ", prefix);
- if (mask & ADVERTISED_Pause) {
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_Pause_BIT, mask)) {
fprintf(stdout, "Symmetric");
- if (mask & ADVERTISED_Asym_Pause)
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask))
fprintf(stdout, " Receive-only");
fprintf(stdout, "\n");
} else {
- if (mask & ADVERTISED_Asym_Pause)
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask))
fprintf(stdout, "Transmit-only\n");
else
fprintf(stdout, "No\n");
}
fprintf(stdout, " %s auto-negotiation: ", an_prefix);
- if (mask & ADVERTISED_Autoneg)
+ if (ethtool_link_mode_test_bit(
+ ETHTOOL_LINK_MODE_Autoneg_BIT, mask))
fprintf(stdout, "Yes\n");
else
fprintf(stdout, "No\n");
}
}
-static int dump_ecmd(struct ethtool_cmd *ep)
+static int
+dump_link_usettings(const struct ethtool_link_usettings *link_usettings)
{
- u32 speed;
-
- dump_supported(ep);
- dump_link_caps("Advertised", "Advertised", ep->advertising, 0);
- if (ep->lp_advertising)
+ dump_supported(link_usettings);
+ dump_link_caps("Advertised", "Advertised",
+ link_usettings->link_modes.advertising, 0);
+ if (!ethtool_link_mode_is_empty(
+ link_usettings->link_modes.lp_advertising))
dump_link_caps("Link partner advertised",
- "Link partner advertised", ep->lp_advertising,
- 0);
+ "Link partner advertised",
+ link_usettings->link_modes.lp_advertising, 0);
fprintf(stdout, " Speed: ");
- speed = ethtool_cmd_speed(ep);
- if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
+ if (link_usettings->base.speed == 0
+ || link_usettings->base.speed == (u16)(-1)
+ || link_usettings->base.speed == (u32)(-1))
fprintf(stdout, "Unknown!\n");
else
- fprintf(stdout, "%uMb/s\n", speed);
+ fprintf(stdout, "%uMb/s\n", link_usettings->base.speed);
fprintf(stdout, " Duplex: ");
- switch (ep->duplex) {
+ switch (link_usettings->base.duplex) {
case DUPLEX_HALF:
fprintf(stdout, "Half\n");
break;
@@ -609,12 +710,12 @@ static int dump_ecmd(struct ethtool_cmd *ep)
fprintf(stdout, "Full\n");
break;
default:
- fprintf(stdout, "Unknown! (%i)\n", ep->duplex);
+ fprintf(stdout, "Unknown! (%i)\n", link_usettings->base.duplex);
break;
};
fprintf(stdout, " Port: ");
- switch (ep->port) {
+ switch (link_usettings->base.port) {
case PORT_TP:
fprintf(stdout, "Twisted Pair\n");
break;
@@ -640,13 +741,13 @@ static int dump_ecmd(struct ethtool_cmd *ep)
fprintf(stdout, "Other\n");
break;
default:
- fprintf(stdout, "Unknown! (%i)\n", ep->port);
+ fprintf(stdout, "Unknown! (%i)\n", link_usettings->base.port);
break;
};
- fprintf(stdout, " PHYAD: %d\n", ep->phy_address);
+ fprintf(stdout, " PHYAD: %d\n", link_usettings->base.phy_address);
fprintf(stdout, " Transceiver: ");
- switch (ep->transceiver) {
+ switch (link_usettings->deprecated.transceiver) {
case XCVR_INTERNAL:
fprintf(stdout, "internal\n");
break;
@@ -659,17 +760,18 @@ static int dump_ecmd(struct ethtool_cmd *ep)
};
fprintf(stdout, " Auto-negotiation: %s\n",
- (ep->autoneg == AUTONEG_DISABLE) ?
+ (link_usettings->base.autoneg == AUTONEG_DISABLE) ?
"off" : "on");
- if (ep->port == PORT_TP) {
+ if (link_usettings->base.port == PORT_TP) {
fprintf(stdout, " MDI-X: ");
- if (ep->eth_tp_mdix_ctrl == ETH_TP_MDI) {
+ if (link_usettings->base.eth_tp_mdix_ctrl == ETH_TP_MDI) {
fprintf(stdout, "off (forced)\n");
- } else if (ep->eth_tp_mdix_ctrl == ETH_TP_MDI_X) {
+ } else if (link_usettings->base.eth_tp_mdix_ctrl
+ == ETH_TP_MDI_X) {
fprintf(stdout, "on (forced)\n");
} else {
- switch (ep->eth_tp_mdix) {
+ switch (link_usettings->base.eth_tp_mdix) {
case ETH_TP_MDI:
fprintf(stdout, "off");
break;
@@ -680,7 +782,8 @@ static int dump_ecmd(struct ethtool_cmd *ep)
fprintf(stdout, "Unknown");
break;
}
- if (ep->eth_tp_mdix_ctrl == ETH_TP_MDI_AUTO)
+ if (link_usettings->base.eth_tp_mdix_ctrl
+ == ETH_TP_MDI_AUTO)
fprintf(stdout, " (auto)");
fprintf(stdout, "\n");
}
@@ -1368,6 +1471,7 @@ static int dump_rxfhash(int fhash, u64 val)
static void dump_eeecmd(struct ethtool_eee *ep)
{
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(link_mode);
fprintf(stdout, " EEE status: ");
if (!ep->supported) {
@@ -1389,9 +1493,16 @@ static void dump_eeecmd(struct ethtool_eee *ep)
else
fprintf(stdout, " disabled\n");
- dump_link_caps("Supported EEE", "", ep->supported, 1);
- dump_link_caps("Advertised EEE", "", ep->advertised, 1);
- dump_link_caps("Link partner advertised EEE", "", ep->lp_advertised, 1);
+ ethtool_link_mode_zero(link_mode);
+
+ link_mode[0] = ep->supported;
+ dump_link_caps("Supported EEE", "", link_mode, 1);
+
+ link_mode[0] = ep->advertised;
+ dump_link_caps("Advertised EEE", "", link_mode, 1);
+
+ link_mode[0] = ep->lp_advertised;
+ dump_link_caps("Link partner advertised EEE", "", link_mode, 1);
}
#define N_SOTS 7
@@ -2247,10 +2358,220 @@ static int do_sfeatures(struct cmd_context *ctx)
return 0;
}
-static int do_gset(struct cmd_context *ctx)
+static struct ethtool_link_usettings *
+__do_ioctl_glinksettings(struct cmd_context *ctx)
+{
+ int err;
+ struct {
+ struct ethtool_link_settings req;
+ __u32 link_mode_data[3*__ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
+ } ecmd;
+ struct ethtool_link_usettings *link_usettings;
+ unsigned u32_offs;
+
+ /* Handshake with kernel to determine number of words for link
+ * mode bitmaps. When requested number of bitmap words is not
+ * the one expected by kernel, the latter returns the integer
+ * opposite of what it is expecting. We request length 0 below
+ * (aka. invalid bitmap length) to get this info.
+ */
+ memset(&ecmd, 0, sizeof(ecmd));
+ ecmd.req.cmd = ETHTOOL_GLINKSETTINGS;
+ err = send_ioctl(ctx, &ecmd);
+ if (err < 0)
+ return NULL;
+
+ /* see above: we expect a strictly negative value from kernel.
+ */
+ if (ecmd.req.link_mode_masks_nwords >= 0
+ || ecmd.req.cmd != ETHTOOL_GLINKSETTINGS)
+ return NULL;
+
+ /* got the real ecmd.req.link_mode_masks_nwords,
+ * now send the real request
+ */
+ ecmd.req.cmd = ETHTOOL_GLINKSETTINGS;
+ ecmd.req.link_mode_masks_nwords = -ecmd.req.link_mode_masks_nwords;
+ err = send_ioctl(ctx, &ecmd);
+ if (err < 0)
+ return NULL;
+
+ if (ecmd.req.link_mode_masks_nwords <= 0
+ || ecmd.req.cmd != ETHTOOL_GLINKSETTINGS)
+ return NULL;
+
+ /* Convert to usettings struct */
+ link_usettings = calloc(1, sizeof(*link_usettings));
+ if (link_usettings == NULL)
+ return NULL;
+
+ /* keep transceiver 0 */
+ memcpy(&link_usettings->base, &ecmd.req, sizeof(link_usettings->base));
+
+ /* copy link mode bitmaps */
+ u32_offs = 0;
+ memcpy(link_usettings->link_modes.supported,
+ &ecmd.link_mode_data[u32_offs],
+ 4*ecmd.req.link_mode_masks_nwords);
+
+ u32_offs += ecmd.req.link_mode_masks_nwords;
+ memcpy(link_usettings->link_modes.advertising,
+ &ecmd.link_mode_data[u32_offs],
+ 4*ecmd.req.link_mode_masks_nwords);
+
+ u32_offs += ecmd.req.link_mode_masks_nwords;
+ memcpy(link_usettings->link_modes.lp_advertising,
+ &ecmd.link_mode_data[u32_offs],
+ 4*ecmd.req.link_mode_masks_nwords);
+
+ return link_usettings;
+}
+
+static int
+__do_ioctl_slinksettings(struct cmd_context *ctx,
+ const struct ethtool_link_usettings *link_usettings)
+{
+ struct {
+ struct ethtool_link_settings req;
+ __u32 link_mode_data[3*__ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
+ } ecmd;
+ unsigned u32_offs;
+
+ /* refuse to send ETHTOOL_SLINKSETTINGS ioctl if
+ * link_usettings was retrieved with ETHTOOL_GSET
+ */
+ if (link_usettings->base.cmd != ETHTOOL_GLINKSETTINGS)
+ return -1;
+
+ /* refuse to send ETHTOOL_SLINKSETTINGS ioctl if deprecated fields
+ * were set
+ */
+ if (link_usettings->deprecated.transceiver)
+ return -1;
+
+ if (link_usettings->base.link_mode_masks_nwords <= 0)
+ return -1;
+
+ memcpy(&ecmd.req, &link_usettings->base, sizeof(ecmd.req));
+ ecmd.req.cmd = ETHTOOL_SLINKSETTINGS;
+
+ /* copy link mode bitmaps */
+ u32_offs = 0;
+ memcpy(&ecmd.link_mode_data[u32_offs],
+ link_usettings->link_modes.supported,
+ 4*ecmd.req.link_mode_masks_nwords);
+
+ u32_offs += ecmd.req.link_mode_masks_nwords;
+ memcpy(&ecmd.link_mode_data[u32_offs],
+ link_usettings->link_modes.advertising,
+ 4*ecmd.req.link_mode_masks_nwords);
+
+ u32_offs += ecmd.req.link_mode_masks_nwords;
+ memcpy(&ecmd.link_mode_data[u32_offs],
+ link_usettings->link_modes.lp_advertising,
+ 4*ecmd.req.link_mode_masks_nwords);
+
+ return send_ioctl(ctx, &ecmd);
+}
+
+static struct ethtool_link_usettings *
+__do_ioctl_gset(struct cmd_context *ctx)
{
int err;
struct ethtool_cmd ecmd;
+ struct ethtool_link_usettings *link_usettings;
+
+ memset(&ecmd, 0, sizeof(ecmd));
+ ecmd.cmd = ETHTOOL_GSET;
+ err = send_ioctl(ctx, &ecmd);
+ if (err < 0)
+ return NULL;
+
+ link_usettings = calloc(1, sizeof(*link_usettings));
+ if (link_usettings == NULL)
+ return NULL;
+
+ /* remember that ETHTOOL_GSET was used */
+ link_usettings->base.cmd = ETHTOOL_GSET;
+
+ link_usettings->base.link_mode_masks_nwords = 1;
+ link_usettings->link_modes.supported[0] = ecmd.supported;
+ link_usettings->link_modes.advertising[0] = ecmd.advertising;
+ link_usettings->link_modes.lp_advertising[0] = ecmd.lp_advertising;
+ link_usettings->base.speed = ethtool_cmd_speed(&ecmd);
+ link_usettings->base.duplex = ecmd.duplex;
+ link_usettings->base.port = ecmd.port;
+ link_usettings->base.phy_address = ecmd.phy_address;
+ link_usettings->deprecated.transceiver = ecmd.transceiver;
+ link_usettings->base.autoneg = ecmd.autoneg;
+ link_usettings->base.mdio_support = ecmd.mdio_support;
+ /* ignored (fully deprecated): maxrxpkt, maxtxpkt */
+ link_usettings->base.eth_tp_mdix = ecmd.eth_tp_mdix;
+ link_usettings->base.eth_tp_mdix_ctrl = ecmd.eth_tp_mdix_ctrl;
+
+ return link_usettings;
+}
+
+static bool ethtool_link_mode_is_backward_compatible(const u32 *mask)
+{
+ unsigned int i;
+
+ for (i = 1 ; i < __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32 ; ++i)
+ if (mask[i])
+ return false;
+
+ return true;
+}
+
+static int
+__do_ioctl_sset(struct cmd_context *ctx,
+ const struct ethtool_link_usettings *link_usettings)
+{
+ struct ethtool_cmd ecmd;
+
+ /* refuse to send ETHTOOL_SSET ioctl if link_usettings was
+ * retrieved with ETHTOOL_GLINKSETTINGS
+ */
+ if (link_usettings->base.cmd != ETHTOOL_GSET)
+ return -1;
+
+ if (link_usettings->base.link_mode_masks_nwords <= 0)
+ return -1;
+
+ /* refuse to sset if any bit > 31 is set */
+ if (!ethtool_link_mode_is_backward_compatible(
+ link_usettings->link_modes.supported))
+ return -1;
+ if (!ethtool_link_mode_is_backward_compatible(
+ link_usettings->link_modes.advertising))
+ return -1;
+ if (!ethtool_link_mode_is_backward_compatible(
+ link_usettings->link_modes.lp_advertising))
+ return -1;
+
+ memset(&ecmd, 0, sizeof(ecmd));
+ ecmd.cmd = ETHTOOL_SSET;
+
+ ecmd.supported = link_usettings->link_modes.supported[0];
+ ecmd.advertising = link_usettings->link_modes.advertising[0];
+ ecmd.lp_advertising = link_usettings->link_modes.lp_advertising[0];
+ ethtool_cmd_speed_set(&ecmd, link_usettings->base.speed);
+ ecmd.duplex = link_usettings->base.duplex;
+ ecmd.port = link_usettings->base.port;
+ ecmd.phy_address = link_usettings->base.phy_address;
+ ecmd.transceiver = link_usettings->deprecated.transceiver;
+ ecmd.autoneg = link_usettings->base.autoneg;
+ ecmd.mdio_support = link_usettings->base.mdio_support;
+ /* ignored (fully deprecated): maxrxpkt, maxtxpkt */
+ ecmd.eth_tp_mdix = link_usettings->base.eth_tp_mdix;
+ ecmd.eth_tp_mdix_ctrl = link_usettings->base.eth_tp_mdix_ctrl;
+ return send_ioctl(ctx, &ecmd);
+}
+
+static int do_gset(struct cmd_context *ctx)
+{
+ int err;
+ struct ethtool_link_usettings *link_usettings;
struct ethtool_wolinfo wolinfo;
struct ethtool_value edata;
int allfail = 1;
@@ -2260,10 +2581,12 @@ static int do_gset(struct cmd_context *ctx)
fprintf(stdout, "Settings for %s:\n", ctx->devname);
- ecmd.cmd = ETHTOOL_GSET;
- err = send_ioctl(ctx, &ecmd);
- if (err == 0) {
- err = dump_ecmd(&ecmd);
+ link_usettings = __do_ioctl_glinksettings(ctx);
+ if (link_usettings == NULL)
+ link_usettings = __do_ioctl_gset(ctx);
+ if (link_usettings != NULL) {
+ err = dump_link_usettings(link_usettings);
+ free(link_usettings);
if (err)
return err;
allfail = 0;
@@ -2322,8 +2645,10 @@ static int do_sset(struct cmd_context *ctx)
int autoneg_wanted = -1;
int phyad_wanted = -1;
int xcvr_wanted = -1;
- int full_advertising_wanted = -1;
- int advertising_wanted = -1;
+ u32 *full_advertising_wanted = NULL;
+ u32 *advertising_wanted = NULL;
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(_mask_full_advertising_wanted);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(_mask_advertising_wanted);
int gset_changed = 0; /* did anything in GSET change? */
u32 wol_wanted = 0;
int wol_change = 0;
@@ -2337,7 +2662,7 @@ static int do_sset(struct cmd_context *ctx)
int argc = ctx->argc;
char **argp = ctx->argp;
int i;
- int err;
+ int err = 0;
for (i = 0; i < ARRAY_SIZE(flags_msglvl); i++)
flag_to_cmdline_info(flags_msglvl[i].name,
@@ -2411,7 +2736,12 @@ static int do_sset(struct cmd_context *ctx)
i += 1;
if (i >= argc)
exit_bad_args();
- full_advertising_wanted = get_int(argp[i], 16);
+ if (parse_hex_u32_bitmap(
+ argp[i],
+ __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBITS,
+ _mask_full_advertising_wanted))
+ exit_bad_args();
+ full_advertising_wanted = _mask_full_advertising_wanted;
} else if (!strcmp(argp[i], "phyad")) {
gset_changed = 1;
i += 1;
@@ -2468,75 +2798,89 @@ static int do_sset(struct cmd_context *ctx)
}
}
- if (full_advertising_wanted < 0) {
+ if (full_advertising_wanted == NULL) {
/* User didn't supply a full advertisement bitfield:
* construct one from the specified speed and duplex.
*/
+ int adv_bit = -1;
+
if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF)
- advertising_wanted = ADVERTISED_10baseT_Half;
+ adv_bit = ETHTOOL_LINK_MODE_10baseT_Half_BIT;
else if (speed_wanted == SPEED_10 &&
duplex_wanted == DUPLEX_FULL)
- advertising_wanted = ADVERTISED_10baseT_Full;
+ adv_bit = ETHTOOL_LINK_MODE_10baseT_Full_BIT;
else if (speed_wanted == SPEED_100 &&
duplex_wanted == DUPLEX_HALF)
- advertising_wanted = ADVERTISED_100baseT_Half;
+ adv_bit = ETHTOOL_LINK_MODE_100baseT_Half_BIT;
else if (speed_wanted == SPEED_100 &&
duplex_wanted == DUPLEX_FULL)
- advertising_wanted = ADVERTISED_100baseT_Full;
+ adv_bit = ETHTOOL_LINK_MODE_100baseT_Full_BIT;
else if (speed_wanted == SPEED_1000 &&
duplex_wanted == DUPLEX_HALF)
- advertising_wanted = ADVERTISED_1000baseT_Half;
+ adv_bit = ETHTOOL_LINK_MODE_1000baseT_Half_BIT;
else if (speed_wanted == SPEED_1000 &&
duplex_wanted == DUPLEX_FULL)
- advertising_wanted = ADVERTISED_1000baseT_Full;
+ adv_bit = ETHTOOL_LINK_MODE_1000baseT_Full_BIT;
else if (speed_wanted == SPEED_2500 &&
duplex_wanted == DUPLEX_FULL)
- advertising_wanted = ADVERTISED_2500baseX_Full;
+ adv_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
else if (speed_wanted == SPEED_10000 &&
duplex_wanted == DUPLEX_FULL)
- advertising_wanted = ADVERTISED_10000baseT_Full;
- else
- /* auto negotiate without forcing,
- * all supported speed will be assigned below
- */
- advertising_wanted = 0;
+ adv_bit = ETHTOOL_LINK_MODE_10000baseT_Full_BIT;
+
+ if (adv_bit >= 0) {
+ advertising_wanted = _mask_advertising_wanted;
+ ethtool_link_mode_zero(advertising_wanted);
+ ethtool_link_mode_set_bit(
+ adv_bit, advertising_wanted);
+ }
+ /* otherwise: auto negotiate without forcing,
+ * all supported speed will be assigned below
+ */
}
if (gset_changed) {
- struct ethtool_cmd ecmd;
+ struct ethtool_link_usettings *link_usettings;
- ecmd.cmd = ETHTOOL_GSET;
- err = send_ioctl(ctx, &ecmd);
- if (err < 0) {
+ link_usettings = __do_ioctl_glinksettings(ctx);
+ if (link_usettings == NULL)
+ link_usettings = __do_ioctl_gset(ctx);
+ if (link_usettings == NULL) {
perror("Cannot get current device settings");
+ err = -1;
} else {
/* Change everything the user specified. */
if (speed_wanted != -1)
- ethtool_cmd_speed_set(&ecmd, speed_wanted);
+ link_usettings->base.speed = speed_wanted;
if (duplex_wanted != -1)
- ecmd.duplex = duplex_wanted;
+ link_usettings->base.duplex = duplex_wanted;
if (port_wanted != -1)
- ecmd.port = port_wanted;
+ link_usettings->base.port = port_wanted;
if (mdix_wanted != -1) {
/* check driver supports MDI-X */
- if (ecmd.eth_tp_mdix_ctrl != ETH_TP_MDI_INVALID)
- ecmd.eth_tp_mdix_ctrl = mdix_wanted;
+ if (link_usettings->base.eth_tp_mdix_ctrl
+ != ETH_TP_MDI_INVALID)
+ link_usettings->base.eth_tp_mdix_ctrl
+ = mdix_wanted;
else
- fprintf(stderr, "setting MDI not supported\n");
+ fprintf(stderr,
+ "setting MDI not supported\n");
}
if (autoneg_wanted != -1)
- ecmd.autoneg = autoneg_wanted;
+ link_usettings->base.autoneg = autoneg_wanted;
if (phyad_wanted != -1)
- ecmd.phy_address = phyad_wanted;
+ link_usettings->base.phy_address = phyad_wanted;
if (xcvr_wanted != -1)
- ecmd.transceiver = xcvr_wanted;
+ link_usettings->deprecated.transceiver
+ = xcvr_wanted;
/* XXX If the user specified speed or duplex
* then we should mask the advertised modes
* accordingly. For now, warn that we aren't
* doing that.
*/
if ((speed_wanted != -1 || duplex_wanted != -1) &&
- ecmd.autoneg && advertising_wanted == 0) {
+ link_usettings->base.autoneg &&
+ advertising_wanted == NULL) {
fprintf(stderr, "Cannot advertise");
if (speed_wanted >= 0)
fprintf(stderr, " speed %d",
@@ -2548,37 +2892,56 @@ static int do_sset(struct cmd_context *ctx)
fprintf(stderr, "\n");
}
if (autoneg_wanted == AUTONEG_ENABLE &&
- advertising_wanted == 0) {
+ advertising_wanted == NULL) {
+ unsigned int i;
+
/* Auto negotiation enabled, but with
* unspecified speed and duplex: enable all
* supported speeds and duplexes.
*/
- ecmd.advertising =
- (ecmd.advertising &
- ~ALL_ADVERTISED_MODES) |
- (ALL_ADVERTISED_MODES & ecmd.supported);
+ ethtool_link_mode_for_each_u32(i) {
+ u32 sup = link_usettings->link_modes.supported[i];
+ u32 *adv = link_usettings->link_modes.advertising + i;
+
+ *adv = ((*adv & ~all_advertised_modes[i])
+ | (sup & all_advertised_modes[i]));
+ }
/* If driver supports unknown flags, we cannot
* be sure that we enable all link modes.
*/
- if ((ecmd.supported & ALL_ADVERTISED_FLAGS) !=
- ecmd.supported) {
- fprintf(stderr, "Driver supports one "
- "or more unknown flags\n");
+ ethtool_link_mode_for_each_u32(i) {
+ u32 sup = link_usettings->link_modes.supported[i];
+
+ if ((sup & all_advertised_flags[i]) != sup) {
+ fprintf(stderr, "Driver supports one or more unknown flags\n");
+ break;
+ }
}
- } else if (advertising_wanted > 0) {
+ } else if (advertising_wanted != NULL) {
+ unsigned int i;
+
/* Enable all requested modes */
- ecmd.advertising =
- (ecmd.advertising &
- ~ALL_ADVERTISED_MODES) |
- advertising_wanted;
- } else if (full_advertising_wanted > 0) {
- ecmd.advertising = full_advertising_wanted;
+ ethtool_link_mode_for_each_u32(i) {
+ u32 *adv = link_usettings->link_modes.advertising + i;
+
+ *adv = ((*adv & ~all_advertised_modes[i])
+ | advertising_wanted[i]);
+ }
+ } else if (full_advertising_wanted != NULL) {
+ ethtool_link_mode_copy(
+ link_usettings->link_modes.advertising,
+ full_advertising_wanted);
}
/* Try to perform the update. */
- ecmd.cmd = ETHTOOL_SSET;
- err = send_ioctl(ctx, &ecmd);
+ if (link_usettings->base.cmd == ETHTOOL_GLINKSETTINGS)
+ err = __do_ioctl_slinksettings(ctx,
+ link_usettings);
+ else
+ err = __do_ioctl_sset(ctx,
+ link_usettings);
+ free(link_usettings);
if (err < 0)
perror("Cannot set new settings");
}
@@ -4230,6 +4593,8 @@ int main(int argc, char **argp)
struct cmd_context ctx;
int k;
+ init_global_link_mode_masks();
+
/* Skip command name */
argp++;
argc--;
diff --git a/internal.h b/internal.h
index e98f532..5e78b5c 100644
--- a/internal.h
+++ b/internal.h
@@ -12,6 +12,7 @@
#ifdef HAVE_CONFIG_H
#include "ethtool-config.h"
#endif
+#include <stdbool.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
@@ -122,6 +123,72 @@ static inline int test_bit(unsigned int nr, const unsigned long *addr)
ETH_FLAG_TXVLAN | ETH_FLAG_NTUPLE | \
ETH_FLAG_RXHASH)
+/* internal API for link mode bitmap interaction with kernel. */
+
+#define __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32 \
+ (SCHAR_MAX)
+#define __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBITS \
+ (32*__ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32)
+#define __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBYTES \
+ (4*__ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32)
+#define __ETHTOOL_DECLARE_LINK_MODE_MASK(name) \
+ u32 name[__ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]
+
+struct ethtool_link_usettings {
+ struct {
+ __u8 transceiver;
+ } deprecated;
+ struct ethtool_link_settings base;
+ struct {
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
+ } link_modes;
+};
+
+#define ethtool_link_mode_for_each_u32(index) \
+ for ((index) = 0 ; \
+ (index) < __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32 ; \
+ ++(index))
+
+static inline void ethtool_link_mode_zero(u32 *dst)
+{
+ memset(dst, 0, __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBYTES);
+}
+
+static inline bool ethtool_link_mode_is_empty(const u32 *mask)
+{
+ unsigned int i;
+
+ ethtool_link_mode_for_each_u32(i)
+ {
+ if (mask[i] != 0)
+ return false;
+ }
+
+ return true;
+}
+
+static inline void ethtool_link_mode_copy(u32 *dst, const u32 *src)
+{
+ memcpy(dst, src, __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBYTES);
+}
+
+static inline int ethtool_link_mode_test_bit(unsigned int nr, const u32 *mask)
+{
+ if (nr >= __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBITS)
+ return !!0;
+ return !!(mask[nr / 32] & (1 << (nr % 32)));
+}
+
+static inline int ethtool_link_mode_set_bit(unsigned int nr, u32 *mask)
+{
+ if (nr >= __ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBITS)
+ return -1;
+ mask[nr / 32] |= (1 << (nr % 32));
+ return 0;
+}
+
/* Context for sub-commands */
struct cmd_context {
const char *devname; /* net device name */
diff --git a/test-cmdline.c b/test-cmdline.c
index 2fd7cbb..a94edea 100644
--- a/test-cmdline.c
+++ b/test-cmdline.c
@@ -37,7 +37,20 @@ static struct test_case {
{ 1, "--change devname autoneg foo" },
{ 1, "-s devname autoneg" },
{ 0, "--change devname advertise 0x1" },
+ { 0, "--change devname advertise 0xf" },
+ { 0, "--change devname advertise 0Xf" },
+ { 0, "--change devname advertise 1" },
+ { 0, "--change devname advertise f" },
+ { 0, "--change devname advertise 01" },
+ { 0, "--change devname advertise 0f" },
+ { 0, "--change devname advertise 0xfffffffffffffffffffffffffffffffff" },
+ { 0, "--change devname advertise fffffffffffffffffffffffffffffffff" },
+ { 0, "--change devname advertise 0x0000fffffffffffffffffffffffffffff" },
+ { 0, "--change devname advertise 0000fffffffffffffffffffffffffffff" },
+ { 1, "-s devname advertise" },
+ { 1, "-s devname advertise 0x" },
{ 1, "-s devname advertise foo" },
+ { 1, "-s devname advertise 0xfoo" },
{ 1, "--change devname advertise" },
{ 0, "-s devname phyad 1" },
{ 1, "--change devname phyad foo" },
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related
* Re: [ethtool PATCH v4 10/11] ethtool.c: add support for ETHTOOL_xLINKSETTINGS ioctls
From: David Decotigny @ 2016-03-15 23:42 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev@vger.kernel.org, Jeff Garzik, David Miller,
Vidya Sagar Ravipati, Joe Perches
In-Reply-To: <1457919156.3331.90.camel@decadent.org.uk>
Just sent v5 of the series: now only this patch left. Applied all your
suggestions.
Many thanks for the 2 bugs you caught, path was not covered by my
tests. v5 was tested on a 10G nic with: ethtool -s eth1 msglvl 0x15
speed 10000 duplex full
On Sun, Mar 13, 2016 at 6:32 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Fri, 2016-03-11 at 09:58 -0800, David Decotigny wrote:
> [...]
>> +static int parse_hex_u32_bitmap(const char *s,
>> + unsigned int nbits, u32 *result)
>> +{
>> + const unsigned nwords = __KERNEL_DIV_ROUND_UP(nbits, 32);
>> + size_t slen = strlen(s);
>> + size_t i;
>> +
>> + /* ignore optional '0x' prefix */
>> + if ((slen > 2) && (
>> + (0 == memcmp(s, "0x", 2)
>> + || (0 == memcmp(s, "0X", 2))))) {
>
> memcmp() is a really poor tool for comparing strings. You should use
> strncasecmp() here.
done.
>
>> + slen -= 2;
>> + s += 2;
>> + }
>> +
>> + if (slen > 8*nwords) /* up to 2 digits per byte */
>
> The '*' operator should have spaces around it. Please run
> checkpatch.pl over your ethtool patches to catch style errors like this
> (there are many others in this one).
both done. a few new lines over 80 chars that I prefer to keep,
otherwise line break would make code harder to read imho.
>
> [...]
>> +static void dump_link_caps(const char *prefix, const char *an_prefix,
>> + const u32 *mask, int link_mode_only)
>> {
>> static const struct {
>> int same_line; /* print on same line as previous */
>> - u32 value;
>> + unsigned bit_indice;
>
> bit_index
done.
>
> [...]
>> @@ -558,51 +655,57 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
>> }
>> }
>> if (did1 == 0)
>> - fprintf(stdout, "Not reported");
>> + fprintf(stdout, "Not reported");
>
> Good catch, but whitespace fixes belong in another patch.
removed for now.
>
> [...]
>
>> @@ -2248,10 +2360,219 @@ static int do_sfeatures(struct cmd_context *ctx)
>> return 0;
>> }
>>
>> -static int do_gset(struct cmd_context *ctx)
>> +static struct ethtool_link_usettings *
>> +__do_ioctl_glinksettings(struct cmd_context * ctx)
>> +{
>> + int err;
>> + struct {
>> + struct ethtool_link_settings req;
>> + __u32 link_mode_data[3*__ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
>> + } ecmd;
>> + struct ethtool_link_usettings *link_usettings;
>> + unsigned u32_offs;
>> +
>> + /* handshake with kernel to determine number of words for link
>> + * mode bitmaps */
>> + memset(&ecmd, 0, sizeof(ecmd));
>> + ecmd.req.cmd = ETHTOOL_GLINKSETTINGS;
>> + err = send_ioctl(ctx, &ecmd);
>> + if (err < 0)
>> + return NULL;
>> +
>> + if (ecmd.req.link_mode_masks_nwords >= 0 || ecmd.req.cmd)
>> + return NULL;
>
> Oops, I missed the kernel side of this. Clearing the cmd field is a
> very strange thing to do and inconsistent with every other ethtool
> command, so I've just sent a patch to change the kernel side of that.
> You'll need to change this to match.
>
> (I also think the negative size is a bit weird, but don't feel so
> strongly about it.)
updated based on your patch. sent a suggestion
http://marc.info/?l=linux-netdev&m=145806871332416 , looks like it was
not added properly to the thread, because I don't see it as a reply to
your patch in patchwork. sorry about this.
>
> [...]
>> + link_usettings = malloc(sizeof(*link_usettings));
>> + if (NULL == link_usettings)
>
> Comparison is the wrong way round.
done.
>
>> + return NULL;
>> +
>> + memset(link_usettings, 0, sizeof(*link_usettings));
>
> Could use calloc() instead of malloc() + memset().
done.
>
>> + /* keep transceiver 0 */
>> + memcpy(&link_usettings->base, &ecmd.req, sizeof(link_usettings->base));
>> + /* remember that ETHTOOL_GLINKSETTINGS was used */
>> + link_usettings->base.cmd = ETHTOOL_GLINKSETTINGS;
>
> This is redundant as we know that ecmd.req.cmd ==
> ETHTOOL_GLINKSETTINGS.
done.
>
> [...]
>> +static struct ethtool_link_usettings *
>> +__do_ioctl_gset(struct cmd_context * ctx)
>> {
> [...]
>> + link_usettings->base.link_mode_masks_nwords = 1;
>> + link_usettings->link_modes.supported[0] = ecmd.supported;
>> + link_usettings->link_modes.advertising[0] = ecmd.advertising;
>> + link_usettings->link_modes.lp_advertising[0] = ecmd.lp_advertising;
>> + link_usettings->base.speed = ethtool_cmd_speed(&ecmd);
>> + link_usettings->base.duplex = ecmd.duplex;
>> + link_usettings->base.port = ecmd.port;
>> + link_usettings->base.phy_address = ecmd.phy_address;
>> + link_usettings->deprecated.transceiver = ecmd.transceiver;
>> + link_usettings->base.autoneg = ecmd.autoneg;
>> + link_usettings->base.mdio_support = ecmd.mdio_support;
>> + /* ignored (fully deprecated): maxrxpkt, maxtxpkt */
>> + link_usettings->base.eth_tp_mdix = ecmd.eth_tp_mdix;
>> + link_usettings->base.eth_tp_mdix_ctrl = ecmd.eth_tp_mdix_ctrl;
>> + link_usettings->deprecated.transceiver = ecmd.transceiver;
>
> Duplicate assignment.
done.
>
>> + return link_usettings;
>> +}
>> +
>> +static bool ethtool_link_mode_is_backward_compatible(
>> + const u32 *mask)
>
> Don't wrap function parameter lists or statements that are already
> under 80 characters.
done.
>
> [...]
>> +static void
>> +__free_link_usettings(struct ethtool_link_usettings *link_usettings) {
>> + free(link_usettings);
>> +}
>
> Is this function ever likely to do more than just free()? If not,
> don't bother abstracting that.
free() is likely to stay good enough. updated.
>
> [...]
>> @@ -2469,75 +2799,88 @@ static int do_sset(struct cmd_context *ctx)
>> }
>> }
>>
>> - if (full_advertising_wanted < 0) {
>> + if (NULL == full_advertising_wanted) {
>> /* User didn't supply a full advertisement bitfield:
>> * construct one from the specified speed and duplex.
>> */
>> + int adv_bit = -1;
>> +
>> if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF)
>> - advertising_wanted = ADVERTISED_10baseT_Half;
>> + adv_bit = ETHTOOL_LINK_MODE_10baseT_Half_BIT;
>> else if (speed_wanted == SPEED_10 &&
>> duplex_wanted == DUPLEX_FULL)
>> - advertising_wanted = ADVERTISED_10baseT_Full;
>> + adv_bit = ETHTOOL_LINK_MODE_10baseT_Full_BIT;
>> else if (speed_wanted == SPEED_100 &&
>> duplex_wanted == DUPLEX_HALF)
>> - advertising_wanted = ADVERTISED_100baseT_Half;
>> + adv_bit = ETHTOOL_LINK_MODE_100baseT_Half_BIT;
>> else if (speed_wanted == SPEED_100 &&
>> duplex_wanted == DUPLEX_FULL)
>> - advertising_wanted = ADVERTISED_100baseT_Full;
>> + adv_bit = ETHTOOL_LINK_MODE_100baseT_Full_BIT;
>> else if (speed_wanted == SPEED_1000 &&
>> duplex_wanted == DUPLEX_HALF)
>> - advertising_wanted = ADVERTISED_1000baseT_Half;
>> + adv_bit = ETHTOOL_LINK_MODE_1000baseT_Half_BIT;
>> else if (speed_wanted == SPEED_1000 &&
>> duplex_wanted == DUPLEX_FULL)
>> - advertising_wanted = ADVERTISED_1000baseT_Full;
>> + adv_bit = ETHTOOL_LINK_MODE_1000baseT_Full_BIT;
>> else if (speed_wanted == SPEED_2500 &&
>> duplex_wanted == DUPLEX_FULL)
>> - advertising_wanted = ADVERTISED_2500baseX_Full;
>> + adv_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
>> else if (speed_wanted == SPEED_10000 &&
>> duplex_wanted == DUPLEX_FULL)
>> - advertising_wanted = ADVERTISED_10000baseT_Full;
>> - else
>> - /* auto negotiate without forcing,
>> - * all supported speed will be assigned below
>> - */
>> - advertising_wanted = 0;
>> + adv_bit = ETHTOOL_LINK_MODE_10000baseT_Full_BIT;
>> +
>> + if (adv_bit >= 0) {
>> + advertising_wanted = _mask_advertising_wanted;
>
> If I'm not mistaken, _mask_advertising_wanted is uninitialised at this
> point so you need to clear it before setting the one bit.
great catch! thanks.
>
>> + ethtool_link_mode_set_bit(
>> + adv_bit, advertising_wanted);
> [...]
>> @@ -2549,37 +2892,56 @@ static int do_sset(struct cmd_context *ctx)
>> fprintf(stderr, "\n");
>> }
>> if (autoneg_wanted == AUTONEG_ENABLE &&
>> - advertising_wanted == 0) {
>> + NULL == advertising_wanted) {
>> + unsigned int i;
>> +
>> /* Auto negotiation enabled, but with
>> * unspecified speed and duplex: enable all
>> * supported speeds and duplexes.
>> */
>> - ecmd.advertising =
>> - (ecmd.advertising &
>> - ~ALL_ADVERTISED_MODES) |
>> - (ALL_ADVERTISED_MODES & ecmd.supported);
>> + ethtool_link_mode_for_each_u32(i)
>> + {
>
> Brace belongs on the previous line (everywhere you use
> ethtool_link_mode_for_each_u32()).
done.
>
> [...]
>> - } else if (advertising_wanted > 0) {
>> + } else if (NULL != advertising_wanted) {
>> + unsigned int i;
>> +
>> /* Enable all requested modes */
>> - ecmd.advertising =
>> - (ecmd.advertising &
>> - ~ALL_ADVERTISED_MODES) |
>> - advertising_wanted;
>> - } else if (full_advertising_wanted > 0) {
>> - ecmd.advertising = full_advertising_wanted;
>> + ethtool_link_mode_for_each_u32(i)
>> + {
>> + u32 *adv = link_usettings->link_modes.advertising + i;
>> + *adv = ((*adv & all_advertised_modes[i])
> [...]
>
> Should be ~all_advertised_modes[i] rather than all_advertised_modes[i].
aouch! thanks.
>
> Ben.
>
> --
> Ben Hutchings
> If at first you don't succeed, you're doing about average.
^ permalink raw reply
* Re: [ethtool PATCH v4 10/11] ethtool.c: add support for ETHTOOL_xLINKSETTINGS ioctls
From: David Decotigny @ 2016-03-15 23:47 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev@vger.kernel.org, Jeff Garzik, David Miller,
Vidya Sagar Ravipati, Joe Perches
In-Reply-To: <CAG88wWZkeiR_X0rKR9WOVjDnz6t9jRZDF7QXmjcAfP+d60BXOw@mail.gmail.com>
Please note that v5 patch depends on Ben Hutchings'
http://patchwork.ozlabs.org/patch/596879/ to make any sense.
On Tue, Mar 15, 2016 at 4:42 PM, David Decotigny <ddecotig@gmail.com> wrote:
> Just sent v5 of the series: now only this patch left. Applied all your
> suggestions.
>
> Many thanks for the 2 bugs you caught, path was not covered by my
> tests. v5 was tested on a 10G nic with: ethtool -s eth1 msglvl 0x15
> speed 10000 duplex full
>
> On Sun, Mar 13, 2016 at 6:32 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>> On Fri, 2016-03-11 at 09:58 -0800, David Decotigny wrote:
>> [...]
>>> +static int parse_hex_u32_bitmap(const char *s,
>>> + unsigned int nbits, u32 *result)
>>> +{
>>> + const unsigned nwords = __KERNEL_DIV_ROUND_UP(nbits, 32);
>>> + size_t slen = strlen(s);
>>> + size_t i;
>>> +
>>> + /* ignore optional '0x' prefix */
>>> + if ((slen > 2) && (
>>> + (0 == memcmp(s, "0x", 2)
>>> + || (0 == memcmp(s, "0X", 2))))) {
>>
>> memcmp() is a really poor tool for comparing strings. You should use
>> strncasecmp() here.
>
> done.
>
>>
>>> + slen -= 2;
>>> + s += 2;
>>> + }
>>> +
>>> + if (slen > 8*nwords) /* up to 2 digits per byte */
>>
>> The '*' operator should have spaces around it. Please run
>> checkpatch.pl over your ethtool patches to catch style errors like this
>> (there are many others in this one).
>
> both done. a few new lines over 80 chars that I prefer to keep,
> otherwise line break would make code harder to read imho.
>
>>
>> [...]
>>> +static void dump_link_caps(const char *prefix, const char *an_prefix,
>>> + const u32 *mask, int link_mode_only)
>>> {
>>> static const struct {
>>> int same_line; /* print on same line as previous */
>>> - u32 value;
>>> + unsigned bit_indice;
>>
>> bit_index
>
> done.
>
>>
>> [...]
>>> @@ -558,51 +655,57 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
>>> }
>>> }
>>> if (did1 == 0)
>>> - fprintf(stdout, "Not reported");
>>> + fprintf(stdout, "Not reported");
>>
>> Good catch, but whitespace fixes belong in another patch.
>
> removed for now.
>
>>
>> [...]
>>
>>> @@ -2248,10 +2360,219 @@ static int do_sfeatures(struct cmd_context *ctx)
>>> return 0;
>>> }
>>>
>>> -static int do_gset(struct cmd_context *ctx)
>>> +static struct ethtool_link_usettings *
>>> +__do_ioctl_glinksettings(struct cmd_context * ctx)
>>> +{
>>> + int err;
>>> + struct {
>>> + struct ethtool_link_settings req;
>>> + __u32 link_mode_data[3*__ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
>>> + } ecmd;
>>> + struct ethtool_link_usettings *link_usettings;
>>> + unsigned u32_offs;
>>> +
>>> + /* handshake with kernel to determine number of words for link
>>> + * mode bitmaps */
>>> + memset(&ecmd, 0, sizeof(ecmd));
>>> + ecmd.req.cmd = ETHTOOL_GLINKSETTINGS;
>>> + err = send_ioctl(ctx, &ecmd);
>>> + if (err < 0)
>>> + return NULL;
>>> +
>>> + if (ecmd.req.link_mode_masks_nwords >= 0 || ecmd.req.cmd)
>>> + return NULL;
>>
>> Oops, I missed the kernel side of this. Clearing the cmd field is a
>> very strange thing to do and inconsistent with every other ethtool
>> command, so I've just sent a patch to change the kernel side of that.
>> You'll need to change this to match.
>>
>> (I also think the negative size is a bit weird, but don't feel so
>> strongly about it.)
>
> updated based on your patch. sent a suggestion
> http://marc.info/?l=linux-netdev&m=145806871332416 , looks like it was
> not added properly to the thread, because I don't see it as a reply to
> your patch in patchwork. sorry about this.
>
>>
>> [...]
>>> + link_usettings = malloc(sizeof(*link_usettings));
>>> + if (NULL == link_usettings)
>>
>> Comparison is the wrong way round.
>
> done.
>
>>
>>> + return NULL;
>>> +
>>> + memset(link_usettings, 0, sizeof(*link_usettings));
>>
>> Could use calloc() instead of malloc() + memset().
>
> done.
>
>>
>>> + /* keep transceiver 0 */
>>> + memcpy(&link_usettings->base, &ecmd.req, sizeof(link_usettings->base));
>>> + /* remember that ETHTOOL_GLINKSETTINGS was used */
>>> + link_usettings->base.cmd = ETHTOOL_GLINKSETTINGS;
>>
>> This is redundant as we know that ecmd.req.cmd ==
>> ETHTOOL_GLINKSETTINGS.
>
> done.
>
>>
>> [...]
>>> +static struct ethtool_link_usettings *
>>> +__do_ioctl_gset(struct cmd_context * ctx)
>>> {
>> [...]
>>> + link_usettings->base.link_mode_masks_nwords = 1;
>>> + link_usettings->link_modes.supported[0] = ecmd.supported;
>>> + link_usettings->link_modes.advertising[0] = ecmd.advertising;
>>> + link_usettings->link_modes.lp_advertising[0] = ecmd.lp_advertising;
>>> + link_usettings->base.speed = ethtool_cmd_speed(&ecmd);
>>> + link_usettings->base.duplex = ecmd.duplex;
>>> + link_usettings->base.port = ecmd.port;
>>> + link_usettings->base.phy_address = ecmd.phy_address;
>>> + link_usettings->deprecated.transceiver = ecmd.transceiver;
>>> + link_usettings->base.autoneg = ecmd.autoneg;
>>> + link_usettings->base.mdio_support = ecmd.mdio_support;
>>> + /* ignored (fully deprecated): maxrxpkt, maxtxpkt */
>>> + link_usettings->base.eth_tp_mdix = ecmd.eth_tp_mdix;
>>> + link_usettings->base.eth_tp_mdix_ctrl = ecmd.eth_tp_mdix_ctrl;
>>> + link_usettings->deprecated.transceiver = ecmd.transceiver;
>>
>> Duplicate assignment.
>
> done.
>
>>
>>> + return link_usettings;
>>> +}
>>> +
>>> +static bool ethtool_link_mode_is_backward_compatible(
>>> + const u32 *mask)
>>
>> Don't wrap function parameter lists or statements that are already
>> under 80 characters.
>
> done.
>
>>
>> [...]
>>> +static void
>>> +__free_link_usettings(struct ethtool_link_usettings *link_usettings) {
>>> + free(link_usettings);
>>> +}
>>
>> Is this function ever likely to do more than just free()? If not,
>> don't bother abstracting that.
>
> free() is likely to stay good enough. updated.
>
>>
>> [...]
>>> @@ -2469,75 +2799,88 @@ static int do_sset(struct cmd_context *ctx)
>>> }
>>> }
>>>
>>> - if (full_advertising_wanted < 0) {
>>> + if (NULL == full_advertising_wanted) {
>>> /* User didn't supply a full advertisement bitfield:
>>> * construct one from the specified speed and duplex.
>>> */
>>> + int adv_bit = -1;
>>> +
>>> if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF)
>>> - advertising_wanted = ADVERTISED_10baseT_Half;
>>> + adv_bit = ETHTOOL_LINK_MODE_10baseT_Half_BIT;
>>> else if (speed_wanted == SPEED_10 &&
>>> duplex_wanted == DUPLEX_FULL)
>>> - advertising_wanted = ADVERTISED_10baseT_Full;
>>> + adv_bit = ETHTOOL_LINK_MODE_10baseT_Full_BIT;
>>> else if (speed_wanted == SPEED_100 &&
>>> duplex_wanted == DUPLEX_HALF)
>>> - advertising_wanted = ADVERTISED_100baseT_Half;
>>> + adv_bit = ETHTOOL_LINK_MODE_100baseT_Half_BIT;
>>> else if (speed_wanted == SPEED_100 &&
>>> duplex_wanted == DUPLEX_FULL)
>>> - advertising_wanted = ADVERTISED_100baseT_Full;
>>> + adv_bit = ETHTOOL_LINK_MODE_100baseT_Full_BIT;
>>> else if (speed_wanted == SPEED_1000 &&
>>> duplex_wanted == DUPLEX_HALF)
>>> - advertising_wanted = ADVERTISED_1000baseT_Half;
>>> + adv_bit = ETHTOOL_LINK_MODE_1000baseT_Half_BIT;
>>> else if (speed_wanted == SPEED_1000 &&
>>> duplex_wanted == DUPLEX_FULL)
>>> - advertising_wanted = ADVERTISED_1000baseT_Full;
>>> + adv_bit = ETHTOOL_LINK_MODE_1000baseT_Full_BIT;
>>> else if (speed_wanted == SPEED_2500 &&
>>> duplex_wanted == DUPLEX_FULL)
>>> - advertising_wanted = ADVERTISED_2500baseX_Full;
>>> + adv_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
>>> else if (speed_wanted == SPEED_10000 &&
>>> duplex_wanted == DUPLEX_FULL)
>>> - advertising_wanted = ADVERTISED_10000baseT_Full;
>>> - else
>>> - /* auto negotiate without forcing,
>>> - * all supported speed will be assigned below
>>> - */
>>> - advertising_wanted = 0;
>>> + adv_bit = ETHTOOL_LINK_MODE_10000baseT_Full_BIT;
>>> +
>>> + if (adv_bit >= 0) {
>>> + advertising_wanted = _mask_advertising_wanted;
>>
>> If I'm not mistaken, _mask_advertising_wanted is uninitialised at this
>> point so you need to clear it before setting the one bit.
>
> great catch! thanks.
>
>>
>>> + ethtool_link_mode_set_bit(
>>> + adv_bit, advertising_wanted);
>> [...]
>>> @@ -2549,37 +2892,56 @@ static int do_sset(struct cmd_context *ctx)
>>> fprintf(stderr, "\n");
>>> }
>>> if (autoneg_wanted == AUTONEG_ENABLE &&
>>> - advertising_wanted == 0) {
>>> + NULL == advertising_wanted) {
>>> + unsigned int i;
>>> +
>>> /* Auto negotiation enabled, but with
>>> * unspecified speed and duplex: enable all
>>> * supported speeds and duplexes.
>>> */
>>> - ecmd.advertising =
>>> - (ecmd.advertising &
>>> - ~ALL_ADVERTISED_MODES) |
>>> - (ALL_ADVERTISED_MODES & ecmd.supported);
>>> + ethtool_link_mode_for_each_u32(i)
>>> + {
>>
>> Brace belongs on the previous line (everywhere you use
>> ethtool_link_mode_for_each_u32()).
>
> done.
>
>>
>> [...]
>>> - } else if (advertising_wanted > 0) {
>>> + } else if (NULL != advertising_wanted) {
>>> + unsigned int i;
>>> +
>>> /* Enable all requested modes */
>>> - ecmd.advertising =
>>> - (ecmd.advertising &
>>> - ~ALL_ADVERTISED_MODES) |
>>> - advertising_wanted;
>>> - } else if (full_advertising_wanted > 0) {
>>> - ecmd.advertising = full_advertising_wanted;
>>> + ethtool_link_mode_for_each_u32(i)
>>> + {
>>> + u32 *adv = link_usettings->link_modes.advertising + i;
>>> + *adv = ((*adv & all_advertised_modes[i])
>> [...]
>>
>> Should be ~all_advertised_modes[i] rather than all_advertised_modes[i].
>
> aouch! thanks.
>
>>
>> Ben.
>>
>> --
>> Ben Hutchings
>> If at first you don't succeed, you're doing about average.
^ permalink raw reply
* [PATCH net-next 0/3] Minor BPF follow-ups
From: Daniel Borkmann @ 2016-03-16 0:42 UTC (permalink / raw)
To: davem; +Cc: alexei.starovoitov, netdev, Daniel Borkmann
Some minor last follow-ups I still had in my queue. The first one adds
readability support for __sk_buff's tc_classid member, the remaining
two are some minor cleanups. For details please see individual patches.
Thanks!
Daniel Borkmann (3):
bpf: make skb->tc_classid also readable
bpf, dst: add and use dst_tclassid helper
ip_tunnels, bpf: define IP_TUNNEL_OPTS_MAX and use it
include/net/dst.h | 12 ++++++++++++
include/net/ip_tunnels.h | 7 +++++++
net/core/filter.c | 30 +++++++++---------------------
net/ipv4/ip_tunnel_core.c | 6 ++++++
net/openvswitch/flow.h | 2 +-
5 files changed, 35 insertions(+), 22 deletions(-)
--
1.9.3
^ permalink raw reply
* [PATCH net-next 3/3] ip_tunnels, bpf: define IP_TUNNEL_OPTS_MAX and use it
From: Daniel Borkmann @ 2016-03-16 0:42 UTC (permalink / raw)
To: davem; +Cc: alexei.starovoitov, netdev, Daniel Borkmann
In-Reply-To: <cover.1458085814.git.daniel@iogearbox.net>
eBPF defines this as BPF_TUNLEN_MAX and OVS just uses the hard-coded
value inside struct sw_flow_key. Thus, add and use IP_TUNNEL_OPTS_MAX
for this, which makes the code a bit more generic and allows to remove
BPF_TUNLEN_MAX from eBPF code.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/net/ip_tunnels.h | 7 +++++++
net/core/filter.c | 9 ++-------
net/ipv4/ip_tunnel_core.c | 6 ++++++
net/openvswitch/flow.h | 2 +-
4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 5dc2e45..c35dda9 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -7,6 +7,8 @@
#include <linux/socket.h>
#include <linux/types.h>
#include <linux/u64_stats_sync.h>
+#include <linux/bitops.h>
+
#include <net/dsfield.h>
#include <net/gro_cells.h>
#include <net/inet_ecn.h>
@@ -57,6 +59,11 @@ struct ip_tunnel_key {
#define IP_TUNNEL_INFO_TX 0x01 /* represents tx tunnel parameters */
#define IP_TUNNEL_INFO_IPV6 0x02 /* key contains IPv6 addresses */
+/* Maximum tunnel options length. */
+#define IP_TUNNEL_OPTS_MAX \
+ GENMASK((FIELD_SIZEOF(struct ip_tunnel_info, \
+ options_len) * BITS_PER_BYTE) - 1, 0)
+
struct ip_tunnel_info {
struct ip_tunnel_key key;
#ifdef CONFIG_DST_CACHE
diff --git a/net/core/filter.c b/net/core/filter.c
index 4c35d83..b7177d0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1904,8 +1904,6 @@ static const struct bpf_func_proto bpf_skb_set_tunnel_key_proto = {
.arg4_type = ARG_ANYTHING,
};
-#define BPF_TUNLEN_MAX 255
-
static u64 bpf_skb_set_tunnel_opt(u64 r1, u64 r2, u64 size, u64 r4, u64 r5)
{
struct sk_buff *skb = (struct sk_buff *) (long) r1;
@@ -1915,7 +1913,7 @@ static u64 bpf_skb_set_tunnel_opt(u64 r1, u64 r2, u64 size, u64 r4, u64 r5)
if (unlikely(info != &md->u.tun_info || (size & (sizeof(u32) - 1))))
return -EINVAL;
- if (unlikely(size > BPF_TUNLEN_MAX))
+ if (unlikely(size > IP_TUNNEL_OPTS_MAX))
return -ENOMEM;
ip_tunnel_info_opts_set(info, from, size);
@@ -1936,13 +1934,10 @@ static const struct bpf_func_proto *
bpf_get_skb_set_tunnel_proto(enum bpf_func_id which)
{
if (!md_dst) {
- BUILD_BUG_ON(FIELD_SIZEOF(struct ip_tunnel_info,
- options_len) != 1);
-
/* Race is not possible, since it's called from verifier
* that is holding verifier mutex.
*/
- md_dst = metadata_dst_alloc_percpu(BPF_TUNLEN_MAX,
+ md_dst = metadata_dst_alloc_percpu(IP_TUNNEL_OPTS_MAX,
GFP_KERNEL);
if (!md_dst)
return NULL;
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index eaca244..d27276f 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -398,6 +398,12 @@ static const struct lwtunnel_encap_ops ip6_tun_lwt_ops = {
void __init ip_tunnel_core_init(void)
{
+ /* If you land here, make sure whether increasing ip_tunnel_info's
+ * options_len is a reasonable choice with its usage in front ends
+ * (f.e., it's part of flow keys, etc).
+ */
+ BUILD_BUG_ON(IP_TUNNEL_OPTS_MAX != 255);
+
lwtunnel_encap_add_ops(&ip_tun_lwt_ops, LWTUNNEL_ENCAP_IP);
lwtunnel_encap_add_ops(&ip6_tun_lwt_ops, LWTUNNEL_ENCAP_IP6);
}
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index 1d055c5..03378e7 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -55,7 +55,7 @@ struct ovs_tunnel_info {
FIELD_SIZEOF(struct sw_flow_key, recirc_id))
struct sw_flow_key {
- u8 tun_opts[255];
+ u8 tun_opts[IP_TUNNEL_OPTS_MAX];
u8 tun_opts_len;
struct ip_tunnel_key tun_key; /* Encapsulating tunnel key. */
struct {
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 1/3] bpf: make skb->tc_classid also readable
From: Daniel Borkmann @ 2016-03-16 0:42 UTC (permalink / raw)
To: davem; +Cc: alexei.starovoitov, netdev, Daniel Borkmann
In-Reply-To: <cover.1458085814.git.daniel@iogearbox.net>
Currently, the tc_classid from eBPF skb context is write-only, but there's
no good reason for tc programs to limit it to write-only. For example,
it can be used to transfer its state via tail calls where the resulting
tc_classid gets filled gradually.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/core/filter.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 6fc3893..69c7b2f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2069,16 +2069,14 @@ static bool sk_filter_is_valid_access(int off, int size,
static bool tc_cls_act_is_valid_access(int off, int size,
enum bpf_access_type type)
{
- if (off == offsetof(struct __sk_buff, tc_classid))
- return type == BPF_WRITE ? true : false;
-
if (type == BPF_WRITE) {
switch (off) {
case offsetof(struct __sk_buff, mark):
case offsetof(struct __sk_buff, tc_index):
case offsetof(struct __sk_buff, priority):
case offsetof(struct __sk_buff, cb[0]) ...
- offsetof(struct __sk_buff, cb[4]):
+ offsetof(struct __sk_buff, cb[4]):
+ case offsetof(struct __sk_buff, tc_classid):
break;
default:
return false;
@@ -2195,8 +2193,10 @@ static u32 bpf_net_convert_ctx_access(enum bpf_access_type type, int dst_reg,
ctx_off -= offsetof(struct __sk_buff, tc_classid);
ctx_off += offsetof(struct sk_buff, cb);
ctx_off += offsetof(struct qdisc_skb_cb, tc_classid);
- WARN_ON(type != BPF_WRITE);
- *insn++ = BPF_STX_MEM(BPF_H, dst_reg, src_reg, ctx_off);
+ if (type == BPF_WRITE)
+ *insn++ = BPF_STX_MEM(BPF_H, dst_reg, src_reg, ctx_off);
+ else
+ *insn++ = BPF_LDX_MEM(BPF_H, dst_reg, src_reg, ctx_off);
break;
case offsetof(struct __sk_buff, tc_index):
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 2/3] bpf, dst: add and use dst_tclassid helper
From: Daniel Borkmann @ 2016-03-16 0:42 UTC (permalink / raw)
To: davem; +Cc: alexei.starovoitov, netdev, Daniel Borkmann
In-Reply-To: <cover.1458085814.git.daniel@iogearbox.net>
We can just add a small helper dst_tclassid() for retrieving the
dst->tclassid value. It makes the code a bit better in that we can
get rid of the ifdef from filter.c by moving this into the header.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/net/dst.h | 12 ++++++++++++
net/core/filter.c | 9 +--------
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/include/net/dst.h b/include/net/dst.h
index c7329dc..5c98443 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -398,6 +398,18 @@ static inline void skb_tunnel_rx(struct sk_buff *skb, struct net_device *dev,
__skb_tunnel_rx(skb, dev, net);
}
+static inline u32 dst_tclassid(const struct sk_buff *skb)
+{
+#ifdef CONFIG_IP_ROUTE_CLASSID
+ const struct dst_entry *dst;
+
+ dst = skb_dst(skb);
+ if (dst)
+ return dst->tclassid;
+#endif
+ return 0;
+}
+
int dst_discard_out(struct net *net, struct sock *sk, struct sk_buff *skb);
static inline int dst_discard(struct sk_buff *skb)
{
diff --git a/net/core/filter.c b/net/core/filter.c
index 69c7b2f..4c35d83 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1682,14 +1682,7 @@ static const struct bpf_func_proto bpf_get_cgroup_classid_proto = {
static u64 bpf_get_route_realm(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
{
-#ifdef CONFIG_IP_ROUTE_CLASSID
- const struct dst_entry *dst;
-
- dst = skb_dst((struct sk_buff *) (unsigned long) r1);
- if (dst)
- return dst->tclassid;
-#endif
- return 0;
+ return dst_tclassid((struct sk_buff *) (unsigned long) r1);
}
static const struct bpf_func_proto bpf_get_route_realm_proto = {
--
1.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox