* Re: [PATCH 2/4] arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Benjamin Herrenschmidt @ 2014-11-18 0:39 UTC (permalink / raw)
To: Alexander Duyck
Cc: Alexander Duyck, linux-arch, netdev, linux-kernel,
mathieu.desnoyers, peterz, heiko.carstens, mingo, mikey, linux,
donald.c.skidmore, matthew.vick, geert, jeffrey.t.kirsher, romieu,
paulmck, nic_swsd, will.deacon, michael, tony.luck, torvalds,
oleg, schwidefsky, fweisbec, davem
In-Reply-To: <546A5968.1090201@gmail.com>
On Mon, 2014-11-17 at 12:24 -0800, Alexander Duyck wrote:
> Yes and no. So for example on ARM I used the dmb() operation, however
> I
> have to use the barrier at the system level instead of just the inner
> shared domain. However on many other architectures they are just the
> same as the smp_* variants.
>
> Basically the resultant code is somewhere between the smp and non-smp
> barriers in terms of what they cover.
There I don't quite follow you. You need to explain better especially in
the documentation because otherwise people will get it wrong...
If it's ordering in the coherent domain, I fail to see how a DMA agent
is different than another processor when it comes to barriers, so I fail
to see the difference with smp_*
I understand the MMIO vs. memory issue, we do have the same on powerpc,
but that other aspect eludes me.
Ben.
^ permalink raw reply
* Re: Device Tree Binding for Marvell DSA Switch on imx28 board over Mdio Interface
From: Fabio Estevam @ 2014-11-18 0:39 UTC (permalink / raw)
To: Oliver Graute; +Cc: Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <CA+KjHfaKhQL6J3FZjZrs-eh3URBL5geDWV9adpcvo-6SU-9aRg@mail.gmail.com>
Hi Oliver,
On Mon, Nov 17, 2014 at 2:54 PM, Oliver Graute <oliver.graute@gmail.com> wrote:
> Hello Fabio,
>
> and what's the correct one for the imx28 CPU?
I am not familiar with it, sorry.
^ permalink raw reply
* Re: [PATCH 2/4] arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Benjamin Herrenschmidt @ 2014-11-18 0:38 UTC (permalink / raw)
To: paulmck
Cc: Alexander Duyck, linux-arch, netdev, linux-kernel,
mathieu.desnoyers, peterz, heiko.carstens, mingo, mikey, linux,
donald.c.skidmore, matthew.vick, geert, jeffrey.t.kirsher, romieu,
nic_swsd, will.deacon, michael, tony.luck, torvalds, oleg,
schwidefsky, fweisbec, davem
In-Reply-To: <20141117201823.GD5050@linux.vnet.ibm.com>
On Mon, 2014-11-17 at 12:18 -0800, Paul E. McKenney wrote:
> On Mon, Nov 17, 2014 at 09:18:13AM -0800, Alexander Duyck wrote:
> > There are a number of situations where the mandatory barriers rmb() and
> > wmb() are used to order memory/memory operations in the device drivers
> > and those barriers are much heavier than they actually need to be. For
> > example in the case of PowerPC wmb() calls the heavy-weight sync
> > instruction when for memory/memory operations all that is really needed is
> > an lsync or eieio instruction.
>
> Is this still the case if one of the memory operations is MMIO? Last
> I knew, it was not.
I *think* (Alexander, correct me if I'm wrong), that what he wants is
the memory<->memory barriers (the smp_* ones) basically for ordering his
loads or stores from/to the DMA area.
The problem is that the smp_* ones aren't compiled for !CONFIG_SMP
IE. Something like:
- Read valid bit from descriptor
- Read rest of descriptor
That needs an rmb of some sort in between, but a full blown "rmb" will
also order vs. MMIOs and end up being a full sync, while an smp_rmb is a
lwsync which is more lightweight.
Similarily:
- Populate descriptor
- Write valid bit
Same deal with wmb ...
Basically, rmb and wmb order both cachable and non-cachable (memory and
MMIO) which makes them needlessly heavy on powerpc and possibly others
when all you need is to order memory accesses to some DMA data
structures. In that case you really want the normal smp_* variants
except they may not be around...
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCHv2 net 1/4] net: Add vxlan_gso_check() helper
From: Joe Stringer @ 2014-11-18 0:26 UTC (permalink / raw)
To: Or Gerlitz; +Cc: Linux Netdev List
In-Reply-To: <CAJ3xEMhwq2Cuiw8sjY3DHFBEdmgKieNZgxWhGsfZ=Rtz41KhTQ@mail.gmail.com>
On Sunday, November 16, 2014 01:35:52 Or Gerlitz wrote:
> On Fri, Nov 14, 2014 at 2:38 AM, Joe Stringer <joestringer@nicira.com>
wrote:
> > Most NICs that report NETIF_F_GSO_UDP_TUNNEL support VXLAN, and not
> > other UDP-based encapsulation protocols where the format and size of the
> > header differs. This patch implements a generic ndo_gso_check() for
> > VXLAN which will only advertise GSO support when the skb looks like it
> > contains VXLAN (or no UDP tunnelling at all).
> >
> > Implementation shamelessly stolen from Tom Herbert:
> > http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> >
> > Signed-off-by: Joe Stringer <joestringer@nicira.com>
> > ---
> > v2: Merge helpers for be2net, mlx4, qlcnic
> >
> > Use (sizeof(struct udphdr) + sizeof(struct vxlanhdr))
> >
> > v1: Initial post
> > ---
> >
> > drivers/net/vxlan.c | 13 +++++++++++++
> > include/net/vxlan.h | 2 ++
> > 2 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> > index fa9dc45..6b65863 100644
> > --- a/drivers/net/vxlan.c
> > +++ b/drivers/net/vxlan.c
> > @@ -1571,6 +1571,19 @@ static bool route_shortcircuit(struct net_device
> > *dev, struct sk_buff *skb)
> >
> > return false;
> >
> > }
> >
> > +bool vxlan_gso_check(struct sk_buff *skb)
> > +{
> > + if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> > + (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> > + skb->inner_protocol != htons(ETH_P_TEB) ||
> > + (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
> > + sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
> > + return false;
> > +
> > + return true;
> > +}
> > +EXPORT_SYMBOL_GPL(vxlan_gso_check);
>
> Joe, any chance you can make the extra step and inline that in
> vxlan.h? this is fast path call... you will only need to move struct
> vxlanhdr there too.
Thanks for looking this over, I sent a patch.
^ permalink raw reply
* [PATCH net] vxlan: Inline vxlan_gso_check().
From: Joe Stringer @ 2014-11-18 0:24 UTC (permalink / raw)
To: netdev; +Cc: ogerlitz, linux-kernel
Suggested-by: Or Gerlitz <ogerlitz@mellanox.com>
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
drivers/net/vxlan.c | 19 -------------------
include/net/vxlan.h | 18 +++++++++++++++++-
2 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 6b65863..e1e335c 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -67,12 +67,6 @@
#define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required value. */
-/* VXLAN protocol header */
-struct vxlanhdr {
- __be32 vx_flags;
- __be32 vx_vni;
-};
-
/* UDP port for VXLAN traffic.
* The IANA assigned port is 4789, but the Linux default is 8472
* for compatibility with early adopters.
@@ -1571,19 +1565,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
return false;
}
-bool vxlan_gso_check(struct sk_buff *skb)
-{
- if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
- (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
- skb->inner_protocol != htons(ETH_P_TEB) ||
- (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
- sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
- return false;
-
- return true;
-}
-EXPORT_SYMBOL_GPL(vxlan_gso_check);
-
#if IS_ENABLED(CONFIG_IPV6)
static int vxlan6_xmit_skb(struct vxlan_sock *vs,
struct dst_entry *dst, struct sk_buff *skb,
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index afadf8e..57cccd0 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -8,6 +8,12 @@
#define VNI_HASH_BITS 10
#define VNI_HASH_SIZE (1<<VNI_HASH_BITS)
+/* VXLAN protocol header */
+struct vxlanhdr {
+ __be32 vx_flags;
+ __be32 vx_vni;
+};
+
struct vxlan_sock;
typedef void (vxlan_rcv_t)(struct vxlan_sock *vh, struct sk_buff *skb, __be32 key);
@@ -45,7 +51,17 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
__be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df,
__be16 src_port, __be16 dst_port, __be32 vni, bool xnet);
-bool vxlan_gso_check(struct sk_buff *skb);
+static inline bool vxlan_gso_check(struct sk_buff *skb)
+{
+ if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+ (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+ skb->inner_protocol != htons(ETH_P_TEB) ||
+ (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
+ sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
+ return false;
+
+ return true;
+}
/* IP header + UDP + VXLAN + Ethernet header */
#define VXLAN_HEADROOM (20 + 8 + 8 + 14)
--
1.7.10.4
^ permalink raw reply related
* [PATCH net-next] tcp: make connect() mem charging friendly
From: Eric Dumazet @ 2014-11-17 23:49 UTC (permalink / raw)
To: Denys Fedoryshchenko, David Miller; +Cc: netdev, Neal Cardwell, Yuchung Cheng
In-Reply-To: <1416266755.18588.12.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
While working on sk_forward_alloc problems reported by Denys
Fedoryshchenko, we found that tcp connect() (and fastopen) do not call
sk_wmem_schedule() for SYN packet (and/or SYN/DATA packet), so
sk_forward_alloc is negative while connect is in progress.
We can fix this by calling regular sk_stream_alloc_skb() both for the
SYN packet (in tcp_connect()) and the syn_data packet in
tcp_send_syn_data()
Then, tcp_send_syn_data() can avoid copying syn_data as we simply
can manipulate syn_data->cb[] to remove SYN flag (and increment seq)
Instead of open coding memcpy_fromiovecend(), simply use this helper.
This leaves in socket write queue clean fast clone skbs.
This was tested against our fastopen packetdrill tests.
Reported-by: Denys Fedoryshchenko <nuclearcat@nuclearcat.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_output.c | 66 +++++++++++++++-------------------------
1 file changed, 26 insertions(+), 40 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index eb73a1dccf56..377e15d0ba96 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3011,9 +3011,9 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn)
{
struct tcp_sock *tp = tcp_sk(sk);
struct tcp_fastopen_request *fo = tp->fastopen_req;
- int syn_loss = 0, space, i, err = 0, iovlen = fo->data->msg_iovlen;
- struct sk_buff *syn_data = NULL, *data;
+ int syn_loss = 0, space, err = 0;
unsigned long last_syn_loss = 0;
+ struct sk_buff *syn_data;
tp->rx_opt.mss_clamp = tp->advmss; /* If MSS is not cached */
tcp_fastopen_cache_get(sk, &tp->rx_opt.mss_clamp, &fo->cookie,
@@ -3044,48 +3044,38 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn)
/* limit to order-0 allocations */
space = min_t(size_t, space, SKB_MAX_HEAD(MAX_TCP_HEADER));
- syn_data = skb_copy_expand(syn, MAX_TCP_HEADER, space,
- sk->sk_allocation);
- if (syn_data == NULL)
+ syn_data = sk_stream_alloc_skb(sk, space, sk->sk_allocation);
+ if (!syn_data)
+ goto fallback;
+ syn_data->ip_summed = CHECKSUM_PARTIAL;
+ memcpy(syn_data->cb, syn->cb, sizeof(syn->cb));
+ if (memcpy_fromiovecend(skb_put(syn_data, space),
+ fo->data->msg_iov, 0, space))
goto fallback;
- for (i = 0; i < iovlen && syn_data->len < space; ++i) {
- struct iovec *iov = &fo->data->msg_iov[i];
- unsigned char __user *from = iov->iov_base;
- int len = iov->iov_len;
+ /* No more data pending in inet_wait_for_connect() */
+ if (space == fo->size)
+ fo->data = NULL;
+ fo->copied = space;
- if (syn_data->len + len > space)
- len = space - syn_data->len;
- else if (i + 1 == iovlen)
- /* No more data pending in inet_wait_for_connect() */
- fo->data = NULL;
+ tcp_connect_queue_skb(sk, syn_data);
- if (skb_add_data(syn_data, from, len))
- goto fallback;
- }
+ err = tcp_transmit_skb(sk, syn_data, 1, sk->sk_allocation);
- /* Queue a data-only packet after the regular SYN for retransmission */
- data = pskb_copy(syn_data, sk->sk_allocation);
- if (data == NULL)
- goto fallback;
- TCP_SKB_CB(data)->seq++;
- TCP_SKB_CB(data)->tcp_flags &= ~TCPHDR_SYN;
- TCP_SKB_CB(data)->tcp_flags = (TCPHDR_ACK|TCPHDR_PSH);
- tcp_connect_queue_skb(sk, data);
- fo->copied = data->len;
-
- /* syn_data is about to be sent, we need to take current time stamps
- * for the packets that are in write queue : SYN packet and DATA
- */
- skb_mstamp_get(&syn->skb_mstamp);
- data->skb_mstamp = syn->skb_mstamp;
+ syn->skb_mstamp = syn_data->skb_mstamp;
- if (tcp_transmit_skb(sk, syn_data, 0, sk->sk_allocation) == 0) {
+ /* Now full SYN+DATA was cloned and sent (or not),
+ * remove the SYN from the original skb (syn_data)
+ * we keep in write queue in case of a retransmit, as we
+ * also have the SYN packet (with no data) in the same queue.
+ */
+ TCP_SKB_CB(syn_data)->seq++;
+ TCP_SKB_CB(syn_data)->tcp_flags = TCPHDR_ACK | TCPHDR_PSH;
+ if (!err) {
tp->syn_data = (fo->copied > 0);
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPORIGDATASENT);
goto done;
}
- syn_data = NULL;
fallback:
/* Send a regular SYN with Fast Open cookie request option */
@@ -3094,7 +3084,6 @@ fallback:
err = tcp_transmit_skb(sk, syn, 1, sk->sk_allocation);
if (err)
tp->syn_fastopen = 0;
- kfree_skb(syn_data);
done:
fo->cookie.len = -1; /* Exclude Fast Open option for SYN retries */
return err;
@@ -3114,13 +3103,10 @@ int tcp_connect(struct sock *sk)
return 0;
}
- buff = alloc_skb_fclone(MAX_TCP_HEADER + 15, sk->sk_allocation);
- if (unlikely(buff == NULL))
+ buff = sk_stream_alloc_skb(sk, 0, sk->sk_allocation);
+ if (unlikely(!buff))
return -ENOBUFS;
- /* Reserve space for headers. */
- skb_reserve(buff, MAX_TCP_HEADER);
-
tcp_init_nondata_skb(buff, tp->write_seq++, TCPHDR_SYN);
tp->retrans_stamp = tcp_time_stamp;
tcp_connect_queue_skb(sk, buff);
^ permalink raw reply related
* Re: [PATCH net] ixgbe: fix use after free adapter->state test in ixgbe_remove/ixgbe_probe
From: Jeff Kirsher @ 2014-11-17 23:48 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, Mark Rustad
In-Reply-To: <1416267827-11545-1-git-send-email-dborkman@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]
On Tue, 2014-11-18 at 00:43 +0100, Daniel Borkmann wrote:
> While working on a different issue, I noticed an annoying use
> after free bug on my machine when unloading the ixgbe driver:
>
> [ 8642.318797] ixgbe 0000:02:00.1: removed PHC on p2p2
> [ 8642.742716] ixgbe 0000:02:00.1: complete
> [ 8642.743784] BUG: unable to handle kernel paging request at
> ffff8807d3740a90
> [ 8642.744828] IP: [<ffffffffa01c77dc>] ixgbe_remove+0xfc/0x1b0
> [ixgbe]
> [ 8642.745886] PGD 20c6067 PUD 81c1f6067 PMD 81c15a067 PTE
> 80000007d3740060
> [ 8642.746956] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
> [ 8642.748039] Modules linked in: [...]
> [ 8642.752929] CPU: 1 PID: 1225 Comm: rmmod Not tainted 3.18.0-rc2+
> #49
> [ 8642.754203] Hardware name: Supermicro X10SLM-F/X10SLM-F, BIOS 1.1b
> 11/01/2013
> [ 8642.755505] task: ffff8807e34d3fe0 ti: ffff8807b7204000 task.ti:
> ffff8807b7204000
> [ 8642.756831] RIP: 0010:[<ffffffffa01c77dc>] [<ffffffffa01c77dc>]
> ixgbe_remove+0xfc/0x1b0 [ixgbe]
> [...]
> [ 8642.774335] Stack:
> [ 8642.775805] ffff8807ee824098 ffff8807ee824098 ffffffffa01f3000
> ffff8807ee824000
> [ 8642.777326] ffff8807b7207e18 ffffffff8137720f ffff8807ee824098
> ffff8807ee824098
> [ 8642.778848] ffffffffa01f3068 ffff8807ee8240f8 ffff8807b7207e38
> ffffffff8144180f
> [ 8642.780365] Call Trace:
> [ 8642.781869] [<ffffffff8137720f>] pci_device_remove+0x3f/0xc0
> [ 8642.783395] [<ffffffff8144180f>] __device_release_driver+0x7f/0xf0
> [ 8642.784876] [<ffffffff814421f8>] driver_detach+0xb8/0xc0
> [ 8642.786352] [<ffffffff814414a9>] bus_remove_driver+0x59/0xe0
> [ 8642.787783] [<ffffffff814429d0>] driver_unregister+0x30/0x70
> [ 8642.789202] [<ffffffff81375c65>] pci_unregister_driver+0x25/0xa0
> [ 8642.790657] [<ffffffffa01eb38e>] ixgbe_exit_module+0x1c/0xc8e
> [ixgbe]
> [ 8642.792064] [<ffffffff810f93a2>] SyS_delete_module+0x132/0x1c0
> [ 8642.793450] [<ffffffff81012c61>] ? do_notify_resume+0x61/0xa0
> [ 8642.794837] [<ffffffff816d2029>] system_call_fastpath+0x12/0x17
>
> The issue is that test_and_set_bit() done on adapter->state is being
> performed *after* the netdevice has been freed via free_netdev().
>
> When netdev is being allocated on initialization time, it allocates
> a private area, here struct ixgbe_adapter, that resides after the
> net_device structure. In ixgbe_probe(), the device init routine,
> we set up the adapter after alloc_etherdev_mq() on the private area
> and add a reference for the pci_dev as well via pci_set_drvdata().
>
> Both in the error path of ixgbe_probe(), but also on module unload
> when ixgbe_remove() is being called, commit 41c62843eb6a accesses
> adapter after free_netdev(). The patch stores the result in a bool
> and thus fixes above oops on my side.
>
> Fixes: 41c62843eb6a ("ixgbe: Fix rcu warnings induced by LER")
> Cc: Mark Rustad <mark.d.rustad@intel.com>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
Thanks Daniel, I will add your patch to my net queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH net] ixgbe: fix use after free adapter->state test in ixgbe_remove/ixgbe_probe
From: Daniel Borkmann @ 2014-11-17 23:43 UTC (permalink / raw)
To: davem; +Cc: netdev, Mark Rustad, Jeff Kirsher
While working on a different issue, I noticed an annoying use
after free bug on my machine when unloading the ixgbe driver:
[ 8642.318797] ixgbe 0000:02:00.1: removed PHC on p2p2
[ 8642.742716] ixgbe 0000:02:00.1: complete
[ 8642.743784] BUG: unable to handle kernel paging request at ffff8807d3740a90
[ 8642.744828] IP: [<ffffffffa01c77dc>] ixgbe_remove+0xfc/0x1b0 [ixgbe]
[ 8642.745886] PGD 20c6067 PUD 81c1f6067 PMD 81c15a067 PTE 80000007d3740060
[ 8642.746956] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
[ 8642.748039] Modules linked in: [...]
[ 8642.752929] CPU: 1 PID: 1225 Comm: rmmod Not tainted 3.18.0-rc2+ #49
[ 8642.754203] Hardware name: Supermicro X10SLM-F/X10SLM-F, BIOS 1.1b 11/01/2013
[ 8642.755505] task: ffff8807e34d3fe0 ti: ffff8807b7204000 task.ti: ffff8807b7204000
[ 8642.756831] RIP: 0010:[<ffffffffa01c77dc>] [<ffffffffa01c77dc>] ixgbe_remove+0xfc/0x1b0 [ixgbe]
[...]
[ 8642.774335] Stack:
[ 8642.775805] ffff8807ee824098 ffff8807ee824098 ffffffffa01f3000 ffff8807ee824000
[ 8642.777326] ffff8807b7207e18 ffffffff8137720f ffff8807ee824098 ffff8807ee824098
[ 8642.778848] ffffffffa01f3068 ffff8807ee8240f8 ffff8807b7207e38 ffffffff8144180f
[ 8642.780365] Call Trace:
[ 8642.781869] [<ffffffff8137720f>] pci_device_remove+0x3f/0xc0
[ 8642.783395] [<ffffffff8144180f>] __device_release_driver+0x7f/0xf0
[ 8642.784876] [<ffffffff814421f8>] driver_detach+0xb8/0xc0
[ 8642.786352] [<ffffffff814414a9>] bus_remove_driver+0x59/0xe0
[ 8642.787783] [<ffffffff814429d0>] driver_unregister+0x30/0x70
[ 8642.789202] [<ffffffff81375c65>] pci_unregister_driver+0x25/0xa0
[ 8642.790657] [<ffffffffa01eb38e>] ixgbe_exit_module+0x1c/0xc8e [ixgbe]
[ 8642.792064] [<ffffffff810f93a2>] SyS_delete_module+0x132/0x1c0
[ 8642.793450] [<ffffffff81012c61>] ? do_notify_resume+0x61/0xa0
[ 8642.794837] [<ffffffff816d2029>] system_call_fastpath+0x12/0x17
The issue is that test_and_set_bit() done on adapter->state is being
performed *after* the netdevice has been freed via free_netdev().
When netdev is being allocated on initialization time, it allocates
a private area, here struct ixgbe_adapter, that resides after the
net_device structure. In ixgbe_probe(), the device init routine,
we set up the adapter after alloc_etherdev_mq() on the private area
and add a reference for the pci_dev as well via pci_set_drvdata().
Both in the error path of ixgbe_probe(), but also on module unload
when ixgbe_remove() is being called, commit 41c62843eb6a accesses
adapter after free_netdev(). The patch stores the result in a bool
and thus fixes above oops on my side.
Fixes: 41c62843eb6a ("ixgbe: Fix rcu warnings induced by LER")
Cc: Mark Rustad <mark.d.rustad@intel.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d2df4e3..7185703 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7979,6 +7979,7 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
int i, err, pci_using_dac, expected_gts;
unsigned int indices = MAX_TX_QUEUES;
u8 part_str[IXGBE_PBANUM_LENGTH];
+ bool disable_dev = false;
#ifdef IXGBE_FCOE
u16 device_caps;
#endif
@@ -8369,13 +8370,14 @@ err_sw_init:
iounmap(adapter->io_addr);
kfree(adapter->mac_table);
err_ioremap:
+ disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
free_netdev(netdev);
err_alloc_etherdev:
pci_release_selected_regions(pdev,
pci_select_bars(pdev, IORESOURCE_MEM));
err_pci_reg:
err_dma:
- if (!adapter || !test_and_set_bit(__IXGBE_DISABLED, &adapter->state))
+ if (!adapter || disable_dev)
pci_disable_device(pdev);
return err;
}
@@ -8393,6 +8395,7 @@ static void ixgbe_remove(struct pci_dev *pdev)
{
struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
struct net_device *netdev = adapter->netdev;
+ bool disable_dev;
ixgbe_dbg_adapter_exit(adapter);
@@ -8442,11 +8445,12 @@ static void ixgbe_remove(struct pci_dev *pdev)
e_dev_info("complete\n");
kfree(adapter->mac_table);
+ disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
free_netdev(netdev);
pci_disable_pcie_error_reporting(pdev);
- if (!test_and_set_bit(__IXGBE_DISABLED, &adapter->state))
+ if (disable_dev)
pci_disable_device(pdev);
}
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH v3] ixgbe: Look up MAC address in Open Firmware or IDPROM
From: Rustad, Mark D @ 2014-11-17 23:35 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: netdev@vger.kernel.org, Linux NICS, Kirsher, Jeffrey T
In-Reply-To: <yq17fyxus1y.fsf@sermon.lab.mkp.net>
[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]
On Nov 14, 2014, at 6:16 PM, Martin K. Petersen <mkp@mkp.net> wrote:
> @@ -7960,6 +7970,29 @@ int ixgbe_wol_supported(struct ixgbe_adapter *adapter, u16 device_id,
> }
>
> /**
> + * ixgbe_get_platform_mac_addr - Look up MAC address in Open Firmware / IDPROM
> + * @adapter: Pointer to adapter struct
> + */
> +static void ixgbe_get_platform_mac_addr(struct ixgbe_adapter *adapter)
How about tagging adapter with __maybe_unused in this case to avoid warnings when neither CONFIG_OF or CONFIG_SPARC are defined?
> +{
> +#ifdef CONFIG_OF
> + struct device_node *dp = pci_device_to_OF_node(adapter->pdev);
> + struct ixgbe_hw *hw = &adapter->hw;
> + const unsigned char *addr;
> +
> + addr = of_get_mac_address(dp);
> + if (addr) {
> + ether_addr_copy(hw->mac.perm_addr, addr);
> + return;
> + }
> +#endif /* CONFIG_OF */
> +
> +#ifdef CONFIG_SPARC
> + ether_addr_copy(hw->mac.perm_addr, idprom->id_ethaddr);
> +#endif /* CONFIG_SPARC */
> +}
> +
> +/**
> * ixgbe_probe - Device Initialization Routine
> * @pdev: PCI device information struct
> * @ent: entry in ixgbe_pci_tbl
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]
^ permalink raw reply
* Re: [PATCH] ixgbe: Look up MAC address in Open Firmware
From: Rustad, Mark D @ 2014-11-17 23:29 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: Sergei Shtylyov, netdev@vger.kernel.org, Linux NICS
In-Reply-To: <yq1mw7tv8jq.fsf@sermon.lab.mkp.net>
[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]
On Nov 14, 2014, at 12:20 PM, Martin K. Petersen <martin.petersen@oracle.com> wrote:
> /**
> + * ixgbe_of_mac_addr - Look up MAC address in Open Firmware
> + * @adapter: Pointer to adapter struct
> + */
> +static void ixgbe_of_mac_addr(struct ixgbe_adapter *adapter)
> +{
> +#ifdef CONFIG_OF
> + struct device_node *dp = pci_device_to_OF_node(adapter->pdev);
> + struct ixgbe_hw *hw = &adapter->hw;
> + const unsigned char *addr;
> + int len;
> +
> + addr = of_get_property(dp, "local-mac-address", &len);
> + if (addr && len == 6) {
> + e_dev_info("Using OpenPROM MAC address\n");
> + memcpy(hw->mac.perm_addr, addr, 6);
> + }
> +
> + if (!is_valid_ether_addr(hw->mac.perm_addr)) {
> + e_dev_info("Using IDPROM MAC address\n");
> + memcpy(hw->mac.perm_addr, idprom->id_ethaddr, 6);
> + }
> +#endif
> +}
I would prefer that the entire function be wrapped by the #if so that a null implementation can include the prototype. That allows the unused parameter to properly be declared as __always_unused as in:
#else
static void ixgbe_of_mac_addr(struct ixgbe_adapter __always_unused *adapter)
{}
#endif /* CONFIG_OF */
Otherwise, it will throw warnings when compiled with enhanced warning messages.
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
From: Keller, Jacob E @ 2014-11-17 23:28 UTC (permalink / raw)
To: richardcochran@gmail.com
Cc: netdev@vger.kernel.org, davem@davemloft.net, Allan, Bruce W,
Ronciak, John, Kirsher, Jeffrey T, Vick, Matthew
In-Reply-To: <c13112d45627bab7a6d55c6727f022da46d1a2a2.1416265321.git.richardcochran@gmail.com>
On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> The i210 device offers a number of special PTP Hardware Clock features on
> the Software Defined Pins (SDPs). This patch adds support for two of the
> possible functions, namely time stamping external events, and periodic
> output signals.
>
> The assignment of PHC functions to the four SDP can be freely chosen by
> the user.
>
This patch probably needs some code added to igb_ptp_reset which will
re-enable the SDP pins after a MAC reset to the same current settings
stored by the driver. It looks like you maintain all this data in
internal structures, so it should be trivial to add the correct function
calls to the reset path.
If you don't have this code, then any time there is a hardware reset,
these will be disabled. Normal operation can sometimes have resets due
to TX hang. I think even one of the ethtool operations can cause a reset
to occur sometimes. See my comments on one of the other patch in the
series about this, also.
Thanks,
Jake
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
> drivers/net/ethernet/intel/igb/igb.h | 9 ++
> drivers/net/ethernet/intel/igb/igb_main.c | 47 ++++++-
> drivers/net/ethernet/intel/igb/igb_ptp.c | 219 ++++++++++++++++++++++++++++-
> 3 files changed, 269 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 82d891e..f6aca7c 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -343,6 +343,9 @@ struct hwmon_buff {
> };
> #endif
>
> +#define IGB_N_EXTTS 2
> +#define IGB_N_PEROUT 2
> +#define IGB_N_SDP 4
> #define IGB_RETA_SIZE 128
>
> /* board specific private data structure */
> @@ -439,6 +442,12 @@ struct igb_adapter {
> u32 tx_hwtstamp_timeouts;
> u32 rx_hwtstamp_cleared;
>
> + struct ptp_pin_desc sdp_config[IGB_N_SDP];
> + struct {
> + struct timespec start;
> + struct timespec period;
> + } perout[IGB_N_PEROUT];
> +
> char fw_version[32];
> #ifdef CONFIG_IGB_HWMON
> struct hwmon_buff *igb_hwmon_buff;
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 9412661..3a25661 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5387,7 +5387,8 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> struct ptp_clock_event event;
> - u32 tsicr = rd32(E1000_TSICR);
> + struct timespec ts;
> + u32 tsauxc, sec, nsec, tsicr = rd32(E1000_TSICR);
>
> if (tsicr & TSINTR_SYS_WRAP) {
> event.type = PTP_CLOCK_PPS;
> @@ -5400,6 +5401,50 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
> /* retrieve hardware timestamp */
> schedule_work(&adapter->ptp_tx_work);
> }
> +
> + if (tsicr & TSINTR_TT0) {
> + spin_lock(&adapter->tmreg_lock);
> + ts = timespec_add(adapter->perout[0].start,
> + adapter->perout[0].period);
> + wr32(E1000_TRGTTIML0, ts.tv_nsec);
> + wr32(E1000_TRGTTIMH0, ts.tv_sec);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsauxc |= TSAUXC_EN_TT0;
> + wr32(E1000_TSAUXC, tsauxc);
> + adapter->perout[0].start = ts;
> + spin_unlock(&adapter->tmreg_lock);
> + }
> +
> + if (tsicr & TSINTR_TT1) {
> + spin_lock(&adapter->tmreg_lock);
> + ts = timespec_add(adapter->perout[1].start,
> + adapter->perout[1].period);
> + wr32(E1000_TRGTTIML1, ts.tv_nsec);
> + wr32(E1000_TRGTTIMH1, ts.tv_sec);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsauxc |= TSAUXC_EN_TT1;
> + wr32(E1000_TSAUXC, tsauxc);
> + adapter->perout[1].start = ts;
> + spin_unlock(&adapter->tmreg_lock);
> + }
> +
> + if (tsicr & TSINTR_AUTT0) {
> + nsec = rd32(E1000_AUXSTMPL0);
> + sec = rd32(E1000_AUXSTMPH0);
> + event.type = PTP_CLOCK_EXTTS;
> + event.index = 0;
> + event.timestamp = sec * 1000000000ULL + nsec;
> + ptp_clock_event(adapter->ptp_clock, &event);
> + }
> +
> + if (tsicr & TSINTR_AUTT1) {
> + nsec = rd32(E1000_AUXSTMPL1);
> + sec = rd32(E1000_AUXSTMPH1);
> + event.type = PTP_CLOCK_EXTTS;
> + event.index = 1;
> + event.timestamp = sec * 1000000000ULL + nsec;
> + ptp_clock_event(adapter->ptp_clock, &event);
> + }
> }
>
> static irqreturn_t igb_msix_other(int irq, void *data)
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 70d3933..37b9fe6 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -360,16 +360,203 @@ static int igb_ptp_settime_i210(struct ptp_clock_info *ptp,
> return 0;
> }
>
> +static void igb_pin_direction(int pin, int input, u32 *ctrl, u32 *ctrl_ext)
> +{
> + u32 *ptr = pin < 2 ? ctrl : ctrl_ext;
> + u32 mask[IGB_N_SDP] = {
> + E1000_CTRL_SDP0_DIR,
> + E1000_CTRL_SDP1_DIR,
> + E1000_CTRL_EXT_SDP2_DIR,
> + E1000_CTRL_EXT_SDP3_DIR,
> + };
> +
> + if (input)
> + *ptr &= ~mask[pin];
> + else
> + *ptr |= mask[pin];
> +}
> +
> +static void igb_pin_extts(struct igb_adapter *igb, int chan, int pin)
> +{
> + struct e1000_hw *hw = &igb->hw;
> + u32 aux0_sel_sdp[IGB_N_SDP] = {
> + AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
> + };
> + u32 aux1_sel_sdp[IGB_N_SDP] = {
> + AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
> + };
> + u32 ts_sdp_en[IGB_N_SDP] = {
> + TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
> + };
> + u32 ctrl, ctrl_ext, tssdp = 0;
> +
> + ctrl = rd32(E1000_CTRL);
> + ctrl_ext = rd32(E1000_CTRL_EXT);
> + tssdp = rd32(E1000_TSSDP);
> +
> + igb_pin_direction(pin, 1, &ctrl, &ctrl_ext);
> +
> + /* Make sure this pin is not enabled as an ouput. */
> + tssdp &= ~ts_sdp_en[pin];
> +
> + if (chan == 1) {
> + tssdp &= ~AUX1_SEL_SDP3;
> + tssdp |= aux1_sel_sdp[pin] | AUX1_TS_SDP_EN;
> + } else {
> + tssdp &= ~AUX0_SEL_SDP3;
> + tssdp |= aux0_sel_sdp[pin] | AUX0_TS_SDP_EN;
> + }
> +
> + wr32(E1000_TSSDP, tssdp);
> + wr32(E1000_CTRL, ctrl);
> + wr32(E1000_CTRL_EXT, ctrl_ext);
> +}
> +
> +static void igb_pin_perout(struct igb_adapter *igb, int chan, int pin)
> +{
> + struct e1000_hw *hw = &igb->hw;
> + u32 aux0_sel_sdp[IGB_N_SDP] = {
> + AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
> + };
> + u32 aux1_sel_sdp[IGB_N_SDP] = {
> + AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
> + };
> + u32 ts_sdp_en[IGB_N_SDP] = {
> + TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
> + };
> + u32 ts_sdp_sel_tt0[IGB_N_SDP] = {
> + TS_SDP0_SEL_TT0, TS_SDP1_SEL_TT0,
> + TS_SDP2_SEL_TT0, TS_SDP3_SEL_TT0,
> + };
> + u32 ts_sdp_sel_tt1[IGB_N_SDP] = {
> + TS_SDP0_SEL_TT1, TS_SDP1_SEL_TT1,
> + TS_SDP2_SEL_TT1, TS_SDP3_SEL_TT1,
> + };
> + u32 ts_sdp_sel_clr[IGB_N_SDP] = {
> + TS_SDP0_SEL_FC1, TS_SDP1_SEL_FC1,
> + TS_SDP2_SEL_FC1, TS_SDP3_SEL_FC1,
> + };
> + u32 ctrl, ctrl_ext, tssdp = 0;
> +
> + ctrl = rd32(E1000_CTRL);
> + ctrl_ext = rd32(E1000_CTRL_EXT);
> + tssdp = rd32(E1000_TSSDP);
> +
> + igb_pin_direction(pin, 0, &ctrl, &ctrl_ext);
> +
> + /* Make sure this pin is not enabled as an input. */
> + if ((tssdp & AUX0_SEL_SDP3) == aux0_sel_sdp[pin])
> + tssdp &= ~AUX0_TS_SDP_EN;
> +
> + if ((tssdp & AUX1_SEL_SDP3) == aux1_sel_sdp[pin])
> + tssdp &= ~AUX1_TS_SDP_EN;
> +
> + tssdp &= ~ts_sdp_sel_clr[pin];
> + if (chan == 1)
> + tssdp |= ts_sdp_sel_tt1[pin];
> + else
> + tssdp |= ts_sdp_sel_tt0[pin];
> +
> + tssdp |= ts_sdp_en[pin];
> +
> + wr32(E1000_TSSDP, tssdp);
> + wr32(E1000_CTRL, ctrl);
> + wr32(E1000_CTRL_EXT, ctrl_ext);
> +}
> +
> static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
> struct ptp_clock_request *rq, int on)
> {
> struct igb_adapter *igb =
> container_of(ptp, struct igb_adapter, ptp_caps);
> struct e1000_hw *hw = &igb->hw;
> + u32 tsauxc, tsim, tsauxc_mask, tsim_mask, trgttiml, trgttimh;
> unsigned long flags;
> - u32 tsim;
> + struct timespec ts;
> + int pin;
> + s64 ns;
>
> switch (rq->type) {
> + case PTP_CLK_REQ_EXTTS:
> + if (on) {
> + pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS,
> + rq->extts.index);
> + if (pin < 0)
> + return -EBUSY;
> + igb_pin_extts(igb, rq->extts.index, pin);
> + }
> + if (rq->extts.index == 1) {
> + tsauxc_mask = TSAUXC_EN_TS1;
> + tsim_mask = TSINTR_AUTT1;
> + } else {
> + tsauxc_mask = TSAUXC_EN_TS0;
> + tsim_mask = TSINTR_AUTT0;
> + }
> + spin_lock_irqsave(&igb->tmreg_lock, flags);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsim = rd32(E1000_TSIM);
> + if (on) {
> + tsauxc |= tsauxc_mask;
> + tsim |= tsim_mask;
> + } else {
> + tsauxc &= ~tsauxc_mask;
> + tsim &= ~tsim_mask;
> + }
> + wr32(E1000_TSAUXC, tsauxc);
> + wr32(E1000_TSIM, tsim);
> + spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> + return 0;
> +
> + case PTP_CLK_REQ_PEROUT:
> + if (on) {
> + pin = ptp_find_pin(igb->ptp_clock, PTP_PF_PEROUT,
> + rq->perout.index);
> + if (pin < 0)
> + return -EBUSY;
> + igb_pin_perout(igb, rq->perout.index, pin);
> + }
> + ts.tv_sec = rq->perout.period.sec;
> + ts.tv_nsec = rq->perout.period.nsec;
> + ns = timespec_to_ns(&ts);
> + ns = ns >> 1;
> + if (on && ns < 500000LL) {
> + /* 2k interrupts per second is an awful lot. */
> + return -EINVAL;
> + }
> + ts = ns_to_timespec(ns);
> + if (rq->perout.index == 1) {
> + tsauxc_mask = TSAUXC_EN_TT1;
> + tsim_mask = TSINTR_TT1;
> + trgttiml = E1000_TRGTTIML1;
> + trgttimh = E1000_TRGTTIMH1;
> + } else {
> + tsauxc_mask = TSAUXC_EN_TT0;
> + tsim_mask = TSINTR_TT0;
> + trgttiml = E1000_TRGTTIML0;
> + trgttimh = E1000_TRGTTIMH0;
> + }
> + spin_lock_irqsave(&igb->tmreg_lock, flags);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsim = rd32(E1000_TSIM);
> + if (on) {
> + int i = rq->perout.index;
> + igb->perout[i].start.tv_sec = rq->perout.start.sec;
> + igb->perout[i].start.tv_nsec = rq->perout.start.nsec;
> + igb->perout[i].period.tv_sec = ts.tv_sec;
> + igb->perout[i].period.tv_nsec = ts.tv_nsec;
> + wr32(trgttiml, rq->perout.start.sec);
> + wr32(trgttimh, rq->perout.start.nsec);
> + tsauxc |= tsauxc_mask;
> + tsim |= tsim_mask;
> + } else {
> + tsauxc &= ~tsauxc_mask;
> + tsim &= ~tsim_mask;
> + }
> + wr32(E1000_TSAUXC, tsauxc);
> + wr32(E1000_TSIM, tsim);
> + spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> + return 0;
> +
> case PTP_CLK_REQ_PPS:
> spin_lock_irqsave(&igb->tmreg_lock, flags);
> tsim = rd32(E1000_TSIM);
> @@ -380,9 +567,6 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
> wr32(E1000_TSIM, tsim);
> spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> return 0;
> -
> - default:
> - break;
> }
>
> return -EOPNOTSUPP;
> @@ -394,6 +578,20 @@ static int igb_ptp_feature_enable(struct ptp_clock_info *ptp,
> return -EOPNOTSUPP;
> }
>
> +static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
> + enum ptp_pin_function func, unsigned int chan)
> +{
> + switch (func) {
> + case PTP_PF_NONE:
> + case PTP_PF_EXTTS:
> + case PTP_PF_PEROUT:
> + break;
> + case PTP_PF_PHYSYNC:
> + return -1;
> + }
> + return 0;
> +}
> +
> /**
> * igb_ptp_tx_work
> * @work: pointer to work struct
> @@ -784,6 +982,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> struct net_device *netdev = adapter->netdev;
> + int i;
>
> switch (hw->mac.type) {
> case e1000_82576:
> @@ -826,16 +1025,26 @@ void igb_ptp_init(struct igb_adapter *adapter)
> break;
> case e1000_i210:
> case e1000_i211:
> + for (i = 0; i < IGB_N_SDP; i++) {
> + struct ptp_pin_desc *ppd = &adapter->sdp_config[i];
> + snprintf(ppd->name, sizeof(ppd->name), "SDP%d", i);
> + ppd->index = i;
> + ppd->func = PTP_PF_NONE;
> + }
> snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr);
> adapter->ptp_caps.owner = THIS_MODULE;
> adapter->ptp_caps.max_adj = 62499999;
> - adapter->ptp_caps.n_ext_ts = 0;
> + adapter->ptp_caps.n_ext_ts = IGB_N_EXTTS;
> + adapter->ptp_caps.n_per_out = IGB_N_PEROUT;
> + adapter->ptp_caps.n_pins = IGB_N_SDP;
> adapter->ptp_caps.pps = 1;
> + adapter->ptp_caps.pin_config = adapter->sdp_config;
> adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580;
> adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
> adapter->ptp_caps.gettime = igb_ptp_gettime_i210;
> adapter->ptp_caps.settime = igb_ptp_settime_i210;
> adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
> + adapter->ptp_caps.verify = igb_ptp_verify_pin;
> /* Enable the timer functions by clearing bit 31. */
> wr32(E1000_TSAUXC, 0x0);
> break;
^ permalink raw reply
* Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.
From: Keller, Jacob E @ 2014-11-17 23:24 UTC (permalink / raw)
To: richardcochran@gmail.com
Cc: netdev@vger.kernel.org, davem@davemloft.net, Allan, Bruce W,
Ronciak, John, Kirsher, Jeffrey T, Vick, Matthew
In-Reply-To: <e5b4ef7c56119e2d86e42f5680b1b82c9ec1a38e.1416265321.git.richardcochran@gmail.com>
On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> The i210 device can produce an interrupt on the full second. This
> patch allows using this interrupt to generate an internal PPS event
> for adjusting the kernel system time.
>
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++++
> drivers/net/ethernet/intel/igb/igb_ptp.c | 32 +++++++++++++++++++++++++++--
> 2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 7183a56..9412661 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter,
> static void igb_tsync_interrupt(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> + struct ptp_clock_event event;
> u32 tsicr = rd32(E1000_TSICR);
>
> + if (tsicr & TSINTR_SYS_WRAP) {
> + event.type = PTP_CLOCK_PPS;
> + ptp_clock_event(adapter->ptp_clock, &event);
> + }
> +
Nice! I hadn't thought of using the SYS_WRAP around interrupt for pps.
Much simpler than having to setup a SDP pin trigger.
Regards,
Jake
^ permalink raw reply
* Re: [PATCH net-next 2/4] igb: do not clobber the TSAUXC bits on reset.
From: Keller, Jacob E @ 2014-11-17 23:22 UTC (permalink / raw)
To: richardcochran@gmail.com
Cc: netdev@vger.kernel.org, davem@davemloft.net, Allan, Bruce W,
Ronciak, John, Kirsher, Jeffrey T, Vick, Matthew
In-Reply-To: <5c892a6a13d120193fdbd2dd62e9604ae0d5d404.1416265321.git.richardcochran@gmail.com>
On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> The TSAUXC register has a number of different bits, one of which disables
> the main clock function. Previously, the clock was re-enabled by clearing
> the entire register. This patch changes the code to preserve the values
> of the other bits in that register.
>
This is a step in the right direction, but won't fully work due to this
being called after a MAC reset.
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
> drivers/net/ethernet/intel/igb/igb_ptp.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 794c139..ce57128 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -905,6 +905,8 @@ void igb_ptp_stop(struct igb_adapter *adapter)
> void igb_ptp_reset(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> + unsigned long flags;
> + u32 tsauxc;
>
> if (!(adapter->flags & IGB_FLAG_PTP))
> return;
> @@ -923,7 +925,11 @@ void igb_ptp_reset(struct igb_adapter *adapter)
> case e1000_i210:
> case e1000_i211:
> /* Enable the timer functions and interrupts. */
> - wr32(E1000_TSAUXC, 0x0);
> + spin_lock_irqsave(&adapter->tmreg_lock, flags);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsauxc &= ~TSAUXC_DISABLE;
> + wr32(E1000_TSAUXC, tsauxc);
> + spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
This won't work. We call igb_ptp_reset directly after a HW reset (MAC
reset). TSAUXC data is not maintained. We probably need to store driver
state of the TSAUXC bits, and then re-enable them in this reset path.
This code today is fine, because there was no other state we cared about
in the path.
That being said, I don't think this change breaks anything. I just think
it will not solve the problem.
Thanks,
Jake
> wr32(E1000_TSIM, TSYNC_INTERRUPTS);
> wr32(E1000_IMS, E1000_IMS_TS);
> break;
^ permalink raw reply
* Re: [PATCH 2/4] arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Paul E. McKenney @ 2014-11-17 23:17 UTC (permalink / raw)
To: Alexander Duyck
Cc: linux-arch, netdev, linux-kernel, mathieu.desnoyers, peterz, benh,
heiko.carstens, mingo, mikey, linux, donald.c.skidmore,
matthew.vick, geert, jeffrey.t.kirsher, romieu, nic_swsd,
will.deacon, michael, tony.luck, torvalds, oleg, schwidefsky,
fweisbec, davem
In-Reply-To: <546A649D.2000007@redhat.com>
On Mon, Nov 17, 2014 at 01:11:57PM -0800, Alexander Duyck wrote:
> On 11/17/2014 12:18 PM, Paul E. McKenney wrote:
> >On Mon, Nov 17, 2014 at 09:18:13AM -0800, Alexander Duyck wrote:
> >>There are a number of situations where the mandatory barriers rmb() and
> >>wmb() are used to order memory/memory operations in the device drivers
> >>and those barriers are much heavier than they actually need to be. For
> >>example in the case of PowerPC wmb() calls the heavy-weight sync
> >>instruction when for memory/memory operations all that is really needed is
> >>an lsync or eieio instruction.
> >
> >Is this still the case if one of the memory operations is MMIO? Last
> >I knew, it was not.
>
> This barrier is not meant for use in MMIO operations, for that you
> still need a full barrier as I call out in the documentation
> section. What the barrier does is allow for a lightweight barrier
> for accesses to coherent system memory. So for example many device
> drivers have to perform a read of the descriptor to see if the
> device is done with it. We need an rmb() following that check to
> prevent any other accesses.
>
> Right now on x86 that rmb() becomes an lfence instruction and is
> quite expensive, and as it turns out we don't need it since the x86
> doesn't reorder reads. The same kind of thing applies to PowerPC,
> only in that case we use a sync when what we really need is a
> lwsync.
Would it make sense to have a memory barrier that enforced the
non-store-buffer orderings, that is prior reads before later
reads and writes and prior writes before later writes? This was
discussed earlier this year ((http://lwn.net/Articles/586838/,
https://lwn.net/Articles/588300/). If I recall correctly, one of
the biggest obstacles was the name. ;-)
> >>This commit adds a fast (and loose) version of the mandatory memory
> >>barriers rmb() and wmb(). The prefix to the name is actually based on the
> >>version of the functions that already exist in the mips and tile trees.
> >>However I thought it applicable since it gets at what we are trying to
> >>accomplish with these barriers and somethat implies their risky nature.
> >>
> >>These new barriers are not as safe as the standard rmb() and wmb().
> >>Specifically they do not guarantee ordering between cache-enabled and
> >>cache-inhibited memories. The primary use case for these would be to
> >>enforce ordering of memory reads/writes when accessing cache-enabled memory
> >>that is shared between the CPU and a device.
> >>
> >>It may also be noted that there is no fast_mb(). This is due to the fact
> >>that most architectures didn't seem to have a good way to do a full memory
> >>barrier quickly and so they usually resorted to an mb() for their smp_mb
> >>call. As such there no point in adding a fast_mb() function if it is going
> >>to map to mb() for all architectures anyway.
> >
> >I must confess that I still don't entirely understand the motivation.
>
> The motivation is to provide finer grained barriers. So this
> provides an in-between that allows us to "choose the right hammer".
> In the case of PowerPC it is the difference between sync/lwsync, on
> ARM it is dsb()/dmb(), and on x86 it is lfence/barrier().
Ah, so ARM will motivate a fast_wmb(), given its instruction set.
> >Some problems in PowerPC barrier.h called out below.
> >
> > Thanx, Paul
> >
>
> <snip>
>
> >>diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> >>index 22a969c..fe55dea 100644
> >>--- a/Documentation/memory-barriers.txt
> >>+++ b/Documentation/memory-barriers.txt
> >>@@ -1615,6 +1615,47 @@ There are some more advanced barrier functions:
> >> operations" subsection for information on where to use these.
> >>
> >>
> >>+ (*) fast_wmb();
> >>+ (*) fast_rmb();
> >>+
> >>+ These are for use with memory based device I/O to guarantee the ordering
> >>+ of cache-enabled writes or reads with respect to other writes or reads
> >>+ to cache-enabled memory.
> >>+
> >>+ For example, consider a device driver that shares memory with a device
> >>+ and uses a descriptor status value to indicate if the descriptor belongs
> >>+ to the device or the CPU, and a doorbell to notify it when new
> >>+ descriptors are available:
> >>+
> >>+ if (desc->status != DEVICE_OWN) {
> >>+ /* do not read data until we own descriptor */
> >>+ fast_rmb();
> >>+
> >>+ /* read/modify data */
> >>+ read_data = desc->data;
> >>+ desc->data = write_data;
> >>+
> >>+ /* flush modifications before status update */
> >>+ fast_wmb();
> >>+
> >>+ /* assign ownership */
> >>+ desc->status = DEVICE_OWN;
> >>+
> >>+ /* force memory to sync before notifying device via MMIO */
> >>+ wmb();
> >>+
> >>+ /* notify device of new descriptors */
> >>+ writel(DESC_NOTIFY, doorbell);
> >>+ }
> >>+
> >>+ The fast_rmb() allows us guarantee the device has released ownership
> >>+ before we read the data from the descriptor, and he fast_wmb() allows
> >>+ us to guarantee the data is written to the descriptor before the device
> >>+ can see it now has ownership. The wmb() is needed to guarantee that the
> >>+ cache-enabled memory writes have completed before attempting a write to
> >>+ the cache-inhibited MMIO region.
> >>+
> >>+
> >> MMIO WRITE BARRIER
> >> ------------------
>
> The general idea is that the device/CPU follow acquire/release style
> semantics and we need the lightweight barriers to enforce ordering
> to prevent us from accessing the descriptors during those periods
> where we do not own them. As the example shows we still need a full
> barrier when going between MMIO and standard memory. Hopefully that
> is what is conveyed in the documentation bits I have above.
Thank you for the clarification.
> <snip>
>
> >>diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
> >>index cb6d66c..f480097 100644
> >>--- a/arch/powerpc/include/asm/barrier.h
> >>+++ b/arch/powerpc/include/asm/barrier.h
> >>@@ -36,22 +36,20 @@
> >>
> >> #define set_mb(var, value) do { var = value; mb(); } while (0)
> >>
> >>-#ifdef CONFIG_SMP
> >>-
> >> #ifdef __SUBARCH_HAS_LWSYNC
> >> # define SMPWMB LWSYNC
> >> #else
> >> # define SMPWMB eieio
> >> #endif
> >>
> >>-#define __lwsync() __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
> >>+#define fast_rmb() __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
> >>+#define fast_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : : :"memory")
> >>
> >>+#ifdef CONFIG_SMP
> >> #define smp_mb() mb()
> >>-#define smp_rmb() __lwsync()
> >>-#define smp_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : : :"memory")
> >>+#define smp_rmb() fast_rmb()
> >>+#define smp_wmb() fast_wmb()
> >> #else
> >>-#define __lwsync() barrier()
> >>-
> >> #define smp_mb() barrier()
> >> #define smp_rmb() barrier()
> >> #define smp_wmb() barrier()
> >>@@ -69,10 +67,16 @@
> >> #define data_barrier(x) \
> >> asm volatile("twi 0,%0,0; isync" : : "r" (x) : "memory");
> >>
> >>+/*
> >>+ * The use of smp_rmb() in these functions are actually meant to map from
> >>+ * smp_rmb()->fast_rmb()->LWSYNC. This way if smp is disabled then
> >>+ * smp_rmb()->barrier(), or if the platform doesn't support lwsync it will
> >>+ * map to the more heavy-weight sync.
> >>+ */
> >> #define smp_store_release(p, v) \
> >> do { \
> >> compiletime_assert_atomic_type(*p); \
> >>- __lwsync(); \
> >>+ smp_rmb(); \
> >
> >This is not good at all. For smp_store_release(), we absolutely
> >must order prior loads and stores against the assignment on the following
> >line. This is not something that smp_rmb() does, nor is it something
> >that smp_wmb() does. Yes, it might happen to now, but this could easily
> >break in the future -- plus this change is extremely misleading.
> >
> >The original __lwsync() is much more clear.
>
> The problem I had with __lwsync is that it really wasn't all that
> clear. It was the lwsync instruction if SMP was enabled, otherwise
> it was just a barrier call. What I did is move the definition of
> __lwsync in the SMP case into fast_rmb, which in turn is accessed by
> smp_rmb. I tried to make this clear in the comment just above the
> two calls. The resultant assembly code should be exactly the same.
>
> What I could do is have it added back as a smp_lwsync if that works
> for you. That way there is something there to give you a hint that
> it becomes a barrier() call as soon as SMP is disabled.
An smp_lwsync() would be a great improvement!
Thanx, Paul
^ permalink raw reply
* Re: [PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.
From: Jeff Kirsher @ 2014-11-17 23:15 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, David Miller, bruce.w.allan, Jacob Keller, John Ronciak,
Matthew Vick
In-Reply-To: <cover.1416265321.git.richardcochran@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 890 bytes --]
On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> This patch series adds three features: time stamping external events,
> producing a periodic output signal, and an internal PPS event. The
> i210 PCIe card has a 6 pin header with SDP0-3, making it easy to try
> out this new functionality.
>
> An earlier version of this series was posted way back in May, 2013,
> but that version lacked a good way to assign functions to the pins. In
> the mean time, the PHC core has a standard method to configure the
> pins, and this series makes use of it.
>
> Thanks,
> Richard
>
> Richard Cochran (4):
> igb: refactor time sync interrupt handling
> igb: do not clobber the TSAUXC bits on reset.
> igb: enable internal PPS for the i210.
> igb: enable auxiliary PHC functions for the i210.
Thanks Richard, I have added your series of igb patches to my queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
From: Richard Cochran @ 2014-11-17 23:06 UTC (permalink / raw)
To: netdev
Cc: David Miller, bruce.w.allan, Jacob Keller, Jeff Kirsher,
John Ronciak, Matthew Vick
In-Reply-To: <cover.1416265321.git.richardcochran@gmail.com>
The i210 device offers a number of special PTP Hardware Clock features on
the Software Defined Pins (SDPs). This patch adds support for two of the
possible functions, namely time stamping external events, and periodic
output signals.
The assignment of PHC functions to the four SDP can be freely chosen by
the user.
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/intel/igb/igb.h | 9 ++
drivers/net/ethernet/intel/igb/igb_main.c | 47 ++++++-
drivers/net/ethernet/intel/igb/igb_ptp.c | 219 ++++++++++++++++++++++++++++-
3 files changed, 269 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 82d891e..f6aca7c 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -343,6 +343,9 @@ struct hwmon_buff {
};
#endif
+#define IGB_N_EXTTS 2
+#define IGB_N_PEROUT 2
+#define IGB_N_SDP 4
#define IGB_RETA_SIZE 128
/* board specific private data structure */
@@ -439,6 +442,12 @@ struct igb_adapter {
u32 tx_hwtstamp_timeouts;
u32 rx_hwtstamp_cleared;
+ struct ptp_pin_desc sdp_config[IGB_N_SDP];
+ struct {
+ struct timespec start;
+ struct timespec period;
+ } perout[IGB_N_PEROUT];
+
char fw_version[32];
#ifdef CONFIG_IGB_HWMON
struct hwmon_buff *igb_hwmon_buff;
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 9412661..3a25661 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5387,7 +5387,8 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
{
struct e1000_hw *hw = &adapter->hw;
struct ptp_clock_event event;
- u32 tsicr = rd32(E1000_TSICR);
+ struct timespec ts;
+ u32 tsauxc, sec, nsec, tsicr = rd32(E1000_TSICR);
if (tsicr & TSINTR_SYS_WRAP) {
event.type = PTP_CLOCK_PPS;
@@ -5400,6 +5401,50 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
/* retrieve hardware timestamp */
schedule_work(&adapter->ptp_tx_work);
}
+
+ if (tsicr & TSINTR_TT0) {
+ spin_lock(&adapter->tmreg_lock);
+ ts = timespec_add(adapter->perout[0].start,
+ adapter->perout[0].period);
+ wr32(E1000_TRGTTIML0, ts.tv_nsec);
+ wr32(E1000_TRGTTIMH0, ts.tv_sec);
+ tsauxc = rd32(E1000_TSAUXC);
+ tsauxc |= TSAUXC_EN_TT0;
+ wr32(E1000_TSAUXC, tsauxc);
+ adapter->perout[0].start = ts;
+ spin_unlock(&adapter->tmreg_lock);
+ }
+
+ if (tsicr & TSINTR_TT1) {
+ spin_lock(&adapter->tmreg_lock);
+ ts = timespec_add(adapter->perout[1].start,
+ adapter->perout[1].period);
+ wr32(E1000_TRGTTIML1, ts.tv_nsec);
+ wr32(E1000_TRGTTIMH1, ts.tv_sec);
+ tsauxc = rd32(E1000_TSAUXC);
+ tsauxc |= TSAUXC_EN_TT1;
+ wr32(E1000_TSAUXC, tsauxc);
+ adapter->perout[1].start = ts;
+ spin_unlock(&adapter->tmreg_lock);
+ }
+
+ if (tsicr & TSINTR_AUTT0) {
+ nsec = rd32(E1000_AUXSTMPL0);
+ sec = rd32(E1000_AUXSTMPH0);
+ event.type = PTP_CLOCK_EXTTS;
+ event.index = 0;
+ event.timestamp = sec * 1000000000ULL + nsec;
+ ptp_clock_event(adapter->ptp_clock, &event);
+ }
+
+ if (tsicr & TSINTR_AUTT1) {
+ nsec = rd32(E1000_AUXSTMPL1);
+ sec = rd32(E1000_AUXSTMPH1);
+ event.type = PTP_CLOCK_EXTTS;
+ event.index = 1;
+ event.timestamp = sec * 1000000000ULL + nsec;
+ ptp_clock_event(adapter->ptp_clock, &event);
+ }
}
static irqreturn_t igb_msix_other(int irq, void *data)
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 70d3933..37b9fe6 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -360,16 +360,203 @@ static int igb_ptp_settime_i210(struct ptp_clock_info *ptp,
return 0;
}
+static void igb_pin_direction(int pin, int input, u32 *ctrl, u32 *ctrl_ext)
+{
+ u32 *ptr = pin < 2 ? ctrl : ctrl_ext;
+ u32 mask[IGB_N_SDP] = {
+ E1000_CTRL_SDP0_DIR,
+ E1000_CTRL_SDP1_DIR,
+ E1000_CTRL_EXT_SDP2_DIR,
+ E1000_CTRL_EXT_SDP3_DIR,
+ };
+
+ if (input)
+ *ptr &= ~mask[pin];
+ else
+ *ptr |= mask[pin];
+}
+
+static void igb_pin_extts(struct igb_adapter *igb, int chan, int pin)
+{
+ struct e1000_hw *hw = &igb->hw;
+ u32 aux0_sel_sdp[IGB_N_SDP] = {
+ AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
+ };
+ u32 aux1_sel_sdp[IGB_N_SDP] = {
+ AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
+ };
+ u32 ts_sdp_en[IGB_N_SDP] = {
+ TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
+ };
+ u32 ctrl, ctrl_ext, tssdp = 0;
+
+ ctrl = rd32(E1000_CTRL);
+ ctrl_ext = rd32(E1000_CTRL_EXT);
+ tssdp = rd32(E1000_TSSDP);
+
+ igb_pin_direction(pin, 1, &ctrl, &ctrl_ext);
+
+ /* Make sure this pin is not enabled as an ouput. */
+ tssdp &= ~ts_sdp_en[pin];
+
+ if (chan == 1) {
+ tssdp &= ~AUX1_SEL_SDP3;
+ tssdp |= aux1_sel_sdp[pin] | AUX1_TS_SDP_EN;
+ } else {
+ tssdp &= ~AUX0_SEL_SDP3;
+ tssdp |= aux0_sel_sdp[pin] | AUX0_TS_SDP_EN;
+ }
+
+ wr32(E1000_TSSDP, tssdp);
+ wr32(E1000_CTRL, ctrl);
+ wr32(E1000_CTRL_EXT, ctrl_ext);
+}
+
+static void igb_pin_perout(struct igb_adapter *igb, int chan, int pin)
+{
+ struct e1000_hw *hw = &igb->hw;
+ u32 aux0_sel_sdp[IGB_N_SDP] = {
+ AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
+ };
+ u32 aux1_sel_sdp[IGB_N_SDP] = {
+ AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
+ };
+ u32 ts_sdp_en[IGB_N_SDP] = {
+ TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
+ };
+ u32 ts_sdp_sel_tt0[IGB_N_SDP] = {
+ TS_SDP0_SEL_TT0, TS_SDP1_SEL_TT0,
+ TS_SDP2_SEL_TT0, TS_SDP3_SEL_TT0,
+ };
+ u32 ts_sdp_sel_tt1[IGB_N_SDP] = {
+ TS_SDP0_SEL_TT1, TS_SDP1_SEL_TT1,
+ TS_SDP2_SEL_TT1, TS_SDP3_SEL_TT1,
+ };
+ u32 ts_sdp_sel_clr[IGB_N_SDP] = {
+ TS_SDP0_SEL_FC1, TS_SDP1_SEL_FC1,
+ TS_SDP2_SEL_FC1, TS_SDP3_SEL_FC1,
+ };
+ u32 ctrl, ctrl_ext, tssdp = 0;
+
+ ctrl = rd32(E1000_CTRL);
+ ctrl_ext = rd32(E1000_CTRL_EXT);
+ tssdp = rd32(E1000_TSSDP);
+
+ igb_pin_direction(pin, 0, &ctrl, &ctrl_ext);
+
+ /* Make sure this pin is not enabled as an input. */
+ if ((tssdp & AUX0_SEL_SDP3) == aux0_sel_sdp[pin])
+ tssdp &= ~AUX0_TS_SDP_EN;
+
+ if ((tssdp & AUX1_SEL_SDP3) == aux1_sel_sdp[pin])
+ tssdp &= ~AUX1_TS_SDP_EN;
+
+ tssdp &= ~ts_sdp_sel_clr[pin];
+ if (chan == 1)
+ tssdp |= ts_sdp_sel_tt1[pin];
+ else
+ tssdp |= ts_sdp_sel_tt0[pin];
+
+ tssdp |= ts_sdp_en[pin];
+
+ wr32(E1000_TSSDP, tssdp);
+ wr32(E1000_CTRL, ctrl);
+ wr32(E1000_CTRL_EXT, ctrl_ext);
+}
+
static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
struct ptp_clock_request *rq, int on)
{
struct igb_adapter *igb =
container_of(ptp, struct igb_adapter, ptp_caps);
struct e1000_hw *hw = &igb->hw;
+ u32 tsauxc, tsim, tsauxc_mask, tsim_mask, trgttiml, trgttimh;
unsigned long flags;
- u32 tsim;
+ struct timespec ts;
+ int pin;
+ s64 ns;
switch (rq->type) {
+ case PTP_CLK_REQ_EXTTS:
+ if (on) {
+ pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS,
+ rq->extts.index);
+ if (pin < 0)
+ return -EBUSY;
+ igb_pin_extts(igb, rq->extts.index, pin);
+ }
+ if (rq->extts.index == 1) {
+ tsauxc_mask = TSAUXC_EN_TS1;
+ tsim_mask = TSINTR_AUTT1;
+ } else {
+ tsauxc_mask = TSAUXC_EN_TS0;
+ tsim_mask = TSINTR_AUTT0;
+ }
+ spin_lock_irqsave(&igb->tmreg_lock, flags);
+ tsauxc = rd32(E1000_TSAUXC);
+ tsim = rd32(E1000_TSIM);
+ if (on) {
+ tsauxc |= tsauxc_mask;
+ tsim |= tsim_mask;
+ } else {
+ tsauxc &= ~tsauxc_mask;
+ tsim &= ~tsim_mask;
+ }
+ wr32(E1000_TSAUXC, tsauxc);
+ wr32(E1000_TSIM, tsim);
+ spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+ return 0;
+
+ case PTP_CLK_REQ_PEROUT:
+ if (on) {
+ pin = ptp_find_pin(igb->ptp_clock, PTP_PF_PEROUT,
+ rq->perout.index);
+ if (pin < 0)
+ return -EBUSY;
+ igb_pin_perout(igb, rq->perout.index, pin);
+ }
+ ts.tv_sec = rq->perout.period.sec;
+ ts.tv_nsec = rq->perout.period.nsec;
+ ns = timespec_to_ns(&ts);
+ ns = ns >> 1;
+ if (on && ns < 500000LL) {
+ /* 2k interrupts per second is an awful lot. */
+ return -EINVAL;
+ }
+ ts = ns_to_timespec(ns);
+ if (rq->perout.index == 1) {
+ tsauxc_mask = TSAUXC_EN_TT1;
+ tsim_mask = TSINTR_TT1;
+ trgttiml = E1000_TRGTTIML1;
+ trgttimh = E1000_TRGTTIMH1;
+ } else {
+ tsauxc_mask = TSAUXC_EN_TT0;
+ tsim_mask = TSINTR_TT0;
+ trgttiml = E1000_TRGTTIML0;
+ trgttimh = E1000_TRGTTIMH0;
+ }
+ spin_lock_irqsave(&igb->tmreg_lock, flags);
+ tsauxc = rd32(E1000_TSAUXC);
+ tsim = rd32(E1000_TSIM);
+ if (on) {
+ int i = rq->perout.index;
+ igb->perout[i].start.tv_sec = rq->perout.start.sec;
+ igb->perout[i].start.tv_nsec = rq->perout.start.nsec;
+ igb->perout[i].period.tv_sec = ts.tv_sec;
+ igb->perout[i].period.tv_nsec = ts.tv_nsec;
+ wr32(trgttiml, rq->perout.start.sec);
+ wr32(trgttimh, rq->perout.start.nsec);
+ tsauxc |= tsauxc_mask;
+ tsim |= tsim_mask;
+ } else {
+ tsauxc &= ~tsauxc_mask;
+ tsim &= ~tsim_mask;
+ }
+ wr32(E1000_TSAUXC, tsauxc);
+ wr32(E1000_TSIM, tsim);
+ spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+ return 0;
+
case PTP_CLK_REQ_PPS:
spin_lock_irqsave(&igb->tmreg_lock, flags);
tsim = rd32(E1000_TSIM);
@@ -380,9 +567,6 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
wr32(E1000_TSIM, tsim);
spin_unlock_irqrestore(&igb->tmreg_lock, flags);
return 0;
-
- default:
- break;
}
return -EOPNOTSUPP;
@@ -394,6 +578,20 @@ static int igb_ptp_feature_enable(struct ptp_clock_info *ptp,
return -EOPNOTSUPP;
}
+static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
+ enum ptp_pin_function func, unsigned int chan)
+{
+ switch (func) {
+ case PTP_PF_NONE:
+ case PTP_PF_EXTTS:
+ case PTP_PF_PEROUT:
+ break;
+ case PTP_PF_PHYSYNC:
+ return -1;
+ }
+ return 0;
+}
+
/**
* igb_ptp_tx_work
* @work: pointer to work struct
@@ -784,6 +982,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
{
struct e1000_hw *hw = &adapter->hw;
struct net_device *netdev = adapter->netdev;
+ int i;
switch (hw->mac.type) {
case e1000_82576:
@@ -826,16 +1025,26 @@ void igb_ptp_init(struct igb_adapter *adapter)
break;
case e1000_i210:
case e1000_i211:
+ for (i = 0; i < IGB_N_SDP; i++) {
+ struct ptp_pin_desc *ppd = &adapter->sdp_config[i];
+ snprintf(ppd->name, sizeof(ppd->name), "SDP%d", i);
+ ppd->index = i;
+ ppd->func = PTP_PF_NONE;
+ }
snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr);
adapter->ptp_caps.owner = THIS_MODULE;
adapter->ptp_caps.max_adj = 62499999;
- adapter->ptp_caps.n_ext_ts = 0;
+ adapter->ptp_caps.n_ext_ts = IGB_N_EXTTS;
+ adapter->ptp_caps.n_per_out = IGB_N_PEROUT;
+ adapter->ptp_caps.n_pins = IGB_N_SDP;
adapter->ptp_caps.pps = 1;
+ adapter->ptp_caps.pin_config = adapter->sdp_config;
adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580;
adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
adapter->ptp_caps.gettime = igb_ptp_gettime_i210;
adapter->ptp_caps.settime = igb_ptp_settime_i210;
adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
+ adapter->ptp_caps.verify = igb_ptp_verify_pin;
/* Enable the timer functions by clearing bit 31. */
wr32(E1000_TSAUXC, 0x0);
break;
--
1.7.10.4
^ permalink raw reply related
* [PATCH net-next 3/4] igb: enable internal PPS for the i210.
From: Richard Cochran @ 2014-11-17 23:06 UTC (permalink / raw)
To: netdev
Cc: David Miller, bruce.w.allan, Jacob Keller, Jeff Kirsher,
John Ronciak, Matthew Vick
In-Reply-To: <cover.1416265321.git.richardcochran@gmail.com>
The i210 device can produce an interrupt on the full second. This
patch allows using this interrupt to generate an internal PPS event
for adjusting the kernel system time.
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++++
drivers/net/ethernet/intel/igb/igb_ptp.c | 32 +++++++++++++++++++++++++++--
2 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 7183a56..9412661 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter,
static void igb_tsync_interrupt(struct igb_adapter *adapter)
{
struct e1000_hw *hw = &adapter->hw;
+ struct ptp_clock_event event;
u32 tsicr = rd32(E1000_TSICR);
+ if (tsicr & TSINTR_SYS_WRAP) {
+ event.type = PTP_CLOCK_PPS;
+ ptp_clock_event(adapter->ptp_clock, &event);
+ }
+
if (tsicr & E1000_TSICR_TXTS) {
/* acknowledge the interrupt */
wr32(E1000_TSICR, E1000_TSICR_TXTS);
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index ce57128..70d3933 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -360,6 +360,34 @@ static int igb_ptp_settime_i210(struct ptp_clock_info *ptp,
return 0;
}
+static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
+ struct ptp_clock_request *rq, int on)
+{
+ struct igb_adapter *igb =
+ container_of(ptp, struct igb_adapter, ptp_caps);
+ struct e1000_hw *hw = &igb->hw;
+ unsigned long flags;
+ u32 tsim;
+
+ switch (rq->type) {
+ case PTP_CLK_REQ_PPS:
+ spin_lock_irqsave(&igb->tmreg_lock, flags);
+ tsim = rd32(E1000_TSIM);
+ if (on)
+ tsim |= TSINTR_SYS_WRAP;
+ else
+ tsim &= ~TSINTR_SYS_WRAP;
+ wr32(E1000_TSIM, tsim);
+ spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+ return 0;
+
+ default:
+ break;
+ }
+
+ return -EOPNOTSUPP;
+}
+
static int igb_ptp_feature_enable(struct ptp_clock_info *ptp,
struct ptp_clock_request *rq, int on)
{
@@ -802,12 +830,12 @@ void igb_ptp_init(struct igb_adapter *adapter)
adapter->ptp_caps.owner = THIS_MODULE;
adapter->ptp_caps.max_adj = 62499999;
adapter->ptp_caps.n_ext_ts = 0;
- adapter->ptp_caps.pps = 0;
+ adapter->ptp_caps.pps = 1;
adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580;
adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
adapter->ptp_caps.gettime = igb_ptp_gettime_i210;
adapter->ptp_caps.settime = igb_ptp_settime_i210;
- adapter->ptp_caps.enable = igb_ptp_feature_enable;
+ adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
/* Enable the timer functions by clearing bit 31. */
wr32(E1000_TSAUXC, 0x0);
break;
--
1.7.10.4
^ permalink raw reply related
* [PATCH net-next 2/4] igb: do not clobber the TSAUXC bits on reset.
From: Richard Cochran @ 2014-11-17 23:06 UTC (permalink / raw)
To: netdev
Cc: David Miller, bruce.w.allan, Jacob Keller, Jeff Kirsher,
John Ronciak, Matthew Vick
In-Reply-To: <cover.1416265321.git.richardcochran@gmail.com>
The TSAUXC register has a number of different bits, one of which disables
the main clock function. Previously, the clock was re-enabled by clearing
the entire register. This patch changes the code to preserve the values
of the other bits in that register.
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/intel/igb/igb_ptp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 794c139..ce57128 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -905,6 +905,8 @@ void igb_ptp_stop(struct igb_adapter *adapter)
void igb_ptp_reset(struct igb_adapter *adapter)
{
struct e1000_hw *hw = &adapter->hw;
+ unsigned long flags;
+ u32 tsauxc;
if (!(adapter->flags & IGB_FLAG_PTP))
return;
@@ -923,7 +925,11 @@ void igb_ptp_reset(struct igb_adapter *adapter)
case e1000_i210:
case e1000_i211:
/* Enable the timer functions and interrupts. */
- wr32(E1000_TSAUXC, 0x0);
+ spin_lock_irqsave(&adapter->tmreg_lock, flags);
+ tsauxc = rd32(E1000_TSAUXC);
+ tsauxc &= ~TSAUXC_DISABLE;
+ wr32(E1000_TSAUXC, tsauxc);
+ spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
wr32(E1000_TSIM, TSYNC_INTERRUPTS);
wr32(E1000_IMS, E1000_IMS_TS);
break;
--
1.7.10.4
^ permalink raw reply related
* [PATCH net-next 1/4] igb: refactor time sync interrupt handling
From: Richard Cochran @ 2014-11-17 23:06 UTC (permalink / raw)
To: netdev
Cc: David Miller, bruce.w.allan, Jacob Keller, Jeff Kirsher,
John Ronciak, Matthew Vick
In-Reply-To: <cover.1416265321.git.richardcochran@gmail.com>
The code that handles the time sync interrupt is repeated in three
different places. This patch refactors the identical code blocks into
a single helper function.
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 49 +++++++++++------------------
1 file changed, 19 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b0e12e7..7183a56 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5383,6 +5383,19 @@ void igb_update_stats(struct igb_adapter *adapter,
}
}
+static void igb_tsync_interrupt(struct igb_adapter *adapter)
+{
+ struct e1000_hw *hw = &adapter->hw;
+ u32 tsicr = rd32(E1000_TSICR);
+
+ if (tsicr & E1000_TSICR_TXTS) {
+ /* acknowledge the interrupt */
+ wr32(E1000_TSICR, E1000_TSICR_TXTS);
+ /* retrieve hardware timestamp */
+ schedule_work(&adapter->ptp_tx_work);
+ }
+}
+
static irqreturn_t igb_msix_other(int irq, void *data)
{
struct igb_adapter *adapter = data;
@@ -5414,16 +5427,8 @@ static irqreturn_t igb_msix_other(int irq, void *data)
mod_timer(&adapter->watchdog_timer, jiffies + 1);
}
- if (icr & E1000_ICR_TS) {
- u32 tsicr = rd32(E1000_TSICR);
-
- if (tsicr & E1000_TSICR_TXTS) {
- /* acknowledge the interrupt */
- wr32(E1000_TSICR, E1000_TSICR_TXTS);
- /* retrieve hardware timestamp */
- schedule_work(&adapter->ptp_tx_work);
- }
- }
+ if (icr & E1000_ICR_TS)
+ igb_tsync_interrupt(adapter);
wr32(E1000_EIMS, adapter->eims_other);
@@ -6202,16 +6207,8 @@ static irqreturn_t igb_intr_msi(int irq, void *data)
mod_timer(&adapter->watchdog_timer, jiffies + 1);
}
- if (icr & E1000_ICR_TS) {
- u32 tsicr = rd32(E1000_TSICR);
-
- if (tsicr & E1000_TSICR_TXTS) {
- /* acknowledge the interrupt */
- wr32(E1000_TSICR, E1000_TSICR_TXTS);
- /* retrieve hardware timestamp */
- schedule_work(&adapter->ptp_tx_work);
- }
- }
+ if (icr & E1000_ICR_TS)
+ igb_tsync_interrupt(adapter);
napi_schedule(&q_vector->napi);
@@ -6256,16 +6253,8 @@ static irqreturn_t igb_intr(int irq, void *data)
mod_timer(&adapter->watchdog_timer, jiffies + 1);
}
- if (icr & E1000_ICR_TS) {
- u32 tsicr = rd32(E1000_TSICR);
-
- if (tsicr & E1000_TSICR_TXTS) {
- /* acknowledge the interrupt */
- wr32(E1000_TSICR, E1000_TSICR_TXTS);
- /* retrieve hardware timestamp */
- schedule_work(&adapter->ptp_tx_work);
- }
- }
+ if (icr & E1000_ICR_TS)
+ igb_tsync_interrupt(adapter);
napi_schedule(&q_vector->napi);
--
1.7.10.4
^ permalink raw reply related
* [PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.
From: Richard Cochran @ 2014-11-17 23:06 UTC (permalink / raw)
To: netdev
Cc: David Miller, bruce.w.allan, Jacob Keller, Jeff Kirsher,
John Ronciak, Matthew Vick
This patch series adds three features: time stamping external events,
producing a periodic output signal, and an internal PPS event. The
i210 PCIe card has a 6 pin header with SDP0-3, making it easy to try
out this new functionality.
An earlier version of this series was posted way back in May, 2013,
but that version lacked a good way to assign functions to the pins. In
the mean time, the PHC core has a standard method to configure the
pins, and this series makes use of it.
Thanks,
Richard
Richard Cochran (4):
igb: refactor time sync interrupt handling
igb: do not clobber the TSAUXC bits on reset.
igb: enable internal PPS for the i210.
igb: enable auxiliary PHC functions for the i210.
drivers/net/ethernet/intel/igb/igb.h | 9 ++
drivers/net/ethernet/intel/igb/igb_main.c | 100 ++++++++----
drivers/net/ethernet/intel/igb/igb_ptp.c | 251 ++++++++++++++++++++++++++++-
3 files changed, 326 insertions(+), 34 deletions(-)
--
1.7.10.4
^ permalink raw reply
* Re: BCM4313 & brcmsmac & 3.12: only semi-working?
From: Maximilian Engelhardt @ 2014-11-17 22:36 UTC (permalink / raw)
To: Rafał Miłecki, Arend van Spriel
Cc: Michael Tokarev, Seth Forshee, brcm80211 development,
linux-wireless@vger.kernel.org, Network Development
In-Reply-To: <CACna6rw3XgdJz0uKkAuas8RTzozTdFp2mgF5g1uo7LHJVSMt4g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]
On Thursday 09 October 2014 10:21:12 Rafał Miłecki wrote:
> On 9 October 2014 09:52, Arend van Spriel <arend@broadcom.com> wrote:
> > I have been staring at differences brcmsmac and our internal driver (which
> > wl is derived from as well). Would you be able to make an mmiotrace
> > loading
> > wl and brcmsmac.
>
> Arend,
>
> As you noticed earlier, this is a WiFi card with BT. I've checked
> siutils.c from SDK 7.14.43.7 (got it from Netgear R8000) and it has
> three BCM4313 workarounds:
>
>
> 1) si_doattach
> SI_MSG(("Applying 4313 WARs\n"));
> si_pmu_chipcontrol(sih, 0, CCTRL_4313_12MA_LED_DRIVE,
> CCTRL_4313_12MA_LED_DRIVE);
>
> It's already implemented in bcma, see driver_chipcommon_pmu.c
>
>
> 2) si_epa_4313war
> /* EPA Fix */
> W_REG(sii->osh, &cc->gpiocontrol,
> R_REG(sii->osh, &cc->gpiocontrol) | GPIO_CTRL_EPA_EN_MASK);
>
> This is what you already implement in ai_epa_4313war
>
>
> 3) si_pmu_synth_pwrsw_4313_war
> I don't think it's implemented.
>
>
> 4) si_btcombo_p250_4313_war
> I also don't think it's implemented
>
>
> The last one (si_btcombo_p250_4313_war) looks promising, the comment
> above the function says:
> /** WL/BT control for 4313 btcombo boards >= P250 */
> I guess you need to re-implement this function and call it somewhere
> in brcmsmac.
Hi,
I just wanted to ask if there is any progress on this issue since I haven't
heard anything for a month. Please let me know if you need any additional
information.
Greetings,
Maxi
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1 net-next] net/core: include linux/types.h instead of asm/types.h
From: Eric Dumazet @ 2014-11-17 22:05 UTC (permalink / raw)
To: David Miller; +Cc: fabf, linux-kernel, netdev
In-Reply-To: <20141117.163532.788436432373669437.davem@davemloft.net>
On Mon, 2014-11-17 at 16:35 -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 17 Nov 2014 13:22:56 -0800
>
> > On Mon, 2014-11-17 at 22:08 +0100, Fabian Frederick wrote:
> >> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> >> ---
> >> net/core/link_watch.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/core/link_watch.c b/net/core/link_watch.c
> >> index bd0767e..49a9e3e 100644
> >> --- a/net/core/link_watch.c
> >> +++ b/net/core/link_watch.c
> >> @@ -21,7 +21,7 @@
> >> #include <linux/spinlock.h>
> >> #include <linux/workqueue.h>
> >> #include <linux/bitops.h>
> >> -#include <asm/types.h>
> >> +#include <linux/types.h>
> >>
> >
> > We have 99 such occurrences in net/
> >
> > Are you going to send 99 patches ?
>
> Yeah, probably best to consolidate into one or a few patches.
BTW the 99 occurrences are for asm/uaccess.h, it seems I gave the reply
on the wrong patch.
^ permalink raw reply
* [PATCH v2 net-next] icmp: Remove some spurious dropped packet profile hits from the ICMP path
From: Rick Jones @ 2014-11-17 22:04 UTC (permalink / raw)
To: netdev; +Cc: davem
From: Rick Jones <rick.jones2@hp.com>
If icmp_rcv() has successfully processed the incoming ICMP datagram, we
should use consume_skb() rather than kfree_skb() because a hit on the likes
of perf -e skb:kfree_skb is not called-for.
Signed-off-by: Rick Jones <rick.jones2@hp.com>
---
A test system hit with a flood ping hits on perf top -e ksb:kfre_skb before
the change and none after for the normal/success path. The IPv6 path would
be somewhat more ugly. For the time being, just deal with the overlap on
ping_rcv() between the two to avoid a possible double free of an skb.
diff --git a/include/net/ping.h b/include/net/ping.h
index 026479b..f074060 100644
--- a/include/net/ping.h
+++ b/include/net/ping.h
@@ -82,7 +82,7 @@ int ping_common_sendmsg(int family, struct msghdr *msg, size_t len,
int ping_v6_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
size_t len);
int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);
-void ping_rcv(struct sk_buff *skb);
+bool ping_rcv(struct sk_buff *skb);
#ifdef CONFIG_PROC_FS
struct ping_seq_afinfo {
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 36b7bfa..36f5584 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -190,7 +190,7 @@ EXPORT_SYMBOL(icmp_err_convert);
*/
struct icmp_control {
- void (*handler)(struct sk_buff *skb);
+ bool (*handler)(struct sk_buff *skb);
short error; /* This ICMP is classed as an error message */
};
@@ -746,7 +746,7 @@ static bool icmp_tag_validation(int proto)
* ICMP_PARAMETERPROB.
*/
-static void icmp_unreach(struct sk_buff *skb)
+static bool icmp_unreach(struct sk_buff *skb)
{
const struct iphdr *iph;
struct icmphdr *icmph;
@@ -839,10 +839,10 @@ static void icmp_unreach(struct sk_buff *skb)
icmp_socket_deliver(skb, info);
out:
- return;
+ return true;
out_err:
ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
- goto out;
+ return false;
}
@@ -850,17 +850,20 @@ out_err:
* Handle ICMP_REDIRECT.
*/
-static void icmp_redirect(struct sk_buff *skb)
+static bool icmp_redirect(struct sk_buff *skb)
{
if (skb->len < sizeof(struct iphdr)) {
ICMP_INC_STATS_BH(dev_net(skb->dev), ICMP_MIB_INERRORS);
- return;
+ return false;
}
- if (!pskb_may_pull(skb, sizeof(struct iphdr)))
- return;
+ if (!pskb_may_pull(skb, sizeof(struct iphdr))) {
+ /* there aught to be a stat */
+ return false;
+ }
icmp_socket_deliver(skb, icmp_hdr(skb)->un.gateway);
+ return true;
}
/*
@@ -875,7 +878,7 @@ static void icmp_redirect(struct sk_buff *skb)
* See also WRT handling of options once they are done and working.
*/
-static void icmp_echo(struct sk_buff *skb)
+static bool icmp_echo(struct sk_buff *skb)
{
struct net *net;
@@ -891,6 +894,8 @@ static void icmp_echo(struct sk_buff *skb)
icmp_param.head_len = sizeof(struct icmphdr);
icmp_reply(&icmp_param, skb);
}
+ /* should there be an ICMP stat for ignored echos? */
+ return true;
}
/*
@@ -900,7 +905,7 @@ static void icmp_echo(struct sk_buff *skb)
* MUST be accurate to a few minutes.
* MUST be updated at least at 15Hz.
*/
-static void icmp_timestamp(struct sk_buff *skb)
+static bool icmp_timestamp(struct sk_buff *skb)
{
struct timespec tv;
struct icmp_bxm icmp_param;
@@ -927,15 +932,17 @@ static void icmp_timestamp(struct sk_buff *skb)
icmp_param.data_len = 0;
icmp_param.head_len = sizeof(struct icmphdr) + 12;
icmp_reply(&icmp_param, skb);
-out:
- return;
+ return true;
+
out_err:
ICMP_INC_STATS_BH(dev_net(skb_dst(skb)->dev), ICMP_MIB_INERRORS);
- goto out;
+ return false;
}
-static void icmp_discard(struct sk_buff *skb)
+static bool icmp_discard(struct sk_buff *skb)
{
+ /* pretend it was a success */
+ return true;
}
/*
@@ -946,6 +953,7 @@ int icmp_rcv(struct sk_buff *skb)
struct icmphdr *icmph;
struct rtable *rt = skb_rtable(skb);
struct net *net = dev_net(rt->dst.dev);
+ bool success;
if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb)) {
struct sec_path *sp = skb_sec_path(skb);
@@ -1012,7 +1020,12 @@ int icmp_rcv(struct sk_buff *skb)
}
}
- icmp_pointers[icmph->type].handler(skb);
+ success = icmp_pointers[icmph->type].handler(skb);
+
+ if (success) {
+ consume_skb(skb);
+ return 0;
+ }
drop:
kfree_skb(skb);
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 736236c..ce2920f 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -955,7 +955,7 @@ EXPORT_SYMBOL_GPL(ping_queue_rcv_skb);
* All we need to do is get the socket.
*/
-void ping_rcv(struct sk_buff *skb)
+bool ping_rcv(struct sk_buff *skb)
{
struct sock *sk;
struct net *net = dev_net(skb->dev);
@@ -974,11 +974,11 @@ void ping_rcv(struct sk_buff *skb)
pr_debug("rcv on socket %p\n", sk);
ping_queue_rcv_skb(sk, skb_get(skb));
sock_put(sk);
- return;
+ return true;
}
pr_debug("no socket, dropping\n");
- /* We're called from icmp_rcv(). kfree_skb() is done there. */
+ return false;
}
EXPORT_SYMBOL_GPL(ping_rcv);
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 0929340..39b3ff9 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -679,6 +679,7 @@ static int icmpv6_rcv(struct sk_buff *skb)
const struct in6_addr *saddr, *daddr;
struct icmp6hdr *hdr;
u8 type;
+ bool success = false;
if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {
struct sec_path *sp = skb_sec_path(skb);
@@ -726,7 +727,7 @@ static int icmpv6_rcv(struct sk_buff *skb)
break;
case ICMPV6_ECHO_REPLY:
- ping_rcv(skb);
+ success = ping_rcv(skb);
break;
case ICMPV6_PKT_TOOBIG:
@@ -790,7 +791,14 @@ static int icmpv6_rcv(struct sk_buff *skb)
icmpv6_notify(skb, type, hdr->icmp6_code, hdr->icmp6_mtu);
}
- kfree_skb(skb);
+ /* until the v6 path can be better sorted assume failure and
+ * preserve the status quo behaviour for the rest of the paths to here
+ */
+ if (success)
+ consume_skb(skb);
+ else
+ kfree_skb(skb);
+
return 0;
csum_error:
^ permalink raw reply related
* ipip6 - integer underrun when handlince icmpv4 unreachable messages
From: Alexander Wetzel @ 2014-11-17 21:52 UTC (permalink / raw)
To: netdev; +Cc: roque, kuznet, r.venning, nate
Hello netdev,
the current code to translate icmpv4 "destination unreachable" packets
to icmpv6 is later generating an integer underrun when calling
icmpv6_send, by later calling skb_network_header_len and subtracting a
bigger number from a lower one.
The issue is not visible for vanilla kernels and works correctly from
a user perspective, but a kernel with the PAX patches and enabled
"size overflow protection" will panic immediately when it's getting an
icmpv4 destination unreachable packet back for an encapsulated ipv6
packet. (Remote tunnel endpoint not reachable.)
I think I've tracked the issue down and can show you the problem with
the code... as I understand it as non-programmer greping the sources
and googeling functions. I was even able to find a fix which passes
the functionality test, but I'm unqualified to rate the correctness of
it and so are reaching out to you for that.
What happens (output of printk's) with transport_header and
network_header around "skb_reset_network_header" is described below
the function.
Near the end of the mail there are two links to the gentoo bug tracker
and pax forum, suggesting to put this forward to the lkml/netdev for
review, also including more details on the panics.
So here the function "ipip6_err_gen_icmpv6_unreach" from
net/ipv6/sit.c with some remarks and one new line which seems to fix
the problem:
************************************************************************
static int ipip6_err_gen_icmpv6_unreach(struct sk_buff *skb)
{
int ihl = ((const struct iphdr *)skb->data)->ihl*4;
struct rt6_info *rt;
struct sk_buff *skb2;
if (!pskb_may_pull(skb, ihl + sizeof(struct ipv6hdr) + 8))
return 1;
// we clone the ipv4 skb in skb2 to prepare the icmpv6 packet
skb2 = skb_clone(skb, GFP_ATOMIC);
if (!skb2)
return 1;
// we clean up the cloned skb2
skb_dst_drop(skb2);
skb_pull(skb2, ihl);
// The network header is reset
skb_reset_network_header(skb2);
//THE PROPOSED FIX: The following line is NOT in the current code
skb_reset_transport_header(skb2);
rt = rt6_lookup(dev_net(skb->dev), &ipv6_hdr(skb2)->saddr,
NULL, 0, 0);
if (rt && rt->dst.dev)
skb2->dev = rt->dst.dev;
icmpv6_send(skb2, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, 0);
if (rt)
ip6_rt_put(rt);
kfree_skb(skb2);
return 0;
************************************************************************
With debug printk's prior to "skb_reset_network_header(skb2);" I get
the following values when the code is used:
sk2b->transport_header: 62
skb2->network_header : 4e
After "skb_reset_network_header(skb2);" it reads:
sk2b->transport_header: 62
skb2->network_header : 7e
That are the same values which are later causing the integer underrun
(and with PAX a kernel panic) in "skb_network_header_len"
Adding "skb_reset_network_header(skb2);" prevents that, the transport
header size is extended:
sk2b->transport_header: 7e
skb2->network_header : 7e
Some more details on the error, a clean diff for the proposed patch
(without the comments) and the full debugging can be fond here:
https://forums.grsecurity.net/viewtopic.php?t=4083
https://bugs.gentoo.org/show_bug.cgi?id=529352
Can you please verify if this is the correct way to fix it and include
the correct fix - if any - in future kernel releases?
Or do you think that the PAX patch is wrong and an integer underrun is
acceptable here?
I'll check the mailing list archives from time to time for replies,
but I'm not subscribed and if you need me for more information or
tests please add me on CC.
Cheers,
Alexander Wetzel
^ permalink raw reply
* Re: [PATCH Iproute2 next v1] ip link: Add ipvlan support to the iproute2/ip util
From: Mahesh Bandewar @ 2014-11-17 21:54 UTC (permalink / raw)
To: Sathya Perla
Cc: netdev, Stephen Hemminger, Eric Dumazet, Maciej Zenczykowski,
Laurent Chavey, Tim Hockin, David Miller, Brandon Philips,
Pavel Emelianov
In-Reply-To: <CF9D1877D81D214CB0CA0669EFAE020C68CEC1DB@CMEXMB1.ad.emulex.com>
On Sun, Nov 16, 2014 at 10:57 PM, Sathya Perla <Sathya.Perla@emulex.com> wrote:
>
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-
> >
> > Adding basic support to create virtual devices using 'ip'
> > utility. Following is the syntax -
> >
> > ip link add link <master> <virtual> type ipvlan mode [ l2 | l3 ]
> > e.g. ip link add link eth0 ipvl0 type ipvlan mode l3
> >
> ..
> > ---
> > include/linux/if_link.h | 14 ++++++++
> > ip/Makefile | 2 +-
> > ip/iplink.c | 2 +-
> > ip/iplink_ipvlan.c | 85
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 101 insertions(+), 2 deletions(-)
> > create mode 100644 ip/iplink_ipvlan.c
> >
> > diff --git a/include/linux/if_link.h b/include/linux/if_link.h
> > index 47320636361c..ef1e9f73fb15 100644
> > --- a/include/linux/if_link.h
> > +++ b/include/linux/if_link.h
> > @@ -325,6 +325,20 @@ enum macvlan_macaddr_mode {
> > MACVLAN_MACADDR_SET,
> > };
> >
> > +/* IPVLAN section */
> > +enum {
> > + IFLA_IPVLAN_UNSPEC,
> > + IFLA_IPVLAN_MODE,
> > + __IFLA_IPVLAN_MAX,
> > +};
> > +
> > +#define IFLA_IPVLAN_MAX (__IFLA_IPVLAN_MAX - 1)
> > +
> > +enum ipvlan_mode {
> > + IPVLAN_MODE_L2 = 0, /* Process packets all the way upto L2 */
> > + IPVLAN_MODE_L3 = 1, /* Process Packets all the way upto L3 */
> > +};
> > +
> > #define MACVLAN_FLAG_NOPROMISC 1
> >
> >... +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/socket.h>
> > +#include <linux/if_link.h>
> > +
> > +#include "rt_names.h"
> > +#include "utils.h"
> > +#include "ip_common.h"
> > +
> > +static void explain(void)
> > +{
> > + fprintf(stderr, "Usage: ... ipvlan [ mode { l2 | l3 } ]\n");
> > +}
> > +
> > +static int mode_arg(void)
> > +{
> > + fprintf(stderr, "Error: argument of \"mode\" must be either \"l2\", "
> > + "or \"l3\"\n");
> > + return -1;
> I guess you wanted to "return -1" from the caller routine ipvlan_parse_opt()
> and not from this routine.
>
Hmmm, that caught another error where I intended it to be "return
mode_arg()" instead of just "mode_arg" in the caller. This is clearly
wrong! Thanks for catching it.
> > +}
> > +
> > +static int ipvlan_parse_opt(struct link_util *lu, int argc, char **argv,
> > + struct nlmsghdr *n)
> > +{
> > + while (argc > 0) {
> > + if (matches(*argv, "mode") == 0) {
> > + __u16 mode = 0;
> > + NEXT_ARG();
> > +
> > + if (strcmp(*argv, "l2") == 0)
> > + mode = IPVLAN_MODE_L2;
> > + else if (strcmp(*argv, "l3") == 0)
> > + mode = IPVLAN_MODE_L3;
> > + else
> > + mode_arg();
> > +
> > + addattr16(n, 1024, IFLA_IPVLAN_MODE, mode);
> > + } else if (matches(*argv, "help") == 0) {
> > + explain();
> > + return -1;
> > + } else {
> > + fprintf(stderr, "ipvlan: unknown option \"%s\"?\n",
> > *argv);
> > + explain();
> > + return -1;
> > + }
> > + argc--, argv++;
> > + }
> > +
> > + return 0;
> > +}
> > +
^ permalink raw reply
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