* [PATCH 0/2] ipv4: Fix dangling netdev refs
From: David Miller @ 2012-07-31 1:23 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
Eric, this should give you an idea of what I was working on.
It really assumes the universe in place before your change last
night. It is very likely we'll have to revert it and look for
another solution to the problem you were trying to solve. And
actually I don't understand the actual bug very well.
I can only assume that the core issue was that, unlike back when
we had the routing cache, the fib_info nexthops are not persistent
memory like the routing cache hash table was?
Otherwise I can see absolutely no change in reference counting and
dst destruction logic between the routing cache, and how I modified
fib_info nexthop cached routes to behave.
Anyways, the first patch caches routes in the nexthop exception
entries.
And the second patch has a global list for uncached routes so we
can purge netdevice references properly when such devices are
unregistered.
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* [PATCH 1/2] ipv4: Cache routes in nexthop exception entries.
From: David Miller @ 2012-07-31 1:23 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
Signed-off-by: David S. Miller <davem@davemloft.net>
---
include/net/ip_fib.h | 1 +
net/ipv4/fib_semantics.c | 4 +++
net/ipv4/route.c | 82 ++++++++++++++++++++++++++----------------------
3 files changed, 49 insertions(+), 38 deletions(-)
diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index e69c3a4..c4770fc 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -54,6 +54,7 @@ struct fib_nh_exception {
u32 fnhe_pmtu;
__be32 fnhe_gw;
unsigned long fnhe_expires;
+ struct rtable *fnhe_rth;
unsigned long fnhe_stamp;
};
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index e55171f..eaccdb5 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -153,6 +153,10 @@ static void free_nh_exceptions(struct fib_nh *nh)
struct fib_nh_exception *next;
next = rcu_dereference_protected(fnhe->fnhe_next, 1);
+
+ if (fnhe->fnhe_rth)
+ dst_release(&fnhe->fnhe_rth->dst);
+
kfree(fnhe);
fnhe = next;
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d6eabcf..e2abb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -587,7 +587,7 @@ static void ip_rt_build_flow_key(struct flowi4 *fl4, const struct sock *sk,
build_sk_flow_key(fl4, sk);
}
-static DEFINE_SEQLOCK(fnhe_seqlock);
+static DEFINE_SPINLOCK(fnhe_lock);
static struct fib_nh_exception *fnhe_oldest(struct fnhe_hash_bucket *hash)
{
@@ -599,6 +599,10 @@ static struct fib_nh_exception *fnhe_oldest(struct fnhe_hash_bucket *hash)
if (time_before(fnhe->fnhe_stamp, oldest->fnhe_stamp))
oldest = fnhe;
}
+ if (oldest->fnhe_rth) {
+ dst_release(&oldest->fnhe_rth->dst);
+ oldest->fnhe_rth = NULL;
+ }
return oldest;
}
@@ -620,7 +624,7 @@ static void update_or_create_fnhe(struct fib_nh *nh, __be32 daddr, __be32 gw,
int depth;
u32 hval = fnhe_hashfun(daddr);
- write_seqlock_bh(&fnhe_seqlock);
+ spin_lock_bh(&fnhe_lock);
hash = nh->nh_exceptions;
if (!hash) {
@@ -667,7 +671,7 @@ static void update_or_create_fnhe(struct fib_nh *nh, __be32 daddr, __be32 gw,
fnhe->fnhe_stamp = jiffies;
out_unlock:
- write_sequnlock_bh(&fnhe_seqlock);
+ spin_unlock_bh(&fnhe_lock);
return;
}
@@ -1167,36 +1171,37 @@ static struct fib_nh_exception *find_exception(struct fib_nh *nh, __be32 daddr)
static void rt_bind_exception(struct rtable *rt, struct fib_nh_exception *fnhe,
__be32 daddr)
{
- __be32 fnhe_daddr, gw;
- unsigned long expires;
- unsigned int seq;
- u32 pmtu;
-
-restart:
- seq = read_seqbegin(&fnhe_seqlock);
- fnhe_daddr = fnhe->fnhe_daddr;
- gw = fnhe->fnhe_gw;
- pmtu = fnhe->fnhe_pmtu;
- expires = fnhe->fnhe_expires;
- if (read_seqretry(&fnhe_seqlock, seq))
- goto restart;
-
- if (daddr != fnhe_daddr)
- return;
+ spin_lock_bh(&fnhe_lock);
+
+ if (daddr == fnhe->fnhe_daddr) {
+ struct rtable *orig;
- if (pmtu) {
- unsigned long diff = expires - jiffies;
+ if (fnhe->fnhe_pmtu) {
+ unsigned long expires = fnhe->fnhe_expires;
+ unsigned long diff = expires - jiffies;
- if (time_before(jiffies, expires)) {
- rt->rt_pmtu = pmtu;
- dst_set_expires(&rt->dst, diff);
+ if (time_before(jiffies, expires)) {
+ rt->rt_pmtu = fnhe->fnhe_pmtu;
+ dst_set_expires(&rt->dst, diff);
+ }
}
+ if (fnhe->fnhe_gw) {
+ rt->rt_flags |= RTCF_REDIRECTED;
+ rt->rt_gateway = fnhe->fnhe_gw;
+ }
+
+ orig = fnhe->fnhe_rth;
+ if (orig)
+ dst_release(&orig->dst);
+
+ rt->dst.flags |= DST_RCU_FREE;
+ dst_hold(&rt->dst);
+ fnhe->fnhe_rth = rt;
+
+ fnhe->fnhe_stamp = jiffies;
}
- if (gw) {
- rt->rt_flags |= RTCF_REDIRECTED;
- rt->rt_gateway = gw;
- }
- fnhe->fnhe_stamp = jiffies;
+
+ spin_unlock_bh(&fnhe_lock);
}
static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
@@ -1236,13 +1241,13 @@ static void rt_set_nexthop(struct rtable *rt, __be32 daddr,
if (nh->nh_gw && nh->nh_scope == RT_SCOPE_LINK)
rt->rt_gateway = nh->nh_gw;
- if (unlikely(fnhe))
- rt_bind_exception(rt, fnhe, daddr);
dst_init_metrics(&rt->dst, fi->fib_metrics, true);
#ifdef CONFIG_IP_ROUTE_CLASSID
rt->dst.tclassid = nh->nh_tclassid;
#endif
- if (!(rt->dst.flags & DST_NOCACHE))
+ if (unlikely(fnhe))
+ rt_bind_exception(rt, fnhe, daddr);
+ else if (!(rt->dst.flags & DST_NOCACHE))
rt_cache_route(nh, rt);
}
@@ -1741,18 +1746,19 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
fnhe = NULL;
if (fi) {
fnhe = find_exception(&FIB_RES_NH(*res), fl4->daddr);
- if (!fnhe) {
+ if (fnhe)
+ rth = fnhe->fnhe_rth;
+ else
rth = FIB_RES_NH(*res).nh_rth_output;
- if (rt_cache_valid(rth)) {
- dst_hold(&rth->dst);
- return rth;
- }
+ if (rt_cache_valid(rth)) {
+ dst_hold(&rth->dst);
+ return rth;
}
}
rth = rt_dst_alloc(dev_out,
IN_DEV_CONF_GET(in_dev, NOPOLICY),
IN_DEV_CONF_GET(in_dev, NOXFRM),
- fi && !fnhe);
+ fi);
if (!rth)
return ERR_PTR(-ENOBUFS);
--
1.7.11.2
^ permalink raw reply related
* [PATCH 2/2] ipv4: Properly purge netdev references on uncached routes.
From: David Miller @ 2012-07-31 1:23 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
When a device is unregistered, we have to purge all of the
references to it that may exist in the entire system.
If a route is uncached, we currently have no way of accomplishing
this.
So create a global list that is scanned when a network device goes
down. This mirrors the logic in net/core/dst.c's dst_ifdown().
Signed-off-by: David S. Miller <davem@davemloft.net>
---
include/net/route.h | 3 +++
net/ipv4/fib_frontend.c | 1 +
net/ipv4/route.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++---
net/ipv4/xfrm4_policy.c | 1 +
4 files changed, 69 insertions(+), 4 deletions(-)
diff --git a/include/net/route.h b/include/net/route.h
index 8c52bc6..776a27f 100644
--- a/include/net/route.h
+++ b/include/net/route.h
@@ -57,6 +57,8 @@ struct rtable {
/* Miscellaneous cached information */
u32 rt_pmtu;
+
+ struct list_head rt_uncached;
};
static inline bool rt_is_input_route(const struct rtable *rt)
@@ -107,6 +109,7 @@ extern struct ip_rt_acct __percpu *ip_rt_acct;
struct in_device;
extern int ip_rt_init(void);
extern void rt_cache_flush(struct net *net, int how);
+extern void rt_flush_dev(struct net_device *dev);
extern struct rtable *__ip_route_output_key(struct net *, struct flowi4 *flp);
extern struct rtable *ip_route_output_flow(struct net *, struct flowi4 *flp,
struct sock *sk);
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 8732cc7..c43ae3f 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -1046,6 +1046,7 @@ static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
if (event == NETDEV_UNREGISTER) {
fib_disable_ip(dev, 2, -1);
+ rt_flush_dev(dev);
return NOTIFY_DONE;
}
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index e2abb0d..7111fce 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -147,6 +147,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
struct sk_buff *skb, u32 mtu);
static void ip_do_redirect(struct dst_entry *dst, struct sock *sk,
struct sk_buff *skb);
+static void ipv4_dst_destroy(struct dst_entry *dst);
static void ipv4_dst_ifdown(struct dst_entry *dst, struct net_device *dev,
int how)
@@ -170,6 +171,7 @@ static struct dst_ops ipv4_dst_ops = {
.default_advmss = ipv4_default_advmss,
.mtu = ipv4_mtu,
.cow_metrics = ipv4_cow_metrics,
+ .destroy = ipv4_dst_destroy,
.ifdown = ipv4_dst_ifdown,
.negative_advice = ipv4_negative_advice,
.link_failure = ipv4_link_failure,
@@ -1168,9 +1170,11 @@ static struct fib_nh_exception *find_exception(struct fib_nh *nh, __be32 daddr)
return NULL;
}
-static void rt_bind_exception(struct rtable *rt, struct fib_nh_exception *fnhe,
+static bool rt_bind_exception(struct rtable *rt, struct fib_nh_exception *fnhe,
__be32 daddr)
{
+ bool ret = false;
+
spin_lock_bh(&fnhe_lock);
if (daddr == fnhe->fnhe_daddr) {
@@ -1199,14 +1203,18 @@ static void rt_bind_exception(struct rtable *rt, struct fib_nh_exception *fnhe,
fnhe->fnhe_rth = rt;
fnhe->fnhe_stamp = jiffies;
+ ret = true;
}
spin_unlock_bh(&fnhe_lock);
+
+ return ret;
}
-static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
+static bool rt_cache_route(struct fib_nh *nh, struct rtable *rt)
{
struct rtable *orig, *prev, **p = &nh->nh_rth_output;
+ bool ret = true;
if (rt_is_input_route(rt))
p = &nh->nh_rth_input;
@@ -1221,6 +1229,48 @@ static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
dst_release(&orig->dst);
} else {
dst_release(&rt->dst);
+ ret = false;
+ }
+
+ return ret;
+}
+
+static DEFINE_SPINLOCK(rt_uncached_lock);
+static LIST_HEAD(rt_uncached_list);
+
+static void rt_add_uncached_list(struct rtable *rt)
+{
+ spin_lock_bh(&rt_uncached_lock);
+ list_add_tail(&rt->rt_uncached, &rt_uncached_list);
+ spin_unlock_bh(&rt_uncached_lock);
+}
+
+static void ipv4_dst_destroy(struct dst_entry *dst)
+{
+ struct rtable *rt = (struct rtable *) dst;
+
+ if (dst->flags & DST_NOCACHE) {
+ spin_lock_bh(&rt_uncached_lock);
+ list_del(&rt->rt_uncached);
+ spin_unlock_bh(&rt_uncached_lock);
+ }
+}
+
+void rt_flush_dev(struct net_device *dev)
+{
+ if (!list_empty(&rt_uncached_list)) {
+ struct net *net = dev_net(dev);
+ struct rtable *rt;
+
+ spin_lock_bh(&rt_uncached_lock);
+ list_for_each_entry(rt, &rt_uncached_list, rt_uncached) {
+ if (rt->dst.dev != dev)
+ continue;
+ rt->dst.dev = net->loopback_dev;
+ dev_hold(rt->dst.dev);
+ dev_put(dev);
+ }
+ spin_unlock_bh(&rt_uncached_lock);
}
}
@@ -1236,6 +1286,8 @@ static void rt_set_nexthop(struct rtable *rt, __be32 daddr,
struct fib_nh_exception *fnhe,
struct fib_info *fi, u16 type, u32 itag)
{
+ bool cached = false;
+
if (fi) {
struct fib_nh *nh = &FIB_RES_NH(*res);
@@ -1246,10 +1298,12 @@ static void rt_set_nexthop(struct rtable *rt, __be32 daddr,
rt->dst.tclassid = nh->nh_tclassid;
#endif
if (unlikely(fnhe))
- rt_bind_exception(rt, fnhe, daddr);
+ cached = rt_bind_exception(rt, fnhe, daddr);
else if (!(rt->dst.flags & DST_NOCACHE))
- rt_cache_route(nh, rt);
+ cached = rt_cache_route(nh, rt);
}
+ if (unlikely(!cached))
+ rt_add_uncached_list(rt);
#ifdef CONFIG_IP_ROUTE_CLASSID
#ifdef CONFIG_IP_MULTIPLE_TABLES
@@ -1316,6 +1370,7 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
rth->rt_iif = 0;
rth->rt_pmtu = 0;
rth->rt_gateway = 0;
+ INIT_LIST_HEAD(&rth->rt_uncached);
if (our) {
rth->dst.input= ip_local_deliver;
rth->rt_flags |= RTCF_LOCAL;
@@ -1441,6 +1496,7 @@ static int __mkroute_input(struct sk_buff *skb,
rth->rt_iif = 0;
rth->rt_pmtu = 0;
rth->rt_gateway = 0;
+ INIT_LIST_HEAD(&rth->rt_uncached);
rth->dst.input = ip_forward;
rth->dst.output = ip_output;
@@ -1607,6 +1663,7 @@ local_input:
rth->rt_iif = 0;
rth->rt_pmtu = 0;
rth->rt_gateway = 0;
+ INIT_LIST_HEAD(&rth->rt_uncached);
if (res.type == RTN_UNREACHABLE) {
rth->dst.input= ip_error;
rth->dst.error= -err;
@@ -1771,6 +1828,7 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
rth->rt_iif = orig_oif ? : 0;
rth->rt_pmtu = 0;
rth->rt_gateway = 0;
+ INIT_LIST_HEAD(&rth->rt_uncached);
RT_CACHE_STAT_INC(out_slow_tot);
@@ -2050,6 +2108,8 @@ struct dst_entry *ipv4_blackhole_route(struct net *net, struct dst_entry *dst_or
rt->rt_type = ort->rt_type;
rt->rt_gateway = ort->rt_gateway;
+ INIT_LIST_HEAD(&rt->rt_uncached);
+
dst_free(new);
}
diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index c628184..681ea2f 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -92,6 +92,7 @@ static int xfrm4_fill_dst(struct xfrm_dst *xdst, struct net_device *dev,
xdst->u.rt.rt_type = rt->rt_type;
xdst->u.rt.rt_gateway = rt->rt_gateway;
xdst->u.rt.rt_pmtu = rt->rt_pmtu;
+ INIT_LIST_HEAD(&xdst->u.rt.rt_uncached);
return 0;
}
--
1.7.11.2
^ permalink raw reply related
* [PATCHv2 net 0/3] Prevent extreme TSO parameters from stalling TX queues
From: Ben Hutchings @ 2012-07-31 1:51 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-net-drivers, Ben Greear, Eric Dumazet,
Stephen Hemminger
The following changes fix a potential DoS by peers or local users on
network interfaces using the sfc driver (and possibly others) with TSO
enabled (as it is by default).
Please apply patches 1 and 2 to the net tree and your stable update
queue. I'm not sure whether patch 3 is really important.
Ben.
Ben Hutchings (3):
net: Allow driver to limit number of GSO segments per skb
sfc: Fix maximum number of TSO segments and minimum TX queue size
tcp: Apply device TSO segment limit earlier
drivers/net/ethernet/sfc/efx.c | 6 ++++++
drivers/net/ethernet/sfc/efx.h | 14 ++++++++++----
drivers/net/ethernet/sfc/ethtool.c | 16 +++++++++++-----
drivers/net/ethernet/sfc/tx.c | 19 +++++++++++++++++++
include/linux/netdevice.h | 2 ++
include/net/sock.h | 2 ++
net/core/dev.c | 4 ++++
net/core/sock.c | 1 +
net/ipv4/tcp.c | 4 +++-
net/ipv4/tcp_cong.c | 3 ++-
net/ipv4/tcp_output.c | 21 ++++++++++++---------
11 files changed, 72 insertions(+), 20 deletions(-)
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCHv2 net 1/3] net: Allow driver to limit number of GSO segments per skb
From: Ben Hutchings @ 2012-07-31 1:57 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-net-drivers, Ben Greear, Eric Dumazet,
Stephen Hemminger
In-Reply-To: <1343699476.2667.69.camel@bwh-desktop.uk.solarflarecom.com>
A peer (or local user) may cause TCP to use a nominal MSS of as little
as 88 (actual MSS of 76 with timestamps). Given that we have a
sufficiently prodigious local sender and the peer ACKs quickly enough,
it is nevertheless possible to grow the window for such a connection
to the point that we will try to send just under 64K at once. This
results in a single skb that expands to 861 segments.
In some drivers with TSO support, such an skb will require hundreds of
DMA descriptors; a substantial fraction of a TX ring or even more than
a full ring. The TX queue selected for the skb may stall and trigger
the TX watchdog repeatedly (since the problem skb will be retried
after the TX reset). This particularly affects sfc, for which the
issue is designated as CVE-2012-3412.
Therefore:
1. Add the field net_device::gso_max_segs holding the device-specific
limit.
2. In netif_skb_features(), if the number of segments is too high then
mask out GSO features to force fall back to software GSO.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
include/linux/netdevice.h | 2 ++
net/core/dev.c | 4 ++++
2 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index eb06e58..a9db4f3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1300,6 +1300,8 @@ struct net_device {
/* for setting kernel sock attribute on TCP connection setup */
#define GSO_MAX_SIZE 65536
unsigned int gso_max_size;
+#define GSO_MAX_SEGS 65535
+ u16 gso_max_segs;
#ifdef CONFIG_DCB
/* Data Center Bridging netlink ops */
diff --git a/net/core/dev.c b/net/core/dev.c
index 0ebaea1..4020646 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2133,6 +2133,9 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
__be16 protocol = skb->protocol;
netdev_features_t features = skb->dev->features;
+ if (skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
+ features &= ~NETIF_F_GSO_MASK;
+
if (protocol == htons(ETH_P_8021Q)) {
struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
protocol = veh->h_vlan_encapsulated_proto;
@@ -5942,6 +5945,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
dev_net_set(dev, &init_net);
dev->gso_max_size = GSO_MAX_SIZE;
+ dev->gso_max_segs = GSO_MAX_SEGS;
INIT_LIST_HEAD(&dev->napi_list);
INIT_LIST_HEAD(&dev->unreg_list);
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCHv2 net 2/3] sfc: Fix maximum number of TSO segments and minimum TX queue size
From: Ben Hutchings @ 2012-07-31 1:57 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-net-drivers, Ben Greear, Eric Dumazet,
Stephen Hemminger
In-Reply-To: <1343699476.2667.69.camel@bwh-desktop.uk.solarflarecom.com>
Currently an skb requiring TSO may not fit within a minimum-size TX
queue. The TX queue selected for the skb may stall and trigger the TX
watchdog repeatedly (since the problem skb will be retried after the
TX reset). This issue is designated as CVE-2012-3412.
Set the maximum number of TSO segments for our devices to 100. This
should make no difference to behaviour unless the actual MSS is less
than about 700. Increase the minimum TX queue size accordingly to
allow for 2 worst-case skbs, so that there will definitely be space
to add an skb after we wake a queue.
To avoid invalidating existing configurations, change
efx_ethtool_set_ringparam() to fix up values that are too small rather
than returning -EINVAL.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 6 ++++++
drivers/net/ethernet/sfc/efx.h | 14 ++++++++++----
drivers/net/ethernet/sfc/ethtool.c | 16 +++++++++++-----
drivers/net/ethernet/sfc/tx.c | 19 +++++++++++++++++++
4 files changed, 46 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 70554a1..65a8d49 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1503,6 +1503,11 @@ static int efx_probe_all(struct efx_nic *efx)
goto fail2;
}
+ BUILD_BUG_ON(EFX_DEFAULT_DMAQ_SIZE < EFX_RXQ_MIN_ENT);
+ if (WARN_ON(EFX_DEFAULT_DMAQ_SIZE < EFX_TXQ_MIN_ENT(efx))) {
+ rc = -EINVAL;
+ goto fail3;
+ }
efx->rxq_entries = efx->txq_entries = EFX_DEFAULT_DMAQ_SIZE;
rc = efx_probe_filters(efx);
@@ -2070,6 +2075,7 @@ static int efx_register_netdev(struct efx_nic *efx)
net_dev->irq = efx->pci_dev->irq;
net_dev->netdev_ops = &efx_netdev_ops;
SET_ETHTOOL_OPS(net_dev, &efx_ethtool_ops);
+ net_dev->gso_max_segs = EFX_TSO_MAX_SEGS;
rtnl_lock();
diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
index be8f915..70755c9 100644
--- a/drivers/net/ethernet/sfc/efx.h
+++ b/drivers/net/ethernet/sfc/efx.h
@@ -30,6 +30,7 @@ extern netdev_tx_t
efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb);
extern void efx_xmit_done(struct efx_tx_queue *tx_queue, unsigned int index);
extern int efx_setup_tc(struct net_device *net_dev, u8 num_tc);
+extern unsigned int efx_tx_max_skb_descs(struct efx_nic *efx);
/* RX */
extern int efx_probe_rx_queue(struct efx_rx_queue *rx_queue);
@@ -52,10 +53,15 @@ extern void efx_schedule_slow_fill(struct efx_rx_queue *rx_queue);
#define EFX_MAX_EVQ_SIZE 16384UL
#define EFX_MIN_EVQ_SIZE 512UL
-/* The smallest [rt]xq_entries that the driver supports. Callers of
- * efx_wake_queue() assume that they can subsequently send at least one
- * skb. Falcon/A1 may require up to three descriptors per skb_frag. */
-#define EFX_MIN_RING_SIZE (roundup_pow_of_two(2 * 3 * MAX_SKB_FRAGS))
+/* Maximum number of TCP segments we support for soft-TSO */
+#define EFX_TSO_MAX_SEGS 100
+
+/* The smallest [rt]xq_entries that the driver supports. RX minimum
+ * is a bit arbitrary. For TX, we must have space for at least 2
+ * TSO skbs.
+ */
+#define EFX_RXQ_MIN_ENT 128U
+#define EFX_TXQ_MIN_ENT(efx) (2 * efx_tx_max_skb_descs(efx))
/* Filters */
extern int efx_probe_filters(struct efx_nic *efx);
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 10536f9..8cba2df 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -680,21 +680,27 @@ static int efx_ethtool_set_ringparam(struct net_device *net_dev,
struct ethtool_ringparam *ring)
{
struct efx_nic *efx = netdev_priv(net_dev);
+ u32 txq_entries;
if (ring->rx_mini_pending || ring->rx_jumbo_pending ||
ring->rx_pending > EFX_MAX_DMAQ_SIZE ||
ring->tx_pending > EFX_MAX_DMAQ_SIZE)
return -EINVAL;
- if (ring->rx_pending < EFX_MIN_RING_SIZE ||
- ring->tx_pending < EFX_MIN_RING_SIZE) {
+ if (ring->rx_pending < EFX_RXQ_MIN_ENT) {
netif_err(efx, drv, efx->net_dev,
- "TX and RX queues cannot be smaller than %ld\n",
- EFX_MIN_RING_SIZE);
+ "RX queues cannot be smaller than %u\n",
+ EFX_RXQ_MIN_ENT);
return -EINVAL;
}
- return efx_realloc_channels(efx, ring->rx_pending, ring->tx_pending);
+ txq_entries = max(ring->tx_pending, EFX_TXQ_MIN_ENT(efx));
+ if (txq_entries != ring->tx_pending)
+ netif_warn(efx, drv, efx->net_dev,
+ "increasing TX queue size to minimum of %u\n",
+ txq_entries);
+
+ return efx_realloc_channels(efx, ring->rx_pending, txq_entries);
}
static int efx_ethtool_set_pauseparam(struct net_device *net_dev,
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 9b225a7..1871343 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -119,6 +119,25 @@ efx_max_tx_len(struct efx_nic *efx, dma_addr_t dma_addr)
return len;
}
+unsigned int efx_tx_max_skb_descs(struct efx_nic *efx)
+{
+ /* Header and payload descriptor for each output segment, plus
+ * one for every input fragment boundary within a segment
+ */
+ unsigned int max_descs = EFX_TSO_MAX_SEGS * 2 + MAX_SKB_FRAGS;
+
+ /* Possibly one more per segment for the alignment workaround */
+ if (EFX_WORKAROUND_5391(efx))
+ max_descs += EFX_TSO_MAX_SEGS;
+
+ /* Possibly more for PCIe page boundaries within input fragments */
+ if (PAGE_SIZE > EFX_PAGE_SIZE)
+ max_descs += max_t(unsigned int, MAX_SKB_FRAGS,
+ DIV_ROUND_UP(GSO_MAX_SIZE, EFX_PAGE_SIZE));
+
+ return max_descs;
+}
+
/*
* Add a socket buffer to a TX queue
*
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCHv2 net 3/3] tcp: Apply device TSO segment limit earlier
From: Ben Hutchings @ 2012-07-31 2:11 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-net-drivers, Ben Greear, Eric Dumazet,
Stephen Hemminger
In-Reply-To: <1343699476.2667.69.camel@bwh-desktop.uk.solarflarecom.com>
Cache the device gso_max_segs in sock::sk_gso_max_segs and use it to
limit the size of TSO skbs. This avoids the need to fall back to
software GSO for local TCP senders.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
This is similar to v1 patch 1, but using the device's gso_max_segs
instead of a constant. It also covers a couple of additional cases
where sk_gso_max_size is currently used. It improves performance in the
case that the limit is reached, but this may not be worth doing if
legitimate peers don't cause us to hit that limit.
Ben.
include/net/sock.h | 2 ++
net/core/sock.c | 1 +
net/ipv4/tcp.c | 4 +++-
net/ipv4/tcp_cong.c | 3 ++-
net/ipv4/tcp_output.c | 21 ++++++++++++---------
5 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index e067f8c..25a823a 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -218,6 +218,7 @@ struct cg_proto;
* @sk_route_nocaps: forbidden route capabilities (e.g NETIF_F_GSO_MASK)
* @sk_gso_type: GSO type (e.g. %SKB_GSO_TCPV4)
* @sk_gso_max_size: Maximum GSO segment size to build
+ * @sk_gso_max_segs: Maximum number of GSO segments
* @sk_lingertime: %SO_LINGER l_linger setting
* @sk_backlog: always used with the per-socket spinlock held
* @sk_callback_lock: used with the callbacks in the end of this struct
@@ -338,6 +339,7 @@ struct sock {
netdev_features_t sk_route_nocaps;
int sk_gso_type;
unsigned int sk_gso_max_size;
+ u16 sk_gso_max_segs;
int sk_rcvlowat;
unsigned long sk_lingertime;
struct sk_buff_head sk_error_queue;
diff --git a/net/core/sock.c b/net/core/sock.c
index 2676a88..34cc7bb 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1403,6 +1403,7 @@ void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
} else {
sk->sk_route_caps |= NETIF_F_SG | NETIF_F_HW_CSUM;
sk->sk_gso_max_size = dst->dev->gso_max_size;
+ sk->sk_gso_max_segs = dst->dev->gso_max_segs;
}
}
}
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index e7e6eea..2109ff4 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -811,7 +811,9 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
old_size_goal + mss_now > xmit_size_goal)) {
xmit_size_goal = old_size_goal;
} else {
- tp->xmit_size_goal_segs = xmit_size_goal / mss_now;
+ tp->xmit_size_goal_segs =
+ min_t(u16, xmit_size_goal / mss_now,
+ sk->sk_gso_max_segs);
xmit_size_goal = tp->xmit_size_goal_segs * mss_now;
}
}
diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index 4d4db16..1432cdb 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -291,7 +291,8 @@ bool tcp_is_cwnd_limited(const struct sock *sk, u32 in_flight)
left = tp->snd_cwnd - in_flight;
if (sk_can_gso(sk) &&
left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd &&
- left * tp->mss_cache < sk->sk_gso_max_size)
+ left * tp->mss_cache < sk->sk_gso_max_size &&
+ left < sk->sk_gso_max_segs)
return true;
return left <= tcp_max_tso_deferred_mss(tp);
}
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 33cd065..0c9db8f 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1522,21 +1522,21 @@ static void tcp_cwnd_validate(struct sock *sk)
* when we would be allowed to send the split-due-to-Nagle skb fully.
*/
static unsigned int tcp_mss_split_point(const struct sock *sk, const struct sk_buff *skb,
- unsigned int mss_now, unsigned int cwnd)
+ unsigned int mss_now, unsigned int max_segs)
{
const struct tcp_sock *tp = tcp_sk(sk);
- u32 needed, window, cwnd_len;
+ u32 needed, window, max_len;
window = tcp_wnd_end(tp) - TCP_SKB_CB(skb)->seq;
- cwnd_len = mss_now * cwnd;
+ max_len = mss_now * max_segs;
- if (likely(cwnd_len <= window && skb != tcp_write_queue_tail(sk)))
- return cwnd_len;
+ if (likely(max_len <= window && skb != tcp_write_queue_tail(sk)))
+ return max_len;
needed = min(skb->len, window);
- if (cwnd_len <= needed)
- return cwnd_len;
+ if (max_len <= needed)
+ return max_len;
return needed - needed % mss_now;
}
@@ -1765,7 +1765,8 @@ static bool tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
limit = min(send_win, cong_win);
/* If a full-sized TSO skb can be sent, do it. */
- if (limit >= sk->sk_gso_max_size)
+ if (limit >= min_t(unsigned int, sk->sk_gso_max_size,
+ sk->sk_gso_max_segs * tp->mss_cache))
goto send_now;
/* Middle in queue won't get any more data, full sendable already? */
@@ -1999,7 +2000,9 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
limit = mss_now;
if (tso_segs > 1 && !tcp_urg_mode(tp))
limit = tcp_mss_split_point(sk, skb, mss_now,
- cwnd_quota);
+ min_t(unsigned int,
+ cwnd_quota,
+ sk->sk_gso_max_segs));
if (skb->len > limit &&
unlikely(tso_fragment(sk, skb, limit, mss_now, gfp)))
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* Re: [RFC v2 1/2] PCI-Express Non-Transparent Bridge Support
From: Jianbin Kang @ 2012-07-31 3:35 UTC (permalink / raw)
To: Jon Mason; +Cc: Bjorn Helgaas, linux-kernel, netdev, linux-pci, Dave Jiang
In-Reply-To: <20120730181542.GA987@jonmason-lab>
> I've tried to make it all generic enough that non-Intel NTBs should plug in with
> minimal changes to ntb_hw.c. If their design is too divergent, then a slight
> redesign of ntb_hw.c might be necessary. But from what I've seen of other
> designs on the internet, they appear to be extremely similar. The transport and
> client drivers were written with the hardware abstracted away as much as
> possible to prevent the need to modify it for different hardware. If there is
> anything which is Intel hardware specific, I'd be happy to change it to make it
> more generic.
In ntb_process_tx(), ntb uses hard-coding 'memcpy_toio' to copy data
to remote.
Is it better to provide a function pointer like 'tx()' and call qp->tx().
memcpy_toio is a slow operation. Some hardware can setup a dma
transfer and wait.
IMHO, the best way is to handle tx in async mode. But it requires
lots of modification.
^ permalink raw reply
* [PATCH] ipv4: Restore old dst_free() behavior.
From: David Miller @ 2012-07-31 5:38 UTC (permalink / raw)
To: edumazet; +Cc: netdev
Eric, this is what I'd like to propose.
It seems the problem you were likely running into was simply
the fact that we were not inserting an RCU grace period for
the dst_free() that we do when purging a FIB nexthop.
So this reverts your change, and instead adds the necessary
call_rcu_bh() wrapper around the dst_free() done in fib_semantics.c
That makes it so that we don't need all of that inc_not_zero stuff for
sockets, and the special dst flag. If we set the pointer to NULL,
then do the dst_free() via RCU, we can test that refcount safely in
dst_free() since it can only decrease at that point.
What do you think? Does it pass your tests?
Thanks.
diff --git a/include/net/dst.h b/include/net/dst.h
index 31a9fd3..baf5978 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -61,7 +61,6 @@ struct dst_entry {
#define DST_NOPEER 0x0040
#define DST_FAKE_RTABLE 0x0080
#define DST_XFRM_TUNNEL 0x0100
-#define DST_RCU_FREE 0x0200
unsigned short pending_confirm;
@@ -383,6 +382,12 @@ static inline void dst_free(struct dst_entry *dst)
__dst_free(dst);
}
+static inline void dst_rcu_free(struct rcu_head *head)
+{
+ struct dst_entry *dst = container_of(head, struct dst_entry, rcu_head);
+ dst_free(dst);
+}
+
static inline void dst_confirm(struct dst_entry *dst)
{
dst->pending_confirm = 1;
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index e3fd34c..613cfa4 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -249,17 +249,4 @@ static inline __u8 inet_sk_flowi_flags(const struct sock *sk)
return flags;
}
-static inline void inet_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
-{
- struct dst_entry *dst = skb_dst(skb);
-
- if (atomic_inc_not_zero(&dst->__refcnt)) {
- if (!(dst->flags & DST_RCU_FREE))
- dst->flags |= DST_RCU_FREE;
-
- sk->sk_rx_dst = dst;
- inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
- }
-}
-
#endif /* _INET_SOCK_H */
diff --git a/net/core/dst.c b/net/core/dst.c
index d9e33eb..069d51d 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -258,15 +258,6 @@ again:
}
EXPORT_SYMBOL(dst_destroy);
-static void dst_rcu_destroy(struct rcu_head *head)
-{
- struct dst_entry *dst = container_of(head, struct dst_entry, rcu_head);
-
- dst = dst_destroy(dst);
- if (dst)
- __dst_free(dst);
-}
-
void dst_release(struct dst_entry *dst)
{
if (dst) {
@@ -274,14 +265,10 @@ void dst_release(struct dst_entry *dst)
newrefcnt = atomic_dec_return(&dst->__refcnt);
WARN_ON(newrefcnt < 0);
- if (unlikely(dst->flags & (DST_NOCACHE | DST_RCU_FREE)) && !newrefcnt) {
- if (dst->flags & DST_RCU_FREE) {
- call_rcu_bh(&dst->rcu_head, dst_rcu_destroy);
- } else {
- dst = dst_destroy(dst);
- if (dst)
- __dst_free(dst);
- }
+ if (unlikely(dst->flags & DST_NOCACHE) && !newrefcnt) {
+ dst = dst_destroy(dst);
+ if (dst)
+ __dst_free(dst);
}
}
}
@@ -333,14 +320,11 @@ EXPORT_SYMBOL(__dst_destroy_metrics_generic);
*/
void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst)
{
- bool hold;
-
WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
/* If dst not in cache, we must take a reference, because
* dst_release() will destroy dst as soon as its refcount becomes zero
*/
- hold = (dst->flags & (DST_NOCACHE | DST_RCU_FREE)) == DST_NOCACHE;
- if (unlikely(hold)) {
+ if (unlikely(dst->flags & DST_NOCACHE)) {
dst_hold(dst);
skb_dst_set(skb, dst);
} else {
diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index 2671977..85a3604 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -184,12 +184,6 @@ static __inline__ unsigned int dn_hash(__le16 src, __le16 dst)
return dn_rt_hash_mask & (unsigned int)tmp;
}
-static inline void dst_rcu_free(struct rcu_head *head)
-{
- struct dst_entry *dst = container_of(head, struct dst_entry, rcu_head);
- dst_free(dst);
-}
-
static inline void dnrt_free(struct dn_route *rt)
{
call_rcu_bh(&rt->dst.rcu_head, dst_rcu_free);
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index e55171f..67bbaf5 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -161,6 +161,17 @@ static void free_nh_exceptions(struct fib_nh *nh)
kfree(hash);
}
+static void rt_nexthop_free(struct rtable **rtp)
+{
+ struct rtable *rt = *rtp;
+
+ if (!rt)
+ return;
+ *rtp = NULL;
+
+ call_rcu_bh(&rt->dst.rcu_head, dst_rcu_free);
+}
+
/* Release a nexthop info record */
static void free_fib_info_rcu(struct rcu_head *head)
{
@@ -171,10 +182,8 @@ static void free_fib_info_rcu(struct rcu_head *head)
dev_put(nexthop_nh->nh_dev);
if (nexthop_nh->nh_exceptions)
free_nh_exceptions(nexthop_nh);
- if (nexthop_nh->nh_rth_output)
- dst_release(&nexthop_nh->nh_rth_output->dst);
- if (nexthop_nh->nh_rth_input)
- dst_release(&nexthop_nh->nh_rth_input->dst);
+ rt_nexthop_free(&nexthop_nh->nh_rth_output);
+ rt_nexthop_free(&nexthop_nh->nh_rth_input);
} endfor_nexthops(fi);
release_net(fi->fib_net);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d6eabcf..fc1a81c 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1199,6 +1199,11 @@ restart:
fnhe->fnhe_stamp = jiffies;
}
+static inline void rt_free(struct rtable *rt)
+{
+ call_rcu_bh(&rt->dst.rcu_head, dst_rcu_free);
+}
+
static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
{
struct rtable *orig, *prev, **p = &nh->nh_rth_output;
@@ -1208,14 +1213,17 @@ static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
orig = *p;
- rt->dst.flags |= DST_RCU_FREE;
- dst_hold(&rt->dst);
prev = cmpxchg(p, orig, rt);
if (prev == orig) {
if (orig)
- dst_release(&orig->dst);
+ rt_free(orig);
} else {
- dst_release(&rt->dst);
+ /* Routes we intend to cache in the FIB nexthop have
+ * the DST_NOCACHE bit clear. However, if we are
+ * unsuccessful at storing this route into the cache
+ * we really need to set it.
+ */
+ rt->dst.flags |= DST_NOCACHE;
}
}
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 9be30b0..a356e1f 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5604,7 +5604,8 @@ void tcp_finish_connect(struct sock *sk, struct sk_buff *skb)
tcp_set_state(sk, TCP_ESTABLISHED);
if (skb != NULL) {
- inet_sk_rx_dst_set(sk, skb);
+ sk->sk_rx_dst = dst_clone(skb_dst(skb));
+ inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
security_inet_conn_established(sk, skb);
}
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 7f91e5a..2fbd992 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1617,19 +1617,19 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
#endif
if (sk->sk_state == TCP_ESTABLISHED) { /* Fast path */
- struct dst_entry *dst = sk->sk_rx_dst;
-
sock_rps_save_rxhash(sk, skb);
- if (dst) {
+ if (sk->sk_rx_dst) {
+ struct dst_entry *dst = sk->sk_rx_dst;
if (inet_sk(sk)->rx_dst_ifindex != skb->skb_iif ||
dst->ops->check(dst, 0) == NULL) {
dst_release(dst);
sk->sk_rx_dst = NULL;
}
}
- if (unlikely(sk->sk_rx_dst == NULL))
- inet_sk_rx_dst_set(sk, skb);
-
+ if (unlikely(sk->sk_rx_dst == NULL)) {
+ sk->sk_rx_dst = dst_clone(skb_dst(skb));
+ inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
+ }
if (tcp_rcv_established(sk, skb, tcp_hdr(skb), skb->len)) {
rsk = sk;
goto reset;
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 232a90c..3f1cc20 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -387,7 +387,8 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
struct tcp_sock *oldtp = tcp_sk(sk);
struct tcp_cookie_values *oldcvp = oldtp->cookie_values;
- inet_sk_rx_dst_set(newsk, skb);
+ newsk->sk_rx_dst = dst_clone(skb_dst(skb));
+ inet_sk(newsk)->rx_dst_ifindex = skb->skb_iif;
/* TCP Cookie Transactions require space for the cookie pair,
* as it differs for each connection. There is no need to
^ permalink raw reply related
* Re: [3.5 regression / mcs7830 / bisected] bridge constantly toggeling between disabled and forwarding
From: Michael Leun @ 2012-07-31 5:52 UTC (permalink / raw)
To: linux; +Cc: davem, netdev, linux-kernel, gregkh
In-Reply-To: <20120724013634.11bf1360@xenia.leun.net>
On Tue, 24 Jul 2012 01:36:34 +0200
Michael Leun <lkml20120218@newton.leun.net> wrote:
My report might sound like I'm complaining that link state detection
works because link state detection was implemented - sorry, but thats
not true.
I do NOT see this link state changes if there is no traffic on the
interface, but I start seeing them once I start pinging. I think this
is not the idea of link state detection?
I would very much appreciate if you could have a look into that,
because it is rather annoying.
> On Mon, 23 Jul 2012 09:15:04 +0200
> Michael Leun <lkml20120218@newton.leun.net> wrote:
>
> [see issue description below]
>
> Bisecting yielded
>
> b1ff4f96fd1c63890d78d8939c6e0f2b44ce3113 is the first bad commit
> commit b1ff4f96fd1c63890d78d8939c6e0f2b44ce3113
> Author: Ondrej Zary <linux@rainbow-software.org>
> Date: Fri Jun 1 10:29:08 2012 +0000
>
> mcs7830: Implement link state detection
>
> Add .status callback that detects link state changes.
> Tested with MCS7832CV-AA chip (9710:7830, identified as rev.C by the driver).
> Fixes https://bugzilla.kernel.org/show_bug.cgi?id=28532
>
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> :040000 040000 5480780cb5e75c57122a621fc3bab0108c16be27 d97efd9cc0a465dff76bcd3a3c547f718f2a5345 M drivers
>
>
> Reverting that from 3.5 makes the issue go away.
>
> > Hi,
> >
> > when I use my usb ethernet adapter
> >
> > # > lsusb
> > [...]
> > Bus 002 Device 009: ID 9710:7830 MosChip Semiconductor MCS7830 10/100 Mbps Ethernet adapter
> > [...]
> >
> > as port of an bridge
> >
> > > # brctl addbr br0
> > > # brctl addif br0 eth0
> > > # brctl addif br0 ue5
> > > # ifconfig ue5 up
> > > # ifconfig br0 up
> >
> > (Also does happen when eth0 is not part of the bridge, but the logs I
> > had available were from that situation...)
> >
> > I constantly get messages showing the interface toggeling between
> > disabled and forwarding state:
> >
> > Jul 23 07:40:50 elektra kernel: [ 1539.497337] br0: port 2(ue5) entered disabled state
> > Jul 23 07:40:50 elektra kernel: [ 1539.554992] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:40:50 elektra kernel: [ 1539.555005] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:40:51 elektra kernel: [ 1540.496242] br0: port 2(ue5) entered disabled state
> > Jul 23 07:40:51 elektra kernel: [ 1540.552534] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:40:51 elektra kernel: [ 1540.552548] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:40:52 elektra kernel: [ 1541.550413] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:40:53 elektra kernel: [ 1542.529672] br0: port 2(ue5) entered disabled state
> > Jul 23 07:40:53 elektra kernel: [ 1542.587162] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:40:53 elektra kernel: [ 1542.587175] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:40:54 elektra kernel: [ 1543.585309] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:41:00 elektra kernel: [ 1549.360600] br0: port 2(ue5) entered disabled state
> > Jul 23 07:41:00 elektra kernel: [ 1549.442998] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:41:00 elektra kernel: [ 1549.443011] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:41:01 elektra kernel: [ 1550.357686] br0: port 2(ue5) entered disabled state
> > Jul 23 07:41:01 elektra kernel: [ 1550.408208] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:41:01 elektra kernel: [ 1550.408222] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:41:02 elektra kernel: [ 1551.407656] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:41:03 elektra kernel: [ 1552.401578] br0: port 2(ue5) entered disabled state
> > Jul 23 07:41:03 elektra kernel: [ 1552.474773] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:41:03 elektra kernel: [ 1552.474786] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:41:04 elektra kernel: [ 1553.472487] br0: port 2(ue5) entered forwarding state
> > Jul 23 07:41:05 elektra kernel: [ 1554.356138] br0: port 2(ue5) entered disabled state
> > [...]
> >
> > This does (in the same situation, nothing else than the kernel changed)
> > not happen with 3.4.5.
> >
> > Does anybody have an idea what the issue might be or do I need to bisect?
>
>
> --
> MfG,
>
> Michael Leun
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
MfG,
Michael Leun
^ permalink raw reply
* Re: [PATCH] ipv4: Restore old dst_free() behavior.
From: Eric Dumazet @ 2012-07-31 5:54 UTC (permalink / raw)
To: David Miller; +Cc: edumazet, netdev
In-Reply-To: <20120730.223827.74792864437911339.davem@davemloft.net>
On Mon, 2012-07-30 at 22:38 -0700, David Miller wrote:
> Eric, this is what I'd like to propose.
>
> It seems the problem you were likely running into was simply
> the fact that we were not inserting an RCU grace period for
> the dst_free() that we do when purging a FIB nexthop.
>
> So this reverts your change, and instead adds the necessary
> call_rcu_bh() wrapper around the dst_free() done in fib_semantics.c
>
> That makes it so that we don't need all of that inc_not_zero stuff for
> sockets, and the special dst flag. If we set the pointer to NULL,
> then do the dst_free() via RCU, we can test that refcount safely in
> dst_free() since it can only decrease at that point.
>
> What do you think? Does it pass your tests?
>
> Thanks.
I'll test that ASAP, I was trying to understand why Linus tree gave me a
non workable machine ( a panic in igb driver ... NULL RIP) .
I dont understand how I did not have this bug with net tree.
Please give me a couple of hours, I need to break my fast ;)
^ permalink raw reply
* Re: [PATCH] ipv4: Restore old dst_free() behavior.
From: David Miller @ 2012-07-31 5:57 UTC (permalink / raw)
To: eric.dumazet; +Cc: edumazet, netdev
In-Reply-To: <1343714097.21269.38.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 31 Jul 2012 07:54:57 +0200
> Please give me a couple of hours, I need to break my fast ;)
No problem.
Meanwhile, I'm busy rebasing my uncached list patches :-)
^ permalink raw reply
* [BUG] igb: panic at boot time with latest Linus tree
From: Eric Dumazet @ 2012-07-31 6:14 UTC (permalink / raw)
To: David Miller, Jeff Kirsher; +Cc: netdev
For information, I get this each time I boot on latest Linus tree :
RTNL is left locked, so machine unusable.
No problem with David net tree, so thats a bit strange...
Not sure I'll have time to investigate today
(And tomorrow I am off for the day)
[ 11.153682] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 11.153806] IP: [< (null)>] (null)
[ 11.153872] PGD 310544067 PUD 311b2d067 PMD 0
[ 11.153945] Oops: 0010 [#1] SMP
[ 11.154012] Modules linked in: nfsd exportfs nfs lockd auth_rpcgss sunrpc asix usbnet rt61pci crc_itu_t rt2x00pci rt2x00lib eeprom_93cx6 tg3 ixgbe mdio igb
[ 11.154227] CPU 10
[ 11.154239] Pid: 2476, comm: NetworkManager Not tainted 3.5.0+ #123 Hewlett-Packard HP Z600 Workstation/0B54h
[ 11.154437] RIP: 0010:[<0000000000000000>] [< (null)>] (null)
[ 11.154574] RSP: 0018:ffff88030f667630 EFLAGS: 00010282
[ 11.154645] RAX: 0000000017cf7980 RBX: ffff88031080f100 RCX: 0000000000000200
[ 11.154720] RDX: 0000000000000040 RSI: ffffea0017cf7980 RDI: ffff880611bc1098
[ 11.154794] RBP: ffff88030f667678 R08: 0000000000000002 R09: 0000000000000000
[ 11.154868] R10: ffffea0017cf7980 R11: 0000000000000000 R12: ffffc90007d58000
[ 11.154942] R13: ffff880611bc1098 R14: ffff8803106ff000 R15: ffff8805f3de6040
[ 11.155016] FS: 00007fa2e4b94800(0000) GS:ffff88061fc80000(0000) knlGS:0000000000000000
[ 11.155148] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 11.155220] CR2: 0000000000000000 CR3: 000000031066a000 CR4: 00000000000007e0
[ 11.155283] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 11.155347] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 11.155411] Process NetworkManager (pid: 2476, threadinfo ffff88030f666000, task ffff880310dd2d00)
[ 11.155523] Stack:
[ 11.155579] ffffffffa00071fe 0000000000000000 00ffff00a000400a ffff88030f667678
[ 11.155712] ffff88030f610700 0000000000000001 ffff88030f610708 ffff88030f610740
[ 11.155840] 0000000000000001 ffff88030f6676b8 ffffffffa000834b ffff88030f610700
[ 11.155969] Call Trace:
[ 11.156036] [<ffffffffa00071fe>] ? igb_alloc_rx_buffers+0x13e/0x2d0 [igb]
[ 11.156104] [<ffffffffa000834b>] igb_configure+0x34b/0x4d0 [igb]
[ 11.156170] [<ffffffffa0008572>] __igb_open+0xa2/0x510 [igb]
[ 11.156237] [<ffffffff812c0731>] ? find_next_bit+0x21/0xd0
[ 11.156303] [<ffffffffa0008b50>] igb_open+0x10/0x20 [igb]
[ 11.156369] [<ffffffff8155288f>] __dev_open+0x8f/0xe0
[ 11.156432] [<ffffffff81552b31>] __dev_change_flags+0xa1/0x180
[ 11.156495] [<ffffffff81552cc8>] dev_change_flags+0x28/0x70
[ 11.156559] [<ffffffff8155f664>] do_setlink+0x284/0x9e0
[ 11.156624] [<ffffffff81560c8a>] ? rtnl_fill_ifinfo+0x92a/0xb30
[ 11.156690] [<ffffffff812cc220>] ? nla_parse+0x30/0xe0
[ 11.156755] [<ffffffff81561b35>] rtnl_newlink+0x345/0x580
[ 11.156820] [<ffffffff812655b9>] ? selinux_capable+0x39/0x50
[ 11.156885] [<ffffffff81262538>] ? security_capable+0x18/0x20
[ 11.156948] [<ffffffff81561384>] rtnetlink_rcv_msg+0x124/0x310
[ 11.157012] [<ffffffff8113794b>] ? kfree+0x3b/0x160
[ 11.157074] [<ffffffff81561260>] ? __rtnl_unlock+0x20/0x20
[ 11.157137] [<ffffffff8157a569>] netlink_rcv_skb+0xa9/0xd0
[ 11.157200] [<ffffffff8155ed55>] rtnetlink_rcv+0x25/0x40
[ 11.157262] [<ffffffff81579f4d>] netlink_unicast+0x1ad/0x230
[ 11.157326] [<ffffffff8157a286>] netlink_sendmsg+0x2b6/0x310
[ 11.157396] [<ffffffff815381ac>] sock_sendmsg+0xdc/0xf0
[ 11.157459] [<ffffffff8153aab8>] ? move_addr_to_kernel+0x68/0xb0
[ 11.157526] [<ffffffff81539b12>] __sys_sendmsg+0x392/0x3a0
[ 11.157590] [<ffffffff81118bbe>] ? handle_mm_fault+0x13e/0x210
[ 11.157656] [<ffffffff81155008>] ? __d_free+0x48/0x70
[ 11.157720] [<ffffffff8115e616>] ? mntput+0x26/0x40
[ 11.157783] [<ffffffff81140371>] ? __fput+0x191/0x250
[ 11.157846] [<ffffffff8153b809>] sys_sendmsg+0x49/0x90
[ 11.157911] [<ffffffff816ca652>] system_call_fastpath+0x16/0x1b
[ 11.157973] Code: Bad RIP value.
[ 11.158041] RIP [< (null)>] (null)
[ 11.158106] RSP <ffff88030f667630>
[ 11.158163] CR2: 0000000000000000
[ 11.158227] ---[ end trace bbfaed088efd61cb ]---
[ 11.158300] NetworkManager (2476) used greatest stack depth: 2936 bytes left
^ permalink raw reply
* Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
From: Xufeng Zhang @ 2012-07-31 6:17 UTC (permalink / raw)
To: Vlad Yasevich
Cc: Neil Horman, xufeng zhang, sri, davem, linux-sctp, netdev,
linux-kernel
In-Reply-To: <CA+=dFzh1wQx2rBs_2RAwrXsz79WS3njnO8=2ntQZUbB5So69gg@mail.gmail.com>
I'm wondering if the below solution is fine to you which is based on
your changes.
BTW, I have verified this patch and it works ok for all the situation,
but only one problem persists:
there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS
which happens during sending lots of small error DATA chunks.
Thanks,
Xufeng Zhang
---
diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..62c34f5 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h
@@ -110,6 +110,7 @@ typedef enum {
SCTP_CMD_SEND_NEXT_ASCONF, /* Send the next ASCONF after ACK */
SCTP_CMD_PURGE_ASCONF_QUEUE, /* Purge all asconf queues.*/
SCTP_CMD_SET_ASOC, /* Restore association context */
+ SCTP_CMD_GEN_BAD_STREAM, /* Invalid Stream errors happened command */
SCTP_CMD_LAST
} sctp_verb_t;
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index fc5e600..3d218e0 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1183,6 +1183,9 @@ struct sctp_outq {
*/
struct list_head abandoned;
+ /* Put Invalid Stream error chunks on this list */
+ struct list_head bad_stream_err;
+
/* How many unackd bytes do we have in-flight? */
__u32 outstanding_bytes;
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index e7aa177..1e87b0b 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc,
struct sctp_outq *q)
INIT_LIST_HEAD(&q->retransmit);
INIT_LIST_HEAD(&q->sacked);
INIT_LIST_HEAD(&q->abandoned);
+ INIT_LIST_HEAD(&q->bad_stream_err);
q->fast_rtx = 0;
q->outstanding_bytes = 0;
@@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q)
list_del_init(&chunk->list);
sctp_chunk_free(chunk);
}
+
+ /* Throw away any pending Invalid Stream error chunks */
+ list_for_each_entry_safe(chunk, tmp, &q->bad_stream_err, list) {
+ list_del_init(&chunk->list);
+ sctp_chunk_free(chunk);
+ }
}
/* Free the outqueue structure and any related pending chunks. */
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index fe99628..ab63fa1 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -1060,6 +1060,17 @@ static void sctp_cmd_send_asconf(struct
sctp_association *asoc)
}
}
+static void sctp_cmd_make_inv_stream_err(sctp_cmd_seq_t *commands,
+ struct sctp_association *asoc)
+{
+ struct sctp_chunk *err, *tmp;
+ struct sctp_outq *q = &asoc->outqueue;
+
+ list_for_each_entry_safe(err, tmp, &q->bad_stream_err, list) {
+ list_del_init(&err->list);
+ sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err));
+ struct sctp_association *asoc)
+{
+ struct sctp_chunk *err, *tmp;
+ struct sctp_outq *q = &asoc->outqueue;
+
+ list_for_each_entry_safe(err, tmp, &q->bad_stream_err, list) {
+ list_del_init(&err->list);
+ sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err));
+ }
+}
/* These three macros allow us to pull the debugging code out of the
* main flow of sctp_do_sm() to keep attention focused on the real
@@ -1724,6 +1735,10 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
asoc = cmd->obj.asoc;
break;
+ case SCTP_CMD_GEN_BAD_STREAM:
+ sctp_cmd_make_inv_stream_err(commands, asoc);
+ break;
+
default:
pr_warn("Impossible command: %u, %p\n",
cmd->verb, cmd->obj.ptr);
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 9fca103..cab539f 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -2967,8 +2967,14 @@ discard_force:
return SCTP_DISPOSITION_DISCARD;
discard_noforce:
- if (chunk->end_of_packet)
+ if (chunk->end_of_packet) {
+ struct sctp_outq *q = &asoc->outqueue;
+
sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force);
+ /* Queue the INVALID STREAM error after the SACK if
one is needed. */
+ if (!list_empty(&q->bad_stream_err))
+ sctp_add_cmd_sf(commands,
SCTP_CMD_GEN_BAD_STREAM, SCTP_NULL());
+ }
return SCTP_DISPOSITION_DISCARD;
consume:
@@ -3037,11 +3043,15 @@ sctp_disposition_t
sctp_sf_eat_data_fast_4_4(const struct sctp_endpoint *ep,
* with a SACK, a SHUTDOWN chunk, and restart the T2-shutdown timer
*/
if (chunk->end_of_packet) {
+ struct sctp_outq *q = &asoc->outqueue;
/* We must delay the chunk creation since the cumulative
* TSN has not been updated yet.
*/
sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SHUTDOWN, SCTP_NULL());
sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE());
+ /* Queue the INVALID STREAM error after the SACK if
one is needed. */
+ if (!list_empty(&q->bad_stream_err))
+ sctp_add_cmd_sf(commands,
SCTP_CMD_GEN_BAD_STREAM, SCTP_NULL());
sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
}
@@ -6136,6 +6146,7 @@ static int sctp_eat_data(const struct
sctp_association *asoc,
*/
sid = ntohs(data_hdr->stream);
if (sid >= asoc->c.sinit_max_instreams) {
+ struct sctp_outq *q = &asoc->outqueue;
/* Mark tsn as received even though we drop it */
sctp_add_cmd_sf(commands, SCTP_CMD_REPORT_TSN, SCTP_U32(tsn));
@@ -6144,8 +6155,7 @@ static int sctp_eat_data(const struct
sctp_association *asoc,
sizeof(data_hdr->stream),
sizeof(u16));
if (err)
- sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
- SCTP_CHUNK(err));
+ list_add_tail(&err->list, &q->bad_stream_err);
return SCTP_IERROR_BAD_STREAM;
}
^ permalink raw reply related
* Re: [BUG] igb: panic at boot time with latest Linus tree
From: Jeff Kirsher @ 2012-07-31 6:26 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1343715255.21269.47.camel@edumazet-glaptop>
[-- Attachment #1: Type: text/plain, Size: 7783 bytes --]
On Tue, 2012-07-31 at 08:14 +0200, Eric Dumazet wrote:
> For information, I get this each time I boot on latest Linus tree :
>
> RTNL is left locked, so machine unusable.
>
> No problem with David net tree, so thats a bit strange...
>
> Not sure I'll have time to investigate today
> (And tomorrow I am off for the day)
>
> [ 11.153682] BUG: unable to handle kernel NULL pointer dereference at (null)
> [ 11.153806] IP: [< (null)>] (null)
> [ 11.153872] PGD 310544067 PUD 311b2d067 PMD 0
> [ 11.153945] Oops: 0010 [#1] SMP
> [ 11.154012] Modules linked in: nfsd exportfs nfs lockd auth_rpcgss sunrpc asix usbnet rt61pci crc_itu_t rt2x00pci rt2x00lib eeprom_93cx6 tg3 ixgbe mdio igb
> [ 11.154227] CPU 10
> [ 11.154239] Pid: 2476, comm: NetworkManager Not tainted 3.5.0+ #123 Hewlett-Packard HP Z600 Workstation/0B54h
> [ 11.154437] RIP: 0010:[<0000000000000000>] [< (null)>] (null)
> [ 11.154574] RSP: 0018:ffff88030f667630 EFLAGS: 00010282
> [ 11.154645] RAX: 0000000017cf7980 RBX: ffff88031080f100 RCX: 0000000000000200
> [ 11.154720] RDX: 0000000000000040 RSI: ffffea0017cf7980 RDI: ffff880611bc1098
> [ 11.154794] RBP: ffff88030f667678 R08: 0000000000000002 R09: 0000000000000000
> [ 11.154868] R10: ffffea0017cf7980 R11: 0000000000000000 R12: ffffc90007d58000
> [ 11.154942] R13: ffff880611bc1098 R14: ffff8803106ff000 R15: ffff8805f3de6040
> [ 11.155016] FS: 00007fa2e4b94800(0000) GS:ffff88061fc80000(0000) knlGS:0000000000000000
> [ 11.155148] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 11.155220] CR2: 0000000000000000 CR3: 000000031066a000 CR4: 00000000000007e0
> [ 11.155283] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 11.155347] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [ 11.155411] Process NetworkManager (pid: 2476, threadinfo ffff88030f666000, task ffff880310dd2d00)
> [ 11.155523] Stack:
> [ 11.155579] ffffffffa00071fe 0000000000000000 00ffff00a000400a ffff88030f667678
> [ 11.155712] ffff88030f610700 0000000000000001 ffff88030f610708 ffff88030f610740
> [ 11.155840] 0000000000000001 ffff88030f6676b8 ffffffffa000834b ffff88030f610700
> [ 11.155969] Call Trace:
> [ 11.156036] [<ffffffffa00071fe>] ? igb_alloc_rx_buffers+0x13e/0x2d0 [igb]
> [ 11.156104] [<ffffffffa000834b>] igb_configure+0x34b/0x4d0 [igb]
> [ 11.156170] [<ffffffffa0008572>] __igb_open+0xa2/0x510 [igb]
> [ 11.156237] [<ffffffff812c0731>] ? find_next_bit+0x21/0xd0
> [ 11.156303] [<ffffffffa0008b50>] igb_open+0x10/0x20 [igb]
> [ 11.156369] [<ffffffff8155288f>] __dev_open+0x8f/0xe0
> [ 11.156432] [<ffffffff81552b31>] __dev_change_flags+0xa1/0x180
> [ 11.156495] [<ffffffff81552cc8>] dev_change_flags+0x28/0x70
> [ 11.156559] [<ffffffff8155f664>] do_setlink+0x284/0x9e0
> [ 11.156624] [<ffffffff81560c8a>] ? rtnl_fill_ifinfo+0x92a/0xb30
> [ 11.156690] [<ffffffff812cc220>] ? nla_parse+0x30/0xe0
> [ 11.156755] [<ffffffff81561b35>] rtnl_newlink+0x345/0x580
> [ 11.156820] [<ffffffff812655b9>] ? selinux_capable+0x39/0x50
> [ 11.156885] [<ffffffff81262538>] ? security_capable+0x18/0x20
> [ 11.156948] [<ffffffff81561384>] rtnetlink_rcv_msg+0x124/0x310
> [ 11.157012] [<ffffffff8113794b>] ? kfree+0x3b/0x160
> [ 11.157074] [<ffffffff81561260>] ? __rtnl_unlock+0x20/0x20
> [ 11.157137] [<ffffffff8157a569>] netlink_rcv_skb+0xa9/0xd0
> [ 11.157200] [<ffffffff8155ed55>] rtnetlink_rcv+0x25/0x40
> [ 11.157262] [<ffffffff81579f4d>] netlink_unicast+0x1ad/0x230
> [ 11.157326] [<ffffffff8157a286>] netlink_sendmsg+0x2b6/0x310
> [ 11.157396] [<ffffffff815381ac>] sock_sendmsg+0xdc/0xf0
> [ 11.157459] [<ffffffff8153aab8>] ? move_addr_to_kernel+0x68/0xb0
> [ 11.157526] [<ffffffff81539b12>] __sys_sendmsg+0x392/0x3a0
> [ 11.157590] [<ffffffff81118bbe>] ? handle_mm_fault+0x13e/0x210
> [ 11.157656] [<ffffffff81155008>] ? __d_free+0x48/0x70
> [ 11.157720] [<ffffffff8115e616>] ? mntput+0x26/0x40
> [ 11.157783] [<ffffffff81140371>] ? __fput+0x191/0x250
> [ 11.157846] [<ffffffff8153b809>] sys_sendmsg+0x49/0x90
> [ 11.157911] [<ffffffff816ca652>] system_call_fastpath+0x16/0x1b
> [ 11.157973] Code: Bad RIP value.
> [ 11.158041] RIP [< (null)>] (null)
> [ 11.158106] RSP <ffff88030f667630>
> [ 11.158163] CR2: 0000000000000000
> [ 11.158227] ---[ end trace bbfaed088efd61cb ]---
> [ 11.158300] NetworkManager (2476) used greatest stack depth: 2936 bytes left
>
>
I have an igb patch current in test to fix a panic in igb. Here is the
patch:
From: Emil Tantilov <emil.s.tantilov@intel.com>
Subject: igb: fix panic while dumping packets on Tx hang with IOMMU
This patch resolves a "BUG: unable to handle kernel paging request
at ..." oops while dumping packet data. The issue occurs with IOMMU
enabled due to the address provided by phys_to_virt().
This patch avoids phys_to_virt() by making using skb->data and the
address of the pages allocated for Rx.
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 19 +++++++++----------
1 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
b/drivers/net/ethernet/intel/igb/igb_main.c
index 447e131..8d7e5da 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -462,10 +462,10 @@ static void igb_dump(struct igb_adapter *adapter)
(u64)buffer_info->time_stamp,
buffer_info->skb, next_desc);
- if (netif_msg_pktdata(adapter) &&
buffer_info->dma != 0)
+ if (netif_msg_pktdata(adapter) &&
buffer_info->skb)
print_hex_dump(KERN_INFO, "",
DUMP_PREFIX_ADDRESS,
- 16, 1,
phys_to_virt(buffer_info->dma),
+ 16, 1, buffer_info->skb->data,
buffer_info->length, true);
}
}
@@ -547,18 +547,17 @@ rx_ring_summary:
(u64)buffer_info->dma,
buffer_info->skb, next_desc);
- if (netif_msg_pktdata(adapter)) {
+ if (netif_msg_pktdata(adapter) &&
+ buffer_info->dma &&
buffer_info->skb) {
print_hex_dump(KERN_INFO, "",
- DUMP_PREFIX_ADDRESS,
- 16, 1,
-
phys_to_virt(buffer_info->dma),
- IGB_RX_HDR_LEN, true);
+ DUMP_PREFIX_ADDRESS,
+ 16, 1,
buffer_info->skb->data,
+ IGB_RX_HDR_LEN, true);
print_hex_dump(KERN_INFO, "",
DUMP_PREFIX_ADDRESS,
16, 1,
- phys_to_virt(
- buffer_info->page_dma +
- buffer_info->page_offset),
+
page_address(buffer_info->page) +
+
buffer_info->page_offset,
PAGE_SIZE/2, true);
}
}
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
From: xufeng zhang @ 2012-07-31 6:51 UTC (permalink / raw)
To: Vlad Yasevich
Cc: Xufeng Zhang, Neil Horman, sri, davem, linux-sctp, netdev,
linux-kernel
In-Reply-To: <CA+=dFzhXY9aJ6_Yu-4a3g+6RyN5_mxXY=U3HnzKdi8TH-rqWcA@mail.gmail.com>
Sorry, please ignore the above patch, there was an paste error.
Please check the following patch.
============================================
I'm wondering if the below solution is fine to you which is based on
your changes.
BTW, I have verified this patch and it works ok for all the situation,
but only one problem persists:
there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS
which happens during sending lots of small error DATA chunks.
Thanks,
Xufeng Zhang
---
include/net/sctp/command.h | 1 +
include/net/sctp/structs.h | 3 +++
net/sctp/outqueue.c | 7 +++++++
net/sctp/sm_sideeffect.c | 16 ++++++++++++++++
net/sctp/sm_statefuns.c | 17 ++++++++++++++---
5 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..62c34f5 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h
@@ -110,6 +110,7 @@ typedef enum {
SCTP_CMD_SEND_NEXT_ASCONF, /* Send the next ASCONF after ACK */
SCTP_CMD_PURGE_ASCONF_QUEUE, /* Purge all asconf queues.*/
SCTP_CMD_SET_ASOC, /* Restore association context */
+ SCTP_CMD_GEN_BAD_STREAM, /* Invalid Stream errors happened command */
SCTP_CMD_LAST
} sctp_verb_t;
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index fc5e600..3d218e0 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1183,6 +1183,9 @@ struct sctp_outq {
*/
struct list_head abandoned;
+ /* Put Invalid Stream error chunks on this list */
+ struct list_head bad_stream_err;
+
/* How many unackd bytes do we have in-flight? */
__u32 outstanding_bytes;
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index e7aa177..1e87b0b 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q)
INIT_LIST_HEAD(&q->retransmit);
INIT_LIST_HEAD(&q->sacked);
INIT_LIST_HEAD(&q->abandoned);
+ INIT_LIST_HEAD(&q->bad_stream_err);
q->fast_rtx = 0;
q->outstanding_bytes = 0;
@@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q)
list_del_init(&chunk->list);
sctp_chunk_free(chunk);
}
+
+ /* Throw away any pending Invalid Stream error chunks */
+ list_for_each_entry_safe(chunk, tmp,&q->bad_stream_err, list) {
+ list_del_init(&chunk->list);
+ sctp_chunk_free(chunk);
+ }
}
/* Free the outqueue structure and any related pending chunks. */
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index fe99628..4698593 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -1060,6 +1060,18 @@ static void sctp_cmd_send_asconf(struct sctp_association *asoc)
}
}
+static void sctp_cmd_make_inv_stream_err(sctp_cmd_seq_t *commands,
+ struct sctp_association *asoc)
+{
+ struct sctp_chunk *err, *tmp;
+ struct sctp_outq *q =&asoc->outqueue;
+
+ list_for_each_entry_safe(err, tmp,&q->bad_stream_err, list) {
+ list_del_init(&err->list);
+ sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
+ SCTP_CHUNK(err));
+ }
+}
/* These three macros allow us to pull the debugging code out of the
* main flow of sctp_do_sm() to keep attention focused on the real
@@ -1724,6 +1736,10 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
asoc = cmd->obj.asoc;
break;
+ case SCTP_CMD_GEN_BAD_STREAM:
+ sctp_cmd_make_inv_stream_err(commands, asoc);
+ break;
+
default:
pr_warn("Impossible command: %u, %p\n",
cmd->verb, cmd->obj.ptr);
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 9fca103..1c1bcd9 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -2967,8 +2967,14 @@ discard_force:
return SCTP_DISPOSITION_DISCARD;
discard_noforce:
- if (chunk->end_of_packet)
+ if (chunk->end_of_packet) {
+ struct sctp_outq *q =&asoc->outqueue;
sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force);
+ /* Queue the INVALID STREAM error after the SACK if one is needed. */
+ if (!list_empty(&q->bad_stream_err))
+ sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM,
+ SCTP_NULL());
+ }
return SCTP_DISPOSITION_DISCARD;
consume:
@@ -3037,11 +3043,16 @@ sctp_disposition_t sctp_sf_eat_data_fast_4_4(const struct sctp_endpoint *ep,
* with a SACK, a SHUTDOWN chunk, and restart the T2-shutdown timer
*/
if (chunk->end_of_packet) {
+ struct sctp_outq *q =&asoc->outqueue;
/* We must delay the chunk creation since the cumulative
* TSN has not been updated yet.
*/
sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SHUTDOWN, SCTP_NULL());
sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE());
+ /* Queue the INVALID STREAM error after the SACK if one is needed. */
+ if (!list_empty(&q->bad_stream_err))
+ sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM,
+ SCTP_NULL());
sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
}
@@ -6136,6 +6147,7 @@ static int sctp_eat_data(const struct sctp_association *asoc,
*/
sid = ntohs(data_hdr->stream);
if (sid>= asoc->c.sinit_max_instreams) {
+ struct sctp_outq *q =&asoc->outqueue;
/* Mark tsn as received even though we drop it */
sctp_add_cmd_sf(commands, SCTP_CMD_REPORT_TSN, SCTP_U32(tsn));
@@ -6144,8 +6156,7 @@ static int sctp_eat_data(const struct sctp_association *asoc,
sizeof(data_hdr->stream),
sizeof(u16));
if (err)
- sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
- SCTP_CHUNK(err));
+ list_add_tail(&err->list,&q->bad_stream_err);
return SCTP_IERROR_BAD_STREAM;
}
--
1.7.0.2
^ permalink raw reply related
* [PATCH] [XFRM] Fix unexpected SA hard expiration after changing date
From: Fan Du @ 2012-07-31 7:43 UTC (permalink / raw)
To: davem, herbert; +Cc: netdev
In-Reply-To: <1343720634-1176-1-git-send-email-fdu@windriver.com>
After SA is setup, one timer is armed to detect soft/hard expiration,
however the timer handler uses xtime to do the math. This makes hard
expiration occurs first before soft expiration after setting new date
with big interval. As a result new child SA is deleted before rekeying
the new one.
Signed-off-by: Fan Du <fdu@windriver.com>
---
include/net/xfrm.h | 4 ++++
net/xfrm/xfrm_state.c | 22 ++++++++++++++++++----
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index d9509eb..62b619e 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -213,6 +213,9 @@ struct xfrm_state {
struct xfrm_lifetime_cur curlft;
struct tasklet_hrtimer mtimer;
+ /* used to fix curlft->add_time when changing date */
+ long saved_tmo;
+
/* Last used time */
unsigned long lastused;
@@ -238,6 +241,7 @@ static inline struct net *xs_net(struct xfrm_state *x)
/* xflags - make enum if more show up */
#define XFRM_TIME_DEFER 1
+#define XFRM_SOFT_EXPIRE 2
enum {
XFRM_STATE_VOID,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 5b228f9..fb64dc6 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -415,8 +415,17 @@ static enum hrtimer_restart xfrm_timer_handler(struct hrtimer * me)
if (x->lft.hard_add_expires_seconds) {
long tmo = x->lft.hard_add_expires_seconds +
x->curlft.add_time - now;
- if (tmo <= 0)
- goto expired;
+ if (tmo <= 0) {
+ if (x->xflags & XFRM_SOFT_EXPIRE) {
+ /* enter hard expire without soft expire first?!
+ * setting a new date could trigger this.
+ * workarbound: fix x->curflt.add_time by below:
+ */
+ x->curlft.add_time = now - x->saved_tmo - 1;
+ tmo = x->lft.hard_add_expires_seconds - x->saved_tmo;
+ } else
+ goto expired;
+ }
if (tmo < next)
next = tmo;
}
@@ -433,10 +443,14 @@ static enum hrtimer_restart xfrm_timer_handler(struct hrtimer * me)
if (x->lft.soft_add_expires_seconds) {
long tmo = x->lft.soft_add_expires_seconds +
x->curlft.add_time - now;
- if (tmo <= 0)
+ if (tmo <= 0) {
warn = 1;
- else if (tmo < next)
+ x->xflags &= ~XFRM_SOFT_EXPIRE;
+ } else if (tmo < next) {
next = tmo;
+ x->xflags |= XFRM_SOFT_EXPIRE;
+ x->saved_tmo = tmo;
+ }
}
if (x->lft.soft_use_expires_seconds) {
long tmo = x->lft.soft_use_expires_seconds +
--
1.7.1
^ permalink raw reply related
* [XFRM][PATCH v5] Fix unexpected SA hard expiration after setting new date
From: Fan Du @ 2012-07-31 7:43 UTC (permalink / raw)
To: davem, herbert; +Cc: netdev
Hi, Dave
Hope v5 is better than previous ones :)
Any comments are really welcome!
Thanks
Changelog:
v1->v2
1) use xflags instead of creating new flags(suggested by Steffen Klassert)
v2->v3
1) fix email problem, and remove cc to myself(requested by David Miller)
v3->v4
1) fix typo when clearing XFRM_SOFT_EXPIRE(thanks for David Miller)
2) fix email problem, and remove cc to myself AGAIN!!!
v4->v5
1) remove unnecessary empty line (David Miller)
*Background*:
Once IPsec SAs are created between two peers, kernel setup a timer to monitor
two events: soft/hard expiration. However the timer handler use xtime to
caculate whether it's soft or hard expiration event.
normal code flow(hard expire time:100s, soft expire time:82s)
a) When new SAs created, xfrm_timer_handler is called one second
after its creation. At this point, calculate soft expire
interval(81s), setup the timer;
b) soft expire occur, rearm the timer with hard expire interval(18s)
then notify racoon2 about soft expire event. racoon2 will create
new SAs.
c) hard expire happen, notify racoon2 about it. racoon2 will delete
the old SAs.
*Scenario*:
Setting a new date before b),and after a) could result c) happens first,
As a result, old SAs is deleted before new ones are created. Normally
new SAs will be created by the next time networking traffic, but there
is a small time being when networking connection is down, this could
result in upper layer connections failed in tel comm area, thus it's
better to keep it strict in sequence.
*Workaround*:
set new time could happen:
1) before a), then SAs is updated with new time.
2) before b),and after a)
2a) When new SAs created, xfrm_timer_handler is called one second
after its creation. At this point, calculate soft expire
interval(81s), setup the timer;(set flag to mark next time should
be soft time expire)
<<---- new date comes
2b) soft expire occur, the calculation results in a hard time expire
event, but flag is set, so catch ya. Sync the addtime, and rearm
the timer with hard expire interval(18s), then notify racoon2
about soft expire event;
2c) hard expire happen, notify racoon2 about it;
so everything is in order.
3) after b), hard expire always happened anyway.
^ permalink raw reply
* Re: [BUG] igb: panic at boot time with latest Linus tree
From: Eric Dumazet @ 2012-07-31 8:42 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: David Miller, netdev
In-Reply-To: <1343715971.2230.4.camel@jtkirshe-mobl>
On Mon, 2012-07-30 at 23:26 -0700, Jeff Kirsher wrote:
> On Tue, 2012-07-31 at 08:14 +0200, Eric Dumazet wrote:
> > For information, I get this each time I boot on latest Linus tree :
> >
> > RTNL is left locked, so machine unusable.
> >
> > No problem with David net tree, so thats a bit strange...
> >
> > Not sure I'll have time to investigate today
> > (And tomorrow I am off for the day)
> >
> >
> >
>
> I have an igb patch current in test to fix a panic in igb. Here is the
> patch:
>
Ah sorry for the false alarm, it appears I had igb module in a out of date initrd file (coming from my net tree)
^ permalink raw reply
* Re: [BUG] igb: panic at boot time with latest Linus tree
From: Jeff Kirsher @ 2012-07-31 8:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1343724174.21269.168.camel@edumazet-glaptop>
[-- Attachment #1: Type: text/plain, Size: 889 bytes --]
On Tue, 2012-07-31 at 10:42 +0200, Eric Dumazet wrote:
> On Mon, 2012-07-30 at 23:26 -0700, Jeff Kirsher wrote:
> > On Tue, 2012-07-31 at 08:14 +0200, Eric Dumazet wrote:
> > > For information, I get this each time I boot on latest Linus tree :
> > >
> > > RTNL is left locked, so machine unusable.
> > >
> > > No problem with David net tree, so thats a bit strange...
> > >
> > > Not sure I'll have time to investigate today
> > > (And tomorrow I am off for the day)
> > >
> > >
> > >
> >
> > I have an igb patch current in test to fix a panic in igb. Here is the
> > patch:
> >
>
>
>
> Ah sorry for the false alarm, it appears I had igb module in a out of date initrd file (coming from my net tree)
No problem, thanks for the feedback. Whether or not it was false alarm,
the feedback is always a good thing which helps us improve.
Cheers,
Jeff
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] net: Allow to create links with given ifindex
From: Pavel Emelyanov @ 2012-07-31 9:03 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Linux Netdev List, David Miller
In-Reply-To: <87fw89h5zk.fsf@xmission.com>
On 07/30/2012 02:56 PM, Eric W. Biederman wrote:
> ebiederm@xmission.com (Eric W. Biederman) writes:
>
>> Pavel Emelyanov <xemul@parallels.com> writes:
>>
>>> Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
>>> is not zero. I propose to allow requesting ifindices on link creation. This
>>> is required by the checkpoint-restore to correctly restore a net namespace
>>> (i.e. -- a container). The question what to do with pre-created devices such
>>> as lo or sit fbdev is open, but for manually created devices this can be
>>> solved by this patch.
>>
>> Have you walked through and found the locations where we still rely on
>> ifindex being globally unique?
>>
>> Last time I was working in this area there were serveral places where
>> things were indexed by just the interface index.
>
> If it is really safe to make ifindex per network namespace at this
> point you can make dev_new_ifindex have a per network namespace base
> counter, and that will fix your problems with the loopback device.
Not it's not so unfortunately :(
First, let's imagine that on host A the loopback device got registered as
first device, but on host B for some reason some other device got registered
first. In that case after migration from A to B the lo on B will have index
equals 2. And there's no any strict requirement that lo's per net operations
are registered first. Please, correct me if I'm wrong.
Next. In fact, lo is not the only problem. Look at the e.g. sit versus ipgre
fallback devices. Both gets created on netns creation and obtain whatever
ifindices are generated for them. Even if we make ifidex per netns chances
that sit gets registered _strictly_ before ipgre equal zero, since they are
both modules.
> Unless you have done the work to root out the last of dependencies on
> ifindex being globally unique I think you will run into some operational
> problems.
I totally agree with that. Before doing this patch I revisited the ancient
attempt to make ifindices per netns and checked the issues Dave and you
discussed then -- I have looked through how the ifindices are used in the
networking code and found no places where the system-wide uniqueness is still
required. That's why I proposed this patch for inclusion. If you know the
places I've missed, please let me know, I will work on it.
> Eric
>
> .
>
Thanks,
Pavel
^ permalink raw reply
* Re: [PATCH 1/2] net: Allow to create links with given ifindex
From: Pavel Emelyanov @ 2012-07-31 9:06 UTC (permalink / raw)
To: Eric W. Biederman, Eric Dumazet; +Cc: Linux Netdev List, David Miller
In-Reply-To: <87k3xlfmym.fsf@xmission.com>
> I'm not seeing anything obvious in the network stack with a quick skim,
> but before we start relying on the property that interface indicies are
> not globally unique I expect an good hard look at the networking stack
> to see if any of those cases where there were problems still exist.
Just an idea -- is it worth moving the possibility to have ifindidces intersect
under CONFIG_<SOMETHING> (EXPERT/CHECKPOINT_RESTORE) to let wider audience check
the code in real-life?
> Eric
>
> .
>
Thanks,
Pavel
^ permalink raw reply
* Re: [PATCH] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Marc Kleine-Budde @ 2012-07-31 9:56 UTC (permalink / raw)
To: Olivier Sobrie; +Cc: Wolfgang Grandegger, linux-can, netdev
In-Reply-To: <20120730133323.GA13941@hposo>
[-- Attachment #1: Type: text/plain, Size: 51547 bytes --]
On 07/30/2012 03:33 PM, Olivier Sobrie wrote:
> Hi Marc,
>
> On Mon, Jul 30, 2012 at 01:11:46PM +0200, Marc Kleine-Budde wrote:
>> On 07/30/2012 07:32 AM, Olivier Sobrie wrote:
>>> This driver provides support for several Kvaser CAN/USB devices.
>>> Such kind of devices supports up to three can network interfaces.
>>>
>>> It has been tested with a Kvaser USB Leaf Light (one network interface)
>>> connected to a pch_can interface.
>>> The firmware version of the Kvaser device was 2.5.205.
>>
>> Please add linux-usb@vger.kernel.org to Cc for review of the USB part.
>
> Ok I'll do it when I send the new version of the patch.
> But it might be a good idea to add an entry in the MAINTAINERS file so
> that when someone sends a patch they are aware of this when the
> get_maintainer script is invoked.
Interesting Idea. We should discuss this here, however we should not
bother the USB List when sending USB unrelated patches.
>> Please combine .h and .c file. Please make use of netdev_LEVEL() for
>> error printing, not dev_LEVEL().
>
> I'll combine the .c and .h.
> I used the netdev_LEVEL() everywhere it was possible. It requires to
> have access to a pointer to netdev which is not always possible;
> that's the reason why I used dev_LEVEL().
I see, you used it when channel is invalid. So you have obviously no netdev.
>> Please review if all members of the struct kvaser_msg are properly
>> aligned. You never access the struct kvaser_msg_* members directly, as
>> they are unaligned. Please check for le16 and le32 access. You missed to
>> convert the bitrate.
>
> Indeed. Thanks. I'll check if I didn't missed another one.
Tnx
>> Please check if your driver survives hot-unplugging while sending and
>> receiving CAN frames at maximum laod.
>
> I tested this with two Kvaser sending frames with "cangen can0 -g 0 -i"
> never saw a crash.
Please test send sending and receiving at the same time.
>> More comments inline,
>> regards, Marc
>>
>>> List of Kvaser devices supported by the driver:
>>> - Kvaser Leaf prototype (P010v2 and v3)
>>> - Kvaser Leaf Light (P010v3)
>>> - Kvaser Leaf Professional HS
>>> - Kvaser Leaf SemiPro HS
>>> - Kvaser Leaf Professional LS
>>> - Kvaser Leaf Professional SWC
>>> - Kvaser Leaf Professional LIN
>>> - Kvaser Leaf SemiPro LS
>>> - Kvaser Leaf SemiPro SWC
>>> - Kvaser Memorator II, Prototype
>>> - Kvaser Memorator II HS/HS
>>> - Kvaser USBcan Professional HS/HS
>>> - Kvaser Leaf Light GI
>>> - Kvaser Leaf Professional HS (OBD-II connector)
>>> - Kvaser Memorator Professional HS/LS
>>> - Kvaser Leaf Light "China"
>>> - Kvaser BlackBird SemiPro
>>> - Kvaser OEM Mercury
>>> - Kvaser OEM Leaf
>>> - Kvaser USBcan R
>>>
>>> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
>>> ---
>>> drivers/net/can/usb/Kconfig | 33 ++
>>> drivers/net/can/usb/Makefile | 1 +
>>> drivers/net/can/usb/kvaser_usb.c | 1062 ++++++++++++++++++++++++++++++++++++++
>>> drivers/net/can/usb/kvaser_usb.h | 237 +++++++++
>>> 4 files changed, 1333 insertions(+)
>>> create mode 100644 drivers/net/can/usb/kvaser_usb.c
>>> create mode 100644 drivers/net/can/usb/kvaser_usb.h
>>>
>>> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
>>> index 0a68768..578955f 100644
>>> --- a/drivers/net/can/usb/Kconfig
>>> +++ b/drivers/net/can/usb/Kconfig
>>> @@ -13,6 +13,39 @@ config CAN_ESD_USB2
>>> This driver supports the CAN-USB/2 interface
>>> from esd electronic system design gmbh (http://www.esd.eu).
>>>
>>> +config CAN_KVASER_USB
>>> + tristate "Kvaser CAN/USB interface"
>>> + ---help---
>>> + This driver adds support for Kvaser CAN/USB devices like Kvaser
>>> + Leaf Light.
>>> +
>>> + The driver gives support for the following devices:
>>> + - Kvaser Leaf prototype (P010v2 and v3)
>>> + - Kvaser Leaf Light (P010v3)
>>> + - Kvaser Leaf Professional HS
>>> + - Kvaser Leaf SemiPro HS
>>> + - Kvaser Leaf Professional LS
>>> + - Kvaser Leaf Professional SWC
>>> + - Kvaser Leaf Professional LIN
>>> + - Kvaser Leaf SemiPro LS
>>> + - Kvaser Leaf SemiPro SWC
>>> + - Kvaser Memorator II, Prototype
>>> + - Kvaser Memorator II HS/HS
>>> + - Kvaser USBcan Professional HS/HS
>>> + - Kvaser Leaf Light GI
>>> + - Kvaser Leaf Professional HS (OBD-II connector)
>>> + - Kvaser Memorator Professional HS/LS
>>> + - Kvaser Leaf Light "China"
>>> + - Kvaser BlackBird SemiPro
>>> + - Kvaser OEM Mercury
>>> + - Kvaser OEM Leaf
>>> + - Kvaser USBcan R
>>> +
>>> + If unsure, say N.
>>> +
>>> + To compile this driver as a module, choose M here: the
>>> + module will be called kvaser_usb.
>>> +
>>> config CAN_PEAK_USB
>>> tristate "PEAK PCAN-USB/USB Pro interfaces"
>>> ---help---
>>> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
>>> index da6d1d3..80a2ee4 100644
>>> --- a/drivers/net/can/usb/Makefile
>>> +++ b/drivers/net/can/usb/Makefile
>>> @@ -4,6 +4,7 @@
>>>
>>> obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
>>> obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
>>> +obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o
>>> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
>>>
>>> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
>>> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
>>> new file mode 100644
>>> index 0000000..4965480
>>> --- /dev/null
>>> +++ b/drivers/net/can/usb/kvaser_usb.c
>>> @@ -0,0 +1,1062 @@
>>> +/*
>>
>> Please add a license statement and probably your copyright:
>>
>> * 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 version 2.
>>
>> You also should copy the copyright from the drivers you used:
>>
>>> + * Parts of this driver are based on the following:
>>> + * - Kvaser linux leaf driver (version 4.78)
>>
>> I just downloaded their driver and noticed that it's quite sparse on
>> stating the license the code is released under.
>> "doc/HTMLhelp/copyright.htx" is quite restrictive, the word GPL occurs 3
>> times, all in MODULE_LICENSE("GPL"). Running modinfo on the usbcan.ko
>> shows "license: GPL"
>
> I'll add the license statement.
> In fact it's the leaf.ko which is used for this device and it is under
> GPL as modinfo said.
I just talked to my boss and we're the same opinion, that
MODULE_LICENSE("GPL") is a technical term and not relevant if the
included license doesn't say a word about GPL. If the kvaser tarball
violates the GPL, however is written on different sheet of paper (as we
say in Germany).
So I cannot put my S-o-b under this driver as long as we haven't talked
to kvaser.
>>> + * - CAN driver for esd CAN-USB/2
>>> + */
>>> +
>>> +#include <linux/init.h>
>>> +#include <linux/completion.h>
>>> +#include <linux/module.h>
>>> +#include <linux/netdevice.h>
>>> +#include <linux/usb.h>
>>> +
>>> +#include <linux/can.h>
>>> +#include <linux/can/dev.h>
>>> +#include <linux/can/error.h>
>>> +
>>> +#include "kvaser_usb.h"
>>> +
>>> +struct kvaser_usb_tx_urb_context {
>>> + struct kvaser_usb_net_priv *priv;
>>
>> Huh - how does this work without forward declaration?
>
> It works.
Yes, obviously :)
> "In C and C++ it is possible to declare pointers to structs before
> declaring their struct layout, provided the pointers are not
> dereferenced--this is known as forward declaration."
>
> See http://www.linuxtopia.org/online_books/an_introduction_to_gcc/gccintro_94.html
Thanks for the link.
>>
>>> + u32 echo_index;
>>> + int dlc;
>>> +};
>>> +
>>> +struct kvaser_usb {
>>> + struct usb_device *udev;
>>> + struct kvaser_usb_net_priv *nets[MAX_NET_DEVICES];
>>> +
>>> + struct usb_anchor rx_submitted;
>>> +
>>> + u32 fw_version;
>>> + unsigned int nchannels;
>>> +
>>> + bool rxinitdone;
>>> +};
>>> +
>>> +struct kvaser_usb_net_priv {
>>> + struct can_priv can;
>>> +
>>> + atomic_t active_tx_urbs;
>>> + struct usb_anchor tx_submitted;
>>> + struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
>>> +
>>> + int open_time;
>>
>> please remove open_time
>
> Ok.
>
>>
>>> + struct completion start_stop_comp;
>>> +
>>> + struct kvaser_usb *dev;
>>> + struct net_device *netdev;
>>> + int channel;
>>> + struct can_berr_counter bec;
>>> +};
>>> +
>>> +static struct usb_device_id kvaser_usb_table[] = {
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_DEVEL_PRODUCT_ID) },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_PRODUCT_ID) },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LS_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_SWC_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LIN_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_LS_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_SWC_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_DEVEL_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSHS_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_UPRO_HSHS_PRODUCT_ID) },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_GI_PRODUCT_ID) },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_OBDII_PRODUCT_ID),
>>> + .driver_info = KVASER_HAS_SILENT_MODE },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSLS_PRODUCT_ID) },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_CH_PRODUCT_ID) },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_BLACKBIRD_SPRO_PRODUCT_ID) },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_MERCURY_PRODUCT_ID) },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_LEAF_PRODUCT_ID) },
>>> + { USB_DEVICE(KVASER_VENDOR_ID, USB_CAN_R_PRODUCT_ID) },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
>>> +
>>> +static int kvaser_usb_send_msg(const struct kvaser_usb *dev,
>>> + struct kvaser_msg *msg)
>> inline?
>
> Ok.
>
>>> +{
>>> + int actual_len;
>>> +
>>> + return usb_bulk_msg(dev->udev, usb_sndbulkpipe(dev->udev, 1),
>> ^
>> Can you please introduce a #define for this.
>
> Ok. No problem.
>
>>
>>> + msg, msg->len, &actual_len, USB_SEND_TIMEOUT);
>>> +}
>>> +
>>> +static int kvaser_usb_wait_msg(const struct kvaser_usb *dev, u8 id,
>>> + struct kvaser_msg *msg)
>>> +{
>>> + struct kvaser_msg *tmp;
>>> + void *buf;
>>> + int actual_len;
>>> + int err;
>>> + int pos = 0;
>>> +
>>> + buf = kzalloc(RX_BUFFER_SIZE, GFP_KERNEL);
>>> + if (!buf)
>>> + return -ENOMEM;
>>> +
>>> + err = usb_bulk_msg(dev->udev, usb_rcvbulkpipe(dev->udev, 129),
>> ^^^
>> dito
>
> Ok too.
>
>>
>>> + buf, RX_BUFFER_SIZE, &actual_len,
>>> + USB_RECEIVE_TIMEOUT);
>>> + if (err < 0) {
>>> + kfree(buf);
>>> + return err;
>>> + }
>>> +
>>> + while (pos < actual_len) {
>>
>> Please check that pos + sizeof(*msg) is < actual_len, as you fill access
>> it later.
>
> I'll instead perform a check on 'pos + tmp->len < actual_len' and copy
> only tmp->len instead of sizeof(*msg).
> Thanks.
Even better, saves some bytes to be copied. Take care not to deref tmp,
unless you checked that tmp is in valid memory.
>>
>>> + tmp = buf + pos;
>>> +
>>> + if (!tmp->len)
>>> + break;
>>> +
>>> + if (tmp->id == id) {
>>> + memcpy(msg, tmp, sizeof(*msg));
>>> + kfree(buf);
>>> + return 0;
>>> + }
>>> +
>>> + pos += tmp->len;
>>> + }
>>> +
>>> + kfree(buf);
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static int kvaser_usb_send_simple_msg(const struct kvaser_usb *dev,
>>> + u8 msg_id, int channel)
>>> +{
>>> + struct kvaser_msg msg;
>>> + int err;
>>> +
>>> + msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_simple);
>>> + msg.id = msg_id;
>>> + msg.u.simple.channel = channel;
>>> + msg.u.simple.tid = 0xff;
>>
>> Please use C99 struct initializer.
>>
>> struct kvaser_msg msg = {
>> .len = ,
>> .id =,
>> };
>
> Ok.
>
>>
>>
>>> +
>>> + err = kvaser_usb_send_msg(dev, &msg);
>>
>> just:
>> return err;
>
> Ok.
>
>>
>>> + if (err)
>>> + return err;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
>>> +{
>>> + struct kvaser_msg msg;
>>> + int err;
>>> +
>>> + err = kvaser_usb_send_simple_msg(dev, CMD_GET_SOFTWARE_INFO, 0);
>>> + if (err)
>>> + return err;
>>> +
>>> + err = kvaser_usb_wait_msg(dev, CMD_GET_SOFTWARE_INFO_REPLY, &msg);
>>> + if (err)
>>> + return err;
>>> +
>>> + dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int kvaser_usb_get_card_info(struct kvaser_usb *dev)
>>> +{
>>> + struct kvaser_msg msg;
>>> + int err;
>>> +
>>> + err = kvaser_usb_send_simple_msg(dev, CMD_GET_CARD_INFO, 0);
>>> + if (err)
>>> + return err;
>>> +
>>> + err = kvaser_usb_wait_msg(dev, CMD_GET_CARD_INFO_REPLY, &msg);
>>> + if (err)
>>> + return err;
>>> +
>>> + dev->nchannels = msg.u.cardinfo.nchannels;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
>>> + const struct kvaser_msg *msg)
>>> +{
>>> + struct net_device_stats *stats;
>>> + struct kvaser_usb_tx_urb_context *context;
>>> + struct kvaser_usb_net_priv *priv;
>>> + u8 channel = msg->u.tx_acknowledge.channel;
>>> + u8 tid = msg->u.tx_acknowledge.tid;
>>> +
>>> + if (channel >= dev->nchannels) {
>>> + dev_err(dev->udev->dev.parent,
>>> + "Invalid channel number (%d)\n", channel);
>>> + return;
>>> + }
>>
>> can you do a check for (channel >= dev->nchannels), in a central place?
>> e.g. kvaser_usb_handle_message()?
>
> The problem is that channel is not always at the same place in the
> messages I get from the hardware. 'tid' and 'channel' are inverted for
> tx and rx frames.
> e.g.
Grr...who's written that firmware :D
>
> struct kvaser_msg_tx_can {
> u8 channel;
> u8 tid;
> u8 msg[14];
> u8 padding;
> u8 flags;
> } __packed;
>
> struct kvaser_msg_busparams {
> u8 tid;
> u8 channel;
> __le32 bitrate;
> u8 tseg1;
> u8 tseg2;
> u8 sjw;
> u8 no_samp;
> } __packed;
>
>>
>>> +
>>> + priv = dev->nets[channel];
>>> +
>>> + if (!netif_device_present(priv->netdev))
>>> + return;
>>> +
>>> + stats = &priv->netdev->stats;
>>> +
>>> + context = &priv->tx_contexts[tid % MAX_TX_URBS];
>>> +
>>> + /*
>>> + * It looks like the firmware never sets the flags field of the
>>> + * tx_acknowledge frame and never reports a transmit failure.
>>> + * If the can message can't be transmited (e.g. incompatible
>>> + * bitrates), a frame CMD_CAN_ERROR_EVENT is sent (with a null
>>> + * tid) and the firmware tries to transmit again the packet until
>>> + * it succeeds. Once the packet is successfully transmitted, then
>>> + * the tx_acknowledge frame is sent.
>>> + */
>>> +
>>> + stats->tx_packets++;
>>> + stats->tx_bytes += context->dlc;
>>> + can_get_echo_skb(priv->netdev, context->echo_index);
>>> +
>>> + context->echo_index = MAX_TX_URBS;
>>> + atomic_dec(&priv->active_tx_urbs);
>>> +
>>> + netif_wake_queue(priv->netdev);
>>> +}
>>> +
>>> +static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
>>> + const struct kvaser_msg *msg)
>>> +{
>>> + struct can_frame *cf;
>>> + struct sk_buff *skb;
>>> + struct net_device_stats *stats;
>>> + struct kvaser_usb_net_priv *priv;
>>> + u8 channel, status, txerr, rxerr;
>>> +
>>> + if (msg->id == CMD_CAN_ERROR_EVENT) {
>>> + channel = msg->u.error_event.channel;
>>> + status = msg->u.error_event.status;
>>> + txerr = msg->u.error_event.tx_errors_count;
>>> + rxerr = msg->u.error_event.rx_errors_count;
>>> + } else {
>>> + channel = msg->u.chip_state_event.channel;
>>> + status = msg->u.chip_state_event.status;
>>> + txerr = msg->u.chip_state_event.tx_errors_count;
>>> + rxerr = msg->u.chip_state_event.rx_errors_count;
>>> + }
>>> +
>>> + if (channel >= dev->nchannels) {
>>> + dev_err(dev->udev->dev.parent,
>>> + "Invalid channel number (%d)\n", channel);
>>> + return;
>>> + }
>>> +
>>> + priv = dev->nets[channel];
>>> + stats = &priv->netdev->stats;
>>> +
>>> + skb = alloc_can_err_skb(priv->netdev, &cf);
>>> + if (!skb) {
>>> + stats->rx_dropped++;
>>> + return;
>>> + }
>>> +
>>> + if ((status & M16C_STATE_BUS_OFF) ||
>>> + (status & M16C_STATE_BUS_RESET)) {
>>> + priv->can.state = CAN_STATE_BUS_OFF;
>>> + cf->can_id |= CAN_ERR_BUSOFF;
>>> + can_bus_off(priv->netdev);
you should increment priv->can.can_stats.bus_off
What does the firmware do in this state? Does it automatically try to
recover and try to send the outstanding frames?
If so, you should turn of the CAN interface, it possible. See:
http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L986
Please test Bus-Off behaviour:
- setup working CAN network
- short circuit CAN-H and CAN-L wires
- start "candump any,0:0,#FFFFFFFF" on one shell
- send one can frame on the other
then
- remove the short circuit
- see if the can frame is transmitted to the other side
- it should show up as an echo'ed CAN frame on the sender side
Repeat the same test with disconnecting CAN-H and CAN-L from the other
CAN station instead of short circuit.
Please send the output from candump.
>>> + } else if (status & M16C_STATE_BUS_ERROR) {
>>> + priv->can.state = CAN_STATE_ERROR_WARNING;
>>> + priv->can.can_stats.error_warning++;
>>> + } else if (status & M16C_STATE_BUS_PASSIVE) {
>>> + priv->can.state = CAN_STATE_ERROR_PASSIVE;
>>> + priv->can.can_stats.error_passive++;
>>> + } else {
>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> + cf->can_id |= CAN_ERR_PROT;
>>> + cf->data[2] = CAN_ERR_PROT_ACTIVE;
>>> + }
>>> +
>>> + if (msg->id == CMD_CAN_ERROR_EVENT) {
>>> + u8 error_factor = msg->u.error_event.error_factor;
>>> +
>>> + priv->can.can_stats.bus_error++;
>>> + stats->rx_errors++;
>>> +
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> +
>>> + if ((priv->can.state == CAN_STATE_ERROR_WARNING) ||
>>> + (priv->can.state == CAN_STATE_ERROR_PASSIVE)) {
>>> + cf->data[1] = (txerr > rxerr) ?
>>> + CAN_ERR_CRTL_TX_PASSIVE
>>> + : CAN_ERR_CRTL_RX_PASSIVE;
>>> + }
>>> +
>>> + if (error_factor & M16C_EF_ACKE)
>>> + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK |
>>> + CAN_ERR_PROT_LOC_ACK_DEL);
>>> + if (error_factor & M16C_EF_CRCE)
>>> + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
>>> + CAN_ERR_PROT_LOC_CRC_DEL);
>>> + if (error_factor & M16C_EF_FORME)
>>> + cf->data[2] |= CAN_ERR_PROT_FORM;
>>> + if (error_factor & M16C_EF_STFE)
>>> + cf->data[2] |= CAN_ERR_PROT_STUFF;
>>> + if (error_factor & M16C_EF_BITE0)
>>> + cf->data[2] |= CAN_ERR_PROT_BIT0;
>>> + if (error_factor & M16C_EF_BITE1)
>>> + cf->data[2] |= CAN_ERR_PROT_BIT1;
>>> + if (error_factor & M16C_EF_TRE)
>>> + cf->data[2] |= CAN_ERR_PROT_TX;
>>> + }
>>> +
>>> + cf->data[6] = txerr;
>>> + cf->data[7] = rxerr;
>>> +
>>> + netif_rx(skb);
>>> +
>>> + priv->bec.txerr = txerr;
>>> + priv->bec.rxerr = rxerr;
>>> +
>>> + stats->rx_packets++;
>>> + stats->rx_bytes += cf->can_dlc;
>>> +}
>>> +
>>> +static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
>>> + const struct kvaser_msg *msg)
>>> +{
>>> + struct kvaser_usb_net_priv *priv;
>>> + struct can_frame *cf;
>>> + struct sk_buff *skb;
>>> + struct net_device_stats *stats;
>>> + u8 channel = msg->u.rx_can.channel;
>>> +
>>> + if (channel >= dev->nchannels) {
>>> + dev_err(dev->udev->dev.parent,
>>> + "Invalid channel number (%d)\n", channel);
>>> + return;
>>> + }
>>> +
>>> + priv = dev->nets[channel];
>>> + stats = &priv->netdev->stats;
>>> +
>>> + skb = alloc_can_skb(priv->netdev, &cf);
>>> + if (skb == NULL) {
>>> + stats->tx_dropped++;
>>> + return;
>>> + }
>>> +
>>> + cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
>>> + (msg->u.rx_can.msg[1] & 0x3f);
>>> + cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
>>> +
>>> + if (msg->id == CMD_RX_EXT_MESSAGE) {
>>> + cf->can_id <<= 18;
>>> + cf->can_id += ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
>> |=
>>
>> is more appropriate here
>
> Ok.
>
>>
>>> + ((msg->u.rx_can.msg[3] & 0xff) << 6) |
>>> + (msg->u.rx_can.msg[4] & 0x3f);
>>> + cf->can_id |= CAN_EFF_FLAG;
>>> + }
>>> +
>>> + if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME) {
>>> + cf->can_id |= CAN_RTR_FLAG;
>>> + } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
>>> + MSG_FLAG_NERR)) {
>>> + cf->can_id |= CAN_ERR_FLAG;
>>> + cf->can_dlc = CAN_ERR_DLC;
>>
>> What kind of error is this? Can you set cf->data? What about the
>> original cd->can_id? What about the stats->rx_*error* stats?
>
> Good question I've to take a look to this.
>
>>
>>> + } else if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
>>> + cf->can_id = CAN_ERR_FLAG | CAN_ERR_CRTL;
>>> + cf->can_dlc = CAN_ERR_DLC;
>>> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>>> +
>>> + stats->rx_over_errors++;
>>> + stats->rx_errors++;
>>> + } else if (!msg->u.rx_can.flag) {
>>> + memcpy(cf->data, &msg->u.rx_can.msg[6], cf->can_dlc);
>>> + } else {
>>> + kfree_skb(skb);
>>> + return;
>>> + }
>>> +
>>> + netif_rx(skb);
>>> +
>>> + stats->rx_packets++;
>>> + stats->rx_bytes += cf->can_dlc;
>>> +}
>>> +
>>> +static void kvaser_usb_start_stop_chip_reply(const struct kvaser_usb *dev,
>>> + const struct kvaser_msg *msg)
>>> +{
>>> + struct kvaser_usb_net_priv *priv;
>>> + u8 channel = msg->u.simple.channel;
>>> +
>>> + if (channel >= dev->nchannels) {
>>> + dev_err(dev->udev->dev.parent,
>>> + "Invalid channel number (%d)\n", channel);
>>> + return;
>>> + }
>>> +
>>> + priv = dev->nets[channel];
>>> +
>>> + complete(&priv->start_stop_comp);
>>> +}
>>> +
>>> +static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
>>> + const struct kvaser_msg *msg)
>>> +{
>>> + switch (msg->id) {
>>> + case CMD_START_CHIP_REPLY:
>>> + case CMD_STOP_CHIP_REPLY:
>>> + kvaser_usb_start_stop_chip_reply(dev, msg);
>>> + break;
>>> +
>>> + case CMD_RX_STD_MESSAGE:
>>> + case CMD_RX_EXT_MESSAGE:
>>> + kvaser_usb_rx_can_msg(dev, msg);
>>> + break;
>>> +
>>> + case CMD_CHIP_STATE_EVENT:
>>> + case CMD_CAN_ERROR_EVENT:
>>> + kvaser_usb_rx_error(dev, msg);
>>> + break;
>>> +
>>> + case CMD_TX_ACKNOWLEDGE:
>>> + kvaser_usb_tx_acknowledge(dev, msg);
>>> + break;
>>> +
>>> + default:
>>> + dev_warn(dev->udev->dev.parent,
>>> + "Unhandled message (%d)\n", msg->id);
>>> + break;
>>> + }
>>> +}
>>> +
>>> +static void kvaser_usb_read_bulk_callback(struct urb *urb)
>>> +{
>>> + struct kvaser_usb *dev = urb->context;
>>> + struct kvaser_msg *msg;
>>> + int pos = 0;
>>> + int err, i;
>>> +
>>> + switch (urb->status) {
>>> + case 0:
>>> + break;
>>> + case -ENOENT:
>>> + case -ESHUTDOWN:
>>> + return;
>>> + default:
>>> + dev_info(dev->udev->dev.parent, "Rx URB aborted (%d)\n",
>>> + urb->status);
>>> + goto resubmit_urb;
>>> + }
>>> +
>>> + while (pos < urb->actual_length) {
>>
>> please check here for pos + sizeof(*msg), too
>
> Same as above.
>
>>
>>> + msg = urb->transfer_buffer + pos;
>>> +
>>> + if (!msg->len)
>>> + break;
>>> +
>>> + kvaser_usb_handle_message(dev, msg);
>>> +
>>> + if (pos > urb->actual_length) {
>>> + dev_err(dev->udev->dev.parent, "Format error\n");
>>> + break;
>>> + }
>>> +
>>> + pos += msg->len;
>>> + }
>>> +
>>> +resubmit_urb:
>>> + usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 129),
>> ^^^
>>
>> use #define
>
> Ok.
>
>>
>>> + urb->transfer_buffer, RX_BUFFER_SIZE,
>>> + kvaser_usb_read_bulk_callback, dev);
>>> +
>>> + err = usb_submit_urb(urb, GFP_ATOMIC);
>>> + if (err == -ENODEV) {
>>> + for (i = 0; i < dev->nchannels; i++) {
>>> + if (!dev->nets[i])
>>> + continue;
>>> +
>>> + netif_device_detach(dev->nets[i]->netdev);
>>> + }
>>> + } else if (err) {
>>> + dev_err(dev->udev->dev.parent,
>>> + "Failed resubmitting read bulk urb: %d\n", err);
>>> + }
>>> +
>>> + return;
>>> +}
>>> +
>>> +static int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev)
>>> +{
>>> + int i, err = 0;
>>> +
>>> + if (dev->rxinitdone)
>>> + return 0;
>>> +
>>> + for (i = 0; i < MAX_RX_URBS; i++) {
>>> + struct urb *urb = NULL;
>>> + u8 *buf = NULL;
>>> +
>>> + urb = usb_alloc_urb(0, GFP_KERNEL);
>>> + if (!urb) {
>>> + dev_warn(dev->udev->dev.parent,
>>> + "No memory left for URBs\n");
>>> + err = -ENOMEM;
>>> + break;
>>> + }
>>> +
>>> + buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE,
>>> + GFP_KERNEL, &urb->transfer_dma);
>>> + if (!buf) {
>>> + dev_warn(dev->udev->dev.parent,
>>> + "No memory left for USB buffer\n");
>>> + usb_free_urb(urb);
>>> + err = -ENOMEM;
>>> + break;
>>> + }
>>> +
>>> + usb_fill_bulk_urb(urb, dev->udev,
>>> + usb_rcvbulkpipe(dev->udev, 129),
>>
>> use #define
>
> Ok.
>
>>
>>> + buf, RX_BUFFER_SIZE,
>>> + kvaser_usb_read_bulk_callback, dev);
>>> + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>>> + usb_anchor_urb(urb, &dev->rx_submitted);
>>> +
>>> + err = usb_submit_urb(urb, GFP_KERNEL);
>>> + if (err) {
>>> + usb_unanchor_urb(urb);
>>> + usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
>>> + urb->transfer_dma);
>>> + break;
>>> + }
>>> +
>>> + usb_free_urb(urb);
>>> + }
>>> +
>>> + if (i == 0) {
>>> + dev_warn(dev->udev->dev.parent,
>>> + "Cannot setup read URBs, error %d\n", err);
>>> + return err;
>>> + } else if (i < MAX_RX_URBS) {
>>> + dev_warn(dev->udev->dev.parent,
>>> + "RX performances may be slow\n");
>>> + }
>>> +
>>> + dev->rxinitdone = true;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int kvaser_usb_set_opt_mode(const struct kvaser_usb_net_priv *priv)
>>> +{
>>> + struct kvaser_msg msg;
>>> +
>>> + memset(&msg, 0x00, sizeof(msg));
>>> + msg.id = CMD_SET_CTRL_MODE;
>>> + msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_ctrl_mode);
>>> + msg.u.ctrl_mode.tid = 0xff;
>>> + msg.u.ctrl_mode.channel = priv->channel;
>>
>> please use C99 struct initializers
>
> Ok.
>
>>
>>> +
>>> + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
>>> + msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_SILENT;
>>> + else
>>> + msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_NORMAL;
>>> +
>>> + return kvaser_usb_send_msg(priv->dev, &msg);
>>> +}
>>> +
>>> +static int kvaser_usb_start_chip(struct kvaser_usb_net_priv *priv)
>>> +{
>>> + int err;
>>> +
>>> + init_completion(&priv->start_stop_comp);
>>> +
>>> + err = kvaser_usb_send_simple_msg(priv->dev, CMD_START_CHIP,
>>> + priv->channel);
>>> + if (err)
>>> + return err;
>>> +
>>> + if (!wait_for_completion_timeout(&priv->start_stop_comp,
>>> + msecs_to_jiffies(START_TIMEOUT)))
>>> + return -ETIMEDOUT;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int kvaser_usb_open(struct net_device *netdev)
>>> +{
>>> + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
>>> + struct kvaser_usb *dev = priv->dev;
>>> + int err;
>>> +
>>> + err = open_candev(netdev);
>>> + if (err)
>>> + return err;
>>> +
>>> + err = kvaser_usb_setup_rx_urbs(dev);
>>> + if (err)
>>> + return err;
>>> +
>>> + err = kvaser_usb_set_opt_mode(priv);
>>> + if (err)
>>> + return err;
>>> +
>>> + err = kvaser_usb_start_chip(priv);
>>> + if (err) {
>>> + netdev_warn(netdev, "Cannot start device, error %d\n", err);
>>> + close_candev(netdev);
>>> + return err;
>>> + }
>>> +
>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> + priv->open_time = jiffies;
>>> + netif_start_queue(netdev);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
>>> +{
>>> + int i;
>>> +
>>> + usb_kill_anchored_urbs(&priv->tx_submitted);
>>> + atomic_set(&priv->active_tx_urbs, 0);
>>> +
>>> + for (i = 0; i < MAX_TX_URBS; i++)
>> ARRAY_SIZE(priv->tx_contexts) instead of MAX_TX_URBS
>
> Ok.
>
>>> + priv->tx_contexts[i].echo_index = MAX_TX_URBS;
>>> +}
>>> +
>>> +static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
>>> +{
>>> + int i;
>>> +
>>> + usb_kill_anchored_urbs(&dev->rx_submitted);
>>> +
>>> + for (i = 0; i < MAX_NET_DEVICES; i++) {
>> ARRAY_SIZE()
>>> + struct kvaser_usb_net_priv *priv = dev->nets[i];
>>> +
>>> + if (priv)
>>> + kvaser_usb_unlink_tx_urbs(priv);
>>> + }
>>> +}
>>> +
>>> +static int kvaser_usb_stop_chip(struct kvaser_usb_net_priv *priv)
>>> +{
>>> + int err;
>>> +
>>> + init_completion(&priv->start_stop_comp);
>>> +
>>> + err = kvaser_usb_send_simple_msg(priv->dev, CMD_STOP_CHIP,
>>> + priv->channel);
>>> + if (err)
>>> + return err;
>>> +
>>> + if (!wait_for_completion_timeout(&priv->start_stop_comp,
>>> + msecs_to_jiffies(STOP_TIMEOUT)))
>>> + return -ETIMEDOUT;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int kvaser_usb_flush_queue(struct kvaser_usb_net_priv *priv)
>>> +{
>>> + struct kvaser_msg msg;
>>> +
>>> + memset(&msg, 0x00, sizeof(msg));
>>> + msg.id = CMD_FLUSH_QUEUE;
>>> + msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_flush_queue);
>>> + msg.u.flush_queue.channel = priv->channel;
>> C99 initialziers, please
>
> Ok.
>
>>> +
>>> + return kvaser_usb_send_msg(priv->dev, &msg);
>>> +}
>>> +
>>> +static int kvaser_usb_close(struct net_device *netdev)
>>> +{
>>> + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
>>> + int err;
>>> +
>>> + netif_stop_queue(netdev);
>>> +
>>> + err = kvaser_usb_flush_queue(priv);
>>> + if (err)
>>> + netdev_warn(netdev, "Cannot flush queue, error %d\n", err);
>>> +
>>> + err = kvaser_usb_stop_chip(priv);
>>> + if (err) {
>>> + netdev_warn(netdev, "Cannot stop device, error %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + kvaser_usb_unlink_tx_urbs(priv);
>>> +
>>> + priv->can.state = CAN_STATE_STOPPED;
>>> + close_candev(priv->netdev);
>>> + priv->open_time = 0;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void kvaser_usb_write_bulk_callback(struct urb *urb)
>>> +{
>>> + struct kvaser_usb_tx_urb_context *context = urb->context;
>>> + struct kvaser_usb_net_priv *priv;
>>> + struct net_device *netdev;
>>> +
>>> + if (WARN_ON(!context))
>>> + return;
>>> +
>>> + priv = context->priv;
>>> + netdev = priv->netdev;
>>> +
>>> + usb_free_coherent(urb->dev, urb->transfer_buffer_length,
>>> + urb->transfer_buffer, urb->transfer_dma);
>>> +
>>> + if (!netif_device_present(netdev))
>>> + return;
>>> +
>>> + if (urb->status)
>>> + netdev_info(netdev, "Tx URB aborted (%d)\n", urb->status);
>>> +
>>> + netdev->trans_start = jiffies;
>>
>> Is trans_start needed? at least for non-usb devices it works without.
>
> I don't know, I'll try to figure this out.
> I see it's used in the two others CAN/USB drivers, 'ems_usb.c' and
> 'esd_usb2.c'
>
>>
>>> +}
>>> +
>>> +static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>>> + struct net_device *netdev)
>>> +{
>>> + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
>>> + struct kvaser_usb *dev = priv->dev;
>>> + struct net_device_stats *stats = &netdev->stats;
>>> + struct can_frame *cf = (struct can_frame *)skb->data;
>>> + struct kvaser_usb_tx_urb_context *context = NULL;
>>> + struct urb *urb;
>>> + void *buf;
>>> + struct kvaser_msg *msg;
>>> + int i, err;
>>> + int ret = NETDEV_TX_OK;
>>> +
>>> + if (can_dropped_invalid_skb(netdev, skb))
>>> + return NETDEV_TX_OK;
>>> +
>>> + urb = usb_alloc_urb(0, GFP_ATOMIC);
>>> + if (!urb) {
>>> + netdev_err(netdev, "No memory left for URBs\n");
>>> + stats->tx_dropped++;
>>> + dev_kfree_skb(skb);
>>> + goto nourbmem;
>>> + }
>>> +
>>> + buf = usb_alloc_coherent(dev->udev, sizeof(struct kvaser_msg),
>>> + GFP_ATOMIC, &urb->transfer_dma);
>>> + if (!buf) {
>>> + netdev_err(netdev, "No memory left for USB buffer\n");
>>> + stats->tx_dropped++;
>>> + dev_kfree_skb(skb);
>>> + goto nobufmem;
>>> + }
>>> +
>>> + msg = (struct kvaser_msg *)buf;
>>> + msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
>>> + msg->u.tx_can.flags = 0;
>>> + msg->u.tx_can.channel = priv->channel;
>>> +
>>> + if (cf->can_id & CAN_EFF_FLAG) {
>>> + msg->id = CMD_TX_EXT_MESSAGE;
>>> + msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
>>> + msg->u.tx_can.msg[1] = (cf->can_id >> 18) & 0x3f;
>>> + msg->u.tx_can.msg[2] = (cf->can_id >> 14) & 0x0f;
>>> + msg->u.tx_can.msg[3] = (cf->can_id >> 6) & 0xff;
>>> + msg->u.tx_can.msg[4] = cf->can_id & 0x3f;
>>> + } else {
>>> + msg->id = CMD_TX_STD_MESSAGE;
>>> + msg->u.tx_can.msg[0] = (cf->can_id >> 6) & 0x1f;
>>> + msg->u.tx_can.msg[1] = cf->can_id & 0x3f;
>>> + }
>>> +
>>> + msg->u.tx_can.msg[5] = cf->can_dlc;
>>> + memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
>>> +
>>> + if (cf->can_id & CAN_RTR_FLAG)
>>> + msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
>>> +
>>> + for (i = 0; i < MAX_TX_URBS; i++) {
>> ARRAY_SIZE
>
> Ok.
>
>>> + if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
>>> + context = &priv->tx_contexts[i];
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (!context) {
>>> + netdev_warn(netdev, "cannot find free context\n");
>>> + ret = NETDEV_TX_BUSY;
>>> + goto releasebuf;
>>> + }
>>> +
>>> + context->priv = priv;
>>> + context->echo_index = i;
>>> + context->dlc = cf->can_dlc;
>>> +
>>> + msg->u.tx_can.tid = context->echo_index;
>>> +
>>> + usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 1),
>>> + buf, msg->len,
>>> + kvaser_usb_write_bulk_callback, context);
>>> + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>>> + usb_anchor_urb(urb, &priv->tx_submitted);
>>> +
>>> + can_put_echo_skb(skb, netdev, context->echo_index);
>>> +
>>> + atomic_inc(&priv->active_tx_urbs);
>>> +
>>> + if (atomic_read(&priv->active_tx_urbs) >= MAX_TX_URBS)
>>> + netif_stop_queue(netdev);
>>> +
>>> + err = usb_submit_urb(urb, GFP_ATOMIC);
>>> + if (unlikely(err)) {
>>> + can_free_echo_skb(netdev, context->echo_index);
>>> +
>>> + atomic_dec(&priv->active_tx_urbs);
>>> + usb_unanchor_urb(urb);
>>> +
>>> + stats->tx_dropped++;
>>> +
>>> + if (err == -ENODEV)
>>> + netif_device_detach(netdev);
>>> + else
>>> + netdev_warn(netdev, "Failed tx_urb %d\n", err);
>>> +
>>> + goto releasebuf;
>>> + }
>>> +
>>> + netdev->trans_start = jiffies;
>>> +
>>> + usb_free_urb(urb);
>>> +
>>> + return NETDEV_TX_OK;
>>> +
>>> +releasebuf:
>>> + usb_free_coherent(dev->udev, sizeof(struct kvaser_msg),
>>> + buf, urb->transfer_dma);
>>> +nobufmem:
>>> + usb_free_urb(urb);
>>> +nourbmem:
>>> + return ret;
>>> +}
>>> +
>>> +static const struct net_device_ops kvaser_usb_netdev_ops = {
>>> + .ndo_open = kvaser_usb_open,
>>> + .ndo_stop = kvaser_usb_close,
>>> + .ndo_start_xmit = kvaser_usb_start_xmit,
>>> +};
>>> +
>>> +static struct can_bittiming_const kvaser_usb_bittiming_const = {
>>> + .name = "kvaser_usb",
>>> + .tseg1_min = KVASER_USB_TSEG1_MIN,
>>> + .tseg1_max = KVASER_USB_TSEG1_MAX,
>>> + .tseg2_min = KVASER_USB_TSEG2_MIN,
>>> + .tseg2_max = KVASER_USB_TSEG2_MAX,
>>> + .sjw_max = KVASER_USB_SJW_MAX,
>>> + .brp_min = KVASER_USB_BRP_MIN,
>>> + .brp_max = KVASER_USB_BRP_MAX,
>>> + .brp_inc = KVASER_USB_BRP_INC,
>>> +};
>>> +
>>> +static int kvaser_usb_set_bittiming(struct net_device *netdev)
>>> +{
>>> + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
>>> + struct can_bittiming *bt = &priv->can.bittiming;
>>> + struct kvaser_usb *dev = priv->dev;
>>> + struct kvaser_msg msg;
>>> +
>>> + msg.id = CMD_SET_BUS_PARAMS;
>>> + msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_busparams);
>>> + msg.u.busparams.channel = priv->channel;
>>> + msg.u.busparams.tid = 0xff;
>>> + msg.u.busparams.bitrate = bt->bitrate;
>>
>> bitrate is le32
>
> Indeed ! I'll fix this.
>
>>
>>> + msg.u.busparams.sjw = bt->sjw;
>>> + msg.u.busparams.tseg1 = bt->prop_seg + bt->phase_seg1;
>>> + msg.u.busparams.tseg2 = bt->phase_seg2;
>>
>> C99 initializers, please
>>
>>> +
>>> + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
>>> + msg.u.busparams.no_samp = 3;
>>> + else
>>> + msg.u.busparams.no_samp = 1;
>>> +
>>> + return kvaser_usb_send_msg(dev, &msg);
>>> +}
>>> +
>>> +static int kvaser_usb_set_mode(struct net_device *netdev,
>>> + enum can_mode mode)
>>> +{
>>> + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
>>> +
>>> + if (!priv->open_time)
>>> + return -EINVAL;
>>> +
>>> + switch (mode) {
>>> + case CAN_MODE_START:
>>> + if (netif_queue_stopped(netdev))
>>> + netif_wake_queue(netdev);
>>
>> No need to restart your USB device?
>
> No. I don't think so.
> The module continuously tries to transmit the frame and isn't stopped.
> So there is no need to restart it if it has been explicitely stopped.
>
> When it cannot transmit, the module try again and sends continuously
> CMD_CAN_ERROR_EVENT frames until it succeeds to transmit the frame.
> If the device is stopped with the command CMD_STOP_CHIP then it stops
> sending these CMD_CAN_ERROR_EVENT.
> Should I handle this in another manner?
If the firmware automatically recovers from busoff (like the at91 does),
you should stop the chip it priv->can.restart_ms == 0 and let the chip
continue working otherwise.
>
>>
>>> + break;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int kvaser_usb_get_berr_counter(const struct net_device *netdev,
>>> + struct can_berr_counter *bec)
>>> +{
>>> + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
>>> +
>>> + bec->txerr = priv->bec.txerr;
>>> + bec->rxerr = priv->bec.rxerr;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int kvaser_usb_init_one(struct usb_interface *intf,
>>> + const struct usb_device_id *id, int channel)
>>> +{
>>> + struct kvaser_usb *dev = usb_get_intfdata(intf);
>>> + struct net_device *netdev;
>>> + struct kvaser_usb_net_priv *priv;
>>> + int i, err;
>>> +
>>> + netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
>>> + if (!netdev) {
>>> + dev_err(&intf->dev, "Cannot alloc candev\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + priv = netdev_priv(netdev);
>>> +
>>> + init_usb_anchor(&priv->tx_submitted);
>>> + atomic_set(&priv->active_tx_urbs, 0);
>>> +
>>> + for (i = 0; i < MAX_TX_URBS; i++)
>>> + priv->tx_contexts[i].echo_index = MAX_TX_URBS;
>>> +
>>> + priv->dev = dev;
>>> + priv->netdev = netdev;
>>> + priv->channel = channel;
>>> +
>>> + priv->can.state = CAN_STATE_STOPPED;
>>> + priv->can.clock.freq = CAN_USB_CLOCK;
>>> + priv->can.bittiming_const = &kvaser_usb_bittiming_const;
>>> + priv->can.do_set_bittiming = kvaser_usb_set_bittiming;
>>> + priv->can.do_set_mode = kvaser_usb_set_mode;
>>> + priv->can.do_get_berr_counter = kvaser_usb_get_berr_counter;
>>> + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
>>> + if (id->driver_info & KVASER_HAS_SILENT_MODE)
>>> + priv->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY;
>>> +
>>> + netdev->flags |= IFF_ECHO;
>>> +
>>> + netdev->netdev_ops = &kvaser_usb_netdev_ops;
>>> +
>>> + SET_NETDEV_DEV(netdev, &intf->dev);
>>> +
>>> + err = register_candev(netdev);
>>> + if (err) {
>>> + dev_err(&intf->dev, "Failed to register can device\n");
>>> + free_candev(netdev);
>>> + return err;
>>> + }
>>> +
>>> + dev->nets[channel] = priv;
>>> + netdev_info(netdev, "device %s registered\n", netdev->name);
>>
>> netdev_info should take care of printing the device's name.
>
> Ok.
>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int kvaser_usb_probe(struct usb_interface *intf,
>>> + const struct usb_device_id *id)
>>> +{
>>> + struct kvaser_usb *dev;
>>> + int err = -ENOMEM;
>>> + int i;
>>> +
>>> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>
>> Who will free dev on driver unload? Please make use of devm_kzalloc().
>
> Ok. kfree is missing is disconnect().
> I'll replace it by devm_kzalloc() and devm_free().
The beauty of devm_kzalloc is you don't have to call *_free, its
automatically called if probe fails or when remove function has been called.
>
>>
>>> + if (!dev)
>>> + return -ENOMEM;
>>> +
>>> + dev->udev = interface_to_usbdev(intf);
>>> +
>>> + init_usb_anchor(&dev->rx_submitted);
>>> +
>>> + usb_set_intfdata(intf, dev);
>>> +
>>> + if (kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, 0)) {
>>> + dev_err(&intf->dev, "Cannot reset kvaser\n");
>>> + goto error;
>>> + }
>>> +
>>> + if (kvaser_usb_get_software_info(dev)) {
>>> + dev_err(&intf->dev, "Cannot get software infos\n");
>>> + goto error;
>>> + }
>>> +
>>> + if (kvaser_usb_get_card_info(dev)) {
>>> + dev_err(&intf->dev, "Cannot get card infos\n");
>>> + goto error;
>>> + }
>>> +
>>> + dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
>>> + ((dev->fw_version >> 24) & 0xff),
>>> + ((dev->fw_version >> 16) & 0xff),
>>> + (dev->fw_version & 0xffff));
>>> +
>>> + for (i = 0; i < dev->nchannels; i++)
>>> + kvaser_usb_init_one(intf, id, i);
>>> +
>>> + return 0;
>>> +
>>> +error:
>>> + kfree(dev);
>>> + return err;
>>> +}
>>> +
>>> +static void kvaser_usb_disconnect(struct usb_interface *intf)
>>> +{
>>> + struct kvaser_usb *dev = usb_get_intfdata(intf);
>>> + int i;
>>> +
>>> + usb_set_intfdata(intf, NULL);
>>> +
>>> + if (!dev)
>>> + return;
>>> +
>>> + for (i = 0; i < dev->nchannels; i++) {
>>> + if (!dev->nets[i])
>>> + continue;
>>> +
>>> + unregister_netdev(dev->nets[i]->netdev);
>>> + free_candev(dev->nets[i]->netdev);
>>> + }
>>> +
>>> + kvaser_usb_unlink_all_urbs(dev);
>>> +}
>>> +
>>> +static struct usb_driver kvaser_usb_driver = {
>>> + .name = "kvaser_usb",
>>> + .probe = kvaser_usb_probe,
>>> + .disconnect = kvaser_usb_disconnect,
>>> + .id_table = kvaser_usb_table
>>> +};
>>> +
>>> +module_usb_driver(kvaser_usb_driver);
>>> +
>>> +MODULE_AUTHOR("Olivier Sobrie <olivier@sobrie.be>");
>>> +MODULE_DESCRIPTION("Can driver for Kvaser CAN/USB devices");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/net/can/usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb.h
>>> new file mode 100644
>>> index 0000000..8e0b6ab
>>> --- /dev/null
>>> +++ b/drivers/net/can/usb/kvaser_usb.h
>>> @@ -0,0 +1,237 @@
>>> +#ifndef _KVASER_USB_H_
>>> +#define _KVASER_USB_H_
>>> +
>>> +#define MAX_TX_URBS 16
>> Please no tab between define and macro name
>
> Ok I didn't know it was not allowed... checkpatch didn't complain.
It's allowed, but not used without tab it's more common, at least among
CAN drivers.
>
>>> +#define MAX_RX_URBS 4
>>> +#define START_TIMEOUT 1000
>>> +#define STOP_TIMEOUT 1000
>>> +#define USB_SEND_TIMEOUT 1000
>>> +#define USB_RECEIVE_TIMEOUT 1000
>>> +#define RX_BUFFER_SIZE 3072
>>> +#define CAN_USB_CLOCK 8000000
>>> +#define MAX_NET_DEVICES 3
>>> +
>>> +/* Kvaser USB devices */
>>> +#define KVASER_VENDOR_ID 0x0bfd
>>> +#define USB_LEAF_DEVEL_PRODUCT_ID 10
>>> +#define USB_LEAF_LITE_PRODUCT_ID 11
>>> +#define USB_LEAF_PRO_PRODUCT_ID 12
>>> +#define USB_LEAF_SPRO_PRODUCT_ID 14
>>> +#define USB_LEAF_PRO_LS_PRODUCT_ID 15
>>> +#define USB_LEAF_PRO_SWC_PRODUCT_ID 16
>>> +#define USB_LEAF_PRO_LIN_PRODUCT_ID 17
>>> +#define USB_LEAF_SPRO_LS_PRODUCT_ID 18
>>> +#define USB_LEAF_SPRO_SWC_PRODUCT_ID 19
>>> +#define USB_MEMO2_DEVEL_PRODUCT_ID 22
>>> +#define USB_MEMO2_HSHS_PRODUCT_ID 23
>>> +#define USB_UPRO_HSHS_PRODUCT_ID 24
>>> +#define USB_LEAF_LITE_GI_PRODUCT_ID 25
>>> +#define USB_LEAF_PRO_OBDII_PRODUCT_ID 26
>>> +#define USB_MEMO2_HSLS_PRODUCT_ID 27
>>> +#define USB_LEAF_LITE_CH_PRODUCT_ID 28
>>> +#define USB_BLACKBIRD_SPRO_PRODUCT_ID 29
>>> +#define USB_OEM_MERCURY_PRODUCT_ID 34
>>> +#define USB_OEM_LEAF_PRODUCT_ID 35
>>> +#define USB_CAN_R_PRODUCT_ID 39
>>> +
>>> +/* USB devices features */
>>> +#define KVASER_HAS_SILENT_MODE (1 << 0)
>> pleae use BIT(0)
>>> +
>>> +/* Message header size */
>>> +#define MSG_HEADER_LEN 2
>>> +
>>> +/* Can message flags */
>>> +#define MSG_FLAG_ERROR_FRAME (1 << 0)
>>> +#define MSG_FLAG_OVERRUN (1 << 1)
>>> +#define MSG_FLAG_NERR (1 << 2)
>>> +#define MSG_FLAG_WAKEUP (1 << 3)
>>> +#define MSG_FLAG_REMOTE_FRAME (1 << 4)
>>> +#define MSG_FLAG_RESERVED (1 << 5)
>>> +#define MSG_FLAG_TX_ACK (1 << 6)
>>> +#define MSG_FLAG_TX_REQUEST (1 << 7)
>>> +
>>> +/* Can states */
>>> +#define M16C_STATE_BUS_RESET (1 << 0)
>>> +#define M16C_STATE_BUS_ERROR (1 << 4)
>>> +#define M16C_STATE_BUS_PASSIVE (1 << 5)
>>> +#define M16C_STATE_BUS_OFF (1 << 6)
>>> +
>>> +/* Can msg ids */
>>> +#define CMD_RX_STD_MESSAGE 12
>>> +#define CMD_TX_STD_MESSAGE 13
>>> +#define CMD_RX_EXT_MESSAGE 14
>>> +#define CMD_TX_EXT_MESSAGE 15
>>> +#define CMD_SET_BUS_PARAMS 16
>>> +#define CMD_GET_BUS_PARAMS 17
>>> +#define CMD_GET_BUS_PARAMS_REPLY 18
>>> +#define CMD_GET_CHIP_STATE 19
>>> +#define CMD_CHIP_STATE_EVENT 20
>>> +#define CMD_SET_CTRL_MODE 21
>>> +#define CMD_GET_CTRL_MODE 22
>>> +#define CMD_GET_CTRL_MODE_REPLY 23
>>> +#define CMD_RESET_CHIP 24
>>> +#define CMD_RESET_CHIP_REPLY 25
>>> +#define CMD_START_CHIP 26
>>> +#define CMD_START_CHIP_REPLY 27
>>> +#define CMD_STOP_CHIP 28
>>> +#define CMD_STOP_CHIP_REPLY 29
>>> +#define CMD_GET_CARD_INFO2 32
>>> +#define CMD_GET_CARD_INFO 34
>>> +#define CMD_GET_CARD_INFO_REPLY 35
>>> +#define CMD_GET_SOFTWARE_INFO 38
>>> +#define CMD_GET_SOFTWARE_INFO_REPLY 39
>>> +#define CMD_ERROR_EVENT 45
>>> +#define CMD_FLUSH_QUEUE 48
>>> +#define CMD_TX_ACKNOWLEDGE 50
>>> +#define CMD_CAN_ERROR_EVENT 51
>>> +#define CMD_USB_THROTTLE 77
>>> +
>>> +/* error factors */
>>> +#define M16C_EF_ACKE (1 << 0)
>>> +#define M16C_EF_CRCE (1 << 1)
>>> +#define M16C_EF_FORME (1 << 2)
>>> +#define M16C_EF_STFE (1 << 3)
>>> +#define M16C_EF_BITE0 (1 << 4)
>>> +#define M16C_EF_BITE1 (1 << 5)
>>> +#define M16C_EF_RCVE (1 << 6)
>>> +#define M16C_EF_TRE (1 << 7)
>>> +
>>> +/* bittiming parameters */
>>> +#define KVASER_USB_TSEG1_MIN 1
>>> +#define KVASER_USB_TSEG1_MAX 16
>>> +#define KVASER_USB_TSEG2_MIN 1
>>> +#define KVASER_USB_TSEG2_MAX 8
>>> +#define KVASER_USB_SJW_MAX 4
>>> +#define KVASER_USB_BRP_MIN 1
>>> +#define KVASER_USB_BRP_MAX 64
>>> +#define KVASER_USB_BRP_INC 1
>>> +
>>> +/* ctrl modes */
>>> +#define KVASER_CTRL_MODE_NORMAL 1
>>> +#define KVASER_CTRL_MODE_SILENT 2
>>> +#define KVASER_CTRL_MODE_SELFRECEPTION 3
>>> +#define KVASER_CTRL_MODE_OFF 4
>>> +
>>> +struct kvaser_msg_simple {
>>> + u8 tid;
>>> + u8 channel;
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_cardinfo {
>>> + u8 tid;
>>> + u8 nchannels;
>>> + __le32 serial_number;
>>> + __le32 padding;
>>> + __le32 clock_resolution;
>>> + __le32 mfgdate;
>>> + u8 ean[8];
>>> + u8 hw_revision;
>>> + u8 usb_hs_mode;
>>> + __le16 padding2;
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_cardinfo2 {
>>> + u8 tid;
>>> + u8 channel;
>>> + u8 pcb_id[24];
>>> + __le32 oem_unlock_code;
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_softinfo {
>>> + u8 tid;
>>> + u8 channel;
>>> + __le32 sw_options;
>>> + __le32 fw_version;
>>> + __le16 max_outstanding_tx;
>>> + __le16 padding[9];
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_busparams {
>>> + u8 tid;
>>> + u8 channel;
>>> + __le32 bitrate;
>>> + u8 tseg1;
>>> + u8 tseg2;
>>> + u8 sjw;
>>> + u8 no_samp;
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_tx_can {
>>> + u8 channel;
>>> + u8 tid;
>>> + u8 msg[14];
>>> + u8 padding;
>>> + u8 flags;
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_rx_can {
>>> + u8 channel;
>>> + u8 flag;
>>> + __le16 time[3];
>>> + u8 msg[14];
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_chip_state_event {
>>> + u8 tid;
>>> + u8 channel;
>>> + __le16 time[3];
>>> + u8 tx_errors_count;
>>> + u8 rx_errors_count;
>>> + u8 status;
>>> + u8 padding[3];
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_tx_acknowledge {
>>> + u8 channel;
>>> + u8 tid;
>>> + __le16 time[3];
>>> + u8 flags;
>>> + u8 time_offset;
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_error_event {
>>> + u8 tid;
>>> + u8 flags;
>>> + __le16 time[3];
>>> + u8 channel;
>>> + u8 padding;
>>> + u8 tx_errors_count;
>>> + u8 rx_errors_count;
>>> + u8 status;
>>> + u8 error_factor;
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_ctrl_mode {
>>> + u8 tid;
>>> + u8 channel;
>>> + u8 ctrl_mode;
>>> + u8 padding[3];
>>> +} __packed;
>>> +
>>> +struct kvaser_msg_flush_queue {
>>> + u8 tid;
>>> + u8 channel;
>>> + u8 flags;
>>> + u8 padding[3];
>>> +} __packed;
>>> +
>>> +struct kvaser_msg {
>>> + u8 len;
>>> + u8 id;
>>> + union {
>>> + struct kvaser_msg_simple simple;
>>> + struct kvaser_msg_cardinfo cardinfo;
>>> + struct kvaser_msg_cardinfo2 cardinfo2;
>>> + struct kvaser_msg_softinfo softinfo;
>>> + struct kvaser_msg_busparams busparams;
>>> + struct kvaser_msg_tx_can tx_can;
>>> + struct kvaser_msg_rx_can rx_can;
>>> + struct kvaser_msg_chip_state_event chip_state_event;
>>> + struct kvaser_msg_tx_acknowledge tx_acknowledge;
>>> + struct kvaser_msg_error_event error_event;
>>> + struct kvaser_msg_ctrl_mode ctrl_mode;
>>> + struct kvaser_msg_ctrl_mode flush_queue;
>>> + } u;
>>> +} __packed;
>>> +
>>> +#endif
>>>
>>
>>
>> --
>> Pengutronix e.K. | Marc Kleine-Budde |
>> Industrial Linux Solutions | Phone: +49-231-2826-924 |
>> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
>> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>>
>
> Thanks for the review.
np,
regards, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* RE: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO
From: KY Srinivasan @ 2012-07-31 10:34 UTC (permalink / raw)
To: Ben Hutchings
Cc: Olaf Hering, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
apw@canonical.com, devel@linuxdriverproject.org
In-Reply-To: <20120730191912.GH1894@decadent.org.uk>
> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Monday, July 30, 2012 3:19 PM
> To: KY Srinivasan
> Cc: Olaf Hering; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org
> Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> KVP_OP_SET_IP_INFO
>
> On Mon, Jul 30, 2012 at 06:32:15PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Olaf Hering [mailto:olaf@aepfle.de]
> > > Sent: Monday, July 30, 2012 2:03 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
> > > ben@decadent.org.uk
> > > Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> > > KVP_OP_SET_IP_INFO
> > >
> > > On Tue, Jul 24, K. Y. Srinivasan wrote:
> > >
> > > > + /*
> > > > + * Set the configuration for the specified interface with
> > > > + * the information provided. Since there is no standard
> > > > + * way to configure an interface, we will have an external
> > > > + * script that does the job of configuring the interface and
> > > > + * flushing the configuration.
> > > > + *
> > > > + * The parameters passed to this external script are:
> > > > + * 1. A configuration file that has the specified configuration.
> > >
> > > Maybe this should be written as 'A info file that has the requested
> > > network configuration' or something like that.
> >
> > That is the idea. This configuration file simply reflects all the
> > information we have perhaps with some additional constant
> > information. The script is free to ignore what it does not need.
> [...]
>
> This does not strike me as a sensible interface. If scripts are
> 'free to ignore' information then the KVP interface becomes unreliable
> as a means for managing networking on Linux guests. I would suggest
> that at the least the script should be able to report that it did not
> recognise some parts of the configuration. This would be logged
> and/or reported back to the hypervisor.
>
> (This is separate from the issue of constant configuration lines;
> for some distributions the script might recognise but ignore them
> because they have no use on that distribution. I don't see the
> point in constant lines, but they don't seem to result in any
> unreliability.)
Ben,
I see your point. I have cleaned up the contents of the KVP produced
configuration file to not include constant information that can be
auto generated by the distro specific script if it needs to. Also, I have
tried to make the documentation of the contents of the file a little
better. I will send out these new patches soon. Still, there is a possibility that
some of the content of this file may be redundant on a specific distro and I think
that should be fine. For instance, per Olaf's suggestion, I have included a line that
specifies the interface name in the file (in addition to the mac address). Given the
current format of the name of the config file (where the interface name is embedded
in the config file name, this additional name entry may be redundant on some distros.
Once again, thank you for taking the time to review this code.
Regards,
K. Y
^ permalink raw reply
* [PATCH v2] ipv4: Restore old dst_free() behavior.
From: Eric Dumazet @ 2012-07-31 11:08 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20120730.223827.74792864437911339.davem@davemloft.net>
From: Eric Dumazet <edumazet@google.com>
On Mon, 2012-07-30 at 22:38 -0700, David Miller wrote:
> Eric, this is what I'd like to propose.
>
> It seems the problem you were likely running into was simply
> the fact that we were not inserting an RCU grace period for
> the dst_free() that we do when purging a FIB nexthop.
>
> So this reverts your change, and instead adds the necessary
> call_rcu_bh() wrapper around the dst_free() done in fib_semantics.c
>
> That makes it so that we don't need all of that inc_not_zero stuff for
> sockets, and the special dst flag. If we set the pointer to NULL,
> then do the dst_free() via RCU, we can test that refcount safely in
> dst_free() since it can only decrease at that point.
>
> What do you think? Does it pass your tests?
It doesnt.
But I believe I found why, thats the good news ;)
In the past, we used RCU only for input routes, thats why using
call_rcu_bh() was OK.
But now we alse cache output routes, we must use call_rcu(), because
on output path we use rcu_read_lock() 'only', in process context,
not from softirq handler.
If you dont mind, I would like to keep inet_sk_rx_dst_set() helper
because I believe its cleaner and I'll probably add IPv6 stuff on it
(the cookie thing)
Also I added __rcu attributes to nh_rth_output and nh_rth_input to
better document what is going on in this code.
Thanks
[PATCH v2] ipv4: Restore old dst_free() behavior
commit 404e0a8b6a55 (net: ipv4: fix RCU races on dst refcounts) tried
to solve a race but added a problem at device/fib dismantle time :
We really want to call dst_free() as soon as possible, even if sockets
still have dst in their cache.
dst_release() calls in free_fib_info_rcu() are not welcomed.
Root of the problem was that now we also cache output routes (in
nh_rth_output), we must use call_rcu() instead of call_rcu_bh() in
rt_free(), because output route lookups are done in process context.
Based on feedback and initial patch from David Miller (adding another
call_rcu_bh() call in fib, but it appears it was not the right fix)
I left the inet_sk_rx_dst_set() helper and added __rcu attributes
to nh_rth_output and nh_rth_input to better document what is going on in
this code.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
include/net/dst.h | 7 ++++++-
include/net/inet_sock.h | 10 +++-------
include/net/ip_fib.h | 4 ++--
net/core/dst.c | 26 +++++---------------------
net/decnet/dn_route.c | 6 ------
net/ipv4/fib_semantics.c | 21 +++++++++++++++++----
net/ipv4/route.c | 26 +++++++++++++++++---------
7 files changed, 50 insertions(+), 50 deletions(-)
diff --git a/include/net/dst.h b/include/net/dst.h
index 31a9fd3..baf5978 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -61,7 +61,6 @@ struct dst_entry {
#define DST_NOPEER 0x0040
#define DST_FAKE_RTABLE 0x0080
#define DST_XFRM_TUNNEL 0x0100
-#define DST_RCU_FREE 0x0200
unsigned short pending_confirm;
@@ -383,6 +382,12 @@ static inline void dst_free(struct dst_entry *dst)
__dst_free(dst);
}
+static inline void dst_rcu_free(struct rcu_head *head)
+{
+ struct dst_entry *dst = container_of(head, struct dst_entry, rcu_head);
+ dst_free(dst);
+}
+
static inline void dst_confirm(struct dst_entry *dst)
{
dst->pending_confirm = 1;
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index e3fd34c..83b567f 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -253,13 +253,9 @@ static inline void inet_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb
{
struct dst_entry *dst = skb_dst(skb);
- if (atomic_inc_not_zero(&dst->__refcnt)) {
- if (!(dst->flags & DST_RCU_FREE))
- dst->flags |= DST_RCU_FREE;
-
- sk->sk_rx_dst = dst;
- inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
- }
+ dst_hold(dst);
+ sk->sk_rx_dst = dst;
+ inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
}
#endif /* _INET_SOCK_H */
diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index e69c3a4..e521a03 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -81,8 +81,8 @@ struct fib_nh {
__be32 nh_gw;
__be32 nh_saddr;
int nh_saddr_genid;
- struct rtable *nh_rth_output;
- struct rtable *nh_rth_input;
+ struct rtable __rcu *nh_rth_output;
+ struct rtable __rcu *nh_rth_input;
struct fnhe_hash_bucket *nh_exceptions;
};
diff --git a/net/core/dst.c b/net/core/dst.c
index d9e33eb..069d51d 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -258,15 +258,6 @@ again:
}
EXPORT_SYMBOL(dst_destroy);
-static void dst_rcu_destroy(struct rcu_head *head)
-{
- struct dst_entry *dst = container_of(head, struct dst_entry, rcu_head);
-
- dst = dst_destroy(dst);
- if (dst)
- __dst_free(dst);
-}
-
void dst_release(struct dst_entry *dst)
{
if (dst) {
@@ -274,14 +265,10 @@ void dst_release(struct dst_entry *dst)
newrefcnt = atomic_dec_return(&dst->__refcnt);
WARN_ON(newrefcnt < 0);
- if (unlikely(dst->flags & (DST_NOCACHE | DST_RCU_FREE)) && !newrefcnt) {
- if (dst->flags & DST_RCU_FREE) {
- call_rcu_bh(&dst->rcu_head, dst_rcu_destroy);
- } else {
- dst = dst_destroy(dst);
- if (dst)
- __dst_free(dst);
- }
+ if (unlikely(dst->flags & DST_NOCACHE) && !newrefcnt) {
+ dst = dst_destroy(dst);
+ if (dst)
+ __dst_free(dst);
}
}
}
@@ -333,14 +320,11 @@ EXPORT_SYMBOL(__dst_destroy_metrics_generic);
*/
void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst)
{
- bool hold;
-
WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
/* If dst not in cache, we must take a reference, because
* dst_release() will destroy dst as soon as its refcount becomes zero
*/
- hold = (dst->flags & (DST_NOCACHE | DST_RCU_FREE)) == DST_NOCACHE;
- if (unlikely(hold)) {
+ if (unlikely(dst->flags & DST_NOCACHE)) {
dst_hold(dst);
skb_dst_set(skb, dst);
} else {
diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index 2671977..85a3604 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -184,12 +184,6 @@ static __inline__ unsigned int dn_hash(__le16 src, __le16 dst)
return dn_rt_hash_mask & (unsigned int)tmp;
}
-static inline void dst_rcu_free(struct rcu_head *head)
-{
- struct dst_entry *dst = container_of(head, struct dst_entry, rcu_head);
- dst_free(dst);
-}
-
static inline void dnrt_free(struct dn_route *rt)
{
call_rcu_bh(&rt->dst.rcu_head, dst_rcu_free);
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index e55171f..625cf18 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -161,6 +161,21 @@ static void free_nh_exceptions(struct fib_nh *nh)
kfree(hash);
}
+static void rt_nexthop_free(struct rtable __rcu **rtp)
+{
+ struct rtable *rt = rcu_dereference_protected(*rtp, 1);
+
+ if (!rt)
+ return;
+
+ /* Not even needed : RCU_INIT_POINTER(*rtp, NULL);
+ * because we waited an RCU grace period before calling
+ * free_fib_info_rcu()
+ */
+
+ dst_free(&rt->dst);
+}
+
/* Release a nexthop info record */
static void free_fib_info_rcu(struct rcu_head *head)
{
@@ -171,10 +186,8 @@ static void free_fib_info_rcu(struct rcu_head *head)
dev_put(nexthop_nh->nh_dev);
if (nexthop_nh->nh_exceptions)
free_nh_exceptions(nexthop_nh);
- if (nexthop_nh->nh_rth_output)
- dst_release(&nexthop_nh->nh_rth_output->dst);
- if (nexthop_nh->nh_rth_input)
- dst_release(&nexthop_nh->nh_rth_input->dst);
+ rt_nexthop_free(&nexthop_nh->nh_rth_output);
+ rt_nexthop_free(&nexthop_nh->nh_rth_input);
} endfor_nexthops(fi);
release_net(fi->fib_net);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d6eabcf..2bd1074 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1199,23 +1199,31 @@ restart:
fnhe->fnhe_stamp = jiffies;
}
+static inline void rt_free(struct rtable *rt)
+{
+ call_rcu(&rt->dst.rcu_head, dst_rcu_free);
+}
+
static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
{
- struct rtable *orig, *prev, **p = &nh->nh_rth_output;
+ struct rtable *orig, *prev, **p = (struct rtable **)&nh->nh_rth_output;
if (rt_is_input_route(rt))
- p = &nh->nh_rth_input;
+ p = (struct rtable **)&nh->nh_rth_input;
orig = *p;
- rt->dst.flags |= DST_RCU_FREE;
- dst_hold(&rt->dst);
prev = cmpxchg(p, orig, rt);
if (prev == orig) {
if (orig)
- dst_release(&orig->dst);
+ rt_free(orig);
} else {
- dst_release(&rt->dst);
+ /* Routes we intend to cache in the FIB nexthop have
+ * the DST_NOCACHE bit clear. However, if we are
+ * unsuccessful at storing this route into the cache
+ * we really need to set it.
+ */
+ rt->dst.flags |= DST_NOCACHE;
}
}
@@ -1412,7 +1420,7 @@ static int __mkroute_input(struct sk_buff *skb,
do_cache = false;
if (res->fi) {
if (!itag) {
- rth = FIB_RES_NH(*res).nh_rth_input;
+ rth = rcu_dereference(FIB_RES_NH(*res).nh_rth_input);
if (rt_cache_valid(rth)) {
skb_dst_set_noref(skb, &rth->dst);
goto out;
@@ -1574,7 +1582,7 @@ local_input:
do_cache = false;
if (res.fi) {
if (!itag) {
- rth = FIB_RES_NH(res).nh_rth_input;
+ rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input);
if (rt_cache_valid(rth)) {
skb_dst_set_noref(skb, &rth->dst);
err = 0;
@@ -1742,7 +1750,7 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
if (fi) {
fnhe = find_exception(&FIB_RES_NH(*res), fl4->daddr);
if (!fnhe) {
- rth = FIB_RES_NH(*res).nh_rth_output;
+ rth = rcu_dereference(FIB_RES_NH(*res).nh_rth_output);
if (rt_cache_valid(rth)) {
dst_hold(&rth->dst);
return rth;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox