* [PATCH] Fix "ip -6 route add ... nexthop"
From: Vincent Bernat @ 2012-09-06 17:30 UTC (permalink / raw)
To: netdev; +Cc: Vincent Bernat
In-Reply-To: <1346952649-5716-1-git-send-email-bernat@luffy.cx>
IPv6 multipath routes were not accepted by "ip route" because an IPv4
address was expected for each gateway. Use `get_addr()` instead of
`get_addr32()`.
Signed-off-by: Vincent Bernat <bernat@luffy.cx>
---
ip/iproute.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/ip/iproute.c b/ip/iproute.c
index 522dd28..c78d4f7 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -624,16 +624,20 @@ int print_route(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
}
-int parse_one_nh(struct rtattr *rta, struct rtnexthop *rtnh, int *argcp, char ***argvp)
+int parse_one_nh(struct rtmsg *r, struct rtattr *rta, struct rtnexthop *rtnh, int *argcp, char ***argvp)
{
int argc = *argcp;
char **argv = *argvp;
while (++argv, --argc > 0) {
if (strcmp(*argv, "via") == 0) {
+ inet_prefix addr;
NEXT_ARG();
- rta_addattr32(rta, 4096, RTA_GATEWAY, get_addr32(*argv));
- rtnh->rtnh_len += sizeof(struct rtattr) + 4;
+ get_addr(&addr, *argv, r->rtm_family);
+ if (r->rtm_family == AF_UNSPEC)
+ r->rtm_family = addr.family;
+ rta_addattr_l(rta, 4096, RTA_GATEWAY, &addr.data, addr.bytelen);
+ rtnh->rtnh_len += sizeof(struct rtattr) + addr.bytelen;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
if ((rtnh->rtnh_ifindex = ll_name_to_index(*argv)) == 0) {
@@ -685,7 +689,7 @@ int parse_nexthops(struct nlmsghdr *n, struct rtmsg *r, int argc, char **argv)
memset(rtnh, 0, sizeof(*rtnh));
rtnh->rtnh_len = sizeof(*rtnh);
rta->rta_len += rtnh->rtnh_len;
- parse_one_nh(rta, rtnh, &argc, &argv);
+ parse_one_nh(r, rta, rtnh, &argc, &argv);
rtnh = RTNH_NEXT(rtnh);
}
--
1.7.10.4
^ permalink raw reply related
* IPv6 multipath routes
From: Vincent Bernat @ 2012-09-06 17:30 UTC (permalink / raw)
To: netdev
Hi!
It appears that "ip -6 route add" expects IPv4 addresses with nexthop directives:
$ ip -6 route add to 2a01:c9c0:a1:982::/64 proto bird \
nexthop via fe80::ea39:35ff:febd:f9e dev bai2.2008 weight 1 \
nexthop via fe80::ea39:35ff:febd:fd6 dev bai1.2009 weight 1
Error: an IP address is expected rather than "fe80::ea39:35ff:febd:f9e"
The following patch fix this problem. However, it does not work. I now have:
RTNETLINK answers: No such device
I have little knowledge of netlink so it is likely that my patch is
buggy. However, I have found this problem by trying to debug what
appears to be a valid netlink message refused by the kernel with the
same error. Therefore, I suspect that there is also a bug in the kernel.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: Export nd_tbl to allow modules to support IPv6
From: David Miller @ 2012-09-06 17:47 UTC (permalink / raw)
To: tgraf; +Cc: netdev
In-Reply-To: <20120906085102.GA14711@canuck.infradead.org>
From: Thomas Graf <tgraf@suug.ch>
Date: Thu, 6 Sep 2012 04:51:02 -0400
> On Wed, Sep 05, 2012 at 01:06:36PM -0400, David Miller wrote:
>> So if one of our goals is to move towards a situation where all neigh
>> accesses are refcount'less, having those external users makes that
>> nearly impossible.
>>
>> Instead, I'd rather see patches that mark arp_tbl as being exported
>> only for internal usage inside of the tree, so that we can reach that
>> goal.
>>
>> I'm not applying this, sorry.
>
> Fair enough
>
> Does that mean you dismiss neighbour lookups by external users in
> general in order to get rid of the refcnt?
>
> Assuming that lookups would still be ok, would an ipv6 version of
> __ipv4_neigh_lookup() be an acceptable API for external users?
> (Yes there is __ipv6_neigh_lookup() already but unlike the ipv4 version
> it takes the table as first argument)
Right now we're in a transition period where ipv4 is mostly refcount'less
and ipv6 is not.
This is part of the reason I don't want to expose these things, the
calling convention and locking requirements are going to be fluid for
any interface you might propose.
arp_tbl was exported only for in-tree users, and I therefore say that
we should only export nd_tbl for in-tree users as well, since that's
the only way we can audit and update all the referencing callers as
the semantics radically change.
^ permalink raw reply
* Re: [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision
From: David Miller @ 2012-09-06 17:42 UTC (permalink / raw)
To: nicolas.ferre
Cc: bhutchings, netdev, linux-arm-kernel, havard, plagnioj, jamie,
linux-kernel, patrice.vilchez
In-Reply-To: <5048ACBE.2030205@atmel.com>
From: Nicolas Ferre <nicolas.ferre@atmel.com>
Date: Thu, 6 Sep 2012 16:01:34 +0200
> Absolutely, I will do this.
Please, when you receive feedback on your patches, you need to
resubmit the whole patch series for review not just the patches where
changes were asked for.
^ permalink raw reply
* Re: [PATCH 10/12] qlcnic: register dump utility
From: Ben Hutchings @ 2012-09-06 17:38 UTC (permalink / raw)
To: Sony Chacko; +Cc: davem, netdev, Dept_NX_Linux_NIC_Driver
In-Reply-To: <1346912529-17406-11-git-send-email-sony.chacko@qlogic.com>
On Thu, 2012-09-06 at 02:22 -0400, Sony Chacko wrote:
> From: Sony Chacko <sony.chacko@qlogic.com>
>
> Modify 82xx driver to support new adapter - Qlogic 83XX CNA
> Common register dump utility for 82xx and 83xx adapters
[...]
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
> @@ -525,21 +525,7 @@ static void qlcnic_get_channels(struct net_device *dev,
> static int qlcnic_set_channels(struct net_device *dev,
> struct ethtool_channels *channel)
> {
> - struct qlcnic_adapter *adapter = netdev_priv(dev);
> - int err;
> -
> - if (channel->other_count || channel->combined_count ||
> - channel->tx_count != channel->max_tx)
> - return -EINVAL;
> -
> - err = qlcnic_validate_max_rss(dev, channel->max_rx, channel->rx_count);
> - if (err)
> - return err;
> -
> - err = qlcnic_set_max_rss(adapter, channel->rx_count);
> - netdev_info(dev, "allocated 0x%x sds rings\n",
> - adapter->max_sds_rings);
> - return err;
> + return 0;
> }
[...]
No, I don't think so. If you're going to remove support for this
operation then delete the function entirely. And don't put it in a
patch that's supposed to do something unrelated.
Ben.
--
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
* Re: [PATCH 06/12] qlcnic: 83xx data path and HW interfaces routines
From: Ben Hutchings @ 2012-09-06 17:33 UTC (permalink / raw)
To: Sony Chacko; +Cc: davem, netdev, Dept_NX_Linux_NIC_Driver
In-Reply-To: <1346912529-17406-7-git-send-email-sony.chacko@qlogic.com>
On Thu, 2012-09-06 at 02:22 -0400, Sony Chacko wrote:
> From: Sony Chacko <sony.chacko@qlogic.com>
>
> Modify 82xx driver to support new adapter - Qlogic 83XX CNA
> Create 83xx adapter data path and hardware interface routines
[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_hw.c
[...]
> +int qlcnic_83xx_set_settings(struct qlcnic_adapter *adapter,
> + struct ethtool_cmd *ecmd)
> +{
> + int status = 0;
> + u32 config = adapter->ahw->port_config;
> +
> + if (ecmd->autoneg)
> + adapter->ahw->port_config |= BIT_15;
> +
> + switch (ethtool_cmd_speed(ecmd)) {
> + case SPEED_10:
> + adapter->ahw->port_config |= BIT_8;
> + break;
> + case SPEED_100:
> + adapter->ahw->port_config |= BIT_9;
> + break;
> + case SPEED_1000:
> + adapter->ahw->port_config |= BIT_10;
> + break;
> + case SPEED_10000:
> + adapter->ahw->port_config |= BIT_11;
> + break;
> + default:
> + return -EIO;
Should be -EINVAL.
> + }
> +
> + status = qlcnic_83xx_set_port_config(adapter);
> + if (status) {
> + dev_info(&adapter->pdev->dev,
> + "Faild to Set Link Speed and autoneg.\n");
> + adapter->ahw->port_config = config;
> + }
> + return status;
> +}
[...]
> +static u64*
> +qlcnic_83xx_fill_stats(struct qlcnic_adapter *adapter,
> + struct qlcnic_cmd_args *cmd, u64 *data, int type,
> + int *ret)
> +{
> + int err, k, total_regs;
> +
> + *ret = 0;
> + err = qlcnic_issue_cmd(adapter, cmd);
> + if (err != QLCNIC_RCODE_SUCCESS) {
> + dev_info(&adapter->pdev->dev,
> + "Error in get statistics mailbox command\n");
> + *ret = -EIO;
> + return data;
> + }
> + total_regs = cmd->rsp.num;
> + switch (type) {
[...]
> + default:
> + dev_info(&adapter->pdev->dev, "Unknown get statistics mode\n");
It seems like this default case can only be reached in case of a driver
bug, so WARN would be more appropriate.
> + *ret = -EIO;
> + }
> + return data;
> +}
[...]
--
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
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Steven Rostedt @ 2012-09-06 17:15 UTC (permalink / raw)
To: Sasha Levin
Cc: Josh Triplett, Mathieu Desnoyers, Pedro Alves, Tejun Heo,
torvalds, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
mingo, ebiederm, aarcange, ericvh, netdev, eric.dumazet, axboe,
agk, dm-devel, neilb, ccaulfie, teigland, Trond.Myklebust,
bfields, fweisbec, jesse, venkat.x.venkatsubra, ejt, snitzer,
edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <5048CDA2.10300@gmail.com>
On Thu, 2012-09-06 at 18:21 +0200, Sasha Levin wrote:
> On 09/06/2012 06:00 PM, Steven Rostedt wrote:
> >> > I think that that code doesn't make sense. The users of hlist_for_each_* aren't
> >> > supposed to be changing the loop cursor.
> > I totally agree. Modifying the 'node' pointer is just asking for issues.
> > Yes that is error prone, but not due to the double loop. It's due to the
> > modifying of the node pointer that is used internally by the loop
> > counter. Don't do that :-)
>
> While we're on this subject, I haven't actually seen hlist_for_each_entry() code
> that even *touches* 'pos'.
>
> Will people yell at me loudly if I change the prototype of those macros to be:
>
> hlist_for_each_entry(tpos, head, member)
>
> (Dropping the 'pos' parameter), and updating anything that calls those macros to
> drop it as well?
If 'pos' is no longer used in the macro, I don't see any reason for
keeping it around.
-- Steve
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] sctp: check dst validity after IPsec operations
From: Vlad Yasevich @ 2012-09-06 17:03 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: sri, linux-sctp, netdev
In-Reply-To: <5048D219.4020001@6wind.com>
On 09/06/2012 12:40 PM, Nicolas Dichtel wrote:
> Le 06/09/2012 18:04, Vlad Yasevich a écrit :
>> On 09/06/2012 01:40 PM, Nicolas Dichtel wrote:
>>> dst stored in struct sctp_transport needs to be recalculated when
>>> ipsec policy
>>> are updated. We use flow_cache_genid for that.
>>>
>>> For example, if a SCTP connection is established and then an IPsec
>>> policy is
>>> set, the old SCTP flow will not be updated and thus will not use the new
>>> IPsec policy.
>>>
>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>
>> why doesn't this need to be done for TCP? What makes SCTP special in
>> this case?
> Tests prove that the pb does not exist with TCP. I made the patch some
> times ago, I will look again deeply to find the difference.
>
TCP appears to cache the flowi and uses that to re-route the packet.
However, re-route still seems predicated on dst_check()...
>>
>> ip_queue_xmit does an __sk_dst_check() which is essentially what
>> sctp_transport_dst_check() does. That should determine if the
>> currently cached
>> route is valid or not.
> The problem is that route will not be invalidated, because dst->check()
> has no xfrm path so xfrm_dst_check() will never be called.
>
Shouldn't the cache be invalidated in this case? If the cache is
invalidated, that should cause a new lookup. If the cache isn't
invalidated, then any established connections that may now be impacted
by the policy will not pick it up.
-vlad
>
> Regards,
> Nicolas
>
>>
>> Looks like sctp may need to change to using ip_route_output_ports() call
>> as ip_route_output_key may not do all that is necessary
>>
>> -vlad
>>
>>> ---
>>> include/net/sctp/sctp.h | 3 ++-
>>> include/net/sctp/structs.h | 3 +++
>>> net/sctp/ipv6.c | 1 +
>>> net/sctp/protocol.c | 1 +
>>> 4 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>> index 9c6414f..3267246 100644
>>> --- a/include/net/sctp/sctp.h
>>> +++ b/include/net/sctp/sctp.h
>>> @@ -712,7 +712,8 @@ static inline void sctp_v4_map_v6(union sctp_addr
>>> *addr)
>>> */
>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>> sctp_transport *t)
>>> {
>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>> + if ((t->dst && !dst_check(t->dst, 0)) ||
>>> + (t->flow_cache_genid != atomic_read(&flow_cache_genid))) {
>>> dst_release(t->dst);
>>> t->dst = NULL;
>>> }
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index 0fef00f..9dab882 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -948,6 +948,9 @@ struct sctp_transport {
>>>
>>> /* 64-bit random number sent with heartbeat. */
>>> __u64 hb_nonce;
>>> +
>>> + /* used to check validity of dst */
>>> + __u32 flow_cache_genid;
>>> };
>>>
>>> struct sctp_transport *sctp_transport_new(struct net *, const union
>>> sctp_addr *,
>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>> index ea14cb4..2ed7410 100644
>>> --- a/net/sctp/ipv6.c
>>> +++ b/net/sctp/ipv6.c
>>> @@ -349,6 +349,7 @@ out:
>>> struct rt6_info *rt;
>>> rt = (struct rt6_info *)dst;
>>> t->dst = dst;
>>> + t->flow_cache_genid = atomic_read(&flow_cache_genid);
>>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>> &rt->rt6i_dst.addr, &fl6->saddr);
>>> } else {
>>> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
>>> index 2d51842..4795a1a 100644
>>> --- a/net/sctp/protocol.c
>>> +++ b/net/sctp/protocol.c
>>> @@ -512,6 +512,7 @@ out_unlock:
>>> rcu_read_unlock();
>>> out:
>>> t->dst = dst;
>>> + t->flow_cache_genid = atomic_read(&flow_cache_genid);
>>> if (dst)
>>> SCTP_DEBUG_PRINTK("rt_dst:%pI4, rt_src:%pI4\n",
>>> &fl4->daddr, &fl4->saddr);
>>>
>>
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-09-06 17:02 UTC (permalink / raw)
To: Sasha Levin
Cc: Steven Rostedt, Josh Triplett, Pedro Alves, Tejun Heo,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, mingo-X9Un+BFzKDI,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, aarcange-H+wXaHxf7aLQT0dZR+AlfA,
ericvh-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w, axboe-tSWWG44O7X1aa/9Udqfwiw,
agk-H+wXaHxf7aLQT0dZR+AlfA, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
neilb-l3A5Bk7waGM, ccaulfie-H+wXaHxf7aLQT0dZR+AlfA,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw, fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
jesse-l0M0P4e3n4LQT0dZR+AlfA,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ejt-H+wXaHxf7aLQT0dZR+AlfA, snitzer-H+wXaHxf7aLQT0dZR+AlfA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
dev-yBygre7rU0TnMu66kgdUjQ, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
lw-BthXqXjhjHXQFUHtdCDX3A
In-Reply-To: <5048D6DE.8090805-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
* Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> On 09/06/2012 06:50 PM, Mathieu Desnoyers wrote:
> > * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> >> On 09/06/2012 06:00 PM, Steven Rostedt wrote:
> >>>>> I think that that code doesn't make sense. The users of hlist_for_each_* aren't
> >>>>> supposed to be changing the loop cursor.
> >>> I totally agree. Modifying the 'node' pointer is just asking for issues.
> >>> Yes that is error prone, but not due to the double loop. It's due to the
> >>> modifying of the node pointer that is used internally by the loop
> >>> counter. Don't do that :-)
> >>
> >> While we're on this subject, I haven't actually seen hlist_for_each_entry() code
> >> that even *touches* 'pos'.
> >>
> >> Will people yell at me loudly if I change the prototype of those macros to be:
> >>
> >> hlist_for_each_entry(tpos, head, member)
> >>
> >> (Dropping the 'pos' parameter), and updating anything that calls those macros to
> >> drop it as well?
> >
> > I think the intent there is to keep hlist macros and list macros
> > slightly in sync. Given those are vastly used, I'm not sure you want to
> > touch them. But hey, that's just my 2 cents.
>
> Actually, the corresponding list macro looks like this:
>
> list_for_each_entry(pos, head, member)
>
> With 'pos' being the equivalent of 'tpos' in the hlist macros (the type *).
> Changing hlist macro will make them both look as follows:
>
> hlist_for_each_entry(pos, head, member)
> list_for_each_entry(pos, head, member)
>
> So following this suggesting will actually bring them back to sync...
>
> The only issue I can see is that as you've said, they're used almost everywhere,
> so doing something to change that will require some coordination.
if this brings hlist and list in sync, then it looks like an
improvement. It might be good to propose this change as a separate
patchset.
Thanks,
Mathieu
>
>
> Thanks,
> Sasha
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-09-06 17:01 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
Josh Triplett, Steven Rostedt, lw-BthXqXjhjHXQFUHtdCDX3A,
teigland-H+wXaHxf7aLQT0dZR+AlfA, axboe-tSWWG44O7X1aa/9Udqfwiw,
linux-nfs-u79uwXL29TY76Z2rM5mHXA, edumazet-hpIqsD4AKlfQT0dZR+AlfA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Pedro Alves,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
Tejun Heo, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120906165055.GC7385@Krystal>
On 09/06/2012 06:50 PM, Mathieu Desnoyers wrote:
> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
>> On 09/06/2012 06:00 PM, Steven Rostedt wrote:
>>>>> I think that that code doesn't make sense. The users of hlist_for_each_* aren't
>>>>> supposed to be changing the loop cursor.
>>> I totally agree. Modifying the 'node' pointer is just asking for issues.
>>> Yes that is error prone, but not due to the double loop. It's due to the
>>> modifying of the node pointer that is used internally by the loop
>>> counter. Don't do that :-)
>>
>> While we're on this subject, I haven't actually seen hlist_for_each_entry() code
>> that even *touches* 'pos'.
>>
>> Will people yell at me loudly if I change the prototype of those macros to be:
>>
>> hlist_for_each_entry(tpos, head, member)
>>
>> (Dropping the 'pos' parameter), and updating anything that calls those macros to
>> drop it as well?
>
> I think the intent there is to keep hlist macros and list macros
> slightly in sync. Given those are vastly used, I'm not sure you want to
> touch them. But hey, that's just my 2 cents.
Actually, the corresponding list macro looks like this:
list_for_each_entry(pos, head, member)
With 'pos' being the equivalent of 'tpos' in the hlist macros (the type *).
Changing hlist macro will make them both look as follows:
hlist_for_each_entry(pos, head, member)
list_for_each_entry(pos, head, member)
So following this suggesting will actually bring them back to sync...
The only issue I can see is that as you've said, they're used almost everywhere,
so doing something to change that will require some coordination.
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-09-06 16:50 UTC (permalink / raw)
To: Sasha Levin
Cc: Steven Rostedt, Josh Triplett, Pedro Alves, Tejun Heo,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, mingo-X9Un+BFzKDI,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, aarcange-H+wXaHxf7aLQT0dZR+AlfA,
ericvh-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w, axboe-tSWWG44O7X1aa/9Udqfwiw,
agk-H+wXaHxf7aLQT0dZR+AlfA, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
neilb-l3A5Bk7waGM, ccaulfie-H+wXaHxf7aLQT0dZR+AlfA,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw, fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
jesse-l0M0P4e3n4LQT0dZR+AlfA,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ejt-H+wXaHxf7aLQT0dZR+AlfA, snitzer-H+wXaHxf7aLQT0dZR+AlfA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
dev-yBygre7rU0TnMu66kgdUjQ, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
lw-BthXqXjhjHXQFUHtdCDX3A
In-Reply-To: <5048CDA2.10300-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
* Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> On 09/06/2012 06:00 PM, Steven Rostedt wrote:
> >> > I think that that code doesn't make sense. The users of hlist_for_each_* aren't
> >> > supposed to be changing the loop cursor.
> > I totally agree. Modifying the 'node' pointer is just asking for issues.
> > Yes that is error prone, but not due to the double loop. It's due to the
> > modifying of the node pointer that is used internally by the loop
> > counter. Don't do that :-)
>
> While we're on this subject, I haven't actually seen hlist_for_each_entry() code
> that even *touches* 'pos'.
>
> Will people yell at me loudly if I change the prototype of those macros to be:
>
> hlist_for_each_entry(tpos, head, member)
>
> (Dropping the 'pos' parameter), and updating anything that calls those macros to
> drop it as well?
I think the intent there is to keep hlist macros and list macros
slightly in sync. Given those are vastly used, I'm not sure you want to
touch them. But hey, that's just my 2 cents.
Thanks,
Mathieu
>
>
> Thanks,
> Sasha
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 9/10] drivers/net/wireless/rtlwifi/rtl8192de/phy.c: removes unnecessary semicolon
From: Larry Finger @ 2012-09-06 16:49 UTC (permalink / raw)
To: Peter Senna Tschudin
Cc: kernel-janitors, Chaoming Li, John W. Linville, linux-wireless,
netdev, linux-kernel
In-Reply-To: <1346947757-10481-10-git-send-email-peter.senna@gmail.com>
On 09/06/2012 11:09 AM, Peter Senna Tschudin wrote:
> From: Peter Senna Tschudin <peter.senna@gmail.com>
>
> removes unnecessary semicolon
>
> Found by Coccinelle: http://coccinelle.lip6.fr/
>
> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
>
> ---
> drivers/net/wireless/rtlwifi/rtl8192de/phy.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff -u -p a/drivers/net/wireless/rtlwifi/rtl8192de/phy.c b/drivers/net/wireless/rtlwifi/rtl8192de/phy.c
> --- a/drivers/net/wireless/rtlwifi/rtl8192de/phy.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192de/phy.c
> @@ -1314,7 +1314,7 @@ static void _rtl92d_phy_restore_rf_env(s
> struct bb_reg_def *pphyreg = &rtlphy->phyreg_def[rfpath];
>
> RT_TRACE(rtlpriv, COMP_RF, DBG_LOUD, "=====>\n");
> - /*----Restore RFENV control type----*/ ;
> + /*----Restore RFENV control type----*/
> switch (rfpath) {
> case RF90_PATH_A:
> case RF90_PATH_C:
>
>
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
Thanks,
Larry
^ permalink raw reply
* Re: [PATCHv3] virtio-spec: virtio network device multiqueue support
From: Paolo Bonzini @ 2012-09-06 16:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20120906120828.GA1534@redhat.com>
Il 06/09/2012 14:08, Michael S. Tsirkin ha scritto:
> +#define VIRTIO_NET_CTRL_STEERING_HOST 1
You didn't change this occurrence.
Paolo
^ permalink raw reply
* Re: [PATCH] sctp: check dst validity after IPsec operations
From: Nicolas Dichtel @ 2012-09-06 16:40 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: sri, linux-sctp, netdev
In-Reply-To: <5048C984.3030306@gmail.com>
Le 06/09/2012 18:04, Vlad Yasevich a écrit :
> On 09/06/2012 01:40 PM, Nicolas Dichtel wrote:
>> dst stored in struct sctp_transport needs to be recalculated when ipsec policy
>> are updated. We use flow_cache_genid for that.
>>
>> For example, if a SCTP connection is established and then an IPsec policy is
>> set, the old SCTP flow will not be updated and thus will not use the new
>> IPsec policy.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> why doesn't this need to be done for TCP? What makes SCTP special in this case?
Tests prove that the pb does not exist with TCP. I made the patch some times
ago, I will look again deeply to find the difference.
>
> ip_queue_xmit does an __sk_dst_check() which is essentially what
> sctp_transport_dst_check() does. That should determine if the currently cached
> route is valid or not.
The problem is that route will not be invalidated, because dst->check() has no
xfrm path so xfrm_dst_check() will never be called.
Regards,
Nicolas
>
> Looks like sctp may need to change to using ip_route_output_ports() call
> as ip_route_output_key may not do all that is necessary
>
> -vlad
>
>> ---
>> include/net/sctp/sctp.h | 3 ++-
>> include/net/sctp/structs.h | 3 +++
>> net/sctp/ipv6.c | 1 +
>> net/sctp/protocol.c | 1 +
>> 4 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index 9c6414f..3267246 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -712,7 +712,8 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
>> */
>> static inline struct dst_entry *sctp_transport_dst_check(struct
>> sctp_transport *t)
>> {
>> - if (t->dst && !dst_check(t->dst, 0)) {
>> + if ((t->dst && !dst_check(t->dst, 0)) ||
>> + (t->flow_cache_genid != atomic_read(&flow_cache_genid))) {
>> dst_release(t->dst);
>> t->dst = NULL;
>> }
>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>> index 0fef00f..9dab882 100644
>> --- a/include/net/sctp/structs.h
>> +++ b/include/net/sctp/structs.h
>> @@ -948,6 +948,9 @@ struct sctp_transport {
>>
>> /* 64-bit random number sent with heartbeat. */
>> __u64 hb_nonce;
>> +
>> + /* used to check validity of dst */
>> + __u32 flow_cache_genid;
>> };
>>
>> struct sctp_transport *sctp_transport_new(struct net *, const union
>> sctp_addr *,
>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> index ea14cb4..2ed7410 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -349,6 +349,7 @@ out:
>> struct rt6_info *rt;
>> rt = (struct rt6_info *)dst;
>> t->dst = dst;
>> + t->flow_cache_genid = atomic_read(&flow_cache_genid);
>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>> &rt->rt6i_dst.addr, &fl6->saddr);
>> } else {
>> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
>> index 2d51842..4795a1a 100644
>> --- a/net/sctp/protocol.c
>> +++ b/net/sctp/protocol.c
>> @@ -512,6 +512,7 @@ out_unlock:
>> rcu_read_unlock();
>> out:
>> t->dst = dst;
>> + t->flow_cache_genid = atomic_read(&flow_cache_genid);
>> if (dst)
>> SCTP_DEBUG_PRINTK("rt_dst:%pI4, rt_src:%pI4\n",
>> &fl4->daddr, &fl4->saddr);
>>
>
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-09-06 16:21 UTC (permalink / raw)
To: Steven Rostedt
Cc: Josh Triplett, Mathieu Desnoyers, Pedro Alves, Tejun Heo,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, mingo-X9Un+BFzKDI,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, aarcange-H+wXaHxf7aLQT0dZR+AlfA,
ericvh-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w, axboe-tSWWG44O7X1aa/9Udqfwiw,
agk-H+wXaHxf7aLQT0dZR+AlfA, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
neilb-l3A5Bk7waGM, ccaulfie-H+wXaHxf7aLQT0dZR+AlfA,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw, fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
jesse-l0M0P4e3n4LQT0dZR+AlfA,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ejt-H+wXaHxf7aLQT0dZR+AlfA, snitzer-H+wXaHxf7aLQT0dZR+AlfA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
dev-yBygre7rU0TnMu66kgdUjQ, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
lw-BthXqXjhjHXQFUHtdCDX3A
In-Reply-To: <1346947206.1680.36.camel-f9ZlEuEWxVcJvu8Pb33WZ0EMvNT87kid@public.gmane.org>
On 09/06/2012 06:00 PM, Steven Rostedt wrote:
>> > I think that that code doesn't make sense. The users of hlist_for_each_* aren't
>> > supposed to be changing the loop cursor.
> I totally agree. Modifying the 'node' pointer is just asking for issues.
> Yes that is error prone, but not due to the double loop. It's due to the
> modifying of the node pointer that is used internally by the loop
> counter. Don't do that :-)
While we're on this subject, I haven't actually seen hlist_for_each_entry() code
that even *touches* 'pos'.
Will people yell at me loudly if I change the prototype of those macros to be:
hlist_for_each_entry(tpos, head, member)
(Dropping the 'pos' parameter), and updating anything that calls those macros to
drop it as well?
Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/5] dev_<level> and dynamic_debug cleanups
From: Greg Kroah-Hartman @ 2012-09-06 16:13 UTC (permalink / raw)
To: Jim Cromie, Jason Baron
Cc: Kay Sievers, Joe Perches, Andrew Morton, netdev, David S. Miller,
linux-kernel
In-Reply-To: <CAJfuBxyToA9-=c3H88GGba3GFX64kdbP4OCEC0f4trnBnjpU-w@mail.gmail.com>
On Thu, Aug 30, 2012 at 09:48:12PM -0600, Jim Cromie wrote:
> On Thu, Aug 30, 2012 at 11:43 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> > On Sun, Aug 26, 2012 at 5:25 AM, Joe Perches <joe@perches.com> wrote:
> >> The recent commit to fix dynamic_debug was a bit unclean.
> >> Neaten the style for dynamic_debug.
> >> Reduce the stack use of message logging that uses netdev_printk
> >> Add utility functions dev_printk_emit and dev_vprintk_emit for /dev/kmsg.
> >>
> >> Joe Perches (5):
> >> dev_dbg/dynamic_debug: Update to use printk_emit, optimize stack
> >> netdev_printk/dynamic_netdev_dbg: Directly call printk_emit
> >> netdev_printk/netif_printk: Remove a superfluous logging colon
> >> dev: Add dev_vprintk_emit and dev_printk_emit
> >> device and dynamic_debug: Use dev_vprintk_emit and dev_printk_emit
> >>
> >
> > Ive tested this on 2 builds differing only by DYNAMIC_DEBUG
> > It works for me on x86-64
> >
> > However, I just booted a non-dyndbg build on x86-32, and got this.
> >
>
> Ok, transient error, went away with a clean build.
>
> tested-by: Jim Cromie <jim.cromie@gmail.com>
Jason, any ACK on these, or any of the other random dynamic debug
patches floating around? What am I supposed to be applying here?
thanks,
greg k-h
^ permalink raw reply
* [PATCH 9/10] drivers/net/wireless/rtlwifi/rtl8192de/phy.c: removes unnecessary semicolon
From: Peter Senna Tschudin @ 2012-09-06 16:09 UTC (permalink / raw)
To: Larry Finger
Cc: kernel-janitors, Peter Senna Tschudin, Chaoming Li,
John W. Linville, linux-wireless, netdev, linux-kernel
In-Reply-To: <1346947757-10481-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
removes unnecessary semicolon
Found by Coccinelle: http://coccinelle.lip6.fr/
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
drivers/net/wireless/rtlwifi/rtl8192de/phy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/drivers/net/wireless/rtlwifi/rtl8192de/phy.c b/drivers/net/wireless/rtlwifi/rtl8192de/phy.c
--- a/drivers/net/wireless/rtlwifi/rtl8192de/phy.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192de/phy.c
@@ -1314,7 +1314,7 @@ static void _rtl92d_phy_restore_rf_env(s
struct bb_reg_def *pphyreg = &rtlphy->phyreg_def[rfpath];
RT_TRACE(rtlpriv, COMP_RF, DBG_LOUD, "=====>\n");
- /*----Restore RFENV control type----*/ ;
+ /*----Restore RFENV control type----*/
switch (rfpath) {
case RF90_PATH_A:
case RF90_PATH_C:
^ permalink raw reply
* [PATCH 8/10] net/mac80211/scan.c: removes unnecessary semicolon
From: Peter Senna Tschudin @ 2012-09-06 16:09 UTC (permalink / raw)
To: John W. Linville
Cc: kernel-janitors, Peter Senna Tschudin, Johannes Berg,
David S. Miller, linux-wireless, netdev, linux-kernel
In-Reply-To: <1346947757-10481-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
removes unnecessary semicolon
Found by Coccinelle: http://coccinelle.lip6.fr/
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
net/mac80211/scan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/net/mac80211/scan.c b/net/mac80211/scan.c
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -407,7 +407,7 @@ static void ieee80211_scan_state_send_pr
enum ieee80211_band band = local->hw.conf.channel->band;
sdata = rcu_dereference_protected(local->scan_sdata,
- lockdep_is_held(&local->mtx));;
+ lockdep_is_held(&local->mtx));
for (i = 0; i < local->scan_req->n_ssids; i++)
ieee80211_send_probe_req(
^ permalink raw reply
* Re: changing usbnet's API to better deal with cdc-ncm
From: Ming Lei @ 2012-09-06 16:09 UTC (permalink / raw)
To: Bjørn Mork
Cc: Oliver Neukum, alexey.orishko-0IS4wlFg1OjSUeElwK9/Pw,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <87a9x33656.fsf-lbf33ChDnrE/G1V5fR+Y7Q@public.gmane.org>
On Thu, Sep 6, 2012 at 4:30 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
> Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> On Thu, Sep 6, 2012 at 4:12 AM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>>> Hi,
>>>
>>> looking at cdc-ncm it seeems to me that cdc-ncm is forced to play
>>> very dirty games because usbnet doesn't have a notion about aggregating
>>> packets for a single transfer.
>>
>> The Ethernet API we are using does not support transmitting multiple Ethernet
>> frames in a single call, so the aggregation things should be done inside low
>> level driver, in fact it is just what some wlan(802.11n) drivers have
>> been doing.
>>
>> IMO, the current .tx_fixup is intelligent enough to handle aggregation:
>>
>> - return a skb_out for sending if low level drivers think it is
>> ready to send
>> the aggregated frames
>> - return NULL if the low level drivers think they need to wait
>> for more frames
>>
>> Of course, the low level drivers need to start a timer to trigger sending
>> remainder frames in case of timeout and no further frames come from
>> upper protocol stack.
>>
>> Looks the introduced .tx_bundle is not necessary since .tx_fixup is OK.
>
> The minidriver does not have any information about tx in progress. The
Inside .tx_fixup, the low level driver will get the tx progress information.
> strategy above, which is what cdc_ncm uses today, is fine as long as you
> always want to wait as long as you always want to fill as many frames as
> possible in each packet. But what if the queue is empty and the device
For cdc_ncm, the wait time is controlled by cdc_ncm driver, see
cdc_ncm_tx_timeout_start().
> is just sitting there doing nothing while you are waiting for more
> frames? Then you are just adding unnecessary latency.
As said above, cdc_ncm can control the latency by itself.
>
> There are also several usbnet minidrivers for protocols with frame
> aggregation support. Most of them do not currently implement tx
> aggregation, possibly due to the complexity. This makes a common
> aggregation framework interesting in any case. Reimplementing similar
> loginc in multiple minidrivers is meaningless.
If we can abstract some common things about aggregation, it should be
meaningful. As far as I know, most of aggregation protocol is very different,
so almost all aggregation work is only done by low level driver, such as
cdc_ncm.
If we want to implement some aggregation framework, maybe below is
one solution, correct me if it is wrong.
usbnet_start_xmit():
to_send = info->tx_bundle(in_skb, &out_skb)
if (!to_send) {
usbnet_tx_timeout_start(usbnet);
goto not_drop;
}
skb = info->tx_fixup(dev, out_skb, GFP_ATOMIC);
...
Looks the above is no much advantage than doing bundling in .tx_fixup
just like cdc_ncm.
>
> I believe Oliver is adding very useful functionality to usbnet here.
> Functionality which at least cdc_ncm and possibly other existing
> minidrivers can take advantage of immediately. There also seems to be a
Sorry, I don't know other minidrivers, and is there some common
things in building aggregation with cdc_ncm?
Considered that the patch doesn't implement wait_for_more, suggest
Oliver to post a new one with basic wait_for_more support for further
discussion.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 1/10] drivers/net/usb/sierra_net.c: removes unnecessary semicolon
From: Peter Senna Tschudin @ 2012-09-06 16:09 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: kernel-janitors, Peter Senna Tschudin, linux-usb, netdev,
linux-kernel
From: Peter Senna Tschudin <peter.senna@gmail.com>
removes unnecessary semicolon
Found by Coccinelle: http://coccinelle.lip6.fr/
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
drivers/net/usb/sierra_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -842,7 +842,7 @@ static int sierra_net_rx_fixup(struct us
netdev_err(dev->net, "HIP/ETH: Invalid pkt\n");
dev->net->stats.rx_frame_errors++;
- /* dev->net->stats.rx_errors incremented by caller */;
+ /* dev->net->stats.rx_errors incremented by caller */
return 0;
}
^ permalink raw reply
* Re: [PATCH] sctp: check dst validity after IPsec operations
From: Vlad Yasevich @ 2012-09-06 16:04 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: sri, linux-sctp, netdev
In-Reply-To: <1346953229-3825-1-git-send-email-nicolas.dichtel@6wind.com>
On 09/06/2012 01:40 PM, Nicolas Dichtel wrote:
> dst stored in struct sctp_transport needs to be recalculated when ipsec policy
> are updated. We use flow_cache_genid for that.
>
> For example, if a SCTP connection is established and then an IPsec policy is
> set, the old SCTP flow will not be updated and thus will not use the new
> IPsec policy.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
why doesn't this need to be done for TCP? What makes SCTP special in
this case?
ip_queue_xmit does an __sk_dst_check() which is essentially what
sctp_transport_dst_check() does. That should determine if the currently
cached route is valid or not.
Looks like sctp may need to change to using ip_route_output_ports() call
as ip_route_output_key may not do all that is necessary
-vlad
> ---
> include/net/sctp/sctp.h | 3 ++-
> include/net/sctp/structs.h | 3 +++
> net/sctp/ipv6.c | 1 +
> net/sctp/protocol.c | 1 +
> 4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 9c6414f..3267246 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -712,7 +712,8 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
> */
> static inline struct dst_entry *sctp_transport_dst_check(struct sctp_transport *t)
> {
> - if (t->dst && !dst_check(t->dst, 0)) {
> + if ((t->dst && !dst_check(t->dst, 0)) ||
> + (t->flow_cache_genid != atomic_read(&flow_cache_genid))) {
> dst_release(t->dst);
> t->dst = NULL;
> }
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 0fef00f..9dab882 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -948,6 +948,9 @@ struct sctp_transport {
>
> /* 64-bit random number sent with heartbeat. */
> __u64 hb_nonce;
> +
> + /* used to check validity of dst */
> + __u32 flow_cache_genid;
> };
>
> struct sctp_transport *sctp_transport_new(struct net *, const union sctp_addr *,
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index ea14cb4..2ed7410 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -349,6 +349,7 @@ out:
> struct rt6_info *rt;
> rt = (struct rt6_info *)dst;
> t->dst = dst;
> + t->flow_cache_genid = atomic_read(&flow_cache_genid);
> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
> &rt->rt6i_dst.addr, &fl6->saddr);
> } else {
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 2d51842..4795a1a 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -512,6 +512,7 @@ out_unlock:
> rcu_read_unlock();
> out:
> t->dst = dst;
> + t->flow_cache_genid = atomic_read(&flow_cache_genid);
> if (dst)
> SCTP_DEBUG_PRINTK("rt_dst:%pI4, rt_src:%pI4\n",
> &fl4->daddr, &fl4->saddr);
>
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Steven Rostedt @ 2012-09-06 16:00 UTC (permalink / raw)
To: Sasha Levin
Cc: Josh Triplett, Mathieu Desnoyers, Pedro Alves, Tejun Heo,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, mingo-X9Un+BFzKDI,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, aarcange-H+wXaHxf7aLQT0dZR+AlfA,
ericvh-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w, axboe-tSWWG44O7X1aa/9Udqfwiw,
agk-H+wXaHxf7aLQT0dZR+AlfA, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
neilb-l3A5Bk7waGM, ccaulfie-H+wXaHxf7aLQT0dZR+AlfA,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw, fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
jesse-l0M0P4e3n4LQT0dZR+AlfA,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ejt-H+wXaHxf7aLQT0dZR+AlfA, snitzer-H+wXaHxf7aLQT0dZR+AlfA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
dev-yBygre7rU0TnMu66kgdUjQ, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
lw-BthXqXjhjHXQFUHtdCDX3A
In-Reply-To: <5048C615.4070204-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Thu, 2012-09-06 at 17:49 +0200, Sasha Levin wrote:
>
> > Looks reasonable. However, it would break (or rather, not break) on
> > code like this:
> >
> > hash_for_each_entry(...) {
> > if (...) {
> > foo(node);
> > node = NULL;
ug, I didn't even notice this. Ignore my last email :-p
/me needs to wake-up a bit more.
> > break;
> > }
> > }
> >
> > Hiding the double loop still seems error-prone.
>
> I think that that code doesn't make sense. The users of hlist_for_each_* aren't
> supposed to be changing the loop cursor.
I totally agree. Modifying the 'node' pointer is just asking for issues.
Yes that is error prone, but not due to the double loop. It's due to the
modifying of the node pointer that is used internally by the loop
counter. Don't do that :-)
>
> We have three options here:
>
> 1. Stuff everything into a single for(). While not too difficult, it will make
> the readability of the code difficult as it will force us to abandon using
> hlist_for_each_* macros.
>
> 2. Over-complicate everything, and check for 'node == NULL && obj &&
> obj->member.next == NULL' instead. That one will fail only if the user has
> specifically set the object as the last object in the list and the node as NULL.
>
> 3. Use 2 loops which might not work properly if the user does something odd,
> with a big fat warning above them.
>
>
> To sum it up, I'd rather go with 3 and let anyone who does things he shouldn't
> be doing break.
I agree, the double loop itself is not error prone. If you are modifying
'node' you had better know what the hell you are doing.
Actually, it may be something that is legitimate. That is, if you want
to skip to the next bucket, just set node to NULL and do the break (as
Josh had done). This would break if the macro loop changed later on, but
hey, like I said, it's error prone ;-) If you really want to do that,
then hand coding the double loop would be a better bet. IOW, don't use
the macro loop.
-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-09-06 15:49 UTC (permalink / raw)
To: Josh Triplett
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
Steven Rostedt, lw-BthXqXjhjHXQFUHtdCDX3A, Mathieu Desnoyers,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Pedro Alves, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
ejt-H+wXaHxf7aLQT0dZR+AlfA, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA, Tejun Heo,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120906145545.GA17332@leaf>
On 09/06/2012 04:55 PM, Josh Triplett wrote:
> On Thu, Sep 06, 2012 at 03:53:58PM +0200, Sasha Levin wrote:
>> On 09/04/2012 07:01 PM, Mathieu Desnoyers wrote:
>>>> #define do_for_each_ftrace_rec(pg, rec) \
>>>>> for (pg = ftrace_pages_start, rec = &pg->records[pg->index]; \
>>>>> pg && rec == &pg->records[pg->index]; \
>>>>> pg = pg->next) \
>>>>> for (rec = pg->records; rec < &pg->records[pg->index]; rec++)
>>> Maybe in some cases there might be ways to combine the two loops into
>>> one ? I'm not seeing exactly how to do it for this one, but it should
>>> not be impossible. If the inner loop condition can be moved to the outer
>>> loop, and if we use (blah ? loop1_conf : loop2_cond) to test for
>>> different conditions depending on the context, and do the same for the
>>> 3rd argument of the for() loop. The details elude me for now though, so
>>> maybe it's complete non-sense ;)
>>>
>>> It might not be that useful for do_for_each_ftrace_rec, but if we can do
>>> it for the hash table iterator, it might be worth it.
>>
>> So I think that for the hash iterator it might actually be simpler.
>>
>> My solution to making 'break' work in the iterator is:A code like that doesn
>>
>> for (bkt = 0, node = NULL; bkt < HASH_SIZE(name) && node == NULL; bkt++)
>> hlist_for_each_entry(obj, node, &name[bkt], member)
>>
>> We initialize our node loop cursor with NULL in the external loop, and the
>> external loop will have a new condition to loop while that cursor is NULL.
>>
>> My logic is that we can only 'break' when we are iterating over an object in the
>> internal loop. If we're iterating over an object in that loop then 'node != NULL'.
>>
>> This way, if we broke from within the internal loop, the external loop will see
>> node as not NULL, and so it will stop looping itself. On the other hand, if the
>> internal loop has actually ended, then node will be NULL, and the outer loop
>> will keep running.
>>
>> Is there anything I've missed?
>
> Looks reasonable. However, it would break (or rather, not break) on
> code like this:
>
> hash_for_each_entry(...) {
> if (...) {
> foo(node);
> node = NULL;
> break;
> }
> }
>
> Hiding the double loop still seems error-prone.
I think that that code doesn't make sense. The users of hlist_for_each_* aren't
supposed to be changing the loop cursor.
We have three options here:
1. Stuff everything into a single for(). While not too difficult, it will make
the readability of the code difficult as it will force us to abandon using
hlist_for_each_* macros.
2. Over-complicate everything, and check for 'node == NULL && obj &&
obj->member.next == NULL' instead. That one will fail only if the user has
specifically set the object as the last object in the list and the node as NULL.
3. Use 2 loops which might not work properly if the user does something odd,
with a big fat warning above them.
To sum it up, I'd rather go with 3 and let anyone who does things he shouldn't
be doing break.
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH 04/10] net/macb: Fix a race in macb_start_xmit()
From: Havard Skinnemoen @ 2012-09-06 15:49 UTC (permalink / raw)
To: David Miller
Cc: nicolas.ferre, netdev, linux-arm-kernel, plagnioj, jamie,
linux-kernel, patrice.vilchez
In-Reply-To: <20120905.173023.2235590074897156746.davem@davemloft.net>
On Wed, Sep 5, 2012 at 11:30 PM, David Miller <davem@davemloft.net> wrote:
> From: Nicolas Ferre <nicolas.ferre@atmel.com>
> Date: Wed, 5 Sep 2012 10:19:11 +0200
>
>> From: Havard Skinnemoen <havard@skinnemoen.net>
>>
>> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
>> If an underrun just happened (we do this with interrupts disabled, so it might
>> not have been handled yet), the controller starts transmitting from the first
>> entry in the ring, which is usually wrong.
>> Restart the controller after error handling.
>>
>> Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
>> [nicolas.ferre@atmel.com: split patch in topics]
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>
> Accumulating special case code and checks into the hot path of TX packet
> processing is extremely unwise.
>
> Instead, when you handle the TX error conditions and reset the chip you
> should first ensure that there are no flows of control in the transmit
> function of your driver by using the appropriate locking et al. facilities.
IIRC, the hardware resets the ring pointers when an error happens, and
if we set TSTART right after that happens, the hardware will happily
transmit whatever is sitting in the beginning of the ring. This is
what I was trying to avoid.
The details are a bit hazy as it's been a while since I looked at
this, so it could be that simply letting it happen and using a bigger
hammer during reset processing might work just as well. Just want to
make sure y'all understand that we're talking about a race against
hardware, not against interrupt handlers, threads or anything that can
be solved by locking :)
Havard
^ permalink raw reply
* [PATCH] sctp: check dst validity after IPsec operations
From: Nicolas Dichtel @ 2012-09-06 17:40 UTC (permalink / raw)
To: vyasevich, sri, linux-sctp; +Cc: netdev, Nicolas Dichtel
dst stored in struct sctp_transport needs to be recalculated when ipsec policy
are updated. We use flow_cache_genid for that.
For example, if a SCTP connection is established and then an IPsec policy is
set, the old SCTP flow will not be updated and thus will not use the new
IPsec policy.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
include/net/sctp/sctp.h | 3 ++-
include/net/sctp/structs.h | 3 +++
net/sctp/ipv6.c | 1 +
net/sctp/protocol.c | 1 +
4 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 9c6414f..3267246 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -712,7 +712,8 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
*/
static inline struct dst_entry *sctp_transport_dst_check(struct sctp_transport *t)
{
- if (t->dst && !dst_check(t->dst, 0)) {
+ if ((t->dst && !dst_check(t->dst, 0)) ||
+ (t->flow_cache_genid != atomic_read(&flow_cache_genid))) {
dst_release(t->dst);
t->dst = NULL;
}
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 0fef00f..9dab882 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -948,6 +948,9 @@ struct sctp_transport {
/* 64-bit random number sent with heartbeat. */
__u64 hb_nonce;
+
+ /* used to check validity of dst */
+ __u32 flow_cache_genid;
};
struct sctp_transport *sctp_transport_new(struct net *, const union sctp_addr *,
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index ea14cb4..2ed7410 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -349,6 +349,7 @@ out:
struct rt6_info *rt;
rt = (struct rt6_info *)dst;
t->dst = dst;
+ t->flow_cache_genid = atomic_read(&flow_cache_genid);
SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
&rt->rt6i_dst.addr, &fl6->saddr);
} else {
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 2d51842..4795a1a 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -512,6 +512,7 @@ out_unlock:
rcu_read_unlock();
out:
t->dst = dst;
+ t->flow_cache_genid = atomic_read(&flow_cache_genid);
if (dst)
SCTP_DEBUG_PRINTK("rt_dst:%pI4, rt_src:%pI4\n",
&fl4->daddr, &fl4->saddr);
--
1.7.12
^ 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