* Re: [net PATCH v3 2/5] net: virtio: wrap rtnl_lock in test for calling with lock already held
From: John Fastabend @ 2017-01-13 23:56 UTC (permalink / raw)
To: Stephen Hemminger
Cc: jasowang, mst, john.r.fastabend, netdev, alexei.starovoitov,
daniel
In-Reply-To: <58790F02.8090409@gmail.com>
On 17-01-13 09:31 AM, John Fastabend wrote:
> On 17-01-13 08:34 AM, Stephen Hemminger wrote:
>> On Thu, 12 Jan 2017 18:51:00 -0800
>> John Fastabend <john.fastabend@gmail.com> wrote:
>>
>>>
>>> -static void free_receive_bufs(struct virtnet_info *vi)
>>> +static void free_receive_bufs(struct virtnet_info *vi, bool need_lock)
>>> {
>>> struct bpf_prog *old_prog;
>>> int i;
>>>
>>> - rtnl_lock();
>>> + if (need_lock)
>>> + rtnl_lock();
>>> for (i = 0; i < vi->max_queue_pairs; i++) {
>>> while (vi->rq[i].pages)
>>> __free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0);
>>> @@ -1879,7 +1880,8 @@ static void free_receive_bufs(struct virtnet_info *vi)
>>> if (old_prog)
>>> bpf_prog_put(old_prog);
>>> }
>>> - rtnl_unlock();
>>> + if (need_lock)
>>> + rtnl_unlock();
>>> }
>>
>> Conditional locking is bad idea; sparse complains about it and is later source
>> of bugs. The more typical way of doing this in kernel is:
>
> OK I'll use the normal form.
>
>>
>> void _foo(some args)
>> {
>> ASSERT_RTNL();
>>
>> ...
>> }
>>
>> void foo(some args)
>> {
>> rtnl_lock();
>> _foo(some args)
>> rtnl_unlock();
>> }
>>
>>
>
Actually doing this without a rtnl_try_lock() is going to create two more
callbacks in virtio core just for virtio_net. All the other users do not
appear to have locking restrictions. How about the following it at least
helps in that there is no argument passing and if/else on the locks itself
but does use the if around rtnl_try_lock().
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1864,12 +1864,11 @@ static void virtnet_free_queues(struct virtnet_info *vi)
kfree(vi->sq);
}
-static void free_receive_bufs(struct virtnet_info *vi)
+static void _free_receive_bufs(struct virtnet_info *vi)
{
struct bpf_prog *old_prog;
int i;
- rtnl_lock();
for (i = 0; i < vi->max_queue_pairs; i++) {
while (vi->rq[i].pages)
__free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0);
@@ -1879,6 +1878,12 @@ static void free_receive_bufs(struct virtnet_info *vi)
if (old_prog)
bpf_prog_put(old_prog);
}
+}
+
+static void free_receive_bufs(struct virtnet_info *vi)
+{
+ rtnl_lock();
+ _free_receive_bufs(vi);
rtnl_unlock();
}
@@ -2358,7 +2363,10 @@ static void remove_vq_common(struct virtnet_info *vi)
/* Free unused buffers in both send and recv, if any. */
free_unused_bufs(vi);
- free_receive_bufs(vi);
+ if (rtnl_is_locked());
+ _free_receive_bufs(vi);
+ else
+ free_receive_bufs(vi);
free_receive_page_frags(vi);
^ permalink raw reply
* Re: [PATCH net] bpf: rework prog_digest into prog_tag
From: Daniel Borkmann @ 2017-01-13 23:59 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David S. Miller, Alexei Starovoitov, Andrew Lutomirski,
Network Development
In-Reply-To: <CALCETrWD7FuRLH0dC46bMf85mXosC3r54=TAXdar5EsZvbvyzg@mail.gmail.com>
On 01/14/2017 12:49 AM, Andy Lutomirski wrote:
> On Fri, Jan 13, 2017 at 3:41 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 01/14/2017 12:16 AM, Andy Lutomirski wrote:
>>> On Fri, Jan 13, 2017 at 2:38 PM, Daniel Borkmann <daniel@iogearbox.net>
>>> wrote:
>>>>
>>>> Commit 7bd509e311f4 ("bpf: add prog_digest and expose it via
>>>> fdinfo/netlink") was recently discussed, partially due to
>>>> admittedly suboptimal name of "prog_digest" in combination
>>>> with sha1 hash usage, thus inevitably and rightfully concerns
>>>> about its security in terms of collision resistance were
>>>> raised with regards to use-cases.
>>>
>>> Seems reasonable. My only question is whether you'd still want to
>>> switch to SHA-256 just from a code cleanliness perspective. With
>>> SHA-256 you can use the easy streaming API I wrote, but with SHA-1
>>> you're still stuck with the crappy API in lib/, and I'm not
>>> volunteering to fix up the SHA-1 API.
>>
>> We'd need to truncate that in kernel anyway to not get a too long
>> tag, so given that I'm actually fine with it as-is. I was planning
>> to submit the code for testing to bpf selftests for net-next once
>> it's merged back, too.
>
> Unless you want to kill off that vmalloc()+vfree() pair...
That is really just in slow-path, and should that become a bottleneck
compared to the rest of the verification steps or allocs we do there,
then we can always clean it up in net-next.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v2 net-next] Introduce a sysctl that modifies the value of PROT_SOCK.
From: Krister Johansen @ 2017-01-14 0:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Krister Johansen, Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1484231997.15816.36.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Jan 12, 2017 at 06:39:57AM -0800, Eric Dumazet wrote:
> On Wed, 2017-01-11 at 22:52 -0800, Krister Johansen wrote:
> > Add net.ipv4.ip_unprotected_port_start, which is a per namespace sysctl
> > that denotes the first unprotected inet port in the namespace. To
> > disable all protected ports set this to zero. It also checks for
> > overlap with the local port range. The protected and local range may
> > not overlap.
> >
> > The use case for this change is to allow containerized processes to bind
> > to priviliged ports, but prevent them from ever being allowed to modify
> > their container's network configuration. The latter is accomplished by
> > ensuring that the network namespace is not a child of the user
> > namespace. This modification was needed to allow the container manager
> > to disable a namespace's priviliged port restrictions without exposing
> > control of the network namespace to processes in the user namespace.
> >
> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> > ---
> > include/net/ip.h | 10 +++++++++
> > include/net/netns/ipv4.h | 1 +
> > net/ipv4/af_inet.c | 5 ++++-
> > net/ipv4/sysctl_net_ipv4.c | 50 +++++++++++++++++++++++++++++++++++++++++-
> > net/ipv6/af_inet6.c | 3 ++-
> > net/netfilter/ipvs/ip_vs_ctl.c | 7 +++---
> > net/sctp/socket.c | 10 +++++----
> > security/selinux/hooks.c | 3 ++-
>
> Adding a new sysctl without documentation is generally not accepted.
>
> Please take a look at Documentation/networking/ip-sysctl.txt
Thanks for catching this. I'll add an entry to the documentation.
> BTW, sticking to 'unprivileged' ports might be better than 'unprotected'
> which is vague.
I don't have a strong preference about the naming. I'd be happy to
change it to 'unprivileged' instead.
-K
^ permalink raw reply
* Re: [PATCH v2 net-next] Introduce a sysctl that modifies the value of PROT_SOCK.
From: Krister Johansen @ 2017-01-14 0:13 UTC (permalink / raw)
To: David Miller; +Cc: kjlx, stephen, netdev
In-Reply-To: <20170112.092213.864894939381841760.davem@davemloft.net>
On Thu, Jan 12, 2017 at 09:22:13AM -0500, David Miller wrote:
> From: Krister Johansen <kjlx@templeofstupid.com>
> > The use case for this change is to allow containerized processes to bind
> > to priviliged ports, but prevent them from ever being allowed to modify
> > their container's network configuration. The latter is accomplished by
> > ensuring that the network namespace is not a child of the user
> > namespace. This modification was needed to allow the container manager
> > to disable a namespace's priviliged port restrictions without exposing
> > control of the network namespace to processes in the user namespace.
>
> This is what CAP_NET_BIND_SERVICE is for, and why it is a separate
> network privilege, please use it.
It sounds like I may have done an inadequate job of explaining why I
took this approach instead of going the CAP_NET_BIND_SERVICE route.
In this scenario, the network namespace is created and configured first.
Then the containerized processed get placed into a separate user
namespace. This is so that the processes in the container, even if they
somehow manage to obtain extra privilege in the userns, can never modify
the network namespace.
The check in ns_capable() is looking at the priviliges of the user
namespace that created the netns and its parents. Even if I were to
grant a process in the container CAP_NET_BIND_SERVICE, ns_capable()
wouldn't recognize that as being a valid privilige for the netns.
If I were to invert the order of operations and create the userns before
the netns, then the capability would be recognized. However, that also
allows any potential privilege escalation in the userns to bring with it
the potential that an attacker can modify the container's network
configuration.
I'd much rather run the containers without privs, and without the userns
having rights to the netns, to mitigate the risk of an attacker being
able to alter the container's networking configuration.
-K
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Francois Romieu @ 2017-01-14 0:14 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Linux Kernel Network Developers, Michal Hocko,
Chas Williams, Andrey Konovalov
In-Reply-To: <CAM_iQpXd8qTTRT_+CkHnpMNC=CSS9vfB6zKtSjmLV_yya5BL8Q@mail.gmail.com>
Cong Wang <xiyou.wangcong@gmail.com> :
[...]
> If you can justify API is not broken by doing that, I am more than happy
> to do it, as I already stated in the latter patch:
>
> "Of course, the logic itself is suspicious, other sendmsg()
> could handle skb allocation failure very well, not sure
> why ATM has to wait for a successful one here. But probably
> it is too late to change since the errno and behavior is
> visible to user-space. So just leave the logic as it is."
>
> For some reason, no one reads that patch. :-/
Believe it or not but I actually read it.
It changes the logic : the original code would have been unable to
escape the while loop on memory failure. Fine, I don't mind the change.
Actually I believe that these two patches are too shy (and backport
unefficient). Instead of trying to reformulate why, here's what I have
in mind. Uncompiled, caveat emptor, etc.
I'll do a (slow) build and test on saturday's night with a pair of
iphase 5575.
diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..67f76f3 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -62,21 +62,16 @@ static void vcc_remove_socket(struct sock *sk)
write_unlock_irq(&vcc_sklist_lock);
}
-static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
+static bool vcc_tx_ready(struct atm_vcc *vcc, unsigned int size)
{
- struct sk_buff *skb;
struct sock *sk = sk_atm(vcc);
if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
- return NULL;
+ return false;
}
- while (!(skb = alloc_skb(size, GFP_KERNEL)))
- schedule();
- pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
- atomic_add(skb->truesize, &sk->sk_wmem_alloc);
- return skb;
+ return true;
}
static void vcc_sock_destruct(struct sock *sk)
@@ -606,7 +601,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
eff = (size+3) & ~3; /* align to word boundary */
prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
error = 0;
- while (!(skb = alloc_tx(vcc, eff))) {
+ while (!vcc_tx_ready(vcc, eff)) {
if (m->msg_flags & MSG_DONTWAIT) {
error = -EAGAIN;
break;
@@ -628,6 +623,13 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
finish_wait(sk_sleep(sk), &wait);
if (error)
goto out;
+
+ skb = alloc_skb(eff, GFP_KERNEL);
+ if (!skb)
+ goto out;
+ pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
+ atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+
skb->dev = NULL; /* for paths shared with net_device interfaces */
ATM_SKB(skb)->atm_options = vcc->atm_options;
if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {
--
Ueimor
^ permalink raw reply related
* Re: [Patch net] atm: remove an unnecessary loop
From: Francois Romieu @ 2017-01-14 0:15 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Michal Hocko, Chas Williams,
Andrey Konovalov
In-Reply-To: <CAM_iQpUCVh9y8uhZ8Hj0mcjgfMGUQDvaEvSu5Wur1oVn+EZNGQ@mail.gmail.com>
Cong Wang <xiyou.wangcong@gmail.com> :
> On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
[...]
> > alloc_skb() does not need to be in the "while" loop.
>
> This is exactly what I describe in my changelog, don't know
> why you want to repeat it...
Because it is still hidden in a while loop.
You turned the alloc from a two level deep "while" loop to a one level
one. I want it at zero level. alloc_skb(..., GFP_KERNEL) fails ?
So let it be done (see patch in other message).
[...]
> Please don't expect me to fix many things in one patch, let's
> fix each of them separately, agreed?
I am not convinced that several patches are needed to get the whole
picture right.
--
Ueimor
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Francois Romieu @ 2017-01-14 0:24 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Linux Kernel Network Developers, Michal Hocko,
Chas Williams, Andrey Konovalov
In-Reply-To: <20170114001422.GA6874@electric-eye.fr.zoreil.com>
Francois Romieu <romieu@fr.zoreil.com> :
[...]
Now with a proper error code. Have a nice night.
diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..e20d040 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -62,21 +62,16 @@ static void vcc_remove_socket(struct sock *sk)
write_unlock_irq(&vcc_sklist_lock);
}
-static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
+static bool vcc_tx_ready(struct atm_vcc *vcc, unsigned int size)
{
- struct sk_buff *skb;
struct sock *sk = sk_atm(vcc);
if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
- return NULL;
+ return false;
}
- while (!(skb = alloc_skb(size, GFP_KERNEL)))
- schedule();
- pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
- atomic_add(skb->truesize, &sk->sk_wmem_alloc);
- return skb;
+ return true;
}
static void vcc_sock_destruct(struct sock *sk)
@@ -606,7 +601,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
eff = (size+3) & ~3; /* align to word boundary */
prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
error = 0;
- while (!(skb = alloc_tx(vcc, eff))) {
+ while (!vcc_tx_ready(vcc, eff)) {
if (m->msg_flags & MSG_DONTWAIT) {
error = -EAGAIN;
break;
@@ -628,6 +623,15 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
finish_wait(sk_sleep(sk), &wait);
if (error)
goto out;
+
+ skb = alloc_skb(eff, GFP_KERNEL);
+ if (!skb) {
+ error = -ENOMEM;
+ goto out;
+ }
+ pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
+ atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+
skb->dev = NULL; /* for paths shared with net_device interfaces */
ATM_SKB(skb)->atm_options = vcc->atm_options;
if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {
^ permalink raw reply related
* Re: [Patch net] atm: remove an unnecessary loop
From: Cong Wang @ 2017-01-14 0:30 UTC (permalink / raw)
To: Chas Williams
Cc: David Miller, Francois Romieu, Linux Kernel Network Developers,
Michal Hocko, Andrey Konovalov
In-Reply-To: <1484351674.1643.16.camel@gmail.com>
On Fri, Jan 13, 2017 at 3:54 PM, Chas Williams <3chas3@gmail.com> wrote:
> On Fri, 2017-01-13 at 10:20 -0800, Cong Wang wrote:
>> On Fri, Jan 13, 2017 at 9:10 AM, David Miller <davem@davemloft.net> wrote:
>> > From: Francois Romieu <romieu@fr.zoreil.com>
>> > Date: Fri, 13 Jan 2017 01:07:00 +0100
>> >
>> >> Were alloc_skb moved one level up in the call stack, there would be
>> >> no need to use the new wait api in the subsequent page, thus easing
>> >> pre 3.19 longterm kernel maintenance (at least those on korg page).
>> >>
>> >> But it tastes a tad bit too masochistic.
>> >
>> > Lack of error handling of allocation failure is always a huge red
>> > flag. We even long ago tried to do something like this for TCP FIN
>> > handling.
>> >
>> > It's dumb, it doesn't work.
>> >
>> > Therefore I agree that the correct fix is to move the SKB allocation
>> > up one level to vcc_sendmsg() and make it handle errors properly.
>>
>> If you can justify API is not broken by doing that, I am more than happy
>> to do it, as I already stated in the latter patch:
>
> The man page for sendmsg() allows for ENOMEM. See below.
>
Errno is just one part, you miss the behavior behind the logic.
>>
>> "Of course, the logic itself is suspicious, other sendmsg()
>> could handle skb allocation failure very well, not sure
>> why ATM has to wait for a successful one here. But probably
>> it is too late to change since the errno and behavior is
>> visible to user-space. So just leave the logic as it is."
>>
>> For some reason, no one reads that patch. :-/
>
> I read it and I agree. I think it should be moved up/conflated with
> vcc_sendmsg(). vcc_sendmsg() can already return an errno for other
> conditions so if so has written something where they are explicitly
> not expecting a ENOMEM, we really can't help them.
Nope, the reason is never ENOMEM is expected or not. The current
_behavior_ behind this logic might be relied on by user-space.
The behavior here is, when allocation fails, kernel will retry under
certain circumstances, for example, if any fatal signal pending,
returns ERESTARTSYS, etc.. This is what I worry, not just ENOMEM
or not, which is too obvious.
Of course, I could be too conservative, I'd rather not to break things
for -stable at least.
Thanks.
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Cong Wang @ 2017-01-14 0:36 UTC (permalink / raw)
To: Francois Romieu
Cc: David Miller, Linux Kernel Network Developers, Michal Hocko,
Chas Williams, Andrey Konovalov
In-Reply-To: <20170114001422.GA6874@electric-eye.fr.zoreil.com>
On Fri, Jan 13, 2017 at 4:14 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> :
> [...]
>> If you can justify API is not broken by doing that, I am more than happy
>> to do it, as I already stated in the latter patch:
>>
>> "Of course, the logic itself is suspicious, other sendmsg()
>> could handle skb allocation failure very well, not sure
>> why ATM has to wait for a successful one here. But probably
>> it is too late to change since the errno and behavior is
>> visible to user-space. So just leave the logic as it is."
>>
>> For some reason, no one reads that patch. :-/
>
> Believe it or not but I actually read it.
>
> It changes the logic : the original code would have been unable to
> escape the while loop on memory failure. Fine, I don't mind the change.
> Actually I believe that these two patches are too shy (and backport
> unefficient). Instead of trying to reformulate why, here's what I have
> in mind. Uncompiled, caveat emptor, etc.
I just don't want to break things, that is it. If you can convince me your
change will not break any user-space application, again I am more
than just happy about it. My ATM knowledge is close to zero. ;)
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Cong Wang @ 2017-01-14 0:41 UTC (permalink / raw)
To: Francois Romieu
Cc: Linux Kernel Network Developers, Michal Hocko, Chas Williams,
Andrey Konovalov
In-Reply-To: <20170114001519.GB6874@electric-eye.fr.zoreil.com>
On Fri, Jan 13, 2017 at 4:15 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> :
>> On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> [...]
>> > alloc_skb() does not need to be in the "while" loop.
>>
>> This is exactly what I describe in my changelog, don't know
>> why you want to repeat it...
>
> Because it is still hidden in a while loop.
>
> You turned the alloc from a two level deep "while" loop to a one level
> one. I want it at zero level. alloc_skb(..., GFP_KERNEL) fails ?
> So let it be done (see patch in other message).
>
Why I didn't remove all the loops is already stated in the later patch,
you said you read it? I doubt. ;)
> [...]
>> Please don't expect me to fix many things in one patch, let's
>> fix each of them separately, agreed?
>
> I am not convinced that several patches are needed to get the whole
> picture right.
>
My guideline for stable fixes is one patch fixes one problem, maybe
not suitable to you I think. Let's agree to disagree. ;)
^ permalink raw reply
* Re: [net PATCH v3 5/5] virtio_net: XDP support for adjust_head
From: John Fastabend @ 2017-01-14 0:45 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <587933C2.8090003@gmail.com>
On 17-01-13 12:08 PM, John Fastabend wrote:
> On 17-01-12 11:41 PM, Jason Wang wrote:
>>
>>
>> On 2017年01月13日 10:52, John Fastabend wrote:
>>> Add support for XDP adjust head by allocating a 256B header region
>>> that XDP programs can grow into. This is only enabled when a XDP
>>> program is loaded.
>>>
>>> In order to ensure that we do not have to unwind queue headroom push
>>> queue setup below bpf_prog_add. It reads better to do a prog ref
>>> unwind vs another queue setup call.
>>>
>>> At the moment this code must do a full reset to ensure old buffers
>>> without headroom on program add or with headroom on program removal
>>> are not used incorrectly in the datapath. Ideally we would only
>>> have to disable/enable the RX queues being updated but there is no
>>> API to do this at the moment in virtio so use the big hammer. In
>>> practice it is likely not that big of a problem as this will only
>>> happen when XDP is enabled/disabled changing programs does not
>>> require the reset. There is some risk that the driver may either
>>> have an allocation failure or for some reason fail to correctly
>>> negotiate with the underlying backend in this case the driver will
>>> be left uninitialized. I have not seen this ever happen on my test
>>> systems and for what its worth this same failure case can occur
>>> from probe and other contexts in virtio framework.
>>>
>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>> ---
>>> drivers/net/virtio_net.c | 155 ++++++++++++++++++++++++++++++++++++++++------
>>> drivers/virtio/virtio.c | 9 ++-
>>> include/linux/virtio.h | 3 +
>>> 3 files changed, 144 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 6041828..8b897e7 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -28,6 +28,7 @@
>>> #include <linux/slab.h>
>>> #include <linux/cpu.h>
>>> #include <linux/average.h>
>>> +#include <linux/pci.h>
>>> #include <net/busy_poll.h>
>>> static int napi_weight = NAPI_POLL_WEIGHT;
>>> @@ -159,6 +160,9 @@ struct virtnet_info {
>>> /* Ethtool settings */
>>> u8 duplex;
>>> u32 speed;
>>> +
>>> + /* Headroom allocated in RX Queue */
>>> + unsigned int headroom;
>>
>> If this could not be changed in anyway, better use a macro instead of a filed
>> here. And there's even no need to add an extra parameter to
>> add_recvbuf_mergeable().
>
> OK originally I thought this might be dynamic but I agree no need
> for it here.
>
Well there is a bit of an order of operation issue that means we need at
least some bit here to tell us an enablement is pending.
The problem is when we do the reset we need to know that headroom for XDP
is needed. But we can't use the xdp_prog values because xdp_prog can not
be added on an device that is up without headroom otherwise the program
could fail. Plus reset via freeze/restore tears these structures down and
rebuilds them.
How about a boolean bit here instead of an unsigned int,
'bool xdp_headroom_needed'
seems better than an int.
Thanks,
John
^ permalink raw reply
* [PATCH] cxgb4: Remove redundant memset before memcpy
From: Shyam Saini @ 2017-01-14 1:17 UTC (permalink / raw)
To: hariprasad; +Cc: tklauser, netdev, linux-kernel, Shyam Saini
The region set by the call to memset, immediately overwritten by
the subsequent call to memcpy and thus makes the memset redundant.
Also remove the memset((&info, 0, sizeof(info)) on line 398 because
info is memcpy()'ed to before being used in the loop and it isn't
used outside of the loop.
Signed-off-by: Shyam Saini <mayhs11saini@gmail.com>
---
drivers/net/ethernet/chelsio/cxgb4/sched.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/sched.c b/drivers/net/ethernet/chelsio/cxgb4/sched.c
index cbd68a8..c902635 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sched.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sched.c
@@ -397,9 +397,6 @@ static struct sched_class *t4_sched_class_lookup(struct port_info *pi,
struct ch_sched_params info;
struct ch_sched_params tp;
- memset(&info, 0, sizeof(info));
- memset(&tp, 0, sizeof(tp));
-
memcpy(&tp, p, sizeof(tp));
/* Don't try to match class parameter */
tp.u.params.class = SCHED_CLS_NONE;
@@ -409,7 +406,6 @@ static struct sched_class *t4_sched_class_lookup(struct port_info *pi,
if (e->state == SCHED_STATE_UNUSED)
continue;
- memset(&info, 0, sizeof(info));
memcpy(&info, &e->info, sizeof(info));
/* Don't try to match class parameter */
info.u.params.class = SCHED_CLS_NONE;
@@ -458,7 +454,6 @@ static struct sched_class *t4_sched_class_alloc(struct port_info *pi,
if (!e)
goto out;
- memset(&np, 0, sizeof(np));
memcpy(&np, p, sizeof(np));
np.u.params.class = e->idx;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next] net: dsa: mv88e6xxx: add EEPROM support to 6390
From: David Miller @ 2017-01-14 1:18 UTC (permalink / raw)
To: vivien.didelot
Cc: netdev, linux-kernel, kernel, f.fainelli, andrew, cphealy,
nikita.yoush
In-Reply-To: <20170112230716.2083-1-vivien.didelot@savoirfairelinux.com>
From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Thu, 12 Jan 2017 18:07:16 -0500
> The Marvell 6352 chip has a 8-bit address/16-bit data EEPROM access.
> The Marvell 6390 chip has a 16-bit address/8-bit data EEPROM access.
>
> This patch implements the 8-bit data EEPROM access in the mv88e6xxx
> driver and adds its support to chips of the 6390 family.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] liquidio: use fallback for selecting txq
From: David Miller @ 2017-01-14 1:18 UTC (permalink / raw)
To: felix.manlunas; +Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla
In-Reply-To: <20170113001822.GA10813@felix.cavium.com>
From: Felix Manlunas <felix.manlunas@cavium.com>
Date: Thu, 12 Jan 2017 16:18:22 -0800
> From: Satanand Burla <satananda.burla@cavium.com>
>
> Remove assignment to ndo_select_queue so that fallback is used for
> selecting txq. Also remove the now-useless function that used to be
> assigned to ndo_select_queue.
>
> Signed-off-by: Satanand Burla <satananda.burla@cavium.com>
> Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
> Signed-off-by: Derek Chickles <derek.chickles@cavium.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH] i40e: Invoke softirqs after napi_reschedule
From: David Miller @ 2017-01-14 1:27 UTC (permalink / raw)
To: bpoirier; +Cc: jeffrey.t.kirsher, intel-wired-lan, netdev
In-Reply-To: <20170113010414.22708-1-bpoirier@suse.com>
From: Benjamin Poirier <bpoirier@suse.com>
Date: Thu, 12 Jan 2017 17:04:14 -0800
> The following message is logged from time to time when using i40e:
> NOHZ: local_softirq_pending 08
>
> i40e may schedule napi from a workqueue. Afterwards, softirqs are not run
> in a deterministic time frame. The problem is the same as what was
> described in commit ec13ee80145c ("virtio_net: invoke softirqs after
> __napi_schedule") and this patch applies the same fix to i40e.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
I hope to see this from one of Jeff's pull requests to me in the
near future.
^ permalink raw reply
* Re: [PATCH v5 02/13] net: ethernet: aquantia: Common functions and definitions
From: David Miller @ 2017-01-14 1:32 UTC (permalink / raw)
To: Alexander.Loktionov
Cc: netdev, vomlehn, Simon.Edelhaus, Dmitrii.Tarakanov, Pavel.Belous,
Dmitry.Bezrukov
In-Reply-To: <2c056fd544958594f6356a17892c251ea26ea8c8.1484283610.git.vomlehn@texas.net>
From: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
Date: Thu, 12 Jan 2017 21:02:18 -0800
> +#define AQ_OBJ_HEADER spinlock_t lock; atomic_t flags; atomic_t busy_count
> +
> +struct aq_obj_s {
> + AQ_OBJ_HEADER;
> +};
Please don't hide multiple declarations and types inside of a macro,
that makes the code harder to understand.
Use a sub-structure or similar, and pass that sub-structure to the
handlers.
> +#define AQ_OBJ_TST(_OBJ_, _FLAG_) ((_FLAG_) & atomic_read(&(_OBJ_)->flags))
> +
> +#define AQ_OBJ_SET(_OBJ_, _F_) \
...
> +#define AQ_OBJ_CLR(_OBJ_, _F_) \
Please don't reinvent the wheel.
Use test_bit, set_bit, clear_bit, test_and_set_bit, and
test_and_clear_bit. Using an atomic_t for flag bits is completely
inappropriate, that type is primarily meant for atomic counters.
The appropriate type for *_bit() operations is "unsigned long".
^ permalink raw reply
* Re: [PATCH v5 00/13] net: ethernet: aquantia: Add AQtion 2.5/5 GB NIC driver
From: Florian Fainelli @ 2017-01-14 1:38 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous
In-Reply-To: <cover.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> v1: Initial version
> v2: o Make necessary drivers/net/ethernet changes to integrate software
> o Drop intermediate atlantic directory
> o Remove Makefile things only appropriate to out of tree module
> building
> v3: o Move changes to drivers/net/ethernet/{Kconfig,Makefile} to the last
> patch to ensure clean bisection.
> o Removed inline attribute aq_hw_write_req() as it was defined in
> only one .c file.
> o #included pci.h in aq_common.h to get struct pci definition.
> o Modified code to unlock based execution flow rather than using a
> flag.
> o Made a number of functions that were only used in a single file
> static.
> o Cleaned up error and return code handling in various places.
> o Remove AQ_CFG_IP_ALIGN definition.
> o Other minor code clean up.
> v4: o Using do_div for 64 bit division.
> o Modified NIC statistics code.
> o Using build_skb instead netdev_alloc_skb for single fragment
> packets.
> o Removed extra aq_nic.o from Makefile
> v5: o Removed extra newline at the end of the files.
> o Wrapped cover letter lines.
Have not looked at the driver yet, but the threading of your emails is
weird, each patch is in reply to the previous one. It would be more
natural to have all numbered patches be in reply to the cover letter,
which according to the version of git you seem to have used (2.7.4)
should already be the default. In graphical terms what we see right now is:
[PATCH 00/13]
[PATCH 01/13]
[PATCH 02/13]
....
While we should see:
[PATCH 00/13]
[PATCH 01/13]
[PATCH 02/13]
....
Can you fix that for future submissions, this may sound like a cosmetic
thing, but it really helps with threading/reading etc.
Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH v5 11/13] net: ethernet: aquantia: Ethtool support
From: Florian Fainelli @ 2017-01-14 1:47 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous,
Dmitry Bezrukov
In-Reply-To: <b2236a13fcf9ac65f15cfc084de670948517e003.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> Add the driver interfaces required for support by the ethtool utility.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@aquantia.com>
> Signed-off-by: David M. VomLehn <vomlehn@texas.net>
> ---
> drivers/net/ethernet/aquantia/aq_ethtool.c | 250 +++++++++++++++++++++++++++++
> drivers/net/ethernet/aquantia/aq_ethtool.h | 19 +++
> 2 files changed, 269 insertions(+)
> create mode 100644 drivers/net/ethernet/aquantia/aq_ethtool.c
> create mode 100644 drivers/net/ethernet/aquantia/aq_ethtool.h
>
> diff --git a/drivers/net/ethernet/aquantia/aq_ethtool.c b/drivers/net/ethernet/aquantia/aq_ethtool.c
> new file mode 100644
> index 0000000..f11bdb1
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/aq_ethtool.c
> @@ -0,0 +1,250 @@
> +/*
> + * aQuantia Corporation Network Driver
> + * Copyright (C) 2014-2017 aQuantia Corporation. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + */
> +
> +/* File aq_ethtool.c: Definition of ethertool related functions. */
> +
> +#include "aq_ethtool.h"
> +#include "aq_nic.h"
> +
> +static void aq_ethtool_get_regs(struct net_device *ndev,
> + struct ethtool_regs *regs, void *p)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
netdev_priv() returns a void * which requires no casting, please fix
this through the entire 13 patches.
> + u32 regs_count = aq_nic_get_regs_count(aq_nic);
> +
> + memset(p, 0, regs_count * sizeof(u32));
> + aq_nic_get_regs(aq_nic, regs, p);
> +}
> +
> +static int aq_ethtool_get_regs_len(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + u32 regs_count = aq_nic_get_regs_count(aq_nic);
> +
> + return regs_count * sizeof(u32);
> +}
> +
> +static u32 aq_ethtool_get_link(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> +
> + return aq_nic_get_link_speed(aq_nic) ? 1U : 0U;
Can you either use PHYLIB to interface to your PHY (which does all the
nice state machine management) or at the very least ethtool_op_get_link()?
> +}
> +
> +static int aq_ethtool_get_settings(struct net_device *ndev,
> + struct ethtool_cmd *cmd)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> +
> + cmd->port = PORT_TP;
> + cmd->transceiver = XCVR_EXTERNAL;
> +
> + ethtool_cmd_speed_set(cmd, netif_carrier_ok(ndev) ?
> + aq_nic_get_link_speed(aq_nic) : 0U);
> +
> + cmd->duplex = DUPLEX_FULL;
> + aq_nic_get_link_settings(aq_nic, cmd);
> + return 0;
Consider switching to the new ksettings API and filling in a bit more
information like cmd->autoneg?
> +}
> +
> +static int aq_ethtool_set_settings(struct net_device *ndev,
> + struct ethtool_cmd *cmd)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> +
> + return aq_nic_set_link_settings(aq_nic, cmd);
> +}
> +
> +static const char aq_ethtool_stat_names[][ETH_GSTRING_LEN] = {
> + "InPackets",
> + "InUCast",
> + "InMCast",
> + "InBCast",
> + "InErrors",
> + "OutPackets",
> + "OutUCast",
> + "OutMCast",
> + "OutBCast",
> + "InUCastOctects",
> + "OutUCastOctects",
> + "InMCastOctects",
> + "OutMCastOctects",
> + "InBCastOctects",
> + "OutBCastOctects",
> + "InOctects",
> + "OutOctects",
> + "InPacketsDma",
> + "OutPacketsDma",
> + "InOctetsDma",
> + "OutOctetsDma",
> + "InDroppedDma",
> + "Queue[0] InPackets",
> + "Queue[0] OutPackets",
> + "Queue[0] InJumboPackets",
> + "Queue[0] InLroPackets",
> + "Queue[0] InErrors",
> +#if 1 < AQ_CFG_VECS_DEF
Yoda notations are usually frowned upon. Instead of making this decision
here, can you push that down to the actual function reading the statistics?
> +static void aq_ethtool_get_strings(struct net_device *ndev,
> + u32 stringset, u8 *data)
> +{
You need to check that stringset == ETH_SS_STATS here since that's the
only thing you support. ethtool usually does not do it if the ioctl()
returning the data does not exist, but other bogus applications might.
> + memcpy(data, *aq_ethtool_stat_names, sizeof(aq_ethtool_stat_names));
> +}
> +
> +static int aq_ethtool_get_sset_count(struct net_device *ndev, int stringset)
> +{
Same here.
> + return ARRAY_SIZE(aq_ethtool_stat_names);
> +#ifndef AQ_ETHTOOL_H
> +#define AQ_ETHTOOL_H
> +
> +#include "aq_common.h"
> +
> +extern const struct ethtool_ops aq_ethtool_ops;
The extern does not appear necessary here. Alternatively you may define
all the ethtool function pointers in this header file, but leave the
struct ethtool_ops in the main driver file which registers them at the
time of the netdev creation, up to you.
> +
> +#endif /* AQ_ETHTOOL_H */
>
--
Florian
^ permalink raw reply
* Re: [PATCH v5 08/13] net: ethernet: aquantia: PCI operations
From: Florian Fainelli @ 2017-01-14 1:48 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous,
Dmitry Bezrukov
In-Reply-To: <2889e8e87b8b89945f34e57a7f8ca253f204de47.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> Add functions that handle the PCI bus interface.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@aquantia.com>
> Signed-off-by: David M. VomLehn <vomlehn@texas.net>
> ---
> + /*enable interrupts */
> +#if AQ_CFG_FORCE_LEGACY_INT
> + self->irq_type = AQ_IRQ_LEGACY;
> +#else
> + err = pci_enable_msix(self->pdev, self->msix_entry,
> + self->aq_hw_caps.msix_irqs);
You should really try MSI(x) first and fallback to leagcy PCI INT# if
you cannot succeed, asking for a driver rebuild to switch between these
behaviors may not be an option for most people (a module parameter may
be an option but is usually not such a great idea).
--
Florian
^ permalink raw reply
* Re: [PATCH v5 05/13] net: ethernet: aquantia: Support for NIC-specific code
From: Florian Fainelli @ 2017-01-14 2:00 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous,
Dmitry Bezrukov
In-Reply-To: <5e87af3b0effb96698d0c085366b1ba9320a6305.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> Add support for code specific to the Atlantic NIC.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@aquantia.com>
> Signed-off-by: David M. VomLehn <vomlehn@texas.net>
> ---
> drivers/net/ethernet/aquantia/aq_main.c | 291 ++++++++
> drivers/net/ethernet/aquantia/aq_main.h | 17 +
> drivers/net/ethernet/aquantia/aq_nic.c | 910 ++++++++++++++++++++++++
> drivers/net/ethernet/aquantia/aq_nic.h | 108 +++
> drivers/net/ethernet/aquantia/aq_nic_internal.h | 46 ++
> 5 files changed, 1372 insertions(+)
> create mode 100644 drivers/net/ethernet/aquantia/aq_main.c
> create mode 100644 drivers/net/ethernet/aquantia/aq_main.h
> create mode 100644 drivers/net/ethernet/aquantia/aq_nic.c
> create mode 100644 drivers/net/ethernet/aquantia/aq_nic.h
> create mode 100644 drivers/net/ethernet/aquantia/aq_nic_internal.h
>
> diff --git a/drivers/net/ethernet/aquantia/aq_main.c b/drivers/net/ethernet/aquantia/aq_main.c
> new file mode 100644
> index 0000000..18a6012
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/aq_main.c
> @@ -0,0 +1,291 @@
> +/*
> + * aQuantia Corporation Network Driver
> + * Copyright (C) 2014-2017 aQuantia Corporation. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + */
> +
> +/* File aq_main.c: Main file for aQuantia Linux driver. */
> +
> +#include "aq_main.h"
> +#include "aq_nic.h"
> +#include "aq_pci_func.h"
> +#include "aq_ethtool.h"
> +#include "hw_atl/hw_atl_a0.h"
> +#include "hw_atl/hw_atl_b0.h"
> +
> +#include <linux/netdevice.h>
> +#include <linux/module.h>
> +
> +static const struct pci_device_id aq_pci_tbl[] = {
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_0001), },
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_D100), },
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_D107), },
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_D108), },
> + { PCI_VDEVICE(AQUANTIA, HW_ATL_DEVICE_ID_D109), },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(pci, aq_pci_tbl);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION(AQ_CFG_DRV_VERSION);
> +MODULE_AUTHOR(AQ_CFG_DRV_AUTHOR);
> +MODULE_DESCRIPTION(AQ_CFG_DRV_DESC);
Inline the defines directly into that file, and these typically go at
the end of the file.
> +
> +static struct aq_hw_ops *aq_pci_probe_get_hw_ops_by_id(struct pci_dev *pdev)
> +{
> + struct aq_hw_ops *ops = NULL;
> + int err = 0;
> +
> + ops = hw_atl_a0_get_ops_by_id(pdev);
> + if (ops) {
> + err = 0;
> + goto err_exit;
> + }
Should not that come from the pci_device_id .driver_data structure?
> +
> + ops = hw_atl_b0_get_ops_by_id(pdev);
> + if (ops) {
> + err = 0;
> + goto err_exit;
> + }
> +
> +/* the H/W was not recognized */
> + err = -EFAULT;
> +
> +err_exit:
> + return ops;
> +}
> +
> +static int aq_ndev_open(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = NULL;
> + int err = 0;
> +
> + aq_nic = aq_nic_alloc_hot(ndev);
> + if (!aq_nic) {
> + err = -ENOMEM;
> + goto err_exit;
> + }
> + err = aq_nic_init(aq_nic);
> + if (err < 0)
> + goto err_exit;
> + err = aq_nic_start(aq_nic);
> + if (err < 0)
> + goto err_exit;
> +
> +err_exit:
> + if (err < 0) {
Just make the error label be taken only if there is an error instead of
checking here again for a negative error code.
> + if (aq_nic)
> + aq_nic_deinit(aq_nic);
> + }
> + return err;
> +}
> +
> +static int aq_ndev_close(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
No casting needed.
> +
> + aq_nic_stop(aq_nic);
> + aq_nic_deinit(aq_nic);
> + aq_nic_free_hot_resources(aq_nic);
> +
> + return 0;
> +}
> +
> +static int aq_ndev_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + int err = 0;
> +
> + err = aq_nic_xmit(aq_nic, skb);
> + if (err < 0)
> + goto err_exit;
> +
> +err_exit:
> + return err;
Just inline the contents of aq_nic_xmit(), this function serves nothing
useful here.
> +}
> +
> +static int aq_ndev_change_mtu(struct net_device *ndev, int new_mtu)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + int err = 0;
> +
> + if (new_mtu == ndev->mtu) {
> + err = 0;
> + goto err_exit;
> + }
> + if (new_mtu < 68) {
> + err = -EINVAL;
> + goto err_exit;
> + }
What's so special about 68 here?
> + err = aq_nic_set_mtu(aq_nic, new_mtu + ETH_HLEN);
Why do not need to include ETH_HLEN here?
> + if (err < 0)
> + goto err_exit;
> + ndev->mtu = new_mtu;
> +
> + if (netif_running(ndev)) {
> + aq_ndev_close(ndev);
> + aq_ndev_open(ndev);
> + }
> +
> +err_exit:
> + return err;
> +}
> +
> +static int aq_ndev_set_features(struct net_device *ndev,
> + netdev_features_t features)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + struct aq_nic_cfg_s *aq_cfg = aq_nic_get_cfg(aq_nic);
> + bool is_lro = false;
> +
> + if (aq_cfg->hw_features & NETIF_F_LRO) {
> + is_lro = features & NETIF_F_LRO;
> +
> + if (aq_cfg->is_lro != is_lro) {
> + aq_cfg->is_lro = is_lro;
> +
> + if (netif_running(ndev)) {
> + aq_ndev_close(ndev);
> + aq_ndev_open(ndev);
> + }
Can this be made less disruptive to the network interface?
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int aq_ndev_set_mac_address(struct net_device *ndev, void *addr)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + int err = 0;
> +
> + err = eth_mac_addr(ndev, addr);
> + if (err < 0)
> + goto err_exit;
> + err = aq_nic_set_mac(aq_nic, ndev);
> + if (err < 0)
> + goto err_exit;
> +
> +err_exit:
> + return err;
> +}
> +
> +static void aq_ndev_set_multicast_settings(struct net_device *ndev)
> +{
> + struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
> + int err = 0;
> +
> + err = aq_nic_set_packet_filter(aq_nic, ndev->flags);
> + if (err < 0)
> + goto err_exit;
> +
> + if (netdev_mc_count(ndev)) {
> + err = aq_nic_set_multicast_list(aq_nic, ndev);
> + if (err < 0)
> + goto err_exit;
> + }
That's not enough, you need to deal with Unicast address filtering,
promiscuous mode, and if you run out of space in your multicast address
filter.
--
Florian
^ permalink raw reply
* Re: [PATCH v5 01/13] net: ethernet: aquantia: Make and configuration files.
From: Florian Fainelli @ 2017-01-14 2:01 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous,
Dmitry Bezrukov
In-Reply-To: <fe1664e502d28bcb6a5689787b8000e6c9288307.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> Patches to create the make and configuration files.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@aquantia.com>
> Signed-off-by: David M. VomLehn <vomlehn@texas.net>
> ---
> drivers/net/ethernet/aquantia/Kconfig | 24 +++++++++++++++++++
> drivers/net/ethernet/aquantia/Makefile | 42 ++++++++++++++++++++++++++++++++++
> drivers/net/ethernet/aquantia/ver.h | 18 +++++++++++++++
> 3 files changed, 84 insertions(+)
> create mode 100644 drivers/net/ethernet/aquantia/Kconfig
> create mode 100644 drivers/net/ethernet/aquantia/Makefile
> create mode 100644 drivers/net/ethernet/aquantia/ver.h
>
> diff --git a/drivers/net/ethernet/aquantia/Kconfig b/drivers/net/ethernet/aquantia/Kconfig
> new file mode 100644
> index 0000000..a74a4c0
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/Kconfig
> @@ -0,0 +1,24 @@
> +#
> +# aQuantia device configuration
> +#
> +
> +config NET_VENDOR_AQUANTIA
> + bool "aQuantia devices"
> + default y
> + ---help---
> + Set this to y if you have an Ethernet network cards that uses the aQuantia
> + chipset.
Could be interesting specify which specific commercial products available.
> +
> + This option does not build any drivers; it casues the aQuantia
> + drivers that can be built to appear in the list of Ethernet drivers.
> +
> +
> +if NET_VENDOR_AQUANTIA
> +
> +config AQTION
> + tristate "aQuantia AQtion Support"
> + depends on PCI
> + ---help---
> + This enables the support for the aQuantia AQtion Ethernet card.
> +
> +endif # NET_VENDOR_AQUANTIA
> diff --git a/drivers/net/ethernet/aquantia/Makefile b/drivers/net/ethernet/aquantia/Makefile
> new file mode 100644
> index 0000000..e4ae696
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/Makefile
> @@ -0,0 +1,42 @@
> +################################################################################
> +#
> +# aQuantia Ethernet Controller AQtion Linux Driver
> +# Copyright(c) 2014-2017 aQuantia Corporation.
> +#
> +# This program is free software; you can redistribute it and/or modify it
> +# under the terms and conditions of the GNU General Public License,
> +# version 2, as published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope it will be useful, but WITHOUT
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> +# more details.
> +#
> +# You should have received a copy of the GNU General Public License along
> +# with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +# The full GNU General Public License is included in this distribution in
> +# the file called "COPYING".
> +#
> +# Contact Information: <rdc-drv@aquantia.com>
> +# aQuantia Corporation, 105 E. Tasman Dr. San Jose, CA 95134, USA
> +#
> +################################################################################
> +
> +#
> +# Makefile for the AQtion(tm) Ethernet driver
> +#
> +
> +obj-$(CONFIG_AQTION) += atlantic.o
> +
> +atlantic-objs := aq_main.o \
> + aq_nic.o \
> + aq_pci_func.o \
> + aq_vec.o \
> + aq_ring.o \
> + aq_hw_utils.o \
> + aq_ethtool.o \
> + hw_atl/hw_atl_a0.o \
> + hw_atl/hw_atl_b0.o \
> + hw_atl/hw_atl_utils.o \
> + hw_atl/hw_atl_llh.o
> diff --git a/drivers/net/ethernet/aquantia/ver.h b/drivers/net/ethernet/aquantia/ver.h
> new file mode 100644
> index 0000000..636d646
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/ver.h
> @@ -0,0 +1,18 @@
> +/*
> + * aQuantia Corporation Network Driver
> + * Copyright (C) 2014-2017 aQuantia Corporation. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + */
> +
> +#ifndef VER_H
> +#define VER_H
> +
> +#define NIC_MAJOR_DRIVER_VERSION 1
> +#define NIC_MINOR_DRIVER_VERSION 5
> +#define NIC_BUILD_DRIVER_VERSION 339
> +#define NIC_REVISION_DRIVER_VERSION 0
That probably means nothing since it's the first time for the upstream
kernel that such a driver exists. Defining versions of software is such
an interesting subject anyway!
--
Florian
^ permalink raw reply
* Re: [PATCH v5 00/13] net: ethernet: aquantia: Add AQtion 2.5/5 GB NIC driver
From: Florian Fainelli @ 2017-01-14 2:05 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous
In-Reply-To: <cover.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> v1: Initial version
> v2: o Make necessary drivers/net/ethernet changes to integrate software
> o Drop intermediate atlantic directory
> o Remove Makefile things only appropriate to out of tree module
> building
> v3: o Move changes to drivers/net/ethernet/{Kconfig,Makefile} to the last
> patch to ensure clean bisection.
> o Removed inline attribute aq_hw_write_req() as it was defined in
> only one .c file.
> o #included pci.h in aq_common.h to get struct pci definition.
> o Modified code to unlock based execution flow rather than using a
> flag.
> o Made a number of functions that were only used in a single file
> static.
> o Cleaned up error and return code handling in various places.
> o Remove AQ_CFG_IP_ALIGN definition.
> o Other minor code clean up.
> v4: o Using do_div for 64 bit division.
> o Modified NIC statistics code.
> o Using build_skb instead netdev_alloc_skb for single fragment
> packets.
> o Removed extra aq_nic.o from Makefile
> v5: o Removed extra newline at the end of the files.
> o Wrapped cover letter lines.
Few build warnings with W=1 (W=2 is just too verbose) that you may want
to fix, this was with GCC 4.8, newer compilers may flag new warnings as
well:
drivers/net/ethernet/aquantia/aq_main.c: In function
'aq_pci_probe_get_hw_ops_by_id':
drivers/net/ethernet/aquantia/aq_main.c:41:6: warning: variable 'err'
set but not used [-Wunused-but-set-variable]
int err = 0;
^
drivers/net/ethernet/aquantia/aq_main.c: In function 'aq_pci_remove':
drivers/net/ethernet/aquantia/aq_main.c:237:6: warning: variable 'err'
set but not used [-Wunused-but-set-variable]
int err = 0;
^
--
Florian
^ permalink raw reply
* [PATCH] net: add regs attribute to phy device for user diagnose
From: yuan linyu @ 2017-01-14 2:46 UTC (permalink / raw)
To: Florian Fainelli, David S . Miller; +Cc: netdev, yuan linyu
From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
if phy device have register(s) configuration problem,
user can use this attribute to diagnose.
this feature need phy driver maintainer implement.
Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
---
drivers/net/phy/phy_device.c | 26 ++++++++++++++++++++++++++
include/linux/phy.h | 4 ++++
2 files changed, 30 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 92b0838..a400748 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -617,10 +617,36 @@ phy_has_fixups_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR_RO(phy_has_fixups);
+static ssize_t
+phy_regs_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct phy_device *phydev = to_phy_device(dev);
+
+ if (!phydev->drv || !phydev->drv->read_regs)
+ return 0;
+
+ return phydev->drv->read_regs(phydev, buf, PAGE_SIZE);
+}
+
+static ssize_t
+phy_regs_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct phy_device *phydev = to_phy_device(dev);
+
+ if (!phydev->drv || !phydev->drv->write_regs)
+ return 0;
+
+ return phydev->drv->write_regs(phydev, buf, count);
+}
+static DEVICE_ATTR_RW(phy_regs);
+
static struct attribute *phy_dev_attrs[] = {
&dev_attr_phy_id.attr,
&dev_attr_phy_interface.attr,
&dev_attr_phy_has_fixups.attr,
+ &dev_attr_phy_regs.attr,
NULL,
};
ATTRIBUTE_GROUPS(phy_dev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f7d95f6..c9c4ab3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -622,6 +622,10 @@ struct phy_driver {
int (*set_tunable)(struct phy_device *dev,
struct ethtool_tunable *tuna,
const void *data);
+
+ /* Diagnose PHY register configuration issue from user space */
+ ssize_t (*read_regs)(struct phy_device *dev, char *buf, size_t size);
+ int (*write_regs)(struct phy_device *dev, const char *buf, size_t size);
};
#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
struct phy_driver, mdiodrv)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] can: Fix kernel panic at security_sock_rcv_skb
From: Liu Shuo @ 2017-01-14 3:43 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Eric Dumazet, linux-kernel, yanmin_zhang, shuox.liu, Zhang Yanmin,
He, Bo, Marc Kleine-Budde, David S. Miller,
open list:CAN NETWORK LAYER, open list:NETWORKING [GENERAL]
In-Reply-To: <9e6817a0-65b3-ed16-4616-f8e55aee09c4@hartkopp.net>
On Thu 12.Jan'17 at 17:33:38 +0100, Oliver Hartkopp wrote:
>On 01/12/2017 02:01 PM, Eric Dumazet wrote:
>>On Thu, 2017-01-12 at 09:22 +0100, Oliver Hartkopp wrote:
>
>>>But my main concern is:
>>>
>>>The reason why can_rx_delete_receiver() was introduced was the need to
>>>remove a huge number of receivers with can_rx_unregister().
>>>
>>>When you call synchronize_rcu() after each receiver removal this would
>>>potentially lead to a big performance issue when e.g. closing CAN_RAW
>>>sockets with a high number of receivers.
>>>
>>>So the idea was to remove/unlink the receiver hlist_del_rcu(&r->list)
>>>and also kmem_cache_free(rcv_cache, r) by some rcu mechanism - so that
>>>all elements are cleaned up by rcu at a later point.
>>>
>>>Is it possible that the problems emerge due to hlist_del_rcu(&r->list)
>>>and you accidently fix it with your introduced synchronize_rcu()?
>>
>>I agree this patch does not fix the root cause.
>>
>>The main problem seems that the sockets themselves are not RCU
>>protected.
>>
>>If CAN uses RCU for delivery, then sockets should be freed only after
>>one RCU grace period.
>>
>>On recent kernels, following patch could help :
>>
>
>Thanks Eric!
>
>@Liu ShuoX: Can you check if Eric's suggestion fixes the issue in your
>setup?
Sorry for late reply. I was OOO yesterday.
With Eric's hint, i just found his patch that "net: add SOCK_RCU_FREE
socket flag" in the latest kernel. With backporting this one plus Eric's
following patch, it fixs my failure.
Thanks Eric and Oliver!
Shuo
>
>Best regards,
>Oliver
^ permalink raw reply
* Re: [PATCH v5 00/13] net: ethernet: aquantia: Add AQtion 2.5/5 GB NIC driver
From: David VomLehn @ 2017-01-14 3:41 UTC (permalink / raw)
To: Florian Fainelli, Alexander Loktionov, netdev
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous
In-Reply-To: <9a4e95cc-9daa-f883-46c0-f477e28b9f0f@gmail.com>
On 01/13/2017 05:38 PM, Florian Fainelli wrote:
> On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
>> From: David VomLehn <vomlehn@texas.net>
>>
>> v1: Initial version
>> v2: o Make necessary drivers/net/ethernet changes to integrate software
>> o Drop intermediate atlantic directory
>> o Remove Makefile things only appropriate to out of tree module
>> building
>> v3: o Move changes to drivers/net/ethernet/{Kconfig,Makefile} to the last
>> patch to ensure clean bisection.
>> o Removed inline attribute aq_hw_write_req() as it was defined in
>> only one .c file.
>> o #included pci.h in aq_common.h to get struct pci definition.
>> o Modified code to unlock based execution flow rather than using a
>> flag.
>> o Made a number of functions that were only used in a single file
>> static.
>> o Cleaned up error and return code handling in various places.
>> o Remove AQ_CFG_IP_ALIGN definition.
>> o Other minor code clean up.
>> v4: o Using do_div for 64 bit division.
>> o Modified NIC statistics code.
>> o Using build_skb instead netdev_alloc_skb for single fragment
>> packets.
>> o Removed extra aq_nic.o from Makefile
>> v5: o Removed extra newline at the end of the files.
>> o Wrapped cover letter lines.
> Have not looked at the driver yet, but the threading of your emails is
> weird, each patch is in reply to the previous one. It would be more
> natural to have all numbered patches be in reply to the cover letter,
> which according to the version of git you seem to have used (2.7.4)
> should already be the default. In graphical terms what we see right now is:
>
> [PATCH 00/13]
> [PATCH 01/13]
> [PATCH 02/13]
> ....
>
> While we should see:
>
>
> [PATCH 00/13]
> [PATCH 01/13]
> [PATCH 02/13]
> ....
>
> Can you fix that for future submissions, this may sound like a cosmetic
> thing, but it really helps with threading/reading etc.
>
> Thanks!
This looks like it is a consequence of setting the git configuration
item format.thread to deep. I've switched it to shallow, which should
give the threading you suggest.
--
David VL
^ 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