* [PATCH net-next 0/8] liquidio: adding support for ethtool --set-ring feature
From: Felix Manlunas @ 2017-08-12 1:28 UTC (permalink / raw)
To: davem
Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla,
intiyaz.basha
From: Intiyaz Basha <intiyaz.basha@cavium.com>
Code reorganization is required for adding ethtool --set-ring feature.
First seven patches are for code reorganization. The last patch is for
adding this feature.
Intiyaz Basha (8):
liquidio: moved wait_for_pending_requests to octeon_network.h
liquidio: moved update_txq_status to lio_core.c
liquidio: moved octeon_setup_droq to lio_core.c
liquidio: moved liquidio_push_packet to lio_core.c
liquidio: moved liquidio_napi_drv_callback to lio_core.c
liquidio: moved liquidio_napi_poll to lio_core.c
liquidio: moved liquidio_setup_io_queues to lio_core.c
liquidio: added support for ethtool --set-ring feature
.../ethernet/cavium/liquidio/cn23xx_vf_device.h | 2 -
drivers/net/ethernet/cavium/liquidio/lio_core.c | 424 +++++++++++++++++++
drivers/net/ethernet/cavium/liquidio/lio_ethtool.c | 131 ++++++
drivers/net/ethernet/cavium/liquidio/lio_main.c | 448 +--------------------
drivers/net/ethernet/cavium/liquidio/lio_vf_main.c | 379 +----------------
.../net/ethernet/cavium/liquidio/octeon_config.h | 13 +-
.../net/ethernet/cavium/liquidio/octeon_device.c | 14 +-
.../net/ethernet/cavium/liquidio/octeon_device.h | 2 +
.../net/ethernet/cavium/liquidio/octeon_network.h | 29 ++
9 files changed, 617 insertions(+), 825 deletions(-)
--
2.9.0
^ permalink raw reply
* Re: [PATCH V4 net 0/2] ipv6: fix flowlabel issue for reset packet
From: Tom Herbert @ 2017-08-12 1:00 UTC (permalink / raw)
To: Shaohua Li; +Cc: Linux Kernel Network Developers, David S. Miller, Shaohua Li
In-Reply-To: <20170810191314.vo6yl7dgndga4heh@kernel.org>
On Thu, Aug 10, 2017 at 12:13 PM, Shaohua Li <shli@kernel.org> wrote:
> On Thu, Aug 10, 2017 at 11:30:51AM -0700, Tom Herbert wrote:
>> On Thu, Aug 10, 2017 at 9:30 AM, Shaohua Li <shli@kernel.org> wrote:
>> > On Wed, Aug 09, 2017 at 09:40:08AM -0700, Tom Herbert wrote:
>> >> On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li <shli@kernel.org> wrote:
>> >> > From: Shaohua Li <shli@fb.com>
>> >> >
>> >> > Please see below tcpdump output:
>> >> > 21:00:48.109122 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 40) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [S], cksum 0x0529 (incorrect -> 0xf56c), seq 3282214508, win 43690, options [mss 65476,sackOK,TS val 2500903437 ecr 0,nop,wscale 7], length 0
>> >> > 21:00:48.109381 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 40) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [S.], cksum 0x0529 (incorrect -> 0x49ad), seq 1923801573, ack 3282214509, win 43690, options [mss 65476,sackOK,TS val 2500903437 ecr 2500903437,nop,wscale 7], length 0
>> >> > 21:00:48.109548 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1bdf), seq 1, ack 1, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 0
>> >> > 21:00:48.109823 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 62) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [P.], cksum 0x053f (incorrect -> 0xb8b1), seq 1:31, ack 1, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 30
>> >> > 21:00:48.109910 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [.], cksum 0x0521 (incorrect -> 0x1bc1), seq 1, ack 31, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 0
>> >> > 21:00:48.110043 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 56) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [P.], cksum 0x0539 (incorrect -> 0xb726), seq 1:25, ack 31, win 342, options [nop,nop,TS val 2500903438 ecr 2500903437], length 24
>> >> > 21:00:48.110173 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1ba7), seq 31, ack 25, win 342, options [nop,nop,TS val 2500903438 ecr 2500903438], length 0
>> >> > 21:00:48.110211 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [F.], cksum 0x0521 (incorrect -> 0x1ba7), seq 25, ack 31, win 342, options [nop,nop,TS val 2500903438 ecr 2500903437], length 0
>> >> > 21:00:48.151099 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1ba6), seq 31, ack 26, win 342, options [nop,nop,TS val 2500903438 ecr 2500903438], length 0
>> >> > 21:00:49.110524 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 56) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [P.], cksum 0x0539 (incorrect -> 0xb324), seq 31:55, ack 26, win 342, options [nop,nop,TS val 2500904438 ecr 2500903438], length 24
>> >> > 21:00:49.110637 IP6 (flowlabel 0xb34d5, hlim 64, next-header TCP (6) payload length: 20) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [R], cksum 0x0515 (incorrect -> 0x668c), seq 1923801599, win 0, length 0
>> >> >
>> >> > The flowlabel of reset packet (0xb34d5) and flowlabel of normal packet
>> >> > (0xd827f) are different. This causes our router doesn't correctly close tcp
>> >> > connection. The patches try to fix the issue.
>> >> >
>> >> Shaohua,
>> >>
>> >> Can you give some more detail about what the router doesn't close the
>> >> TCP connection means? I'm guessing the problem is either: 1) the
>> >> router is maintaining connection state that includes the flow label in
>> >> a connection tuple. 2) some router in the path is maintaining
>> >> connection state, but when the flow label changes the flow's packet
>> >> are routed through a different router that doesn't have a state for
>> >> the flow it drops the packet. #1 should be easily fix in the router,
>> >> flow labels cannot be used as state. #2 is the known problem that
>> >> stateful firewalls have killed our ability to use multihoming.
>> >
>> > The #2 is exactly the problem we saw.
>> >
>> >> Another consideration is that sk_txhash is also used in routing
>> >> decisions by the local host (flow label is normally derived from
>> >> txhash). If you want to ensure that connections are routed
>> >> consistently for timewait state you might need sk_txhash saved also.
>> >
>> > As far as I understood, we don't use sk_txhash for routing selection. The code
>> > does routing selection with flowlabel user configured, at that time we don't
>> > derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
>> > does routing selection first and then uses ip6_make_flowlabel to build packet
>> > data where we derive flowlabel from skb->hash.
>> >
>> That is assuming one particular use case. Generally, if you want to
>> ensure all packets for a flow take the same path you'll need tx_hash
>> and make it persistent (disable flow bender). For instance, if you
>> were doing UDP encapsulation like in VXLAN the UDP source port
>> selection is unaffected by saved flow label for the lifetime of the
>> flow. So we would still hit #2 in that case and the stateful device
>> doesn't see whole flow. It might be just as easy to move tx_hash in
>> skc_common so that it's available in TW state for this purpose. Then
>> when moving to TW state just copy the tx_hash.
>
> Hi Tom,
>
> My original implementation is to add a tx_hash in tw sock, we then copy sock's
> tx_hash to the tw tx_hash. This does makes things simplier. One concern from
> Eric is this will increase the size of tw sock. If we move tx_hash to
> skc_common, all sock size will increase, is this acceptable?
I think that can only be measured by how critical it is to
persistently route all packets the same exact way for every
connection. Page one of the IP book clearly states that IP packets can
be dropped, duplicated, or received out of order. Received OOO implies
that packet for the same flow are allowed to take different paths. The
requirement that packets for the same flow must always take the same
path through the network was created by stateful middleboxes-- it's
not inherent in the architecture of IP networking. Unfortunately,
we're seeing this become more and more of a problem as more devices
are multi-homed (like smart phones) and these network requirement
cripple our ability to take advantage of features like that.
Personally, I wish the middleboxes fix the problem they created, but I
suppose we need to be pragmatic at least in the short term.
Tom
> Thanks,
> Shaohua
^ permalink raw reply
* Re: unregister_netdevice: waiting for eth0 to become free. Usage count = 1
From: Wei Wang @ 2017-08-12 0:46 UTC (permalink / raw)
To: John Stultz
Cc: Cong Wang, Martin KaFai Lau, lkml, Network Development,
Linux USB List, David S. Miller, Felipe Balbi
In-Reply-To: <CALAqxLV-e=Uz_zG6VYcKSTG0+qJdpPOv0_vagnGx4bABgzO4tg@mail.gmail.com>
> So yes, sorry I haven't been able to get back quicker on the other
> patches sent, was mucking about in other work.
>
> So yea, this patch (potential fix for unregister_netdevice()) seems
> to avoid the issue.
>
> I'm going to do some further testing, but its looking good so far.
>
Great. Thanks.
> Do you still want feedback on the previous changes?
If this patch is good, then I don't really need the previous feedback.
Thanks.
Wei
On Fri, Aug 11, 2017 at 5:31 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Fri, Aug 11, 2017 at 5:10 PM, Wei Wang <weiwan@google.com> wrote:
>>> If after Cong's fix, the issue still happens, could you help try the
>>> patch attached and collect all logs when you try the reproduce the
>>> issue? It would be great to have logs for both success case and the
>>> failure case.
>>>
>>> Thanks so much for your help.
>>>
>>
>> I think we have a potential fix for this issue.
>> Martin and I found that when addrconf_dst_alloc() creates a rt6, it is
>> possible that rt6->dst.dev points to loopback device while
>> rt6->rt6i_idev->dev points to a real device.
>> When the real device goes down, the current fib6 clean up code only
>> checks for rt6->dst.dev and assumes rt6->rt6i_idev->dev is the same.
>> That leaves unreleased refcnt on the real device if rt6->dst.dev
>> points to loopback dev.
>>
>> The attached potential fix is tested by Martin and made sure it fixes his issue.
>>
>> John,
>> It will be great if you can also give it a try and see if it fixes the
>> issue on your side before I submit an official patch.
>
> So yes, sorry I haven't been able to get back quicker on the other
> patches sent, was mucking about in other work.
>
> So yea, this patch (potential fix for unregister_netdevice()) seems
> to avoid the issue.
>
> I'm going to do some further testing, but its looking good so far.
>
> Do you still want feedback on the previous changes?
>
> thanks
> -john
^ permalink raw reply
* Re: unregister_netdevice: waiting for eth0 to become free. Usage count = 1
From: John Stultz @ 2017-08-12 0:31 UTC (permalink / raw)
To: Wei Wang
Cc: Cong Wang, Martin KaFai Lau, lkml, Network Development,
Linux USB List, David S. Miller, Felipe Balbi
In-Reply-To: <CAEA6p_COwXWP97wSHvzokmm8ckQ2QEd7=dfNH04ttDPm_z3bcg@mail.gmail.com>
On Fri, Aug 11, 2017 at 5:10 PM, Wei Wang <weiwan@google.com> wrote:
>> If after Cong's fix, the issue still happens, could you help try the
>> patch attached and collect all logs when you try the reproduce the
>> issue? It would be great to have logs for both success case and the
>> failure case.
>>
>> Thanks so much for your help.
>>
>
> I think we have a potential fix for this issue.
> Martin and I found that when addrconf_dst_alloc() creates a rt6, it is
> possible that rt6->dst.dev points to loopback device while
> rt6->rt6i_idev->dev points to a real device.
> When the real device goes down, the current fib6 clean up code only
> checks for rt6->dst.dev and assumes rt6->rt6i_idev->dev is the same.
> That leaves unreleased refcnt on the real device if rt6->dst.dev
> points to loopback dev.
>
> The attached potential fix is tested by Martin and made sure it fixes his issue.
>
> John,
> It will be great if you can also give it a try and see if it fixes the
> issue on your side before I submit an official patch.
So yes, sorry I haven't been able to get back quicker on the other
patches sent, was mucking about in other work.
So yea, this patch (potential fix for unregister_netdevice()) seems
to avoid the issue.
I'm going to do some further testing, but its looking good so far.
Do you still want feedback on the previous changes?
thanks
-john
^ permalink raw reply
* Re: unregister_netdevice: waiting for eth0 to become free. Usage count = 1
From: Wei Wang @ 2017-08-12 0:25 UTC (permalink / raw)
To: David Ahern
Cc: Cong Wang, John Stultz, Martin KaFai Lau, lkml,
Network Development, Linux USB List, David S. Miller,
Felipe Balbi
In-Reply-To: <a2a0f380-3e49-8772-dde9-bcebd94cd7fd@gmail.com>
On Fri, Aug 11, 2017 at 5:19 PM, David Ahern <dsahern@gmail.com> wrote:
> On 8/11/17 6:10 PM, Wei Wang wrote:
>> I think we have a potential fix for this issue.
>> Martin and I found that when addrconf_dst_alloc() creates a rt6, it is
>> possible that rt6->dst.dev points to loopback device while
>> rt6->rt6i_idev->dev points to a real device.
>> When the real device goes down, the current fib6 clean up code only
>> checks for rt6->dst.dev and assumes rt6->rt6i_idev->dev is the same.
>> That leaves unreleased refcnt on the real device if rt6->dst.dev
>> points to loopback dev.
>
> Yes, host routes and anycast routes.
>
> I have a patch to fix that but it is held up on a few VRF test cases
> failing. Hopefully I can get that figured out next week. These unrelated
> routes against the loopback device have been a source of a number of
> problems (e.g. take down 'lo' and all of IPv6 networking stops for that
> namespace).
Thanks David.
By "a patch to fix that" do you mean after your patch, for every rt6,
rt6->rt6i_idev will be the same as rt6->dst.dev?
^ permalink raw reply
* Re: unregister_netdevice: waiting for eth0 to become free. Usage count = 1
From: David Ahern @ 2017-08-12 0:19 UTC (permalink / raw)
To: Wei Wang, Cong Wang, John Stultz, Martin KaFai Lau
Cc: lkml, Network Development, Linux USB List, David S. Miller,
Felipe Balbi
In-Reply-To: <CAEA6p_COwXWP97wSHvzokmm8ckQ2QEd7=dfNH04ttDPm_z3bcg@mail.gmail.com>
On 8/11/17 6:10 PM, Wei Wang wrote:
> I think we have a potential fix for this issue.
> Martin and I found that when addrconf_dst_alloc() creates a rt6, it is
> possible that rt6->dst.dev points to loopback device while
> rt6->rt6i_idev->dev points to a real device.
> When the real device goes down, the current fib6 clean up code only
> checks for rt6->dst.dev and assumes rt6->rt6i_idev->dev is the same.
> That leaves unreleased refcnt on the real device if rt6->dst.dev
> points to loopback dev.
Yes, host routes and anycast routes.
I have a patch to fix that but it is held up on a few VRF test cases
failing. Hopefully I can get that figured out next week. These unrelated
routes against the loopback device have been a source of a number of
problems (e.g. take down 'lo' and all of IPv6 networking stops for that
namespace).
^ permalink raw reply
* [PATCH net-next 2/2] net: ipv4: add check for l3slave for index returned in IP_PKTINFO
From: David Ahern @ 2017-08-12 0:11 UTC (permalink / raw)
To: netdev; +Cc: David Ahern
In-Reply-To: <1502496675-13349-1-git-send-email-dsahern@gmail.com>
Similar to the loopback device, for packets sent through a VRF device
the index returned in ipi_ifindex needs to be the saved index in
rt_iif.
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/ipv4/ip_sockglue.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index dd68a9ed5e40..e558e4f9597b 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -1207,6 +1207,7 @@ static int do_ip_setsockopt(struct sock *sk, int level,
void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb)
{
struct in_pktinfo *pktinfo = PKTINFO_SKB_CB(skb);
+ bool l3slave = ipv4_l3mdev_skb(IPCB(skb)->flags);
bool prepare = (inet_sk(sk)->cmsg_flags & IP_CMSG_PKTINFO) ||
ipv6_sk_rxinfo(sk);
@@ -1220,7 +1221,7 @@ void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb)
* (e.g., process binds socket to eth0 for Tx which is
* redirected to loopback in the rtable/dst).
*/
- if (pktinfo->ipi_ifindex == LOOPBACK_IFINDEX)
+ if (pktinfo->ipi_ifindex == LOOPBACK_IFINDEX || l3slave)
pktinfo->ipi_ifindex = inet_iif(skb);
pktinfo->ipi_spec_dst.s_addr = fib_compute_spec_dst(skb);
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 1/2] net: vrf: Drop local rtable and rt6_info
From: David Ahern @ 2017-08-12 0:11 UTC (permalink / raw)
To: netdev; +Cc: David Ahern
In-Reply-To: <1502496675-13349-1-git-send-email-dsahern@gmail.com>
The VRF cached rtable and rt6_info for local traffic are no longer
needed and actually prevent local traffic through enslaved devices.
Remove them.
Signed-off-by: David Ahern <dsahern@gmail.com>
---
drivers/net/vrf.c | 122 ++++--------------------------------------------------
1 file changed, 8 insertions(+), 114 deletions(-)
diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index abd2010c48ae..7e19051f3230 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -47,9 +47,7 @@ static unsigned int vrf_net_id;
struct net_vrf {
struct rtable __rcu *rth;
- struct rtable __rcu *rth_local;
struct rt6_info __rcu *rt6;
- struct rt6_info __rcu *rt6_local;
u32 tb_id;
};
@@ -194,42 +192,10 @@ static netdev_tx_t vrf_process_v6_outbound(struct sk_buff *skb,
/* if dst.dev is loopback or the VRF device again this is locally
* originated traffic destined to a local address. Short circuit
- * to Rx path using our local dst
+ * to Rx path
*/
- if (dst->dev == net->loopback_dev || dst->dev == dev) {
- struct net_vrf *vrf = netdev_priv(dev);
- struct rt6_info *rt6_local;
-
- /* release looked up dst and use cached local dst */
- dst_release(dst);
-
- rcu_read_lock();
-
- rt6_local = rcu_dereference(vrf->rt6_local);
- if (unlikely(!rt6_local)) {
- rcu_read_unlock();
- goto err;
- }
-
- /* Ordering issue: cached local dst is created on newlink
- * before the IPv6 initialization. Using the local dst
- * requires rt6i_idev to be set so make sure it is.
- */
- if (unlikely(!rt6_local->rt6i_idev)) {
- rt6_local->rt6i_idev = in6_dev_get(dev);
- if (!rt6_local->rt6i_idev) {
- rcu_read_unlock();
- goto err;
- }
- }
-
- dst = &rt6_local->dst;
- dst_hold(dst);
-
- rcu_read_unlock();
-
- return vrf_local_xmit(skb, dev, &rt6_local->dst);
- }
+ if (dst->dev == dev)
+ return vrf_local_xmit(skb, dev, dst);
skb_dst_set(skb, dst);
@@ -296,30 +262,10 @@ static netdev_tx_t vrf_process_v4_outbound(struct sk_buff *skb,
/* if dst.dev is loopback or the VRF device again this is locally
* originated traffic destined to a local address. Short circuit
- * to Rx path using our local dst
+ * to Rx path
*/
- if (rt->dst.dev == net->loopback_dev || rt->dst.dev == vrf_dev) {
- struct net_vrf *vrf = netdev_priv(vrf_dev);
- struct rtable *rth_local;
- struct dst_entry *dst = NULL;
-
- ip_rt_put(rt);
-
- rcu_read_lock();
-
- rth_local = rcu_dereference(vrf->rth_local);
- if (likely(rth_local)) {
- dst = &rth_local->dst;
- dst_hold(dst);
- }
-
- rcu_read_unlock();
-
- if (unlikely(!dst))
- goto err;
-
- return vrf_local_xmit(skb, vrf_dev, dst);
- }
+ if (rt->dst.dev == vrf_dev)
+ return vrf_local_xmit(skb, vrf_dev, &rt->dst);
skb_dst_set(skb, &rt->dst);
@@ -528,12 +474,10 @@ static struct sk_buff *vrf_ip6_out(struct net_device *vrf_dev,
static void vrf_rt6_release(struct net_device *dev, struct net_vrf *vrf)
{
struct rt6_info *rt6 = rtnl_dereference(vrf->rt6);
- struct rt6_info *rt6_local = rtnl_dereference(vrf->rt6_local);
struct net *net = dev_net(dev);
struct dst_entry *dst;
RCU_INIT_POINTER(vrf->rt6, NULL);
- RCU_INIT_POINTER(vrf->rt6_local, NULL);
synchronize_rcu();
/* move dev in dst's to loopback so this VRF device can be deleted
@@ -546,19 +490,6 @@ static void vrf_rt6_release(struct net_device *dev, struct net_vrf *vrf)
dev_hold(dst->dev);
dst_release(dst);
}
-
- if (rt6_local) {
- if (rt6_local->rt6i_idev) {
- in6_dev_put(rt6_local->rt6i_idev);
- rt6_local->rt6i_idev = NULL;
- }
-
- dst = &rt6_local->dst;
- dev_put(dst->dev);
- dst->dev = net->loopback_dev;
- dev_hold(dst->dev);
- dst_release(dst);
- }
}
static int vrf_rt6_create(struct net_device *dev)
@@ -567,7 +498,7 @@ static int vrf_rt6_create(struct net_device *dev)
struct net_vrf *vrf = netdev_priv(dev);
struct net *net = dev_net(dev);
struct fib6_table *rt6i_table;
- struct rt6_info *rt6, *rt6_local;
+ struct rt6_info *rt6;
int rc = -ENOMEM;
/* IPv6 can be CONFIG enabled and then disabled runtime */
@@ -586,22 +517,7 @@ static int vrf_rt6_create(struct net_device *dev)
rt6->rt6i_table = rt6i_table;
rt6->dst.output = vrf_output6;
- /* create a dst for local routing - packets sent locally
- * to local address via the VRF device as a loopback
- */
- rt6_local = ip6_dst_alloc(net, dev, flags);
- if (!rt6_local) {
- dst_release(&rt6->dst);
- goto out;
- }
-
- rt6_local->rt6i_idev = in6_dev_get(dev);
- rt6_local->rt6i_flags = RTF_UP | RTF_NONEXTHOP | RTF_LOCAL;
- rt6_local->rt6i_table = rt6i_table;
- rt6_local->dst.input = ip6_input;
-
rcu_assign_pointer(vrf->rt6, rt6);
- rcu_assign_pointer(vrf->rt6_local, rt6_local);
rc = 0;
out:
@@ -788,12 +704,10 @@ static struct sk_buff *vrf_l3_out(struct net_device *vrf_dev,
static void vrf_rtable_release(struct net_device *dev, struct net_vrf *vrf)
{
struct rtable *rth = rtnl_dereference(vrf->rth);
- struct rtable *rth_local = rtnl_dereference(vrf->rth_local);
struct net *net = dev_net(dev);
struct dst_entry *dst;
RCU_INIT_POINTER(vrf->rth, NULL);
- RCU_INIT_POINTER(vrf->rth_local, NULL);
synchronize_rcu();
/* move dev in dst's to loopback so this VRF device can be deleted
@@ -806,20 +720,12 @@ static void vrf_rtable_release(struct net_device *dev, struct net_vrf *vrf)
dev_hold(dst->dev);
dst_release(dst);
}
-
- if (rth_local) {
- dst = &rth_local->dst;
- dev_put(dst->dev);
- dst->dev = net->loopback_dev;
- dev_hold(dst->dev);
- dst_release(dst);
- }
}
static int vrf_rtable_create(struct net_device *dev)
{
struct net_vrf *vrf = netdev_priv(dev);
- struct rtable *rth, *rth_local;
+ struct rtable *rth;
if (!fib_new_table(dev_net(dev), vrf->tb_id))
return -ENOMEM;
@@ -829,22 +735,10 @@ static int vrf_rtable_create(struct net_device *dev)
if (!rth)
return -ENOMEM;
- /* create a dst for local ingress routing - packets sent locally
- * to local address via the VRF device as a loopback
- */
- rth_local = rt_dst_alloc(dev, RTCF_LOCAL, RTN_LOCAL, 1, 1, 0);
- if (!rth_local) {
- dst_release(&rth->dst);
- return -ENOMEM;
- }
-
rth->dst.output = vrf_output;
rth->rt_table_id = vrf->tb_id;
- rth_local->rt_table_id = vrf->tb_id;
-
rcu_assign_pointer(vrf->rth, rth);
- rcu_assign_pointer(vrf->rth_local, rth_local);
return 0;
}
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 0/2] net: vrf: Support for local traffic with sockets bound to enslaved devices
From: David Ahern @ 2017-08-12 0:11 UTC (permalink / raw)
To: netdev; +Cc: David Ahern
This set gets local traffic working for sockets bound to enslaved
devices. The local rtable and rt6_info added in June 2016 to get
local traffic in VRFs working is no longer needed and actually
keeps local traffic for sockets bound to an enslaved device from
working. Patch 1 removes them.
Patch 2 adds a fix up for IPv4 IP_PKTINFO to return rt_iif for
packets sent over the VRF device. This is similar to the handling
of loopback.
David Ahern (2):
net: vrf: Drop local rtable and rt6_info
net: ipv4: add check for l3slave for index returned in IP_PKTINFO
drivers/net/vrf.c | 122 ++++---------------------------------------------
net/ipv4/ip_sockglue.c | 3 +-
2 files changed, 10 insertions(+), 115 deletions(-)
--
2.1.4
^ permalink raw reply
* Re: unregister_netdevice: waiting for eth0 to become free. Usage count = 1
From: Wei Wang @ 2017-08-12 0:10 UTC (permalink / raw)
To: Cong Wang, John Stultz, Martin KaFai Lau
Cc: lkml, Network Development, Linux USB List, David S. Miller,
Felipe Balbi
In-Reply-To: <CAEA6p_CooRGNOQN3fosF0JLqmd9aOTuNHKNPE8cbrzDH6c176w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]
> If after Cong's fix, the issue still happens, could you help try the
> patch attached and collect all logs when you try the reproduce the
> issue? It would be great to have logs for both success case and the
> failure case.
>
> Thanks so much for your help.
>
I think we have a potential fix for this issue.
Martin and I found that when addrconf_dst_alloc() creates a rt6, it is
possible that rt6->dst.dev points to loopback device while
rt6->rt6i_idev->dev points to a real device.
When the real device goes down, the current fib6 clean up code only
checks for rt6->dst.dev and assumes rt6->rt6i_idev->dev is the same.
That leaves unreleased refcnt on the real device if rt6->dst.dev
points to loopback dev.
The attached potential fix is tested by Martin and made sure it fixes his issue.
John,
It will be great if you can also give it a try and see if it fixes the
issue on your side before I submit an official patch.
Thanks very much for the help from everyone.
Wei
On Fri, Aug 11, 2017 at 10:25 AM, Wei Wang <weiwan-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Aug 11, 2017 at 9:48 AM, Cong Wang <xiyou.wangcong-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hi,
>>
>> On Thu, Aug 10, 2017 at 11:12 AM, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> On Wed, Aug 9, 2017 at 10:41 PM, Wei Wang <weiwan-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>> Hi John,
>>>>
>>>> Is it possible to try the attached patch?
>>>
>>> Thanks so much for the quick turn around!
>>>
>>> So I dropped all the reverts you suggested, and applied this one
>>> against 4.13-rc4, but I'm still seeing the problematic behavior.
>>
>> Does the following one-line fix make a difference?
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index a640fbcba15d..c145a35763a0 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -141,7 +141,7 @@ static void rt6_uncached_list_del(struct rt6_info *rt)
>> struct uncached_list *ul = rt->rt6i_uncached_list;
>>
>> spin_lock_bh(&ul->lock);
>> - list_del(&rt->rt6i_uncached);
>> + list_del_init(&rt->rt6i_uncached);
>> spin_unlock_bh(&ul->lock);
>> }
>> }
>
>
> Thanks a lot Cong for proposing this fix.
>
> For the last few days, John has been helping me running debug image
> and we found out that the leaked dst is probably in addrconf.c.
> Martin and I are looking through the code and trying to put more debugs.
>
> John,
>
> If after Cong's fix, the issue still happens, could you help try the
> patch attached and collect all logs when you try the reproduce the
> issue? It would be great to have logs for both success case and the
> failure case.
>
> Thanks so much for your help.
>
> Wei
[-- Attachment #2: 0001-potential-fix-for-unregister_netdevice.patch --]
[-- Type: text/x-patch, Size: 1515 bytes --]
From 2d8861808c2029013f6b6e86120ba6902329145b Mon Sep 17 00:00:00 2001
From: Wei Wang <weiwan@google.com>
Date: Fri, 11 Aug 2017 16:36:04 -0700
Subject: [PATCH 1/2] potential fix for unregister_netdevice()
Change-Id: I5d5f6f7a7ad0f5dd769f33487db17ff2570d52ea
---
net/ipv6/route.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96a819d..105922903932 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -417,14 +417,12 @@ static void ip6_dst_ifdown(struct dst_entry *dst, struct net_device *dev,
struct net_device *loopback_dev =
dev_net(dev)->loopback_dev;
- if (dev != loopback_dev) {
- if (idev && idev->dev == dev) {
- struct inet6_dev *loopback_idev =
- in6_dev_get(loopback_dev);
- if (loopback_idev) {
- rt->rt6i_idev = loopback_idev;
- in6_dev_put(idev);
- }
+ if (idev && idev->dev != loopback_dev) {
+ struct inet6_dev *loopback_idev =
+ in6_dev_get(loopback_dev);
+ if (loopback_idev) {
+ rt->rt6i_idev = loopback_idev;
+ in6_dev_put(idev);
}
}
}
@@ -2789,7 +2787,8 @@ static int fib6_ifdown(struct rt6_info *rt, void *arg)
const struct arg_dev_net *adn = arg;
const struct net_device *dev = adn->dev;
- if ((rt->dst.dev == dev || !dev) &&
+ if ((rt->dst.dev == dev || !dev ||
+ rt->rt6i_idev->dev == dev) &&
rt != adn->net->ipv6.ip6_null_entry &&
(rt->rt6i_nsiblings == 0 ||
(dev && netdev_unregistering(dev)) ||
--
2.14.0.434.g98096fd7a8-goog
^ permalink raw reply related
* [PATCH net-next] net: ipv4: remove unnecessary check on orig_oif
From: David Ahern @ 2017-08-12 0:02 UTC (permalink / raw)
To: netdev; +Cc: David Ahern
rt_iif is going to be set to either 0 or orig_oif. If orig_oif
is 0 it amounts to the same end result so remove the check.
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/ipv4/route.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 6324ade81a81..9c9895a6fc0f 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2237,7 +2237,7 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
if (!rth)
return ERR_PTR(-ENOBUFS);
- rth->rt_iif = orig_oif ? : 0;
+ rth->rt_iif = orig_oif;
if (res->table)
rth->rt_table_id = res->table->tb_id;
--
2.1.4
^ permalink raw reply related
* Re: [PATCH v3 net-next 2/2] wan: dscc4: convert to plain DMA API
From: Francois Romieu @ 2017-08-11 23:32 UTC (permalink / raw)
To: David Miller; +Cc: khoroshilov, netdev, linux-kernel, ldv-project
In-Reply-To: <20170811.144935.36255660873272850.davem@davemloft.net>
David Miller <davem@davemloft.net> :
[...]
> Oops, this will need to be sent as a relative fixup as I've applied these
> two patches to net-next, sorry Francois.
No problem. It works perfectly this way.
--
Ueimor
^ permalink raw reply
* Re: Performance comparision / qdisc / no qdisc / queueing linux kernel 4.12
From: Stephen Hemminger @ 2017-08-11 23:16 UTC (permalink / raw)
To: Paweł Staszewski; +Cc: netdev
In-Reply-To: <fa7bc3ce-5dc7-e236-ec35-0afe3fc830bc@itcare.pl>
Any of the current qdisc that do rate control are single threaded.
Since bandwidth is a shared resource, the qdisc has to acquire a lock
and update cache shared data structures.
Don't expect HTB or HFSC to be free.
>
> 4.13-rc3 have kernel panic when deleting qdisc - will try to get panic trace
Please do.
^ permalink raw reply
* Re: [PATCH net-next V2 3/3] tap: XDP support
From: Jakub Kicinski @ 2017-08-11 23:12 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, mst, netdev, linux-kernel, Daniel Borkmann
In-Reply-To: <1502451678-17358-4-git-send-email-jasowang@redhat.com>
On Fri, 11 Aug 2017 19:41:18 +0800, Jason Wang wrote:
> This patch tries to implement XDP for tun. The implementation was
> split into two parts:
>
> - fast path: small and no gso packet. We try to do XDP at page level
> before build_skb(). For XDP_TX, since creating/destroying queues
> were completely under control of userspace, it was implemented
> through generic XDP helper after skb has been built. This could be
> optimized in the future.
> - slow path: big or gso packet. We try to do it after skb was created
> through generic XDP helpers.
>
> Test were done through pktgen with small packets.
>
> xdp1 test shows ~41.1% improvement:
>
> Before: ~1.7Mpps
> After: ~2.3Mpps
>
> xdp_redirect to ixgbe shows ~60% improvement:
>
> Before: ~0.8Mpps
> After: ~1.38Mpps
>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Looks OK to me now :)
Out of curiosity, you say the build_skb() is for "small packets", and it
seems you are always reserving the 256B regardless of XDP being
installed. Does this have no performance impact on non-XDP case?
^ permalink raw reply
* Re: [PATCH net-next 1/3] Add LAN743X driver
From: Stephen Hemminger @ 2017-08-11 22:59 UTC (permalink / raw)
To: Bryan.Whitehead; +Cc: netdev, davem, UNGLinuxDriver
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D264406F603F@CHN-SV-EXMX02.mchp-main.com>
On Fri, 11 Aug 2017 19:47:57 +0000
<Bryan.Whitehead@microchip.com> wrote:
> +static void lan743x_pcidev_remove(struct pci_dev *pdev)
> +{
> + struct net_device *netdev = NULL;
> + struct lan743x_adapter *adapter = NULL;
> +
> + netdev = pci_get_drvdata(pdev);
> + adapter = netdev_priv(netdev);
Useless double initialization
^ permalink raw reply
* Re: [PATCH net-next 1/3] Add LAN743X driver
From: Stephen Hemminger @ 2017-08-11 23:05 UTC (permalink / raw)
To: Bryan.Whitehead; +Cc: netdev, davem, UNGLinuxDriver
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D264406F603F@CHN-SV-EXMX02.mchp-main.com>
On Fri, 11 Aug 2017 19:47:57 +0000
<Bryan.Whitehead@microchip.com> wrote:
> +
> +static struct net_device_stats *mac_get_stats(struct lan743x_adapter *adapter)
> +{
> + struct lan743x_mac *mac = &adapter->mac;
> +
> + memset(&mac->statistics, 0, sizeof(mac->statistics));
> + mac->statistics.rx_packets = lan743x_csr_read(adapter,
> + STAT_RX_TOTAL_FRAMES);
> + mac->statistics.tx_packets = lan743x_csr_read(adapter,
> + STAT_TX_TOTAL_FRAMES);
> + return &mac->statistics;
> +}
The statistics code here is confused.
You are already counting rx_packets in software in napi_poll
Then you get values from MAC. One or the other?
There are two copies of stats, one in netdev and other in your mac structure.
Also what about byte and error counts?
If possible implement 64 bit get_stats64 instead.
^ permalink raw reply
* Re: [PATCH net-next 1/3] Add LAN743X driver
From: Stephen Hemminger @ 2017-08-11 22:57 UTC (permalink / raw)
To: Bryan.Whitehead; +Cc: netdev, davem, UNGLinuxDriver
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D264406F603F@CHN-SV-EXMX02.mchp-main.com>
On Fri, 11 Aug 2017 19:47:57 +0000
<Bryan.Whitehead@microchip.com> wrote:
> +
> +static int lan743x_rx_napi_poll(struct napi_struct *napi, int weight)
> +{
> + int count;
> + bool finished = false;
> + struct lan743x_rx *rx = container_of(napi,
> + struct lan743x_rx, napi);
> + struct lan743x_adapter *adapter = rx->adapter;
> +
> + if (weight < 0)
> + finished = true;
> +
> + count = 0;
> + while (count < weight) {
> + int rx_process_result = -1;
> +
> + /* clear int status bit before reading packet */
> + lan743x_csr_write(adapter, DMAC_INT_STS,
> + DMAC_INT_BIT_RXFRM_(rx->channel_number));
> + lan743x_csr_read(adapter, DMAC_INT_STS);
> +
> + rx_process_result = lan743x_rx_process_packet(rx);
> + if (rx_process_result == RX_PROCESS_RESULT_PACKET_RECEIVED) {
> + count++;
> + } else if (rx_process_result ==
> + RX_PROCESS_RESULT_NOTHING_TO_DO) {
> + finished = true;
> + break;
> + } else if (rx_process_result ==
> + RX_PROCESS_RESULT_PACKET_DROPPED) {
> + continue;
> + } else {
> + NETIF_ERROR(adapter, drv, adapter->netdev,
> + "Unknown rx_process_result == %d",
> + rx_process_result);
> + }
> + }
> +
> + adapter->netdev->stats.rx_packets += count;
> +
> + if (!finished) {
> + NETIF_ASSERT(adapter, drv, adapter->netdev, count == weight);
> + return count;
> + }
> +
> + napi_complete_done(napi, count);
> +
> + lan743x_csr_write(adapter, INT_EN_SET,
> + INT_BIT_DMA_RX_(rx->channel_number));
> + lan743x_csr_read(adapter, INT_STS);
> +
> + return 0;
NAPI poll routine is supposed to return count of packets processed.
If you write the code in a logical manner, you won't need the finished flag either.
Also with net_poll you should be checking return value from napi_complete_done.
^ permalink raw reply
* Re: [PATCH net-next 1/3] Add LAN743X driver
From: Stephen Hemminger @ 2017-08-11 22:52 UTC (permalink / raw)
To: Bryan.Whitehead; +Cc: netdev, davem, UNGLinuxDriver
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D264406F603F@CHN-SV-EXMX02.mchp-main.com>
On Fri, 11 Aug 2017 19:47:57 +0000
<Bryan.Whitehead@microchip.com> wrote:
> +
> +static int lan743x_ethtool_get_eeprom_len(struct net_device *netdev)
> +{
> + return 0;
> +}
> +
This stub should go. If you have no eeprom, take the ethtool default.
^ permalink raw reply
* Re: [PATCH net-next 1/3] Add LAN743X driver
From: Stephen Hemminger @ 2017-08-11 22:50 UTC (permalink / raw)
To: Bryan.Whitehead; +Cc: netdev, davem, UNGLinuxDriver
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D264406F603F@CHN-SV-EXMX02.mchp-main.com>
On Fri, 11 Aug 2017 19:47:57 +0000
<Bryan.Whitehead@microchip.com> wrote:
> +
> + netdev = alloc_etherdev(sizeof(struct lan743x_adapter));
> + if (!netdev) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "alloc_etherdev returned NULL");
> + ret = -ENOMEM;
> + goto clean_up;
> + }
> +
> + strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
> + SET_NETDEV_DEV(netdev, &pdev->dev);
> + pci_set_drvdata(pdev, netdev);
> + adapter = netdev_priv(netdev);
> + if (!adapter) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "netdev_priv returned NULL");
> + ret = -ENOMEM;
> + goto clean_up;
> + }
> + memset(adapter, 0, sizeof(struct lan743x_adapter));
netdev_priv area is already zerod
^ permalink raw reply
* Re: [PATCH net-next 1/3] Add LAN743X driver
From: Stephen Hemminger @ 2017-08-11 22:49 UTC (permalink / raw)
To: Bryan.Whitehead; +Cc: netdev, davem, UNGLinuxDriver
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D264406F603F@CHN-SV-EXMX02.mchp-main.com>
;
> +
> +static void lan743x_rx_isr(void *context, u32 int_sts);
> +
> +static int lan743x_rx_ring_init(struct lan743x_rx *rx);
> +static void lan743x_rx_ring_cleanup(struct lan743x_rx *rx);
> +static int lan743x_rx_init(struct lan743x_rx *rx,
> + struct lan743x_adapter *adapter, int channel_number);
> +static void lan743x_rx_cleanup(struct lan743x_rx *rx);
> +static int lan743x_rx_open(struct lan743x_rx *rx);
> +static void lan743x_rx_close(struct lan743x_rx *rx);
> +
Please don't create a header file full of static declarations.
Header files are for shared data between compilation units.
Also, Linux style is to order functions to minimize the number
of required forward declarations.
^ permalink raw reply
* [PATCH v2] bonding: ratelimit failed speed/duplex update warning
From: Andreas Born @ 2017-08-11 22:36 UTC (permalink / raw)
To: netdev; +Cc: Joe Perches, David Miller, Andreas Born
bond_miimon_commit() handles the UP transition for each slave of a bond
in the case of MII. It is triggered 10 times per second for the default
MII Polling interval of 100ms. For device drivers that do not implement
__ethtool_get_link_ksettings() the call to bond_update_speed_duplex()
fails persistently while the MII status could remain UP. That is, in
this and other cases where the speed/duplex update keeps failing over a
longer period of time while the MII state is UP, a warning is printed
every MII polling interval.
To address these excessive warnings net_ratelimit() should be used.
Printing a warning once would not be sufficient since the call to
bond_update_speed_duplex() could recover to succeed and fail again
later. In that case there would be no new indication what went wrong.
Fixes: b5bf0f5b16b9c (bonding: correctly update link status during mii-commit phase)
Signed-off-by: Andreas Born <futur.andy@googlemail.com>
---
Changes in v2:
* swapped pr_warn_ratelimited() for net_ratelimit()
drivers/net/bonding/bond_main.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 85bb272d2a34..fc63992ab0e0 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2144,9 +2144,10 @@ static void bond_miimon_commit(struct bonding *bond)
if (bond_update_speed_duplex(slave) &&
bond_needs_speed_duplex(bond)) {
slave->link = BOND_LINK_DOWN;
- netdev_warn(bond->dev,
- "failed to get link speed/duplex for %s\n",
- slave->dev->name);
+ if (net_ratelimit())
+ netdev_warn(bond->dev,
+ "failed to get link speed/duplex for %s\n",
+ slave->dev->name);
continue;
}
bond_set_slave_link_state(slave, BOND_LINK_UP,
--
2.14.0
^ permalink raw reply related
* Re: [PATCH net-next 1/3] Add LAN743X driver
From: Andrew Lunn @ 2017-08-11 22:34 UTC (permalink / raw)
To: David Miller; +Cc: Bryan.Whitehead, netdev, UNGLinuxDriver
In-Reply-To: <20170811.151205.2131155817966352627.davem@davemloft.net>
> This thing is huge, I'm not reviewing any more of this enormous submission.
Yes.
Could you split it up a bit. Take the PTP support out for the moment,
and submit it later once the core driver is accepted. The same for any
other optional bits.
Andrew
^ permalink raw reply
* [PATCH net-next v3] vxlan: change vxlan_[config_]validate() to use netlink_ext_ack for error reporting
From: Girish Moodalbail @ 2017-08-11 22:20 UTC (permalink / raw)
To: pshelar, davem, netdev, mschiffer, jbenc, roopa
The kernel log is not where users expect error messages for netlink
requests; as we have extended acks now, we can replace pr_debug() with
NL_SET_ERR_MSG_ATTR().
Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
Signed-off-by: Girish Moodalbail <girish.moodalbail@oracle.com>
---
v2 -> v1:
- improved messages based on the comments from Jiri Benc,
Roopa Prabhu, and David Ahern
v1 -> v2:
- addressed, error messages rewording, comments from Jiri Benc
- started off with what Matthias had, and I covered error reporting
for all of the unsuccessful returns in vxlan_validate() and
vxlan_config_validate()
---
drivers/net/vxlan.c | 99 +++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 73 insertions(+), 26 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 35e84a9e..ae3a1da 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2729,12 +2729,14 @@ static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[],
{
if (tb[IFLA_ADDRESS]) {
if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN) {
- pr_debug("invalid link address (not ethernet)\n");
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_ADDRESS],
+ "Provided link layer address is not Ethernet");
return -EINVAL;
}
if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS]))) {
- pr_debug("invalid all zero ethernet address\n");
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_ADDRESS],
+ "Provided Ethernet address is not unicast");
return -EADDRNOTAVAIL;
}
}
@@ -2742,18 +2744,27 @@ static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[],
if (tb[IFLA_MTU]) {
u32 mtu = nla_get_u32(tb[IFLA_MTU]);
- if (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU)
+ if (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_MTU],
+ "MTU must be between 68 and 65535");
return -EINVAL;
+ }
}
- if (!data)
+ if (!data) {
+ NL_SET_ERR_MSG(extack,
+ "Required attributes not provided to perform the operation");
return -EINVAL;
+ }
if (data[IFLA_VXLAN_ID]) {
u32 id = nla_get_u32(data[IFLA_VXLAN_ID]);
- if (id >= VXLAN_N_VID)
+ if (id >= VXLAN_N_VID) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_ID],
+ "VXLAN ID must be lower than 16777216");
return -ERANGE;
+ }
}
if (data[IFLA_VXLAN_PORT_RANGE]) {
@@ -2761,8 +2772,8 @@ static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[],
= nla_data(data[IFLA_VXLAN_PORT_RANGE]);
if (ntohs(p->high) < ntohs(p->low)) {
- pr_debug("port range %u .. %u not valid\n",
- ntohs(p->low), ntohs(p->high));
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_PORT_RANGE],
+ "Invalid source port range");
return -EINVAL;
}
}
@@ -2919,7 +2930,8 @@ static int vxlan_sock_add(struct vxlan_dev *vxlan)
static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf,
struct net_device **lower,
- struct vxlan_dev *old)
+ struct vxlan_dev *old,
+ struct netlink_ext_ack *extack)
{
struct vxlan_net *vn = net_generic(src_net, vxlan_net_id);
struct vxlan_dev *tmp;
@@ -2933,6 +2945,8 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf,
*/
if ((conf->flags & ~VXLAN_F_ALLOWED_GPE) ||
!(conf->flags & VXLAN_F_COLLECT_METADATA)) {
+ NL_SET_ERR_MSG(extack,
+ "VXLAN GPE does not support this combination of attributes");
return -EINVAL;
}
}
@@ -2947,15 +2961,23 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf,
conf->saddr.sa.sa_family = conf->remote_ip.sa.sa_family;
}
- if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family)
+ if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family) {
+ NL_SET_ERR_MSG(extack,
+ "Local and remote address must be from the same family");
return -EINVAL;
+ }
- if (vxlan_addr_multicast(&conf->saddr))
+ if (vxlan_addr_multicast(&conf->saddr)) {
+ NL_SET_ERR_MSG(extack, "Local address cannot be multicast");
return -EINVAL;
+ }
if (conf->saddr.sa.sa_family == AF_INET6) {
- if (!IS_ENABLED(CONFIG_IPV6))
+ if (!IS_ENABLED(CONFIG_IPV6)) {
+ NL_SET_ERR_MSG(extack,
+ "IPv6 support not enabled in the kernel");
return -EPFNOSUPPORT;
+ }
use_ipv6 = true;
conf->flags |= VXLAN_F_IPV6;
@@ -2967,46 +2989,68 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf,
if (local_type & IPV6_ADDR_LINKLOCAL) {
if (!(remote_type & IPV6_ADDR_LINKLOCAL) &&
- (remote_type != IPV6_ADDR_ANY))
+ (remote_type != IPV6_ADDR_ANY)) {
+ NL_SET_ERR_MSG(extack,
+ "Invalid combination of local and remote address scopes");
return -EINVAL;
+ }
conf->flags |= VXLAN_F_IPV6_LINKLOCAL;
} else {
if (remote_type ==
- (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL))
+ (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL)) {
+ NL_SET_ERR_MSG(extack,
+ "Invalid combination of local and remote address scopes");
return -EINVAL;
+ }
conf->flags &= ~VXLAN_F_IPV6_LINKLOCAL;
}
}
}
- if (conf->label && !use_ipv6)
+ if (conf->label && !use_ipv6) {
+ NL_SET_ERR_MSG(extack,
+ "Label attribute only applies to IPv6 VXLAN devices");
return -EINVAL;
+ }
if (conf->remote_ifindex) {
struct net_device *lowerdev;
lowerdev = __dev_get_by_index(src_net, conf->remote_ifindex);
- if (!lowerdev)
+ if (!lowerdev) {
+ NL_SET_ERR_MSG(extack,
+ "Invalid local interface, device not found");
return -ENODEV;
+ }
#if IS_ENABLED(CONFIG_IPV6)
if (use_ipv6) {
struct inet6_dev *idev = __in6_dev_get(lowerdev);
- if (idev && idev->cnf.disable_ipv6)
+ if (idev && idev->cnf.disable_ipv6) {
+ NL_SET_ERR_MSG(extack,
+ "IPv6 support disabled by administrator");
return -EPERM;
+ }
}
#endif
*lower = lowerdev;
} else {
- if (vxlan_addr_multicast(&conf->remote_ip))
+ if (vxlan_addr_multicast(&conf->remote_ip)) {
+ NL_SET_ERR_MSG(extack,
+ "Local interface required for multicast remote destination");
+
return -EINVAL;
+ }
#if IS_ENABLED(CONFIG_IPV6)
- if (conf->flags & VXLAN_F_IPV6_LINKLOCAL)
+ if (conf->flags & VXLAN_F_IPV6_LINKLOCAL) {
+ NL_SET_ERR_MSG(extack,
+ "Local interface required for link-local local/remote addresses");
return -EINVAL;
+ }
#endif
*lower = NULL;
@@ -3038,6 +3082,8 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf,
tmp->cfg.remote_ifindex != conf->remote_ifindex)
continue;
+ NL_SET_ERR_MSG(extack,
+ "A VXLAN device with the specified VNI already exists");
return -EEXIST;
}
@@ -3097,14 +3143,14 @@ static void vxlan_config_apply(struct net_device *dev,
}
static int vxlan_dev_configure(struct net *src_net, struct net_device *dev,
- struct vxlan_config *conf,
- bool changelink)
+ struct vxlan_config *conf, bool changelink,
+ struct netlink_ext_ack *extack)
{
struct vxlan_dev *vxlan = netdev_priv(dev);
struct net_device *lowerdev;
int ret;
- ret = vxlan_config_validate(src_net, conf, &lowerdev, vxlan);
+ ret = vxlan_config_validate(src_net, conf, &lowerdev, vxlan, extack);
if (ret)
return ret;
@@ -3114,13 +3160,14 @@ static int vxlan_dev_configure(struct net *src_net, struct net_device *dev,
}
static int __vxlan_dev_create(struct net *net, struct net_device *dev,
- struct vxlan_config *conf)
+ struct vxlan_config *conf,
+ struct netlink_ext_ack *extack)
{
struct vxlan_net *vn = net_generic(net, vxlan_net_id);
struct vxlan_dev *vxlan = netdev_priv(dev);
int err;
- err = vxlan_dev_configure(net, dev, conf, false);
+ err = vxlan_dev_configure(net, dev, conf, false, extack);
if (err)
return err;
@@ -3366,7 +3413,7 @@ static int vxlan_newlink(struct net *src_net, struct net_device *dev,
if (err)
return err;
- return __vxlan_dev_create(src_net, dev, &conf);
+ return __vxlan_dev_create(src_net, dev, &conf, extack);
}
static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
@@ -3386,7 +3433,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
memcpy(&old_dst, dst, sizeof(struct vxlan_rdst));
- err = vxlan_dev_configure(vxlan->net, dev, &conf, true);
+ err = vxlan_dev_configure(vxlan->net, dev, &conf, true, extack);
if (err)
return err;
@@ -3592,7 +3639,7 @@ struct net_device *vxlan_dev_create(struct net *net, const char *name,
if (IS_ERR(dev))
return dev;
- err = __vxlan_dev_create(net, dev, conf);
+ err = __vxlan_dev_create(net, dev, conf, NULL);
if (err < 0) {
free_netdev(dev);
return ERR_PTR(err);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next 1/3] Add LAN743X driver
From: Andrew Lunn @ 2017-08-11 22:31 UTC (permalink / raw)
To: Bryan.Whitehead; +Cc: netdev, davem, UNGLinuxDriver
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D264406F603F@CHN-SV-EXMX02.mchp-main.com>
On Fri, Aug 11, 2017 at 07:47:57PM +0000, Bryan.Whitehead@microchip.com wrote:
> From: Bryan Whitehead <Bryan.Whitehead@microchip.com>
>
> Add Microchip LAN743X Driver files
> LAN743X is a PCIe Gigabit ethernet adapter
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
> ---
> drivers/net/ethernet/microchip/lan743x.c | 6842 ++++++++++++++++++++++++++++++
> drivers/net/ethernet/microchip/lan743x.h | 1417 +++++++
> 2 files changed, 8259 insertions(+)
> create mode 100644 drivers/net/ethernet/microchip/lan743x.c
> create mode 100644 drivers/net/ethernet/microchip/lan743x.h
>
> diff --git a/drivers/net/ethernet/microchip/lan743x.c b/drivers/net/ethernet/microchip/lan743x.c
> new file mode 100644
> index 0000000..44d04ac
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan743x.c
> @@ -0,0 +1,6842 @@
> +/*
> + * Copyright (C) 2017 Microchip Technology
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/crc32.h>
> +#include <linux/microchipphy.h>
> +#include <linux/net_tstamp.h>
> +#include <linux/phy.h>
> +#include "lan743x.h"
> +
> +#define DRIVER_AUTHOR "Bryan Whitehead <Bryan.Whitehead@microchip.com>"
> +#define DRIVER_DESC "LAN743x PCIe Gigabit Ethernet Driver"
> +#define DRIVER_NAME "lan743x"
> +#define DRIVER_VERSION "0.2.0.0"
Hi Bryan
Best if you don't use a VERSION string. It is pretty meaningless.
> +
> +/* use ethtool to change the message enable for any given adapter */
> +static int msg_enable = NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK |
> + NETIF_MSG_IFUP | NETIF_MSG_IFDOWN | NETIF_MSG_TX_QUEUED;
> +module_param(msg_enable, int, 0000);
> +MODULE_PARM_DESC(msg_enable, "Override default message enable");
No module parameters please.
> +static int lan743x_pci_init(struct lan743x_adapter *adapter,
> + struct pci_dev *pdev)
> +{
> + int ret = -ENODEV;
> + int bars = 0;
> + struct lan743x_pci *pci = &adapter->pci;
> +
> + NETIF_ASSERT(adapter, probe, adapter->netdev, pdev);
> + memset(pci, 0, sizeof(struct lan743x_pci));
You seem to like memset. But is it actually needed? Adapter is
allocated via alloc_etherdev() so is guaranteed to already be zeroed.
And then you memset all of adaptor. And then you memset this memory
again....
> + pci->pdev = pdev;
> +
> + ret = pci_enable_device_mem(pdev);
> + if (ret) {
> + NETIF_WARNING(adapter, probe, adapter->netdev,
> + "failed pci_enable_device_mem, ret = %d", ret);
> + goto clean_up;
> + }
> + pci->init_flags |= INIT_FLAG_PCI_DEVICE_ENABLED;
> +
> + if (pdev->vendor != PCI_VENDOR_ID_SMSC) {
How would that happen? Do you think the PCI core will call your probe
for any random PCI device? Or just those you list in
lan743x_pcidev_tbl.
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "Unsupported Vendor ID, 0x%04X,", pdev->vendor);
> + ret = -ENODEV;
> + goto clean_up;
> + }
> +
> + if (pdev->device != PCI_DEVICE_ID_SMSC_LAN7430) {
???
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "Unsupported Device ID, 0x%04X", pdev->device);
> + ret = -ENODEV;
> + goto clean_up;
> + }
> +
> + NETIF_INFO(adapter, probe, adapter->netdev,
> + "PCI: Vendor ID = 0x%04X, Device ID = 0x%04X",
> + pdev->vendor, pdev->device);
I doubt this wrapper around netif_info() is going to be accepted. I
also think you need to kill about 90% of your info and err messages.
> +/* CSR */
> +static int lan743x_csr_init(struct lan743x_adapter *adapter)
> +{
> + struct lan743x_csr *csr = &adapter->csr;
> + int result = -ENOMEM;
I don't think this initialization is required.
> + int supported = 0;
> +
> + NETIF_ASSERT(adapter, probe, adapter->netdev, csr);
> + memset(csr, 0, sizeof(struct lan743x_csr));
> +
> + csr->csr_address = lan743x_pci_get_bar_address(adapter, 0);
> + if (!csr->csr_address) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "failed to get csr_address");
> + result = -ENOMEM;
> + goto clean_up;
> + }
> +
> + csr->id_rev = lan743x_csr_read(adapter, ID_REV);
> + csr->fpga_rev = lan743x_csr_read(adapter, FPGA_REV);
> +
> + NETIF_INFO(adapter, probe, adapter->netdev,
> + "ID_REV = 0x%08X, FPGA_REV = %d.%d",
> + csr->id_rev, (csr->fpga_rev) & 0x000000FF,
> + ((csr->fpga_rev) >> 8) & 0x000000FF);
> +
> + if ((csr->id_rev & 0xFFFF0000) == 0x74300000)
> + supported = 1;
> +
> + if (!supported) {
Using the supported variable seems a bit pointless.
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "unsupported adapter, ID_REV = 0x%08X",
> + csr->id_rev);
> + result = -ENODEV;
> + goto clean_up;
> + }
> +
> + result = lan743x_csr_light_reset(adapter);
> + if (result) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "light reset failed");
> + goto clean_up;
> + }
> +
> + result = 0;
> +
> +clean_up:
> + if (result)
> + lan743x_csr_cleanup(adapter);
> + return result;
> +}
> +
> +static inline u32 lan743x_csr_read(struct lan743x_adapter *adapter, int offset)
Please don't use inline. Let the compiler decide.
> +{
> + return ioread32(&adapter->csr.csr_address[offset]);
> +}
> +
> +static inline void lan743x_csr_write(
> + struct lan743x_adapter *adapter, int offset, u32 data)
> +{
> + iowrite32(data, &adapter->csr.csr_address[offset]);
> +}
> +
> +static int lan743x_mdiobus_read(struct mii_bus *bus, int phy_id, int index);
> +static int lan743x_mdiobus_write(struct mii_bus *bus, int phy_id, int index,
> + u16 regval);
Please try to rearrange to code to avoid these.
> +static int lan743x_mac_init(struct lan743x_adapter *adapter)
> +{
> + struct lan743x_mac *mac = &adapter->mac;
> + struct net_device *netdev;
> + u32 data;
> + int ret = -ENODEV;
> + u32 mac_addr_hi = 0;
> + u32 mac_addr_lo = 0;
> + bool mac_address_valid = true;
> +
> + NETIF_ASSERT(adapter, probe, adapter->netdev, mac);
> +
> + memset(mac, 0, sizeof(*mac));
> +
> + netdev = adapter->netdev;
> +
> + ret = lan743x_mac_reset(adapter);
> + if (ret) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "mac reset failed");
> + goto clean_up;
> + }
> +
> + /* setup auto duplex, and speed detection */
> + data = lan743x_csr_read(adapter, MAC_CR);
> + data |= MAC_CR_ADD_ | MAC_CR_ASD_;
> + data |= MAC_CR_CNTR_RST_;
> + lan743x_csr_write(adapter, MAC_CR, data);
> +
> + mutex_init(&mac->tx_mutex);
> + mac->tx_enable_bits = 0;
> + mutex_init(&mac->rx_mutex);
> + mac->rx_enable_bits = 0;
> +
> + mac->mdiobus = mdiobus_alloc();
> + if (!(mac->mdiobus)) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "mdiobus_alloc failed");
> + ret = -ENOMEM;
> + goto clean_up;
> + }
> + mac->flags |= MAC_FLAG_MDIOBUS_ALLOCATED;
> +
> + mutex_init(&mac->mii_mutex);
> + mac->mdiobus->priv = (void *)adapter;
> + mac->mdiobus->read = lan743x_mdiobus_read;
> + mac->mdiobus->write = lan743x_mdiobus_write;
> + mac->mdiobus->name = "lan743x-mdiobus";
> +
> + snprintf(mac->mdiobus->id, MII_BUS_ID_SIZE,
> + "pci-%s", pci_name(adapter->pci.pdev));
> +
> + /* set to internal PHY id */
> + mac->mdiobus->phy_mask = ~(1 << 1);
BIT(1)?
> +
> + /* register mdiobus */
> + ret = mdiobus_register(mac->mdiobus);
> + if (ret < 0) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "failed to register MDIO bus");
> + goto clean_up;
> + }
> + NETIF_INFO(adapter, probe, adapter->netdev,
> + "successfully registered MDIO bus, %s", mac->mdiobus->id);
> + mac->flags |= MAC_FLAG_MDIOBUS_REGISTERED;
> +
> + mac_addr_hi = lan743x_csr_read(adapter, MAC_RX_ADDRH);
> + mac_addr_lo = lan743x_csr_read(adapter, MAC_RX_ADDRL);
> + mac->mac_address[0] = mac_addr_lo & 0xFF;
> + mac->mac_address[1] = (mac_addr_lo >> 8) & 0xFF;
> + mac->mac_address[2] = (mac_addr_lo >> 16) & 0xFF;
> + mac->mac_address[3] = (mac_addr_lo >> 24) & 0xFF;
> + mac->mac_address[4] = mac_addr_hi & 0xFF;
> + mac->mac_address[5] = (mac_addr_hi >> 8) & 0xFF;
> +
> + if (((mac_addr_hi & 0x0000FFFF) == 0x0000FFFF) &&
> + (mac_addr_lo == 0xFFFFFFFF)) {
> + NETIF_INFO(adapter, probe, adapter->netdev,
> + "MAC address not available from EEPROM or OTP");
> + mac_address_valid = false;
> + } else if (!is_valid_ether_addr(mac->mac_address)) {
> + NETIF_WARNING(adapter, probe, adapter->netdev,
> + "MAC address is not valid");
> + mac_address_valid = false;
> + }
> +
> + if (!mac_address_valid) {
> + random_ether_addr(mac->mac_address);
> + NETIF_INFO(adapter, probe, adapter->netdev,
> + "MAC address set to random address");
> + mac_addr_lo = mac->mac_address[0] |
> + (mac->mac_address[1] << 8) |
> + (mac->mac_address[2] << 16) |
> + (mac->mac_address[3] << 24);
> + mac_addr_hi = mac->mac_address[4] |
> + (mac->mac_address[5] << 8);
> + }
> +
> + lan743x_csr_write(adapter, MAC_RX_ADDRL, mac_addr_lo);
> + lan743x_csr_write(adapter, MAC_RX_ADDRH, mac_addr_hi);
Why not use lan743x_mac_set_address()?
> + NETIF_INFO(adapter, probe, adapter->netdev,
> + "MAC Address = %02X:%02X:%02X:%02X:%02X:%02X",
> + mac->mac_address[0], mac->mac_address[1],
> + mac->mac_address[2], mac->mac_address[3],
> + mac->mac_address[4], mac->mac_address[5]);
> +
> + ether_addr_copy(netdev->dev_addr, mac->mac_address);
> +
> + ret = 0;
> +
> +clean_up:
> + if (ret)
> + lan743x_mac_cleanup(adapter);
> + return ret;
> +}
> +
> +#define MAC_MII_READ 1
> +#define MAC_MII_WRITE 0
> +static inline u32 lan743x_mac_mii_access(int id, int index, int read)
Again no inline functions. The compiler will probably inline it
anyway.
> +{
> + u32 ret;
> +
> + ret = ((u32)id << MAC_MII_ACC_PHY_ADDR_SHIFT_) &
> + MAC_MII_ACC_PHY_ADDR_MASK_;
> + ret |= ((u32)index << MAC_MII_ACC_MIIRINDA_SHIFT_) &
> + MAC_MII_ACC_MIIRINDA_MASK_;
It appears you only support C22. So maybe change the function prototype to
u32 lan743x_mac_mii_access(u16 id, u16 index, int read)
and you can avoid the casts in the code.
> + if (read)
> + ret |= MAC_MII_ACC_MII_READ_;
> + else
> + ret |= MAC_MII_ACC_MII_WRITE_;
> + ret |= MAC_MII_ACC_MII_BUSY_;
> +
> + return ret;
> +}
> +
> +static int lan743x_mac_mii_wait_till_not_busy(struct lan743x_adapter *adapter)
> +{
> + unsigned long start_time = jiffies;
> + u32 data;
> +
> + do {
> + data = lan743x_csr_read(adapter, MAC_MII_ACC);
> +
> + if (!(data & MAC_MII_ACC_MII_BUSY_))
> + return 0;
> + } while (!time_after(jiffies, start_time + HZ));
> +
> + NETIF_ERROR(adapter, drv, adapter->netdev, "mii is busy");
> + return -EIO;
> +}
> +
> +static int lan743x_mac_mii_read(struct lan743x_adapter *adapter,
> + int phy_id, int index)
> +{
> + struct lan743x_mac *mac = &adapter->mac;
> + u32 val, addr;
> + int ret;
> +
> + mutex_lock(&mac->mii_mutex);
This is not needed. The mdio core will insure you don't get called
twice. See bus->mdio_lock.
> +
> + /* comfirm MII not busy */
> + ret = lan743x_mac_mii_wait_till_not_busy(adapter);
> + if (ret < 0)
> + goto done;
> +
> + /* set the address, index & direction (read from PHY) */
> + addr = lan743x_mac_mii_access(phy_id, index, MAC_MII_READ);
It is not really an address. So maybe call it reg?
> + lan743x_csr_write(adapter, MAC_MII_ACC, addr);
> +
> + ret = lan743x_mac_mii_wait_till_not_busy(adapter);
> + if (ret < 0)
> + goto done;
> +
> + val = lan743x_csr_read(adapter, MAC_MII_DATA);
> +
> + ret = (int)(val & 0xFFFF);
> +
> +done:
> + mutex_unlock(&mac->mii_mutex);
> +#if (LAN743X_PHY_TRACE_ENABLE != 0)
> + NETIF_INFO(adapter, drv, adapter->netdev,
> + "MII READ: phy_id = %d, index = %d, value = 0x%04X",
> + phy_id, index, ret);
You can get the same information from the mdio trace point.
> +#endif
> + return ret;
> +}
> +
> +static int lan743x_mdiobus_read(struct mii_bus *bus, int phy_id, int index)
> +{
> + struct lan743x_adapter *adapter = bus->priv;
> +
> + return lan743x_mac_mii_read(adapter, phy_id, index);
> +}
> +static int lan743x_mac_set_mtu(struct lan743x_adapter *adapter, int new_mtu)
> +{
> + struct lan743x_mac *mac = &adapter->mac;
> + int ret = 0;
> + u32 mac_rx = 0;
> +
> + if (new_mtu > LAN743X_MAX_FRAME_SIZE)
> + return -EINVAL;
You can set dev->max_mtu and the code will check this for you.
Ah, i see you already do. So why check here?
> + if (new_mtu <= 0)
> + return -EINVAL;
So you can deliberately send runt packets? It seems better to enforce
ETH_MIN_MTU, which the core will do for you by default.
> +
> + mutex_lock(&mac->rx_mutex);
> + if (mac->rx_enable_bits) {
> + ret = lan743x_mac_rx_disable_all(adapter);
> + if (ret) {
> + NETIF_ERROR(adapter, drv, adapter->netdev,
> + "Failed to disable mac");
> + goto done;
> + }
> + }
> +
> + mac_rx = lan743x_csr_read(adapter, MAC_RX);
> + mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_);
> + mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) &
> + MAC_RX_MAX_SIZE_MASK_);
> + lan743x_csr_write(adapter, MAC_RX, mac_rx);
> +
> + if (mac->rx_enable_bits) {
> + ret = lan743x_mac_rx_enable_all(adapter);
> + if (ret) {
> + NETIF_ERROR(adapter, drv, adapter->netdev,
> + "Failed to enable mac");
> + goto done;
> + }
> + }
> +done:
> + mutex_unlock(&mac->rx_mutex);
> + return ret;
> +}
> +static int lan743x_phy_open(struct lan743x_adapter *adapter)
> +{
> + struct lan743x_phy *phy = &adapter->phy;
> + int ret;
> + u32 mii_adv;
> + struct phy_device *phydev;
> + struct net_device *netdev;
> + struct lan743x_mac *mac = &adapter->mac;
> + int phy_id1 = 0;
> + int phy_id2 = 0;
> +
> + netdev = adapter->netdev;
> +
> + NETIF_ASSERT(adapter, ifup, adapter->netdev, mac->mdiobus);
> +
> + phydev = phy_find_first(mac->mdiobus);
> + if (!phydev) {
> + NETIF_ERROR(adapter, ifup, adapter->netdev, "no PHY found");
> + ret = -EIO;
> + goto clean_up;
> + }
> +
> + phydev->irq = PHY_POLL;
It will default to polling.
> +
> + NETIF_INFO(adapter, ifup, adapter->netdev,
> + "phy irq assigned to %d", phydev->irq);
See comment below about phy_attached_info().
> + ret = phy_connect_direct(netdev, phydev,
> + lan743x_phy_link_status_change,
> + PHY_INTERFACE_MODE_GMII);
> + if (ret) {
> + NETIF_ERROR(adapter, ifup, adapter->netdev,
> + "can't attach PHY to %s", mac->mdiobus->id);
> + ret = -EIO;
> + goto clean_up;
> + }
> + phy->flags |= PHY_FLAG_ATTACHED;
> +
> + if (!(phydev->drv)) {
> + NETIF_ERROR(adapter, ifup, adapter->netdev,
> + "Missing PHY Driver");
> + ret = -EIO;
> + goto clean_up;
> + }
Could you please explain how that can happen?
> + phy_id1 = phy_read(phydev, MII_PHYSID1);
> + phy_id2 = phy_read(phydev, MII_PHYSID2);
> + NETIF_INFO(adapter, ifup, adapter->netdev,
> + "PHY_ID1 = 0x%04x", phy_id1);
> + NETIF_INFO(adapter, ifup, adapter->netdev,
> + "PHY_ID2 = 0x%04x", phy_id2);
phy_attached_info()
> +
> + /* MAC doesn't support 1000T Half */
> + phydev->supported &= ~SUPPORTED_1000baseT_Half;
> +
> + /* support both flow controls */
> + phy->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX);
> + phydev->advertising &= ~(ADVERTISED_Pause | ADVERTISED_Asym_Pause);
> + mii_adv = (u32)mii_advertise_flowctrl(phy->fc_request_control);
> + phydev->advertising |= mii_adv_to_ethtool_adv_t(mii_adv);
> +
> + phy->fc_autoneg = phydev->autoneg;
> +
> + /* PHY interrupt enabled here */
> + phy_start(phydev);
> +
> + phy_start_aneg(phydev);
> +
> + phy->flags |= PHY_FLAG_OPENED;
> + ret = 0;
> +
> +clean_up:
> + if (ret)
> + lan743x_phy_close(adapter);
> + return ret;
> +}
> +
> +static int lan743x_netdev_change_mtu(struct net_device *netdev, int new_mtu)
> +{
> + int ret = 0;
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> +
> + NETIF_INFO(adapter, drv, adapter->netdev, "new_mtu = %d", new_mtu);
> + ret = lan743x_mac_set_mtu(adapter, new_mtu);
> + if (!ret)
> + netdev->mtu = new_mtu;
dev_set_mtu() will do this for you.
> + return ret;
> +}
> +
> +
> +static int lan743x_netdev_set_mac_address(struct net_device *netdev,
> + void *addr)
> +{
> + struct lan743x_adapter *adapter = NULL;
> + struct sockaddr *sock_addr = addr;
> +
> + NETIF_ASSERT(adapter, drv, adapter->netdev, netdev);
> +
> + if (netif_running(netdev))
> + return -EBUSY;
> +
> + if (!is_valid_ether_addr(sock_addr->sa_data))
> + return -EADDRNOTAVAIL;
Look at how eth_mac_addr() calls eth_prepare_mac_addr_change().
> +
> + ether_addr_copy(netdev->dev_addr, sock_addr->sa_data);
> +
> + adapter = netdev_priv(netdev);
> + lan743x_mac_set_address(adapter, sock_addr->sa_data);
> + lan743x_rfe_update_mac_address(adapter);
> +
> + return 0;
> +}
> +static int lan743x_ethtool_get_eeprom_len(struct net_device *netdev)
> +{
> + return 0;
> +}
And the point of this is?
> +static int lan743x_ethtool_set_eee(struct net_device *netdev,
> + struct ethtool_eee *eee)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> + struct phy_device *phydev = NULL;
> + u32 buf = 0;
> +
> + if (!netdev)
> + return -EINVAL;
> + adapter = netdev_priv(netdev);
> + if (!adapter)
> + return -EINVAL;
> + phydev = netdev->phydev;
> + if (!phydev)
> + return -EIO;
> + if (!phydev->drv) {
> + NETIF_ERROR(adapter, drv, adapter->netdev,
> + "Missing PHY Driver");
> + return -EIO;
> + }
I don't see any calls to phy_init_eee(), so i'm not sure this works.
> + if (eee->eee_enabled) {
> + buf = lan743x_csr_read(adapter, MAC_CR);
> + buf |= MAC_CR_EEE_EN_;
> + lan743x_csr_write(adapter, MAC_CR, buf);
> +
> + phy_ethtool_set_eee(phydev, eee);
> +
> + buf = (u32)eee->tx_lpi_timer;
> + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
> + NETIF_INFO(adapter, drv, adapter->netdev, "Enabled EEE");
> + } else {
> + buf = lan743x_csr_read(adapter, MAC_CR);
> + buf &= ~MAC_CR_EEE_EN_;
> + lan743x_csr_write(adapter, MAC_CR, buf);
> + NETIF_INFO(adapter, drv, adapter->netdev, "Disabled EEE");
> + }
> +
> + return 0;
> +}
> +
> +static int lan743x_pcidev_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct net_device *netdev = NULL;
> + struct lan743x_adapter *adapter = NULL;
> + int ret = -ENODEV;
> +
> + NETIF_ASSERT(adapter, probe, adapter->netdev, pdev);
> +
> + netdev = alloc_etherdev(sizeof(struct lan743x_adapter));
> + if (!netdev) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "alloc_etherdev returned NULL");
> + ret = -ENOMEM;
> + goto clean_up;
> + }
> +
> + strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
> + SET_NETDEV_DEV(netdev, &pdev->dev);
> + pci_set_drvdata(pdev, netdev);
> + adapter = netdev_priv(netdev);
> + if (!adapter) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "netdev_priv returned NULL");
Cannot happen. One allocation is made for both netdev and the private
area.
> + ret = -ENOMEM;
> + goto clean_up;
> + }
> + memset(adapter, 0, sizeof(struct lan743x_adapter));
and the allocation will zero the memory.
> + adapter->netdev = netdev;
> + adapter->init_flags = 0;
> + adapter->open_flags = 0;
Don't you trust the memset you just did? If memset does not work, you
may as well give up now.
> + strncpy(netdev->name, "eth%d", sizeof(netdev->name));
alloc_etherdev_mqs() will do this for you, as part of
alloc_etherdev().
> + ret = register_netdev(netdev);
> + if (ret < 0) {
> + NETIF_ERROR(adapter, probe, adapter->netdev,
> + "failed to register net device, ret = %d", ret);
> + goto clean_up;
> + }
> + adapter->init_flags |= LAN743X_INIT_FLAG_NETDEV_REGISTERED;
> +
> + NETIF_INFO(adapter, probe, adapter->netdev, "Probe succeeded");
> + ret = 0;
> +
> +clean_up:
> + if (ret && adapter) {
> + NETIF_WARNING(adapter, probe, adapter->netdev,
> + "Incomplete initialization, performing clean up");
> + lan743x_device_cleanup(adapter);
> + }
> + return ret;
> +}
> +
> diff --git a/drivers/net/ethernet/microchip/lan743x.h b/drivers/net/ethernet/microchip/lan743x.h
> new file mode 100644
> index 0000000..8cf4323
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan743x.h
All the following forward declartions need to die, unless you have a
lot of mutual recursion. By having functions in the wrong order, you
are preventing the compiler from being able to optimize your code as
well. It can only inline what it has already seen.
Andrew
^ permalink raw reply
* Re: [PATCH 1/4] MIPS/seccomp: Fix indirect syscall args
From: Kees Cook @ 2017-08-11 22:17 UTC (permalink / raw)
To: James Hogan
Cc: Linux MIPS Mailing List, LKML, Ralf Baechle, David Daney,
Andy Lutomirski, Will Drewry, Oleg Nesterov, Alexei Starovoitov,
Daniel Borkmann, Network Development
In-Reply-To: <20170811205653.21873-2-james.hogan@imgtec.com>
On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@imgtec.com> wrote:
> Since commit 669c4092225f ("MIPS: Give __secure_computing() access to
> syscall arguments."), upon syscall entry when seccomp is enabled,
> syscall_trace_enter() passes a carefully prepared struct seccomp_data
> containing syscall arguments to __secure_computing(). Unfortunately it
> directly uses mips_get_syscall_arg() and fails to take into account the
> indirect O32 system calls (i.e. syscall(2)) which put the system call
> number in a0 and have the arguments shifted up by one entry.
>
> We can't just revert that commit as samples/bpf/tracex5 would break
> again, so use syscall_get_arguments() which already takes indirect
> syscalls into account instead of directly using mips_get_syscall_arg(),
> similar to what populate_seccomp_data() does.
>
> This also removes the redundant error checking of the
> mips_get_syscall_arg() return value (get_user() already zeroes the
> result if an argument from the stack can't be loaded).
>
> Reported-by: James Cowgill <James.Cowgill@imgtec.com>
> Fixes: 669c4092225f ("MIPS: Give __secure_computing() access to syscall arguments.")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Will Drewry <wad@chromium.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> ---
> It would have been much simpler for MIPS arch code to just pass a NULL
> seccomp_data to secure_computing() so populate_seccomp_data() would take
> care of fetching arguments, as it did for MIPS prior to commit
> 669c4092225f ("MIPS: Give __secure_computing() access to syscall
> arguments."), but as that commit mentions it breaks samples/bpf/tracex5,
> which relies on sd being non-NULL at entry to __seccomp_filter().
>
> Arguably the samples/bpf/tracex5 test is flawed, at least for every arch
> except x86 (and now MIPS).
Weird. Yeah, that sample is broken. Allowing NULL sd is totally fine.
The point is that seccomp will use syscall_get_arguments() when it's
NULL (which is effectively what this is doing...)
The reason sd can be _non_-NULL is when an architecture has access to
the args in some way that might be faster than calling
syscall_get_arguments().
Regardless, I'm fine with this change. It should either be this or
reverting 669c4092225f, but it looks like kprobes of
__seccomp_filter() is desired on MIPS...
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> arch/mips/kernel/ptrace.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> index 6dd13641a418..1395654cfc8d 100644
> --- a/arch/mips/kernel/ptrace.c
> +++ b/arch/mips/kernel/ptrace.c
> @@ -872,15 +872,13 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall)
> if (unlikely(test_thread_flag(TIF_SECCOMP))) {
> int ret, i;
> struct seccomp_data sd;
> + unsigned long args[6];
>
> sd.nr = syscall;
> sd.arch = syscall_get_arch();
> - for (i = 0; i < 6; i++) {
> - unsigned long v, r;
> -
> - r = mips_get_syscall_arg(&v, current, regs, i);
> - sd.args[i] = r ? 0 : v;
> - }
> + syscall_get_arguments(current, regs, 0, 6, args);
> + for (i = 0; i < 6; i++)
> + sd.args[i] = args[i];
> sd.instruction_pointer = KSTK_EIP(current);
>
> ret = __secure_computing(&sd);
> --
> 2.13.2
>
--
Kees Cook
Pixel Security
^ 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