Netdev List
 help / color / mirror / Atom feed
* [PATCH net] sock: fix sendmmsg for partial sendmsg
From: Soheil Hassas Yeganeh @ 2016-11-04 19:36 UTC (permalink / raw)
  To: davem, netdev; +Cc: edumazet, willemb, ncardwell, Soheil Hassas Yeganeh

From: Soheil Hassas Yeganeh <soheil@google.com>

Do not send the next message in sendmmsg for partial sendmsg
invocations.

sendmmsg assumes that it can continue sending the next message
when the return value of the individual sendmsg invocations
is positive. It results in corrupting the data for TCP,
SCTP, and UNIX streams.

For example, sendmmsg([["abcd"], ["efgh"]]) can result in a stream
of "aefgh" if the first sendmsg invocation sends only the first
byte while the second sendmsg goes through.

Datagram sockets either send the entire datagram or fail, so
this patch affects only sockets of type SOCK_STREAM and
SOCK_SEQPACKET.

Fixes: 228e548e6020 ("net: Add sendmmsg socket system call")
Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Neal Cardwell <ncardwell@google.com>
---
 net/socket.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/socket.c b/net/socket.c
index 5a9bf5e..272518b 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2038,6 +2038,8 @@ int __sys_sendmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
 		if (err)
 			break;
 		++datagrams;
+		if (msg_data_left(&msg_sys))
+			break;
 		cond_resched();
 	}
 
-- 
2.8.0.rc3.226.g39d4020

^ permalink raw reply related

* Re: [PATCH net-next] sock: do not set sk_err in sock_dequeue_err_skb
From: Willem de Bruijn @ 2016-11-04 19:26 UTC (permalink / raw)
  To: Hannes Frederic Sowa
  Cc: Soheil Hassas Yeganeh, David Miller, Network Development,
	Eric Dumazet, Willem de Bruijn, Neal Cardwell,
	Soheil Hassas Yeganeh, Andy Lutomirski
In-Reply-To: <a3f7ea70-658e-9836-1a09-d1b9586582eb@stressinduktion.org>

On Thu, Nov 3, 2016 at 7:10 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> [also cc'ed Andy, albeit this doesn't seem to solve his initial problem,
> right? <http://www.spinics.net/lists/netdev/msg331753.html>]

Indeed, this does not help disambiguate the source of an error
returned by a socketcall. It only reduces how often sk_err is leaked
into the socket call datapath. Local errors and timestamps never set
sk_err in the first place, so there can be no expectation that they
set it on dequeue.

I suspect that that line in sock_dequeue_err_skb originates from icmp
errors, as those do set sk_err whenever queueing an error. So
reenabling it when there are multiple errors on the queue makes sense,
if all queued errors would be icmp errors. But they are not. And that
path is racy, so there is no guarantee, either way. Processes that
rely on syscall blocking to read the errqueue still block for icmp
errors after this patch, due to the initial sk_err assignment on
enqueue.

Note that the comment in the patch that only TCP socket calls are
blocked is probably incorrect. TCP has its own check in tcp_sendmsg
and returns EPIPE when an sk_err is set, without clearing it. But
other send, recv, connect, .. implementations include a path to
sock_error that aborts the call (e.g., in __skb_try_recv_datagram),
returns the value of sk_err, and clears it. This is an inconsistency
between the protocols.

Andy's original problem mentions recvmsg. We could probably make that
unambiguous by returning a cmsg with the value of sk_err if RECVERR is
set and sk_err is the source of the error. But this still does not
solve the general issue, as other socketcalls, like send and connect,
can also return sk_err. We can modify the number of places that check
for it and currently abort system calls. Mainly sock_error(). One
option is to add a socket option (SOL_SOCKET, as sk_err is not limited
to when INET_RECVERR is set) to make that a noop. If the caller sets
this option it instead has to wait with poll to receive POLLERR and
call getsockopt SO_ERROR and recv MSG_ERRQUEUE to get the asynchronous
error.

^ permalink raw reply

* Re: [PATCH net] fib_trie: correct /proc/net/route for large read buffer
From: Alexander Duyck @ 2016-11-04 19:13 UTC (permalink / raw)
  To: Jason Baron; +Cc: David Miller, Netdev, Andy Whitcroft, Alexander Duyck
In-Reply-To: <0e1e6651-3de6-6a71-cb37-be2942fb70b2@akamai.com>

On Fri, Nov 4, 2016 at 12:07 PM, Jason Baron <jbaron@akamai.com> wrote:
>
>
> On 11/04/2016 02:43 PM, Alexander Duyck wrote:
>>
>> On Fri, Nov 4, 2016 at 7:45 AM, Jason Baron <jbaron@akamai.com> wrote:
>>>
>>> From: Jason Baron <jbaron@akamai.com>
>>>
>>> When read() is called on /proc/net/route requesting a size that is one
>>> entry size (128 bytes) less than m->size or greater, the resulting output
>>> has missing and/or duplicate entries. Since m->size is typically
>>> PAGE_SIZE,
>>> for a PAGE_SIZE of 4,096 this means that reads requesting more than 3,968
>>> bytes will see bogus output.
>>>
>>> For example:
>>>
>>> for i in {100..200}; do
>>>         ip route add 192.168.1.$i dev eth0
>>> done
>>> dd if=/proc/net/route of=/tmp/good bs=1024
>>> dd if=/proc/net/route of=/tmp/bad bs=4096
>>>
>>> # diff -q /tmp/good /tmp/bad
>>> Files /tmp/good and /tmp/bad differ
>>>
>>> I think this has gone unnoticed, since the output of 'netstat -r' and
>>> 'route' is generated by reading in 1,024 byte increments and thus not
>>> corrupted. Further, the number of entries in the route table needs to be
>>> sufficiently large in order to trigger the problematic case.
>>>
>>> The issue arises because fib_route_get_idx() does not properly handle
>>> the case where pos equals iter->pos. This case only arises when we have
>>> a large read buffer size because we end up re-requesting the last entry
>>> that overflowed m->buf. In the case of a smaller read buffer size,
>>> we don't exceed the size of m->buf, and thus fib_route_get_idx() is
>>> called
>>> with pos greater than iter->pos.
>>>
>>> Fix by properly handling the iter->pos == pos case.
>>>
>>> Fixes: 25b97c016b26 ("ipv4: off-by-one in continuation handling in
>>> /proc/net/route")
>>> Cc: Andy Whitcroft <apw@canonical.com>
>>> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>>> ---
>>>  net/ipv4/fib_trie.c | 12 ++++++++++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>>> index 31cef3602585..1017533fc75c 100644
>>> --- a/net/ipv4/fib_trie.c
>>> +++ b/net/ipv4/fib_trie.c
>>> @@ -2411,12 +2411,17 @@ static struct key_vector
>>> *fib_route_get_idx(struct fib_route_iter *iter,
>>>                                             loff_t pos)
>>>  {
>>>         struct key_vector *l, **tp = &iter->tnode;
>>> +       loff_t saved_pos = 0;
>>>         t_key key;
>>>
>>>         /* use cache location of next-to-find key */
>>>         if (iter->pos > 0 && pos >= iter->pos) {
>>>                 pos -= iter->pos;
>>>                 key = iter->key;
>>> +               if (pos == 0) {
>>> +                       saved_pos = iter->pos;
>>> +                       key--;
>>> +               }
>>>         } else {
>>>                 iter->pos = 0;
>>>                 key = 0;
>>> @@ -2436,10 +2441,13 @@ static struct key_vector
>>> *fib_route_get_idx(struct fib_route_iter *iter,
>>>                         break;
>>>         }
>>>
>>> -       if (l)
>>> +       if (l) {
>>>                 iter->key = key;        /* remember it */
>>> -       else
>>> +               if (saved_pos)
>>> +                       iter->pos = saved_pos;
>>> +       } else {
>>>                 iter->pos = 0;          /* forget it */
>>> +       }
>>>
>>>         return l;
>>>  }
>>
>>
>> This doesn't seem correct to me.  I will have to look through this.
>> My understanding is that the value of iter->pos is supposed to be the
>> next position for us to grab, not the last one that was retrieved.  If
>> we are trying to re-request the last value then we should be falling
>> back into the else case for this since pos should be one less than
>> iter->pos.  The problem is the table could change out from under us
>> which is one of the reasons why we don't want to try and rewind the
>> key like you are doing here.
>>
>> - Alex
>>
>
> Hi Alex,
>
> In this case, seq_read() has called m->op->next(), which sets iter->pos
> equal to pos and iter->key to key + 1. However, when we then go to output
> the item associated with key, the 'm->op->next()' call overflows. Thus, we
> have a situation where iter->pos equals pos, iter->key = key + 1, but we
> have not displayed the item at position 'key' (thus the bug is that we miss
> the item at key).
>
> The change I proposed was simply to restart the search from 'key' in this
> case. If that item has disappeared, we will output the next one, or if its
> been replaced we will display its replacement. I think that is
> ok?
>
> The bug could also be fixed by changing:
>
> if (iter->pos > 0 && pos >= iter->pos) {
>
> to say:
>
> if (iter->pos > 0 && pos > iter->pos) {
>
> But that restarts the search on every overflow, which could mean every page
> size, and that seems suboptimal to me. Like-wise, if we make pos 1 less than
> iter->pos that restarts the search. The idea with this patch is to not force
> us to redo the entire search on each overflow.
>
> Thanks,
>
> -Jason

Actually I think the underlying issue is that we still have an
unresolved off by one error.  Specifically offset 0 actually
represents two values, the header for the file and entry 0.  I think
the fix is for us to look at pushing pos to 1 for the start of the
data, and we reserve POS 0 for SEQ_START_TOKEN. Doing that we should
start at the correct offset for each section following the first page.

- Alex

^ permalink raw reply

* Re: [PATCH net] fib_trie: correct /proc/net/route for large read buffer
From: Jason Baron @ 2016-11-04 19:07 UTC (permalink / raw)
  To: Alexander Duyck; +Cc: David Miller, Netdev, Andy Whitcroft, Alexander Duyck
In-Reply-To: <CAKgT0Ud7BVHfzbT=CsXn--_WSSt=hofcM3uNJcxdY=kaJ5psOA@mail.gmail.com>



On 11/04/2016 02:43 PM, Alexander Duyck wrote:
> On Fri, Nov 4, 2016 at 7:45 AM, Jason Baron <jbaron@akamai.com> wrote:
>> From: Jason Baron <jbaron@akamai.com>
>>
>> When read() is called on /proc/net/route requesting a size that is one
>> entry size (128 bytes) less than m->size or greater, the resulting output
>> has missing and/or duplicate entries. Since m->size is typically PAGE_SIZE,
>> for a PAGE_SIZE of 4,096 this means that reads requesting more than 3,968
>> bytes will see bogus output.
>>
>> For example:
>>
>> for i in {100..200}; do
>>         ip route add 192.168.1.$i dev eth0
>> done
>> dd if=/proc/net/route of=/tmp/good bs=1024
>> dd if=/proc/net/route of=/tmp/bad bs=4096
>>
>> # diff -q /tmp/good /tmp/bad
>> Files /tmp/good and /tmp/bad differ
>>
>> I think this has gone unnoticed, since the output of 'netstat -r' and
>> 'route' is generated by reading in 1,024 byte increments and thus not
>> corrupted. Further, the number of entries in the route table needs to be
>> sufficiently large in order to trigger the problematic case.
>>
>> The issue arises because fib_route_get_idx() does not properly handle
>> the case where pos equals iter->pos. This case only arises when we have
>> a large read buffer size because we end up re-requesting the last entry
>> that overflowed m->buf. In the case of a smaller read buffer size,
>> we don't exceed the size of m->buf, and thus fib_route_get_idx() is called
>> with pos greater than iter->pos.
>>
>> Fix by properly handling the iter->pos == pos case.
>>
>> Fixes: 25b97c016b26 ("ipv4: off-by-one in continuation handling in /proc/net/route")
>> Cc: Andy Whitcroft <apw@canonical.com>
>> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> ---
>>  net/ipv4/fib_trie.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>> index 31cef3602585..1017533fc75c 100644
>> --- a/net/ipv4/fib_trie.c
>> +++ b/net/ipv4/fib_trie.c
>> @@ -2411,12 +2411,17 @@ static struct key_vector *fib_route_get_idx(struct fib_route_iter *iter,
>>                                             loff_t pos)
>>  {
>>         struct key_vector *l, **tp = &iter->tnode;
>> +       loff_t saved_pos = 0;
>>         t_key key;
>>
>>         /* use cache location of next-to-find key */
>>         if (iter->pos > 0 && pos >= iter->pos) {
>>                 pos -= iter->pos;
>>                 key = iter->key;
>> +               if (pos == 0) {
>> +                       saved_pos = iter->pos;
>> +                       key--;
>> +               }
>>         } else {
>>                 iter->pos = 0;
>>                 key = 0;
>> @@ -2436,10 +2441,13 @@ static struct key_vector *fib_route_get_idx(struct fib_route_iter *iter,
>>                         break;
>>         }
>>
>> -       if (l)
>> +       if (l) {
>>                 iter->key = key;        /* remember it */
>> -       else
>> +               if (saved_pos)
>> +                       iter->pos = saved_pos;
>> +       } else {
>>                 iter->pos = 0;          /* forget it */
>> +       }
>>
>>         return l;
>>  }
>
> This doesn't seem correct to me.  I will have to look through this.
> My understanding is that the value of iter->pos is supposed to be the
> next position for us to grab, not the last one that was retrieved.  If
> we are trying to re-request the last value then we should be falling
> back into the else case for this since pos should be one less than
> iter->pos.  The problem is the table could change out from under us
> which is one of the reasons why we don't want to try and rewind the
> key like you are doing here.
>
> - Alex
>

Hi Alex,

In this case, seq_read() has called m->op->next(), which sets iter->pos 
equal to pos and iter->key to key + 1. However, when we then go to 
output the item associated with key, the 'm->op->next()' call overflows. 
Thus, we have a situation where iter->pos equals pos, iter->key = key + 
1, but we have not displayed the item at position 'key' (thus the bug is 
that we miss the item at key).

The change I proposed was simply to restart the search from 'key' in 
this case. If that item has disappeared, we will output the next one, or 
if its been replaced we will display its replacement. I think that is
ok?

The bug could also be fixed by changing:

if (iter->pos > 0 && pos >= iter->pos) {

to say:

if (iter->pos > 0 && pos > iter->pos) {

But that restarts the search on every overflow, which could mean every 
page size, and that seems suboptimal to me. Like-wise, if we make pos 1 
less than iter->pos that restarts the search. The idea with this patch 
is to not force us to redo the entire search on each overflow.

Thanks,

-Jason

^ permalink raw reply

* Re: [PATCH net 0/6] Mellanox 100G mlx5 fixes 2016-11-04
From: David Miller @ 2016-11-04 19:00 UTC (permalink / raw)
  To: saeedm; +Cc: netdev, ogerlitz, brouer
In-Reply-To: <1478216927-31328-1-git-send-email-saeedm@mellanox.com>

From: Saeed Mahameed <saeedm@mellanox.com>
Date: Fri,  4 Nov 2016 01:48:41 +0200

> This series contains six hot fixes of the mlx5 core and mlx5e driver.
> 
> Huy fixed an invalid pointer dereference on initialization flow for when
> the selected mlx5 load profile is out of range.
> 
> Or provided three eswitch offloads related fixes
>  - Prevent changing NS of a VF representor. 
>  - Handle matching on vlan priority for offloaded TC rules
>  - Set the actions for offloaded rules properly
> 
> On my part I here addressed the error flow related issues in
> mlx5e_open_channel reported by Jesper just this week.

Series applied, thanks.

^ permalink raw reply

* Re: [PATCH net-next resend 00/13] ring reconfiguration and XDP support
From: David Miller @ 2016-11-04 18:56 UTC (permalink / raw)
  To: jakub.kicinski; +Cc: netdev
In-Reply-To: <1478193129-23476-1-git-send-email-jakub.kicinski@netronome.com>

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu,  3 Nov 2016 17:11:56 +0000

> This set adds support for ethtool channel API and XDP.

Series applied, thank you!

^ permalink raw reply

* [PATCH net-next 2/2] tcp: no longer hold ehash lock while calling tcp_get_info()
From: Eric Dumazet @ 2016-11-04 18:54 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Neal Cardwell, Yuchung Cheng, Soheil Hassas Yeganeh,
	Eric Dumazet, Eric Dumazet
In-Reply-To: <1478285672-3195-1-git-send-email-edumazet@google.com>

We had various problems in the past in tcp_get_info() and used
specific synchronization to avoid deadlocks.

We would like to add more instrumentation points for TCP, and
avoiding grabing socket lock in tcp_getinfo() was too costly.

Being able to lock the socket allows to provide consistent set
of fields.

inet_diag_dump_icsk() can make sure ehash locks are not
held any more when tcp_get_info() is called.

We can remove syncp added in commit d654976cbf85
("tcp: fix a potential deadlock in tcp_get_info()"), but we need
to use lock_sock_fast() instead of spin_lock_bh() since TCP input
path can now be run from process context.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
---
 include/linux/tcp.h  |  2 --
 net/ipv4/inet_diag.c | 48 +++++++++++++++++++++++++++++++++---------------
 net/ipv4/tcp.c       | 20 +++++++++-----------
 net/ipv4/tcp_input.c |  4 ----
 4 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index a17ae7b85218..32a7c7e35b71 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -176,8 +176,6 @@ struct tcp_sock {
 				 * sum(delta(snd_una)), or how many bytes
 				 * were acked.
 				 */
-	struct u64_stats_sync syncp; /* protects 64bit vars (cf tcp_get_info()) */
-
  	u32	snd_una;	/* First byte we want an ack for	*/
  	u32	snd_sml;	/* Last byte of the most recently transmitted small packet */
 	u32	rcv_tstamp;	/* timestamp of last received ACK (for keepalives) */
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 3b34024202d8..4dea33e5f295 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -861,10 +861,11 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
 			 struct netlink_callback *cb,
 			 const struct inet_diag_req_v2 *r, struct nlattr *bc)
 {
+	bool net_admin = netlink_net_capable(cb->skb, CAP_NET_ADMIN);
 	struct net *net = sock_net(skb->sk);
-	int i, num, s_i, s_num;
 	u32 idiag_states = r->idiag_states;
-	bool net_admin = netlink_net_capable(cb->skb, CAP_NET_ADMIN);
+	int i, num, s_i, s_num;
+	struct sock *sk;
 
 	if (idiag_states & TCPF_SYN_RECV)
 		idiag_states |= TCPF_NEW_SYN_RECV;
@@ -877,7 +878,6 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
 
 		for (i = s_i; i < INET_LHTABLE_SIZE; i++) {
 			struct inet_listen_hashbucket *ilb;
-			struct sock *sk;
 
 			num = 0;
 			ilb = &hashinfo->listening_hash[i];
@@ -922,13 +922,14 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
 	if (!(idiag_states & ~TCPF_LISTEN))
 		goto out;
 
+#define SKARR_SZ 16
 	for (i = s_i; i <= hashinfo->ehash_mask; i++) {
 		struct inet_ehash_bucket *head = &hashinfo->ehash[i];
 		spinlock_t *lock = inet_ehash_lockp(hashinfo, i);
 		struct hlist_nulls_node *node;
-		struct sock *sk;
-
-		num = 0;
+		struct sock *sk_arr[SKARR_SZ];
+		int num_arr[SKARR_SZ];
+		int idx, accum, res;
 
 		if (hlist_nulls_empty(&head->chain))
 			continue;
@@ -936,9 +937,12 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
 		if (i > s_i)
 			s_num = 0;
 
+next_chunk:
+		num = 0;
+		accum = 0;
 		spin_lock_bh(lock);
 		sk_nulls_for_each(sk, node, &head->chain) {
-			int state, res;
+			int state;
 
 			if (!net_eq(sock_net(sk), net))
 				continue;
@@ -962,21 +966,35 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
 			if (!inet_diag_bc_sk(bc, sk))
 				goto next_normal;
 
-			res = sk_diag_fill(sk, skb, r,
+			sock_hold(sk);
+			num_arr[accum] = num;
+			sk_arr[accum] = sk;
+			if (++accum == SKARR_SZ)
+				break;
+next_normal:
+			++num;
+		}
+		spin_unlock_bh(lock);
+		res = 0;
+		for (idx = 0; idx < accum; idx++) {
+			if (res >= 0) {
+				res = sk_diag_fill(sk_arr[idx], skb, r,
 					   sk_user_ns(NETLINK_CB(cb->skb).sk),
 					   NETLINK_CB(cb->skb).portid,
 					   cb->nlh->nlmsg_seq, NLM_F_MULTI,
 					   cb->nlh, net_admin);
-			if (res < 0) {
-				spin_unlock_bh(lock);
-				goto done;
+				if (res < 0)
+					num = num_arr[idx];
 			}
-next_normal:
-			++num;
+			sock_gen_put(sk_arr[idx]);
 		}
-
-		spin_unlock_bh(lock);
+		if (res < 0)
+			break;
 		cond_resched();
+		if (accum == SKARR_SZ) {
+			s_num = num + 1;
+			goto next_chunk;
+		}
 	}
 
 done:
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 117982be0cab..a7d54cbcdabb 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -405,7 +405,6 @@ void tcp_init_sock(struct sock *sk)
 	tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;
 	tp->snd_cwnd_clamp = ~0;
 	tp->mss_cache = TCP_MSS_DEFAULT;
-	u64_stats_init(&tp->syncp);
 
 	tp->reordering = sock_net(sk)->ipv4.sysctl_tcp_reordering;
 	tcp_enable_early_retrans(tp);
@@ -2710,9 +2709,8 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
 	const struct tcp_sock *tp = tcp_sk(sk); /* iff sk_type == SOCK_STREAM */
 	const struct inet_connection_sock *icsk = inet_csk(sk);
 	u32 now = tcp_time_stamp, intv;
-	unsigned int start;
-	int notsent_bytes;
 	u64 rate64;
+	bool slow;
 	u32 rate;
 
 	memset(info, 0, sizeof(*info));
@@ -2792,17 +2790,17 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
 
 	info->tcpi_total_retrans = tp->total_retrans;
 
-	do {
-		start = u64_stats_fetch_begin_irq(&tp->syncp);
-		put_unaligned(tp->bytes_acked, &info->tcpi_bytes_acked);
-		put_unaligned(tp->bytes_received, &info->tcpi_bytes_received);
-	} while (u64_stats_fetch_retry_irq(&tp->syncp, start));
+	slow = lock_sock_fast(sk);
+
+	put_unaligned(tp->bytes_acked, &info->tcpi_bytes_acked);
+	put_unaligned(tp->bytes_received, &info->tcpi_bytes_received);
+	info->tcpi_notsent_bytes = max_t(int, 0, tp->write_seq - tp->snd_nxt);
+
+	unlock_sock_fast(sk, slow);
+
 	info->tcpi_segs_out = tp->segs_out;
 	info->tcpi_segs_in = tp->segs_in;
 
-	notsent_bytes = READ_ONCE(tp->write_seq) - READ_ONCE(tp->snd_nxt);
-	info->tcpi_notsent_bytes = max(0, notsent_bytes);
-
 	info->tcpi_min_rtt = tcp_min_rtt(tp);
 	info->tcpi_data_segs_in = tp->data_segs_in;
 	info->tcpi_data_segs_out = tp->data_segs_out;
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index f2c59c8e57ff..a70046fea0e8 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3351,9 +3351,7 @@ static void tcp_snd_una_update(struct tcp_sock *tp, u32 ack)
 	u32 delta = ack - tp->snd_una;
 
 	sock_owned_by_me((struct sock *)tp);
-	u64_stats_update_begin_raw(&tp->syncp);
 	tp->bytes_acked += delta;
-	u64_stats_update_end_raw(&tp->syncp);
 	tp->snd_una = ack;
 }
 
@@ -3363,9 +3361,7 @@ static void tcp_rcv_nxt_update(struct tcp_sock *tp, u32 seq)
 	u32 delta = seq - tp->rcv_nxt;
 
 	sock_owned_by_me((struct sock *)tp);
-	u64_stats_update_begin_raw(&tp->syncp);
 	tp->bytes_received += delta;
-	u64_stats_update_end_raw(&tp->syncp);
 	tp->rcv_nxt = seq;
 }
 
-- 
2.8.0.rc3.226.g39d4020

^ permalink raw reply related

* [PATCH net-next 1/2] tcp: shortcut listeners in tcp_get_info()
From: Eric Dumazet @ 2016-11-04 18:54 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Neal Cardwell, Yuchung Cheng, Soheil Hassas Yeganeh,
	Eric Dumazet, Eric Dumazet
In-Reply-To: <1478285672-3195-1-git-send-email-edumazet@google.com>

Being lockless in tcp_get_info() is hard, because we need to add
specific synchronization in TCP fast path, like seqcount.

Following patch will change inet_diag_dump_icsk() to no longer
hold any lock for non listeners, so that we can properly acquire
socket lock in get_tcp_info() and let it return more consistent counters.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
---
 net/ipv4/tcp.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3251fe71f39f..117982be0cab 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2721,6 +2721,27 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
 
 	info->tcpi_state = sk_state_load(sk);
 
+	/* Report meaningful fields for all TCP states, including listeners */
+	rate = READ_ONCE(sk->sk_pacing_rate);
+	rate64 = rate != ~0U ? rate : ~0ULL;
+	put_unaligned(rate64, &info->tcpi_pacing_rate);
+
+	rate = READ_ONCE(sk->sk_max_pacing_rate);
+	rate64 = rate != ~0U ? rate : ~0ULL;
+	put_unaligned(rate64, &info->tcpi_max_pacing_rate);
+
+	info->tcpi_reordering = tp->reordering;
+	info->tcpi_snd_cwnd = tp->snd_cwnd;
+
+	if (info->tcpi_state == TCP_LISTEN) {
+		/* listeners aliased fields :
+		 * tcpi_unacked -> Number of children ready for accept()
+		 * tcpi_sacked  -> max backlog
+		 */
+		info->tcpi_unacked = sk->sk_ack_backlog;
+		info->tcpi_sacked = sk->sk_max_ack_backlog;
+		return;
+	}
 	info->tcpi_ca_state = icsk->icsk_ca_state;
 	info->tcpi_retransmits = icsk->icsk_retransmits;
 	info->tcpi_probes = icsk->icsk_probes_out;
@@ -2748,13 +2769,9 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
 	info->tcpi_snd_mss = tp->mss_cache;
 	info->tcpi_rcv_mss = icsk->icsk_ack.rcv_mss;
 
-	if (info->tcpi_state == TCP_LISTEN) {
-		info->tcpi_unacked = sk->sk_ack_backlog;
-		info->tcpi_sacked = sk->sk_max_ack_backlog;
-	} else {
-		info->tcpi_unacked = tp->packets_out;
-		info->tcpi_sacked = tp->sacked_out;
-	}
+	info->tcpi_unacked = tp->packets_out;
+	info->tcpi_sacked = tp->sacked_out;
+
 	info->tcpi_lost = tp->lost_out;
 	info->tcpi_retrans = tp->retrans_out;
 	info->tcpi_fackets = tp->fackets_out;
@@ -2768,23 +2785,13 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
 	info->tcpi_rtt = tp->srtt_us >> 3;
 	info->tcpi_rttvar = tp->mdev_us >> 2;
 	info->tcpi_snd_ssthresh = tp->snd_ssthresh;
-	info->tcpi_snd_cwnd = tp->snd_cwnd;
 	info->tcpi_advmss = tp->advmss;
-	info->tcpi_reordering = tp->reordering;
 
 	info->tcpi_rcv_rtt = jiffies_to_usecs(tp->rcv_rtt_est.rtt)>>3;
 	info->tcpi_rcv_space = tp->rcvq_space.space;
 
 	info->tcpi_total_retrans = tp->total_retrans;
 
-	rate = READ_ONCE(sk->sk_pacing_rate);
-	rate64 = rate != ~0U ? rate : ~0ULL;
-	put_unaligned(rate64, &info->tcpi_pacing_rate);
-
-	rate = READ_ONCE(sk->sk_max_pacing_rate);
-	rate64 = rate != ~0U ? rate : ~0ULL;
-	put_unaligned(rate64, &info->tcpi_max_pacing_rate);
-
 	do {
 		start = u64_stats_fetch_begin_irq(&tp->syncp);
 		put_unaligned(tp->bytes_acked, &info->tcpi_bytes_acked);
-- 
2.8.0.rc3.226.g39d4020

^ permalink raw reply related

* [PATCH net-next 0/2] tcp: tcp_get_info() locking changes
From: Eric Dumazet @ 2016-11-04 18:54 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Neal Cardwell, Yuchung Cheng, Soheil Hassas Yeganeh,
	Eric Dumazet, Eric Dumazet

This short series prepares tcp_get_info() for more detailed infos.

In order to not slow down fast path, our goal is to use the normal
socket spinlock instead of custom synchronization.

All we need to ensure is that tcp_get_info() is not called with
ehash lock, which might dead lock, since packet processing would acquire
the spinlocks in reverse way.

Eric Dumazet (2):
  tcp: shortcut listeners in get_tcp_info()
  tcp: no longer hold ehash lock while calling tcp_get_info()

 include/linux/tcp.h  |  2 --
 net/ipv4/inet_diag.c | 48 +++++++++++++++++++++++++++++--------------
 net/ipv4/tcp.c       | 57 ++++++++++++++++++++++++++++------------------------
 net/ipv4/tcp_input.c |  4 ----
 4 files changed, 64 insertions(+), 47 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

^ permalink raw reply

* Re: [PATCH net-next v1 00/10] amd-xgbe: AMD XGBE driver updates 2016-11-03
From: David Miller @ 2016-11-04 18:52 UTC (permalink / raw)
  To: thomas.lendacky; +Cc: netdev
In-Reply-To: <20161103181727.7383.34446.stgit@tlendack-t1.amdoffice.net>

From: Tom Lendacky <thomas.lendacky@amd.com>
Date: Thu, 3 Nov 2016 13:17:28 -0500

> This patch series is targeted at preparing the driver for a new PCI version
> of the hardware.  After this series is applied, a follow-on series will
> introduce the support for the PCI version of the hardware.
> 
> The following updates and fixes are included in this driver update series:
> 
> - Fix formatting of PCS debug register dump
> - Prepare for priority-based FIFO allocation
> - Implement priority-based FIFO allocation
> - Prepare for working with more than one type of PCS/PHY
> - Prepare for the introduction of clause 37 auto-negotiation
> - Add support for clause 37 auto-negotiation
> - Prepare for supporting a new PCS register access method
> - Add support for 64-bit management counter registers
> - Update DMA channel status determination
> - Prepare for supporting PCI devices in addition to platform devices
> 
> This patch series is based on net-next.

Series applied, thanks Tom.

^ permalink raw reply

* Re: [PATCH net-next v2] net: inet: Support UID-based routing
From: David Miller @ 2016-11-04 18:45 UTC (permalink / raw)
  To: lorenzo; +Cc: netdev, ek, eric.dumazet, zenczykowski
In-Reply-To: <1478193823-60516-1-git-send-email-lorenzo@google.com>

From: Lorenzo Colitti <lorenzo@google.com>
Date: Fri,  4 Nov 2016 02:23:40 +0900

> This patchset adds support for per-UID routing.

Looks great, thanks for all of the hard work.

Series applied.

Please submit the necessary iproute2 patches, as needed.

Thanks again.

^ permalink raw reply

* Re: [PATCH net] fib_trie: correct /proc/net/route for large read buffer
From: Alexander Duyck @ 2016-11-04 18:43 UTC (permalink / raw)
  To: Jason Baron; +Cc: David Miller, Netdev, Andy Whitcroft, Alexander Duyck
In-Reply-To: <1478270730-11765-1-git-send-email-jbaron@akamai.com>

On Fri, Nov 4, 2016 at 7:45 AM, Jason Baron <jbaron@akamai.com> wrote:
> From: Jason Baron <jbaron@akamai.com>
>
> When read() is called on /proc/net/route requesting a size that is one
> entry size (128 bytes) less than m->size or greater, the resulting output
> has missing and/or duplicate entries. Since m->size is typically PAGE_SIZE,
> for a PAGE_SIZE of 4,096 this means that reads requesting more than 3,968
> bytes will see bogus output.
>
> For example:
>
> for i in {100..200}; do
>         ip route add 192.168.1.$i dev eth0
> done
> dd if=/proc/net/route of=/tmp/good bs=1024
> dd if=/proc/net/route of=/tmp/bad bs=4096
>
> # diff -q /tmp/good /tmp/bad
> Files /tmp/good and /tmp/bad differ
>
> I think this has gone unnoticed, since the output of 'netstat -r' and
> 'route' is generated by reading in 1,024 byte increments and thus not
> corrupted. Further, the number of entries in the route table needs to be
> sufficiently large in order to trigger the problematic case.
>
> The issue arises because fib_route_get_idx() does not properly handle
> the case where pos equals iter->pos. This case only arises when we have
> a large read buffer size because we end up re-requesting the last entry
> that overflowed m->buf. In the case of a smaller read buffer size,
> we don't exceed the size of m->buf, and thus fib_route_get_idx() is called
> with pos greater than iter->pos.
>
> Fix by properly handling the iter->pos == pos case.
>
> Fixes: 25b97c016b26 ("ipv4: off-by-one in continuation handling in /proc/net/route")
> Cc: Andy Whitcroft <apw@canonical.com>
> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> ---
>  net/ipv4/fib_trie.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 31cef3602585..1017533fc75c 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -2411,12 +2411,17 @@ static struct key_vector *fib_route_get_idx(struct fib_route_iter *iter,
>                                             loff_t pos)
>  {
>         struct key_vector *l, **tp = &iter->tnode;
> +       loff_t saved_pos = 0;
>         t_key key;
>
>         /* use cache location of next-to-find key */
>         if (iter->pos > 0 && pos >= iter->pos) {
>                 pos -= iter->pos;
>                 key = iter->key;
> +               if (pos == 0) {
> +                       saved_pos = iter->pos;
> +                       key--;
> +               }
>         } else {
>                 iter->pos = 0;
>                 key = 0;
> @@ -2436,10 +2441,13 @@ static struct key_vector *fib_route_get_idx(struct fib_route_iter *iter,
>                         break;
>         }
>
> -       if (l)
> +       if (l) {
>                 iter->key = key;        /* remember it */
> -       else
> +               if (saved_pos)
> +                       iter->pos = saved_pos;
> +       } else {
>                 iter->pos = 0;          /* forget it */
> +       }
>
>         return l;
>  }

This doesn't seem correct to me.  I will have to look through this.
My understanding is that the value of iter->pos is supposed to be the
next position for us to grab, not the last one that was retrieved.  If
we are trying to re-request the last value then we should be falling
back into the else case for this since pos should be one less than
iter->pos.  The problem is the table could change out from under us
which is one of the reasons why we don't want to try and rewind the
key like you are doing here.

- Alex

^ permalink raw reply

* Re: [PATCH net-next v2 00/11] net: dsa: mv88e6xxx: refine port operations
From: David Miller @ 2016-11-04 18:40 UTC (permalink / raw)
  To: vivien.didelot; +Cc: netdev, linux-kernel, kernel, f.fainelli, andrew
In-Reply-To: <20161104022336.14273-1-vivien.didelot@savoirfairelinux.com>

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Fri,  4 Nov 2016 03:23:25 +0100

> The Marvell chips have one internal SMI device per port, containing a
> set of registers used to configure a port's link, STP state, default
> VLAN or addresses database, etc.
> 
> This patchset creates port files to implement the port operations as
> described in datasheets, and extend the chip ops structure with them.
> 
> Patches 1 to 6 implement accessors for port's STP state, port based VLAN
> map, default FID, default VID, and 802.1Q mode.
> 
> Patches 7 to 11 implement the port's MAC setup of link state, duplex
> mode, RGMII delay and speed, all accessed through port's register 0x01.
> 
> The new port's MAC setup code is used to re-implement the adjust_link
> code and correctly force the link down before changing any of the MAC
> settings, as requested by the datasheets.
> 
> The port's MAC accessors use values compatible with struct phy_device
> (e.g. DUPLEX_FULL) and extend them when needed (e.g. SPEED_MAX).
> 
> Changes in v2:
> 
>   - Strictly use new _UNFORCED values instead of re-using _UNKNOWN ones.

Series applied, thanks Vivien.

^ permalink raw reply

* Re: [PATCH v2 2/2 ] net: ethernet: nb8800: handle all RGMII definitions
From: Sebastian Frias @ 2016-11-04 18:24 UTC (permalink / raw)
  To: David Miller; +Cc: f.fainelli, mans, netdev, linux-kernel, slash.tmp, andrew
In-Reply-To: <20161104.135448.1013810503648782291.davem@davemloft.net>

Hi David,

On 11/04/2016 06:54 PM, David Miller wrote:
> From: Sebastian Frias <sf84@laposte.net>
> Date: Fri, 4 Nov 2016 18:02:15 +0100
> 
>> Commit a999589ccaae ("phylib: add RGMII-ID interface mode definition")
>> and commit 7d400a4c5897 ("phylib: add PHY interface modes for internal
>> delay for tx and rx only") added several RGMII definitions:
>> PHY_INTERFACE_MODE_RGMII_ID, PHY_INTERFACE_MODE_RGMII_RXID and
>> PHY_INTERFACE_MODE_RGMII_TXID to deal with internal delays.
>>
>> Those are all RGMII modes (1Gbit) and must be considered that way when
>> setting the MAC mode or the pad mode for the HW to work properly.
>>
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> 
> You cannot just repost one part of a patch series when you make changes.
> 
> You must always repost the entire series as a new fresh version, with
> changelog entries added to your "[PATCH v2 0/2] ..." header posting.
> 

Thanks for the information.

I sent v2, and then v3, because v2 had formatting issues, hopefully it is
ok now.

Best regards,

Sebastian

^ permalink raw reply

* [PATCH v3 2/2] net: ethernet: nb8800: handle all RGMII definitions
From: Sebastian Frias @ 2016-11-04 18:23 UTC (permalink / raw)
  To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn
In-Reply-To: <581CD1AF.6030300@laposte.net>

Commit a999589ccaae ("phylib: add RGMII-ID interface mode definition")
and commit 7d400a4c5897 ("phylib: add PHY interface modes for internal
delay for tx and rx only") added several RGMII definitions:
PHY_INTERFACE_MODE_RGMII_ID, PHY_INTERFACE_MODE_RGMII_RXID and
PHY_INTERFACE_MODE_RGMII_TXID to deal with internal delays.

Those are all RGMII modes (1Gbit) and must be considered that way when
setting the MAC mode or the pad mode for the HW to work properly.

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
 drivers/net/ethernet/aurora/nb8800.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index d2855c9..fba2699 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -598,6 +598,7 @@ static irqreturn_t nb8800_irq(int irq, void *dev_id)
 static void nb8800_mac_config(struct net_device *dev)
 {
 	struct nb8800_priv *priv = netdev_priv(dev);
+	struct phy_device *phydev = dev->phydev;
 	bool gigabit = priv->speed == SPEED_1000;
 	u32 mac_mode_mask = RGMII_MODE | HALF_DUPLEX | GMAC_MODE;
 	u32 mac_mode = 0;
@@ -609,7 +610,7 @@ static void nb8800_mac_config(struct net_device *dev)
 		mac_mode |= HALF_DUPLEX;
 
 	if (gigabit) {
-		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+		if (phy_interface_is_rgmii(phydev))
 			mac_mode |= RGMII_MODE;
 
 		mac_mode |= GMAC_MODE;
@@ -1278,9 +1279,8 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII:
-		pad_mode = PAD_MODE_RGMII;
-		break;
-
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 		pad_mode = PAD_MODE_RGMII;
 		break;
-- 
1.7.11.2

^ permalink raw reply related

* [PATCH v3 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
From: Sebastian Frias @ 2016-11-04 18:22 UTC (permalink / raw)
  To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn
In-Reply-To: <581CD1AF.6030300@laposte.net>

The delay can be applied at PHY or MAC level, but since
PHY drivers will apply the delay at PHY level when using
one of the "internal delay" declinations of RGMII mode
(like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
at MAC level causes issues.

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index b59aa35..d2855c9 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;
 
 	default:
-- 
1.7.11.2

^ permalink raw reply related

* [PATCH v3 0/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
From: Sebastian Frias @ 2016-11-04 18:21 UTC (permalink / raw)
  To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn

This is v3 of the series, it fixes formatting issues of v2.

In v2 of the series, only the second patch:
"net: ethernet: nb8800: handle all RGMII definitions" is modified
to account for Florian's suggestion.

Sebastian Frias (2):
  net: ethernet: nb8800: Do not apply TX delay at MAC level
  net: ethernet: nb8800: handle all RGMII definitions

 drivers/net/ethernet/aurora/nb8800.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
1.7.11.2

^ permalink raw reply

* [PATCH v2 2/2] net: ethernet: nb8800: handle all RGMII definitions
From: Sebastian Frias @ 2016-11-04 18:19 UTC (permalink / raw)
  To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn
In-Reply-To: <581CD0CA.9070506@laposte.net>

Commit a999589ccaae ("phylib: add RGMII-ID interface mode definition")
and commit 7d400a4c5897 ("phylib: add PHY interface modes for internal
delay for tx and rx only") added several RGMII definitions:
PHY_INTERFACE_MODE_RGMII_ID, PHY_INTERFACE_MODE_RGMII_RXID and
PHY_INTERFACE_MODE_RGMII_TXID to deal with internal delays.

Those are all RGMII modes (1Gbit) and must be considered that way when
setting the MAC mode or the pad mode for the HW to work properly.

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
drivers/net/ethernet/aurora/nb8800.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index d2855c9..fba2699 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -598,6 +598,7 @@ static irqreturn_t nb8800_irq(int irq, void *dev_id)
static void nb8800_mac_config(struct net_device *dev)
{
struct nb8800_priv *priv = netdev_priv(dev);
+ struct phy_device *phydev = dev->phydev;
bool gigabit = priv->speed == SPEED_1000;
u32 mac_mode_mask = RGMII_MODE | HALF_DUPLEX | GMAC_MODE;
u32 mac_mode = 0;
@@ -609,7 +610,7 @@ static void nb8800_mac_config(struct net_device *dev)
mac_mode |= HALF_DUPLEX;

if (gigabit) {
- if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ if (phy_interface_is_rgmii(phydev))
mac_mode |= RGMII_MODE;

mac_mode |= GMAC_MODE;
@@ -1278,9 +1279,8 @@ static int nb8800_tangox_init(struct net_device *dev)
break;

case PHY_INTERFACE_MODE_RGMII:
- pad_mode = PAD_MODE_RGMII;
- break;
-
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
pad_mode = PAD_MODE_RGMII;
break;
-- 
1.7.11.2

^ permalink raw reply related

* [PATCH v2 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
From: Sebastian Frias @ 2016-11-04 18:18 UTC (permalink / raw)
  To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn
In-Reply-To: <581CD0CA.9070506@laposte.net>

The delay can be applied at PHY or MAC level, but since
PHY drivers will apply the delay at PHY level when using
one of the "internal delay" declinations of RGMII mode
(like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
at MAC level causes issues.

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
drivers/net/ethernet/aurora/nb8800.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index b59aa35..d2855c9 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
break;

case PHY_INTERFACE_MODE_RGMII_TXID:
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;

default:
-- 
1.7.11.2

^ permalink raw reply related

* [PATCH v2 0/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
From: Sebastian Frias @ 2016-11-04 18:17 UTC (permalink / raw)
  To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn

This is v2 of the series, only the second patch:
"net: ethernet: nb8800: handle all RGMII definitions" is modified
to account for Florian's suggestion.

Sebastian Frias (2):
  net: ethernet: nb8800: Do not apply TX delay at MAC level
  net: ethernet: nb8800: handle all RGMII definitions

 drivers/net/ethernet/aurora/nb8800.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
1.7.11.2

^ permalink raw reply

* [net-next v2 7/7] stmmac: dwmac-sti: remove unused priv dev member
From: Joachim Eastwood @ 2016-11-04 17:54 UTC (permalink / raw)
  To: davem; +Cc: Joachim Eastwood, peppe.cavallaro, alexandre.torgue, netdev
In-Reply-To: <20161104175411.12943-1-manabian@gmail.com>

The dev member of struct sti_dwmac is not used anywhere in the driver
so lets just remove it.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
index ac17bff..c9006ab 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
@@ -126,7 +126,6 @@ struct sti_dwmac {
 	struct clk *clk;	/* PHY clock */
 	u32 ctrl_reg;		/* GMAC glue-logic control register */
 	int clk_sel_reg;	/* GMAC ext clk selection register */
-	struct device *dev;
 	struct regmap *regmap;
 	bool gmac_en;
 	u32 speed;
@@ -274,7 +273,6 @@ static int sti_dwmac_parse_data(struct sti_dwmac *dwmac,
 		return err;
 	}
 
-	dwmac->dev = dev;
 	dwmac->interface = of_get_phy_mode(np);
 	dwmac->regmap = regmap;
 	dwmac->gmac_en = of_property_read_bool(np, "st,gmac_en");
-- 
2.10.2

^ permalink raw reply related

* [net-next v2 6/7] stmmac: dwmac-sti: clean up and rename sti_dwmac_init
From: Joachim Eastwood @ 2016-11-04 17:54 UTC (permalink / raw)
  To: davem; +Cc: Joachim Eastwood, peppe.cavallaro, alexandre.torgue, netdev
In-Reply-To: <20161104175411.12943-1-manabian@gmail.com>

Rename sti_dwmac_init to sti_dwmac_set_mode which is a better
description for what it really does.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
index b71888a..ac17bff 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
@@ -229,9 +229,8 @@ static void stid127_fix_retime_src(void *priv, u32 spd)
 	regmap_update_bits(dwmac->regmap, reg, STID127_RETIME_SRC_MASK, val);
 }
 
-static int sti_dwmac_init(struct platform_device *pdev, void *priv)
+static int sti_dwmac_set_mode(struct sti_dwmac *dwmac)
 {
-	struct sti_dwmac *dwmac = priv;
 	struct regmap *regmap = dwmac->regmap;
 	int iface = dwmac->interface;
 	u32 reg = dwmac->ctrl_reg;
@@ -245,7 +244,7 @@ static int sti_dwmac_init(struct platform_device *pdev, void *priv)
 	val = (iface == PHY_INTERFACE_MODE_REVMII) ? 0 : ENMII;
 	regmap_update_bits(regmap, reg, ENMII_MASK, val);
 
-	dwmac->fix_retime_src(priv, dwmac->speed);
+	dwmac->fix_retime_src(dwmac, dwmac->speed);
 
 	return 0;
 }
@@ -350,7 +349,7 @@ static int sti_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = sti_dwmac_init(pdev, plat_dat->bsp_priv);
+	ret = sti_dwmac_set_mode(dwmac);
 	if (ret)
 		goto disable_clk;
 
@@ -389,10 +388,9 @@ static int sti_dwmac_suspend(struct device *dev)
 static int sti_dwmac_resume(struct device *dev)
 {
 	struct sti_dwmac *dwmac = get_stmmac_bsp_priv(dev);
-	struct platform_device *pdev = to_platform_device(dev);
 
 	clk_prepare_enable(dwmac->clk);
-	sti_dwmac_init(pdev, dwmac);
+	sti_dwmac_set_mode(dwmac);
 
 	return stmmac_resume(dev);
 }
-- 
2.10.2

^ permalink raw reply related

* [net-next v2 5/7] stmmac: dwmac-sti: move clk_prepare_enable out of init and add error handling
From: Joachim Eastwood @ 2016-11-04 17:54 UTC (permalink / raw)
  To: davem; +Cc: Joachim Eastwood, peppe.cavallaro, alexandre.torgue, netdev
In-Reply-To: <20161104175411.12943-1-manabian@gmail.com>

Add clock error handling to probe and in the process move clock enabling
out of sti_dwmac_init() to make this easier.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
index 269c7a5..b71888a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
@@ -237,8 +237,6 @@ static int sti_dwmac_init(struct platform_device *pdev, void *priv)
 	u32 reg = dwmac->ctrl_reg;
 	u32 val;
 
-	clk_prepare_enable(dwmac->clk);
-
 	if (dwmac->gmac_en)
 		regmap_update_bits(regmap, reg, EN_MASK, EN);
 
@@ -348,11 +346,23 @@ static int sti_dwmac_probe(struct platform_device *pdev)
 	plat_dat->bsp_priv = dwmac;
 	plat_dat->fix_mac_speed = data->fix_retime_src;
 
-	ret = sti_dwmac_init(pdev, plat_dat->bsp_priv);
+	ret = clk_prepare_enable(dwmac->clk);
 	if (ret)
 		return ret;
 
-	return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	ret = sti_dwmac_init(pdev, plat_dat->bsp_priv);
+	if (ret)
+		goto disable_clk;
+
+	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	if (ret)
+		goto disable_clk;
+
+	return 0;
+
+disable_clk:
+	clk_disable_unprepare(dwmac->clk);
+	return ret;
 }
 
 static int sti_dwmac_remove(struct platform_device *pdev)
@@ -381,6 +391,7 @@ static int sti_dwmac_resume(struct device *dev)
 	struct sti_dwmac *dwmac = get_stmmac_bsp_priv(dev);
 	struct platform_device *pdev = to_platform_device(dev);
 
+	clk_prepare_enable(dwmac->clk);
 	sti_dwmac_init(pdev, dwmac);
 
 	return stmmac_resume(dev);
-- 
2.10.2

^ permalink raw reply related

* [net-next v2 4/7] stmmac: dwmac-sti: move st,gmac_en parsing to sti_dwmac_parse_data
From: Joachim Eastwood @ 2016-11-04 17:54 UTC (permalink / raw)
  To: davem; +Cc: Joachim Eastwood, peppe.cavallaro, alexandre.torgue, netdev
In-Reply-To: <20161104175411.12943-1-manabian@gmail.com>

The sti_dwmac_init() function is called both from probe and resume.
Since DT properties doesn't change between suspend/resume cycles move
parsing of this parameter into sti_dwmac_parse_data() where it belongs.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
index bd6db22..269c7a5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
@@ -128,6 +128,7 @@ struct sti_dwmac {
 	int clk_sel_reg;	/* GMAC ext clk selection register */
 	struct device *dev;
 	struct regmap *regmap;
+	bool gmac_en;
 	u32 speed;
 	void (*fix_retime_src)(void *priv, unsigned int speed);
 };
@@ -233,14 +234,12 @@ static int sti_dwmac_init(struct platform_device *pdev, void *priv)
 	struct sti_dwmac *dwmac = priv;
 	struct regmap *regmap = dwmac->regmap;
 	int iface = dwmac->interface;
-	struct device *dev = dwmac->dev;
-	struct device_node *np = dev->of_node;
 	u32 reg = dwmac->ctrl_reg;
 	u32 val;
 
 	clk_prepare_enable(dwmac->clk);
 
-	if (of_property_read_bool(np, "st,gmac_en"))
+	if (dwmac->gmac_en)
 		regmap_update_bits(regmap, reg, EN_MASK, EN);
 
 	regmap_update_bits(regmap, reg, MII_PHY_SEL_MASK, phy_intf_sels[iface]);
@@ -281,6 +280,7 @@ static int sti_dwmac_parse_data(struct sti_dwmac *dwmac,
 	dwmac->dev = dev;
 	dwmac->interface = of_get_phy_mode(np);
 	dwmac->regmap = regmap;
+	dwmac->gmac_en = of_property_read_bool(np, "st,gmac_en");
 	dwmac->ext_phyclk = of_property_read_bool(np, "st,ext-phyclk");
 	dwmac->tx_retime_src = TX_RETIME_SRC_NA;
 	dwmac->speed = SPEED_100;
-- 
2.10.2

^ permalink raw reply related

* [net-next v2 3/7] stmmac: dwmac-sti: add PM ops and resume function
From: Joachim Eastwood @ 2016-11-04 17:54 UTC (permalink / raw)
  To: davem; +Cc: Joachim Eastwood, peppe.cavallaro, alexandre.torgue, netdev
In-Reply-To: <20161104175411.12943-1-manabian@gmail.com>

Implement PM callbacks and driver remove in the driver instead
of relying on the init/exit hooks in stmmac_platform. This gives
the driver more flexibility in how the code is organized.

Eventually the init/exit callbacks will be deprecated in favor
of the standard PM callbacks and driver remove function.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Tested-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 47 +++++++++++++++++++------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
index f009bf4..bd6db22 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
@@ -253,12 +253,6 @@ static int sti_dwmac_init(struct platform_device *pdev, void *priv)
 	return 0;
 }
 
-static void sti_dwmac_exit(struct platform_device *pdev, void *priv)
-{
-	struct sti_dwmac *dwmac = priv;
-
-	clk_disable_unprepare(dwmac->clk);
-}
 static int sti_dwmac_parse_data(struct sti_dwmac *dwmac,
 				struct platform_device *pdev)
 {
@@ -352,8 +346,6 @@ static int sti_dwmac_probe(struct platform_device *pdev)
 	dwmac->fix_retime_src = data->fix_retime_src;
 
 	plat_dat->bsp_priv = dwmac;
-	plat_dat->init = sti_dwmac_init;
-	plat_dat->exit = sti_dwmac_exit;
 	plat_dat->fix_mac_speed = data->fix_retime_src;
 
 	ret = sti_dwmac_init(pdev, plat_dat->bsp_priv);
@@ -363,6 +355,41 @@ static int sti_dwmac_probe(struct platform_device *pdev)
 	return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
 }
 
+static int sti_dwmac_remove(struct platform_device *pdev)
+{
+	struct sti_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
+	int ret = stmmac_dvr_remove(&pdev->dev);
+
+	clk_disable_unprepare(dwmac->clk);
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int sti_dwmac_suspend(struct device *dev)
+{
+	struct sti_dwmac *dwmac = get_stmmac_bsp_priv(dev);
+	int ret = stmmac_suspend(dev);
+
+	clk_disable_unprepare(dwmac->clk);
+
+	return ret;
+}
+
+static int sti_dwmac_resume(struct device *dev)
+{
+	struct sti_dwmac *dwmac = get_stmmac_bsp_priv(dev);
+	struct platform_device *pdev = to_platform_device(dev);
+
+	sti_dwmac_init(pdev, dwmac);
+
+	return stmmac_resume(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static SIMPLE_DEV_PM_OPS(sti_dwmac_pm_ops, sti_dwmac_suspend,
+					   sti_dwmac_resume);
+
 static const struct sti_dwmac_of_data stih4xx_dwmac_data = {
 	.fix_retime_src = stih4xx_fix_retime_src,
 };
@@ -382,10 +409,10 @@ MODULE_DEVICE_TABLE(of, sti_dwmac_match);
 
 static struct platform_driver sti_dwmac_driver = {
 	.probe  = sti_dwmac_probe,
-	.remove = stmmac_pltfr_remove,
+	.remove = sti_dwmac_remove,
 	.driver = {
 		.name           = "sti-dwmac",
-		.pm		= &stmmac_pltfr_pm_ops,
+		.pm		= &sti_dwmac_pm_ops,
 		.of_match_table = sti_dwmac_match,
 	},
 };
-- 
2.10.2

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox