* Re: [PATCH] net: dev_getfirstbyhwtype() optimization
From: Paul E. McKenney @ 2010-03-19 11:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1268975642.2894.218.camel@edumazet-laptop>
On Fri, Mar 19, 2010 at 06:14:02AM +0100, Eric Dumazet wrote:
> Le jeudi 18 mars 2010 à 19:32 -0700, Paul E. McKenney a écrit :
> > On Thu, Mar 18, 2010 at 10:27:25PM +0100, Eric Dumazet wrote:
> > > Use RCU to avoid RTNL use in dev_getfirstbyhwtype()
> > >
> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > > ---
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 17b1686..0f2e9fc 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -772,14 +772,17 @@ EXPORT_SYMBOL(__dev_getfirstbyhwtype);
> > >
> > > struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type)
> > > {
> > > - struct net_device *dev;
> > > + struct net_device *dev, *ret = NULL;
> > >
> > > - rtnl_lock();
> > > - dev = __dev_getfirstbyhwtype(net, type);
> > > - if (dev)
> > > - dev_hold(dev);
> > > - rtnl_unlock();
> > > - return dev;
> > > + rcu_read_lock();
> > > + for_each_netdev_rcu(net, dev)
> > > + if (dev->type == type) {
> > > + dev_hold(dev);
> > > + ret = dev;
> > > + break;
> > > + }
> > > + rcu_read_unlock();
> > > + return ret;
> >
> > Looks good, but I don't understand how it helps to introduce the
> > local variable "ret".
> >
>
> Thanks for reviewing Paul !
>
> Not only it helps, its necessary :)
>
> for_each_netdev_rcu(net, dev) {
> if (cond) {
> dev_hold(dev);
> break;
> }
> }
> makes no guarantee dev is NULL if we hit the list end :
>
>
> /**
> * list_for_each_entry_rcu - iterate over rcu list of given type
> * @pos: the type * to use as a loop cursor.
> * @head: the head for your list.
> * @member: the name of the list_struct within the struct.
> *
> * This list-traversal primitive may safely run concurrently with
> * the _rcu list-mutation primitives such as list_add_rcu()
> * as long as the traversal is guarded by rcu_read_lock().
> */
> #define list_for_each_entry_rcu(pos, head, member) \
> for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> prefetch(pos->member.next), &pos->member != (head); \
> pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
Right! Got it, thank you!
Thanx, Paul
^ permalink raw reply
* Re: [Bugme-new] [Bug 15507] New: kernel misses 3rd part of tcp handshake (ACK), stays in SYN_RECV state
From: Joshua Roys @ 2010-03-19 11:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Andrew Morton, netdev, bugzilla-daemon, bugme-daemon
In-Reply-To: <1268953951.2894.194.camel@edumazet-laptop>
On 03/18/2010 07:12 PM, Eric Dumazet wrote:
>
> I would say this is expected if httpd server set DEFER_ACCEPT socket
> option.
>
>
>
Gah! You've got to be kidding :) If that's what it is, close the
bug... and I'll go read some man-pages!
Josh
^ permalink raw reply
* Re: Help: the 82577 network driver can't work
From: yucheng.wang @ 2010-03-19 11:10 UTC (permalink / raw)
To: Allan, Bruce W; +Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org
In-Reply-To: <8DD2590731AB5D4C9DBF71A877482A90619CAAC4@orsmsx509.amr.corp.intel.com>
Allan, Bruce W wrote:
> On Wednesday, March 17, 2010 10:40 PM, yucheng.wang wrote:
>> Hi all,
>>
>> I need some help for intel 82577 network card driver, named
>> CONFIG_E1000E in the kernel config.
>>
>> In the kernel 2.6.31 and 2.6.32, the patch works well, but in the
>> kernel
>> 2.6.33 and 2.6.34-rc1 the dmesg shows some problem below,and the
>> network card can't work.
>> **********************************************************************
>> e1000e: Intel(R) PRO/1000 Network Driver - 1.0.2-k2
>> e1000e: Copyright (c) 1999 - 2009 Intel Corporation.
>> e1000e 0000:00:19.0: PCI INT A -> GSI 20 (level, low) -> IRQ 20
>> 0000:00:19.0: 0000:00:19.0: Write-protect NVM Disabled
>> 0000:00:19.0: 0000:00:19.0: Failed to initialize MSI interrupts.
>> Falling back to legacy interrupts.
>> ata4: SATA link down (SStatus 0 SControl 300)
>> ata3: SATA link down (SStatus 0 SControl 300)
>> ata2.00: SATA link down (SStatus 0 SControl 300)
>> ata2.01: SATA link down (SStatus 0 SControl 300)
>> ata1.00: SATA link down (SStatus 0 SControl 0)
>> ata1.01: SATA link down (SStatus 0 SControl 300)
>> e1000e 0000:00:19.0: PCI INT A disabled
>> e1000e: probe of 0000:00:19.0 failed with error -3
>> ********************************************************************
>> I read the source code using 2.6.34-rc-1 kernel.
>> When calling e1000e_git_phy_id(), the function returns a wrong value
>> "-3". The kernel 2.6.31 and 2.6.32 ignores this error, and the kernel
>> run well. The function e1000_acquire_swflag_ich8lan( ), called by
>> e1000e_get_phy_id(), return "-3".
>> ************************** The code
>> ********************************* extcnf_ctrl |=
>> E1000_EXTCNF_CTRL_SWFLAG; ew32(EXTCNF_CTRL, extcnf_ctrl);
>>
>> while (timeout) {
>> extcnf_ctrl = er32(EXTCNF_CTRL);
>> if (extcnf_ctrl& E1000_EXTCNF_CTRL_SWFLAG)
>> break;
>>
>> mdelay(1);
>> timeout--;
>> }
>>
>> if (!timeout) {
>> e_dbg("Failed to acquire the semaphore.\n");
>> extcnf_ctrl&= ~E1000_EXTCNF_CTRL_SWFLAG;
>> ew32(EXTCNF_CTRL, extcnf_ctrl);
>> ret_val = -E1000_ERR_CONFIG;
>> goto out;
>> }
>> ************************** The code
>> ********************************* The kernel can't set the bit
>> E1000_EXTCNF_CTRL_SWFLAG of the register EXTCNF_CTRL.
>>
>> We found the information in the 82577 user manual:
>> In the Extended Configuration Control Register.
>> Software Semaphore FLAG (SWFLAG). This bit is set by the device
>> driver to gain access permission to shared CSR registers with the
>> firmware and hardware. The bit is initialized on power up PCI reset
>> and software reset. and the default value when Reset is "0b".
>>
>> I don't know why the bit E1000_EXTCNF_CTRL_SWFLAG is not right, and
>> not find any information about writing the bit. Due to the bit
>> returns 0 when reset the controller, we wrote the patch below.
>> *****************************The
>> patch********************************** diff --git
>> a/drivers/net/e1000e/ich8lan.c b/drivers/net/e1000e/ich8lan.c index
>> 8b6ecd1..b3e20f4 100644 --- a/drivers/net/e1000e/ich8lan.c
>> +++ b/drivers/net/e1000e/ich8lan.c
>> @@ -224,6 +224,7 @@ static void
>> e1000_power_down_phy_copper_ich8lan(struct e1000_hw *hw);
>> static void e1000_lan_init_done_ich8lan(struct e1000_hw *hw);
>> static s32 e1000_k1_gig_workaround_hv(struct e1000_hw *hw, bool
>> link); static s32 e1000_set_mdio_slow_mode_hv(struct e1000_hw *hw);
>> +static s32 e1000_reset_hw_ich8lan(struct e1000_hw *hw);
>>
>> static inline u16 __er16flash(struct e1000_hw *hw, unsigned long
>> reg) {
>> @@ -276,8 +277,12 @@ static s32 e1000_init_phy_params_pchlan(struct
>> e1000_hw *hw)
>>
>> phy->id = e1000_phy_unknown;
>> ret_val = e1000e_get_phy_id(hw);
>> - if (ret_val)
>> - goto out;
>> + if (ret_val) {
>> + e1000_reset_hw_ich8lan(hw);
>> + ret_val = e1000e_get_phy_id(hw);
>> + if (ret_val)
>> + goto out;
>> + }
>> if ((phy->id == 0) || (phy->id == PHY_REVISION_MASK)) {
>> /*
>> * In case the PHY needs to be in mdio slow mode (eg.
>> 82577),
>> ****************************************************************************
>> I'm not sure it's right, but using the patch, 82577 network can work
>> well.I need more eyes about this, how can I make the patch better?
>> Any suggestion would be helped me make the patch better.
>>
>> By the way, we find 3 patches make it not well.
>>
>> From 0be8401051c716be4533272e983b7eed3d83946d Mon Sep 17 00:00:00
>> 2001 From: Bruce Allan <bruce.w.allan@intel.com>
>> Date: Wed, 2 Dec 2009 17:03:18 +0000
>> Subject: e1000e: correct ICH/PCH PHY operations function pointers
>>
>> From fddaa1aff881c98f524221236af98ce70dcd04cf Mon Sep 17 00:00:00
>> 2001
>>
>> From: Bruce Allan <bruce.w.allan@intel.com>
>> Date: Wed, 13 Jan 2010 01:52:49 +0000
>> Subject: e1000e: MDIO slow mode should always be done for 82577
>>
>> From baf86c9d36826fab0160251bbc87dfab3af48a21 Mon Sep 17 00:00:00
>> 2001
>>
>> From: Bruce Allan <bruce.w.allan@intel.com>
>> Date: Wed, 13 Jan 2010 01:53:08 +0000
>> Subject: e1000e: workaround link issues on busy hub in half duplex on
>> 82577/82578
>>
>> thanks,
>> Yucheng.Wang
>
> [Adding e1000-devel@lists.sf.net (the mailing list for Intel wired GbE drivers)]
>
> Thanks for reporting the issue. The 3 patches you list that appear to break e1000e on 82577 are necessary. The patch you suggest to resolve the issue is almost exactly the same as one we have been working on in-house here at Intel and will be pushed upstream shortly. There are still a couple unresolved issues with it however so it will be another couple days.
Thanks for giving me the info. but there are some problems I want to
understand it for the issue. Would you like to telling me why? All the
problems happens in the kenel 2.6.34-rc-1.
1. When I insmod the module e1000e, the network card can work well. But
when I build the driver in the kernel the "-3" error happens.
theoretically, there is no difference of driver between in the kernel
and modules. The problem is strange to me, so I'm interested to know who
did this?
2. If I use my patch to resolve the issue, which problem would happens
when reset the hardware of network like me to do? And the e1000e problem
is not only support 82577, so the workaround would impact other card
using e1000e, which time and when to reset the hardware is best?
3. I read the source code, and find the function "e1000_read_phy_reg_hv"
cannot return the correct value when execute "file:ich8lan.c,line:290
ret_val = e1000_set_mdio_slow_mode_hv(hw);", so which register
impact this?
4. If the patch from Intel finished, about when I can read the patch?
5. The version of e1000e is "e1000e: Intel(R) PRO/1000 Network Driver -
1.0.2-k2" in the official kernel 2.6.34-rc-1, but in the Intel's
website, the version is "1.1.2.1a", the webpage is
http://downloadcenter.intel.com/SearchResult.aspx?lang=eng&keyword=%22e1000e%22
which difference between the two version of e1000e driver? And I noticed
that in the sourceforge's maillist, the intel's e1000e driver 1.1.2.1a
has some problem when build in 2.6.34-rc-1, which I have been encountered.
I think you were crazy when you see so many questions, and cost you too
much time to answer it. But I'm still want to know the answer of these
questions. Hope you can reply me to understand the e100e network.
Thanks a lot.
yucheng.wang
>
> Thanks again,
> Bruce.
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH net-next-2.6 2/2] can: sja1000: add read/write routines for 8, 16 and 32-bit register access
From: Yegor Yefremov @ 2010-03-19 10:50 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA; +Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w
In-Reply-To: <4BA356A4.5040205-ZJVcf1zZPRSebONBosFW4Q@public.gmane.org>
SJA1000: add read/write routines for 8, 16 and 32-bit register access
add routines for 8, 16 and 32-bit access like in
drivers/i2c/busses/i2c-pca-platform.c
Signed-off-by: Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Index: net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c
===================================================================
--- net-next-2.6.orig/drivers/net/can/sja1000/sja1000_platform.c
+++ net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c
@@ -37,16 +37,36 @@ MODULE_AUTHOR("Sascha Hauer <s.hauer@pen
MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
MODULE_LICENSE("GPL v2");
-static u8 sp_read_reg(const struct sja1000_priv *priv, int reg)
+static u8 sp_read_reg8(const struct sja1000_priv *priv, int reg)
{
return ioread8(priv->reg_base + reg);
}
-static void sp_write_reg(const struct sja1000_priv *priv, int reg, u8 val)
+static void sp_write_reg8(const struct sja1000_priv *priv, int reg, u8 val)
{
iowrite8(val, priv->reg_base + reg);
}
+static u8 sp_read_reg16(const struct sja1000_priv *priv, int reg)
+{
+ return ioread8(priv->reg_base + reg * 2);
+}
+
+static void sp_write_reg16(const struct sja1000_priv *priv, int reg, u8 val)
+{
+ iowrite8(val, priv->reg_base + reg * 2);
+}
+
+static u8 sp_read_reg32(const struct sja1000_priv *priv, int reg)
+{
+ return ioread8(priv->reg_base + reg * 4);
+}
+
+static void sp_write_reg32(const struct sja1000_priv *priv, int reg, u8 val)
+{
+ iowrite8(val, priv->reg_base + reg * 4);
+}
+
static int sp_probe(struct platform_device *pdev)
{
int err;
@@ -92,12 +112,26 @@ static int sp_probe(struct platform_devi
dev->irq = res_irq->start;
priv->irq_flags = res_irq->flags & (IRQF_TRIGGER_MASK | IRQF_SHARED);
priv->reg_base = addr;
- priv->read_reg = sp_read_reg;
- priv->write_reg = sp_write_reg;
priv->can.clock.freq = pdata->clock;
priv->ocr = pdata->ocr;
priv->cdr = pdata->cdr;
+ switch (res_mem->flags & IORESOURCE_MEM_TYPE_MASK) {
+ case IORESOURCE_MEM_32BIT:
+ priv->read_reg = sp_read_reg32;
+ priv->write_reg = sp_write_reg32;
+ break;
+ case IORESOURCE_MEM_16BIT:
+ priv->read_reg = sp_read_reg16;
+ priv->write_reg = sp_write_reg16;
+ break;
+ case IORESOURCE_MEM_8BIT:
+ default:
+ priv->read_reg = sp_read_reg8;
+ priv->write_reg = sp_write_reg8;
+ break;
+ }
+
dev_set_drvdata(&pdev->dev, dev);
SET_NETDEV_DEV(dev, &pdev->dev);
^ permalink raw reply
* Re: [PATCH net-next-2.6 1/2] can: sja1000: allow shared interrupt definition
From: Yegor Yefremov @ 2010-03-19 10:49 UTC (permalink / raw)
To: netdev; +Cc: socketcan-core
In-Reply-To: <4BA3565F.6040907@visionsystems.de>
SJA1000: allow shared interrupt definition
extend the AND mask, so that IRQF_SHARED flag remains
Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 628374c..bec0d3d 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -90,7 +90,7 @@ static int sp_probe(struct platform_device *pdev)
priv = netdev_priv(dev);
dev->irq = res_irq->start;
- priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
+ priv->irq_flags = res_irq->flags & (IRQF_TRIGGER_MASK | IRQF_SHARED);
priv->reg_base = addr;
priv->read_reg = sp_read_reg;
priv->write_reg = sp_write_reg;
^ permalink raw reply related
* [PATCH net-next-2.6 0/2] can: sja1000: sja1000_platform.c fixes
From: Yegor Yefremov @ 2010-03-19 10:47 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA; +Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w
Here are two sja1000_platform.c related fixes:
SJA1000: allow shared interrupt definition
SJA1000: add read/write routines for 8, 16 and 32-bit register access
Best regards,
Yegor
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 9:53 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20100319093210.GA23895@gondor.apana.org.au>
Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 11:12:21AM +0200, Timo Teräs wrote:
>>> That would be better but it's still hacky. Proper reference
>>> counting like we had before would be my preference.
>> Well, the cache entry is still referenced only very shortly,
>> I don't see why keeping bh disabled why doing it is considered
>> a hack. Refcounting the cache entries is trickier. Though,
>> it could be used to optimize the update process: we could safely
>> update it instead of doing now lookup later.
>
> Well we had a nicely type-agnostic cache which is self-contained,
> but your patch is bleeding generic code into xfrm_policy.c, that's
> why I felt it to be hacky :)
>
> Anyway I see how your scheme works now as far as object life
> is concerned, and I agree that it is safe.
>
> However, I wonder if we could do it while still leaving all the
> object life-cycle management stuff (and the BH disabling bits)
> in flow.c
>
> The crux of the issue is that you now have two objects to track
> instead of one. As the direction is a key in the lookup, we're
> really only worried about the outbound case here.
>
> So how about going back to what I suggested earlier, and keeping
> a back-pointer from xfrm_dst to the policy? Of course xfrm_dst
> would also hold a ref count on the policy. You'd only have to
> do it for the top-level xfrm_dst.
>
> It does mean that you'll need to write a different resolver for
> outbound vs. inbound/forward, but that makes sense because we
> only use bundles for outbound policies.
>
> What do you think?
When I first started reading the code, I got confused slightly
on how the garbage collection is happening. What I did not like
in current is the atomic_dec() in flow.c that does not check if
it was turned to zero. Because on policy objects it means you
need to delete it (which would a bug if it happened in flow.c;
the policy gc calls flush before releasing it's own reference),
but on xfrm_dst it's perfectly ok to do atomic_dec() and the
dst core will garbage collect items.
But now, thinking more, it would probably make more sense to
just cache xfrm_dst's and keep ref to the policy on them. So
in general I agree with your recommendation. The only immediate
problem I can think now is that the resolved would need to
atomically check if xfrm_dst is valid, if not, resolve new one.
But creating new xfrm_dst involves taking locks and can sleep
so it cannot be inside the main resolver.
Alternatively, we'd need to:
a) still expose flow cache entry structs and do locking or
refcounting on them
b) have two version of flow lookup: one that calls resolver
with bh disabled and can atomically lookup and update entry
and a second version that lookups, calls validation, if
not-valid it calls resolver with bh enabled and does a new
flow cache lookup to update cache
Also, relatedly. Is there way to release xfrm_dst's child and
route refs when xfrm_bundle_ok fails? This would improve GC
collection of the ipv4 rtable entries they referenced.
- Timo
^ permalink raw reply
* Handling receive packets with limited buffer space
From: Amit Uttamchandani @ 2010-03-19 9:41 UTC (permalink / raw)
To: netdev
In my modified ethoc.c driver, I am using only one receive buffer
descriptor due to limited space. This works fairly well for small
packets (arp, pings, etc.).
However, doing a 'wget' and downloading a file causes lots of drops and
thus results in a terminated connection.
How do I go about dealing with this? I tried stopping the queue
(netif_stop_queue) while I'm processing the rx skb and then resuming it
once I'm done, but this did not seem to help.
Any ideas on this?
Thanks for any help.
Amit
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-19 9:32 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
In-Reply-To: <4BA33FF5.8010104@iki.fi>
On Fri, Mar 19, 2010 at 11:12:21AM +0200, Timo Teräs wrote:
>
>> That would be better but it's still hacky. Proper reference
>> counting like we had before would be my preference.
>
> Well, the cache entry is still referenced only very shortly,
> I don't see why keeping bh disabled why doing it is considered
> a hack. Refcounting the cache entries is trickier. Though,
> it could be used to optimize the update process: we could safely
> update it instead of doing now lookup later.
Well we had a nicely type-agnostic cache which is self-contained,
but your patch is bleeding generic code into xfrm_policy.c, that's
why I felt it to be hacky :)
Anyway I see how your scheme works now as far as object life
is concerned, and I agree that it is safe.
However, I wonder if we could do it while still leaving all the
object life-cycle management stuff (and the BH disabling bits)
in flow.c
The crux of the issue is that you now have two objects to track
instead of one. As the direction is a key in the lookup, we're
really only worried about the outbound case here.
So how about going back to what I suggested earlier, and keeping
a back-pointer from xfrm_dst to the policy? Of course xfrm_dst
would also hold a ref count on the policy. You'd only have to
do it for the top-level xfrm_dst.
It does mean that you'll need to write a different resolver for
outbound vs. inbound/forward, but that makes sense because we
only use bundles for outbound policies.
What do you think?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] pktgen node allocation
From: Eric Dumazet @ 2010-03-19 9:28 UTC (permalink / raw)
To: Robert Olsson; +Cc: David Miller, netdev
In-Reply-To: <19363.14702.909265.380669@gargle.gargle.HOWL>
Le vendredi 19 mars 2010 à 09:44 +0100, Robert Olsson a écrit :
>
> Hi,
> Here is patch to manipulate packet node allocation and implicitly
> how packets are DMA'd etc.
>
> The flag NODE_ALLOC enables the function and numa_node_id();
> when enabled it can also be explicitly controlled via a new
> node parameter
>
> Tested this with 10 Intel 82599 ports w. TYAN S7025 E5520 CPU's.
> Was able to TX/DMA ~80 Gbit/s to Ethernet wires.
>
> Cheers
> --ro
>
I cannot understand how this can help.
__netdev_alloc_skb() is supposed to already take into account NUMA
properties :
int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
If this doesnt work, we should correct core stack, not only pktgen :)
Are you allocating memory in the node where pktgen CPU is running or the
node close to the NIC ?
Thanks
^ permalink raw reply
* [PATCH v3] ipv4: check rt_genid in dst_check
From: Timo Teras @ 2010-03-19 9:20 UTC (permalink / raw)
To: netdev; +Cc: Timo Teras
Xfrm_dst keeps a reference to ipv4 rtable entries on each
cached bundle. The only way to renew xfrm_dst when the underlying
route has changed, is to implement dst_check for this. This is
what ipv6 side does too.
The problems started after 87c1e12b5eeb7b30b4b41291bef8e0b41fc3dde9
which fixed a bug causing xfrm_dst to not get reused, until that all
lookups always generated new xfrm_dst with new route reference
and path mtu worked. But after the fix, the old routes started
to get reused even after they were expired causing pmtu to break
(well it would occationally work if the rtable gc had run recently
and marked the route obsolete causing dst_check to get called).
Signed-off-by: Timo Teras <timo.teras@iki.fi>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
---
net/ipv4/route.c | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index a770df2..32d3961 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1441,7 +1441,7 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
dev_hold(rt->u.dst.dev);
if (rt->idev)
in_dev_hold(rt->idev);
- rt->u.dst.obsolete = 0;
+ rt->u.dst.obsolete = -1;
rt->u.dst.lastuse = jiffies;
rt->u.dst.path = &rt->u.dst;
rt->u.dst.neighbour = NULL;
@@ -1506,7 +1506,7 @@ static struct dst_entry *ipv4_negative_advice(struct dst_entry *dst)
struct dst_entry *ret = dst;
if (rt) {
- if (dst->obsolete) {
+ if (dst->obsolete > 0) {
ip_rt_put(rt);
ret = NULL;
} else if ((rt->rt_flags & RTCF_REDIRECTED) ||
@@ -1726,7 +1726,9 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, u32 mtu)
static struct dst_entry *ipv4_dst_check(struct dst_entry *dst, u32 cookie)
{
- return NULL;
+ if (rt_is_expired((struct rtable *)dst))
+ return NULL;
+ return dst;
}
static void ipv4_dst_destroy(struct dst_entry *dst)
@@ -1888,7 +1890,8 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
if (!rth)
goto e_nobufs;
- rth->u.dst.output= ip_rt_bug;
+ rth->u.dst.output = ip_rt_bug;
+ rth->u.dst.obsolete = -1;
atomic_set(&rth->u.dst.__refcnt, 1);
rth->u.dst.flags= DST_HOST;
@@ -2054,6 +2057,7 @@ static int __mkroute_input(struct sk_buff *skb,
rth->fl.oif = 0;
rth->rt_spec_dst= spec_dst;
+ rth->u.dst.obsolete = -1;
rth->u.dst.input = ip_forward;
rth->u.dst.output = ip_output;
rth->rt_genid = rt_genid(dev_net(rth->u.dst.dev));
@@ -2218,6 +2222,7 @@ local_input:
goto e_nobufs;
rth->u.dst.output= ip_rt_bug;
+ rth->u.dst.obsolete = -1;
rth->rt_genid = rt_genid(net);
atomic_set(&rth->u.dst.__refcnt, 1);
@@ -2444,6 +2449,7 @@ static int __mkroute_output(struct rtable **result,
rth->rt_spec_dst= fl->fl4_src;
rth->u.dst.output=ip_output;
+ rth->u.dst.obsolete = -1;
rth->rt_genid = rt_genid(dev_net(dev_out));
RT_CACHE_STAT_INC(out_slow_tot);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH 1/3] mfd: add support for Janz CMOD-IO PCI MODULbus Carrier Board
From: Wolfgang Grandegger @ 2010-03-19 9:13 UTC (permalink / raw)
To: Ira W. Snyder; +Cc: linux-kernel, socketcan-core, netdev, sameo
In-Reply-To: <1268930324-29841-2-git-send-email-iws@ovro.caltech.edu>
Ira W. Snyder wrote:
> The Janz CMOD-IO PCI MODULbus carrier board is a PCI to MODULbus bridge,
> which may host many different types of MODULbus daughterboards, including
> CAN and GPIO controllers.
>
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
You can add my "Reviewed-by: Wolfgang Grandegger <wg@grandegger.com>".
Just one concern:
> ---
> drivers/mfd/Kconfig | 8 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/janz-cmodio.c | 339 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/janz.h | 54 +++++++
> 4 files changed, 402 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mfd/janz-cmodio.c
> create mode 100644 include/linux/mfd/janz.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8782978..f1858d7 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -27,6 +27,14 @@ config MFD_SM501_GPIO
> lines on the SM501. The platform data is used to supply the
> base number for the first GPIO line to register.
>
> +config MFD_JANZ_CMODIO
> + tristate "Support for Janz CMOD-IO PCI MODULbus Carrier Board"
> + ---help---
> + This is the core driver for the Janz CMOD-IO PCI MODULbus
> + carrier board. This device is a PCI to MODULbus bridge which may
> + host many different types of MODULbus daughterboards, including
> + CAN and GPIO controllers.
> +
> config MFD_ASIC3
> bool "Support for Compaq ASIC3"
> depends on GENERIC_HARDIRQS && GPIOLIB && ARM
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e09eb48..e8fa905 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -3,6 +3,7 @@
> #
>
> obj-$(CONFIG_MFD_SM501) += sm501.o
> +obj-$(CONFIG_MFD_JANZ_CMODIO) += janz-cmodio.o
> obj-$(CONFIG_MFD_ASIC3) += asic3.o tmio_core.o
> obj-$(CONFIG_MFD_SH_MOBILE_SDHI) += sh_mobile_sdhi.o
>
> diff --git a/drivers/mfd/janz-cmodio.c b/drivers/mfd/janz-cmodio.c
> new file mode 100644
> index 0000000..914280e
> --- /dev/null
> +++ b/drivers/mfd/janz-cmodio.c
> @@ -0,0 +1,339 @@
> +/*
> + * Janz CMOD-IO MODULbus Carrier Board PCI Driver
> + *
> + * Copyright (c) 2010 Ira W. Snyder <iws@ovro.caltech.edu>
> + *
> + * Lots of inspiration and code was copied from drivers/mfd/sm501.c
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/mfd/janz.h>
> +
> +#define DRV_NAME "janz-cmodio"
> +
> +/* Size of each MODULbus module in PCI BAR4 */
> +#define CMODIO_MODULBUS_SIZE 0x200
> +
> +/* Maximum number of MODULbus modules on a CMOD-IO carrier board */
> +#define CMODIO_MAX_MODULES 4
> +
> +/* Module Parameters */
> +static unsigned int num_modules = CMODIO_MAX_MODULES;
> +static unsigned char *modules[CMODIO_MAX_MODULES] = {
> + "janz-ican3",
> + "janz-ican3",
> + "",
> + "janz-ttl",
> +};
This is probably not a good default but just your private configuration.
Wolfgang.
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 9:12 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20100319084717.GA23567@gondor.apana.org.au>
Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 10:37:58AM +0200, Timo Teräs wrote:
>> But I changed that. the flow cache now does *not* call local_bh_enable
>> if it returns something. This is deferred until corresponding _put
>> call. So bh's are disable while we are touching the lookup results.
>
> I'm sorry but making a function like flow_cache_lookup return with
> BH disabled is just wrong!
>
>> It'd probably make sense to remove that. And require _lookup to
>> be called with bh disabled so it's more obvious that bh's are
>> disabled when touching the cache entry.
>
> That would be better but it's still hacky. Proper reference
> counting like we had before would be my preference.
Well, the cache entry is still referenced only very shortly,
I don't see why keeping bh disabled why doing it is considered
a hack. Refcounting the cache entries is trickier. Though,
it could be used to optimize the update process: we could safely
update it instead of doing now lookup later.
>> Not a race. We need to keep bh's disabled while touching fce
>> for various reasons.
>
> What are those reasons (apart from this race)?
This. And that the cache is synchronized by flow_cache_flush
executing stuff on other cpu's, ensuring that it's not running
any protected cache accessing code. See below.
>
>> Noone. When policy and dst is on cache there's no reference.
>> The cache generation id's ensure that the object exists when
>> they are in cache. It might make sense to add references to
>> both objects and do a BUG_ON if the flow cache flusher would
>> need to delete an object. I guess this would be the proper
>> way, since that's how the dst stuff works too.
>
> The cache genid is not enough:
>
> CPU1 CPU2
> check genid == OK
> update genid
> kill policy
> kfree on policy
> use policy == BOOM
The sequence goes currently.
CPU1 CPU2
disable_bh
check genid == OK
update genid
call cache_flush
blocks
use policy == OK
and take refcount
enable_bh
cache_flush smpcall executes and ublocks cpu2
returns from cache_flush
kill policy
kfree on policy
- Timo
^ permalink raw reply
* [PATCH] pktgen node allocation
From: Robert Olsson @ 2010-03-19 8:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev, robert
Hi,
Here is patch to manipulate packet node allocation and implicitly
how packets are DMA'd etc.
The flag NODE_ALLOC enables the function and numa_node_id();
when enabled it can also be explicitly controlled via a new
node parameter
Tested this with 10 Intel 82599 ports w. TYAN S7025 E5520 CPU's.
Was able to TX/DMA ~80 Gbit/s to Ethernet wires.
Cheers
--ro
Signed-off-by: Robert Olsson <robert.olsson@its.uu.se>
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 4392381..c195fd0 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -169,7 +169,7 @@
#include <asm/dma.h>
#include <asm/div64.h> /* do_div */
-#define VERSION "2.72"
+#define VERSION "2.73"
#define IP_NAME_SZ 32
#define MAX_MPLS_LABELS 16 /* This is the max label stack depth */
#define MPLS_STACK_BOTTOM htonl(0x00000100)
@@ -190,6 +190,7 @@
#define F_IPSEC_ON (1<<12) /* ipsec on for flows */
#define F_QUEUE_MAP_RND (1<<13) /* queue map Random */
#define F_QUEUE_MAP_CPU (1<<14) /* queue map mirrors smp_processor_id() */
+#define F_NODE (1<<15) /* Node memory alloc*/
/* Thread control flag bits */
#define T_STOP (1<<0) /* Stop run */
@@ -372,6 +373,7 @@ struct pktgen_dev {
u16 queue_map_min;
u16 queue_map_max;
+ int node; /* Memory node */
#ifdef CONFIG_XFRM
__u8 ipsmode; /* IPSEC mode (config) */
@@ -607,6 +609,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
if (pkt_dev->traffic_class)
seq_printf(seq, " traffic_class: 0x%02x\n", pkt_dev->traffic_class);
+ if (pkt_dev->node >= 0)
+ seq_printf(seq, " node: %d\n", pkt_dev->node);
+
seq_printf(seq, " Flags: ");
if (pkt_dev->flags & F_IPV6)
@@ -660,6 +665,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
if (pkt_dev->flags & F_SVID_RND)
seq_printf(seq, "SVID_RND ");
+ if (pkt_dev->flags & F_NODE)
+ seq_printf(seq, "NODE_ALLOC ");
+
seq_puts(seq, "\n");
/* not really stopped, more like last-running-at */
@@ -1074,6 +1082,21 @@ static ssize_t pktgen_if_write(struct file *file,
pkt_dev->dst_mac_count);
return count;
}
+ if (!strcmp(name, "node")) {
+ len = num_arg(&user_buffer[i], 10, &value);
+ if (len < 0)
+ return len;
+
+ i += len;
+
+ if(node_possible(value)) {
+ pkt_dev->node = value;
+ sprintf(pg_result, "OK: node=%d", pkt_dev->node);
+ }
+ else
+ sprintf(pg_result, "ERROR: node not possible");
+ return count;
+ }
if (!strcmp(name, "flag")) {
char f[32];
memset(f, 0, 32);
@@ -1166,12 +1189,18 @@ static ssize_t pktgen_if_write(struct file *file,
else if (strcmp(f, "!IPV6") == 0)
pkt_dev->flags &= ~F_IPV6;
+ else if (strcmp(f, "NODE_ALLOC") == 0)
+ pkt_dev->flags |= F_NODE;
+
+ else if (strcmp(f, "!NODE_ALLOC") == 0)
+ pkt_dev->flags &= ~F_NODE;
+
else {
sprintf(pg_result,
"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
f,
"IPSRC_RND, IPDST_RND, UDPSRC_RND, UDPDST_RND, "
- "MACSRC_RND, MACDST_RND, TXSIZE_RND, IPV6, MPLS_RND, VID_RND, SVID_RND, FLOW_SEQ, IPSEC\n");
+ "MACSRC_RND, MACDST_RND, TXSIZE_RND, IPV6, MPLS_RND, VID_RND, SVID_RND, FLOW_SEQ, IPSEC, NODE_ALLOC\n");
return count;
}
sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
@@ -2572,9 +2601,27 @@ static struct sk_buff *fill_packet_ipv4(struct net_device *odev,
mod_cur_headers(pkt_dev);
datalen = (odev->hard_header_len + 16) & ~0xf;
- skb = __netdev_alloc_skb(odev,
- pkt_dev->cur_pkt_size + 64
- + datalen + pkt_dev->pkt_overhead, GFP_NOWAIT);
+
+ if(pkt_dev->flags & F_NODE) {
+ int node;
+
+ if(pkt_dev->node >= 0)
+ node = pkt_dev->node;
+ else
+ node = numa_node_id();
+
+ skb = __alloc_skb(NET_SKB_PAD + pkt_dev->cur_pkt_size + 64
+ + datalen + pkt_dev->pkt_overhead, GFP_NOWAIT, 0, node);
+ if (likely(skb)) {
+ skb_reserve(skb, NET_SKB_PAD);
+ skb->dev = odev;
+ }
+ }
+ else
+ skb = __netdev_alloc_skb(odev,
+ pkt_dev->cur_pkt_size + 64
+ + datalen + pkt_dev->pkt_overhead, GFP_NOWAIT);
+
if (!skb) {
sprintf(pkt_dev->result, "No memory");
return NULL;
@@ -3674,6 +3721,7 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
pkt_dev->svlan_p = 0;
pkt_dev->svlan_cfi = 0;
pkt_dev->svlan_id = 0xffff;
+ pkt_dev->node = -1;
err = pktgen_setup_dev(pkt_dev, ifname);
if (err)
^ permalink raw reply related
* Re: [PATCH 2/3] can: add support for Janz VMOD-ICAN3 Intelligent CAN module
From: Wolfgang Grandegger @ 2010-03-19 9:01 UTC (permalink / raw)
To: Ira W. Snyder
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, sameo-VuQAYsv1563Yd54FQh9/CA
In-Reply-To: <1268930324-29841-3-git-send-email-iws-lulEs6mt1IksTUYHLfqkUA@public.gmane.org>
Hi Ira,
we already discussed this patch on the SocketCAN mailing list and there
are just a few minor issues and the request to add support for the new
"berr-reporting" option, if feasible. See:
commit 52c793f24054f5dc30d228e37e0e19cc8313f086
Author: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
Date: Mon Feb 22 22:21:17 2010 +0000
can: netlink support for bus-error reporting and counters
This patch makes the bus-error reporting configurable and allows to
retrieve the CAN TX and RX bus error counters via netlink interface.
I have added support for the SJA1000. The TX and RX bus error counters
are also copied to the data fields 6..7 of error messages when state
changes are reported.
Should not be a big deal.
More inline...
Ira W. Snyder wrote:
> The Janz VMOD-ICAN3 is a MODULbus daughterboard which fits onto any
> MODULbus carrier board. It is an intelligent CAN controller with a
> microcontroller and associated firmware.
>
> Signed-off-by: Ira W. Snyder <iws-lulEs6mt1IksTUYHLfqkUA@public.gmane.org>
> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
> drivers/net/can/Kconfig | 10 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/janz-ican3.c | 1659 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1670 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/can/janz-ican3.c
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 05b7517..2c5227c 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -63,6 +63,16 @@ config CAN_BFIN
> To compile this driver as a module, choose M here: the
> module will be called bfin_can.
>
> +config CAN_JANZ_ICAN3
> + tristate "Janz VMOD-ICAN3 Intelligent CAN controller"
> + depends on CAN_DEV && MFD_JANZ_CMODIO
> + ---help---
> + Driver for Janz VMOD-ICAN3 Intelligent CAN controller module, which
> + connects to a MODULbus carrier board.
> +
> + This driver can also be built as a module. If so, the module will be
> + called janz-ican3.ko.
> +
> source "drivers/net/can/mscan/Kconfig"
>
> source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 7a702f2..9047cd0 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -15,5 +15,6 @@ obj-$(CONFIG_CAN_AT91) += at91_can.o
> obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
> obj-$(CONFIG_CAN_MCP251X) += mcp251x.o
> obj-$(CONFIG_CAN_BFIN) += bfin_can.o
> +obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
>
> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> new file mode 100644
> index 0000000..94d4995
> --- /dev/null
> +++ b/drivers/net/can/janz-ican3.c
[snip]
+struct ican3_dev {
> +
> + /* must be the first member */
> + struct can_priv can;
> +
> + /* CAN network device */
> + struct net_device *ndev;
> + struct napi_struct napi;
> +
> + /* Device for printing */
> + struct device *dev;
> +
> + /* module number */
> + unsigned int num;
> +
> + /* base address of registers and IRQ */
> + struct janz_cmodio_onboard_regs __iomem *ctrl;
> + struct ican3_dpm_control *dpmctrl;
> + void __iomem *dpm;
> + int irq;
> +
> + /* old and new style host interface */
> + unsigned int iftype;
> + spinlock_t lock;
Please describe what the lock is used for.
> + /* new host interface */
> + unsigned int rx_int;
> + unsigned int rx_num;
> + unsigned int tx_num;
> +
> + /* fast host interface */
> + unsigned int fastrx_start;
> + unsigned int fastrx_int;
> + unsigned int fastrx_num;
> + unsigned int fasttx_start;
> + unsigned int fasttx_num;
> +
> + /* first free DPM page */
> + unsigned int free_page;
> +};
[snip]
> +static void ican3_to_can_frame(struct ican3_dev *mod,
> + struct ican3_fast_desc *desc,
> + struct can_frame *cf)
> +{
> + if ((desc->command & ICAN3_CAN_TYPE_MASK) == ICAN3_CAN_TYPE_SFF) {
> + dev_dbg(mod->dev, "%s: old frame format\n", __func__);
This prints a debug message for every message which is not really
useful for the user. Please remove.
> + if (desc->data[1] & ICAN3_SFF_RTR)
> + cf->can_id |= CAN_RTR_FLAG;
> +
> + cf->can_id |= desc->data[0] << 3;
> + cf->can_id |= (desc->data[1] & 0xe0) >> 5;
> + cf->can_dlc = desc->data[1] & ICAN3_CAN_DLC_MASK;
> + memcpy(cf->data, &desc->data[2], sizeof(cf->data));
> + } else {
> + dev_dbg(mod->dev, "%s: new frame format\n", __func__);
Ditto.
> + cf->can_dlc = desc->data[0] & ICAN3_CAN_DLC_MASK;
> + if (desc->data[0] & ICAN3_EFF_RTR)
> + cf->can_id |= CAN_RTR_FLAG;
> +
> + if (desc->data[0] & ICAN3_EFF) {
> + cf->can_id |= CAN_EFF_FLAG;
> + cf->can_id |= desc->data[2] << 21; /* 28-21 */
> + cf->can_id |= desc->data[3] << 13; /* 20-13 */
> + cf->can_id |= desc->data[4] << 5; /* 12-5 */
> + cf->can_id |= (desc->data[5] & 0xf8) >> 3;
> + } else {
> + cf->can_id |= desc->data[2] << 3; /* 10-3 */
> + cf->can_id |= desc->data[3] >> 5; /* 2-0 */
> + }
> +
> + memcpy(cf->data, &desc->data[6], sizeof(cf->data));
> + }
> +}
> +
> +static void can_frame_to_ican3(struct ican3_dev *mod,
> + struct can_frame *cf,
> + struct ican3_fast_desc *desc)
> +{
> + /* clear out any stale data in the descriptor */
> + memset(desc->data, 0, sizeof(desc->data));
> +
> + /* we always use the extended format, with the ECHO flag set */
> + desc->command = ICAN3_CAN_TYPE_EFF;
> + desc->data[0] |= cf->can_dlc;
> + desc->data[1] |= ICAN3_ECHO;
> +
> + if (cf->can_id & CAN_RTR_FLAG)
> + desc->data[0] |= ICAN3_EFF_RTR;
> +
> + /* pack the id into the correct places */
> + if (cf->can_id & CAN_EFF_FLAG) {
> + dev_dbg(mod->dev, "%s: extended frame\n", __func__);
Ditto.
> + desc->data[0] |= ICAN3_EFF;
> + desc->data[2] = (cf->can_id & 0x1fe00000) >> 21; /* 28-21 */
> + desc->data[3] = (cf->can_id & 0x001fe000) >> 13; /* 20-13 */
> + desc->data[4] = (cf->can_id & 0x00001fe0) >> 5; /* 12-5 */
> + desc->data[5] = (cf->can_id & 0x0000001f) << 3; /* 4-0 */
> + } else {
> + dev_dbg(mod->dev, "%s: standard frame\n", __func__);
Ditto.
> + desc->data[2] = (cf->can_id & 0x7F8) >> 3; /* bits 10-3 */
> + desc->data[3] = (cf->can_id & 0x007) << 5; /* bits 2-0 */
> + }
> +
> + /* copy the data bits into the descriptor */
> + memcpy(&desc->data[6], cf->data, sizeof(cf->data));
> +}
[snip]
> +/*
> + * Handle CAN Event Indication Messages from the firmware
> + *
> + * The ICAN3 firmware provides the values of some SJA1000 registers when it
> + * generates this message. The code below is largely copied from the
> + * drivers/net/can/sja1000/sja1000.c file, and adapted as necessary
> + */
> +static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> +{
> + struct net_device *dev = mod->ndev;
> + struct net_device_stats *stats = &dev->stats;
> + enum can_state state = mod->can.state;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + u8 status, isrc;
> +
> + /* we can only handle the SJA1000 part */
> + if (msg->data[1] != CEVTIND_CHIP_SJA1000) {
> + dev_err(mod->dev, "unable to handle errors on non-SJA1000\n");
> + return -ENODEV;
> + }
> +
> + /* check the message length for sanity */
> + if (msg->len < 6) {
> + dev_err(mod->dev, "error message too short\n");
> + return -EINVAL;
> + }
> +
> + skb = alloc_can_err_skb(dev, &cf);
> + if (skb == NULL)
> + return -ENOMEM;
> +
> + isrc = msg->data[0];
> + status = msg->data[3];
> +
> + /* data overrun interrupt */
> + if (isrc == CEVTIND_DOI || isrc == CEVTIND_LOST) {
Here and for the other errors below a dev_dbg() would be useful. Please
check sja1000.c.
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> + stats->rx_over_errors++;
> + stats->rx_errors++;
> + dev_info(mod->dev, "%s: overflow frame generated\n", __func__);
s/dev_info/dev_dbg/ ?
> + }
> +
> + /* error warning interrupt */
> + if (isrc == CEVTIND_EI) {
> + if (status & SR_BS) {
> + state = CAN_STATE_BUS_OFF;
> + cf->can_id |= CAN_ERR_BUSOFF;
> + can_bus_off(dev);
> + } else if (status & SR_ES) {
> + state = CAN_STATE_ERROR_WARNING;
> + } else {
> + state = CAN_STATE_ERROR_ACTIVE;
> + }
> + }
> +
> + /* bus error interrupt */
> + if (isrc == CEVTIND_BEI) {
> + u8 ecc = msg->data[2];
Add an empty line, please.
> + mod->can.can_stats.bus_error++;
> + stats->rx_errors++;
> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> + switch (ecc & ECC_MASK) {
> + case ECC_BIT:
> + cf->data[2] |= CAN_ERR_PROT_BIT;
> + break;
> + case ECC_FORM:
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + break;
> + case ECC_STUFF:
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + break;
> + default:
> + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> + cf->data[3] = ecc & ECC_SEG;
> + break;
> + }
> +
> + if ((ecc & ECC_DIR) == 0)
> + cf->data[2] |= CAN_ERR_PROT_TX;
> + }
> +
> + if (state != mod->can.state && (state == CAN_STATE_ERROR_WARNING ||
> + state == CAN_STATE_ERROR_PASSIVE)) {
> + u8 rxerr = msg->data[4];
> + u8 txerr = msg->data[5];
Ditto.
> + cf->can_id |= CAN_ERR_CRTL;
> + if (state == CAN_STATE_ERROR_WARNING) {
> + mod->can.can_stats.error_warning++;
> + cf->data[1] = (txerr > rxerr) ?
> + CAN_ERR_CRTL_TX_WARNING :
> + CAN_ERR_CRTL_RX_WARNING;
> + } else {
> + mod->can.can_stats.error_passive++;
> + cf->data[1] = (txerr > rxerr) ?
> + CAN_ERR_CRTL_TX_PASSIVE :
> + CAN_ERR_CRTL_RX_PASSIVE;
> + }
> + }
> +
> + mod->can.state = state;
> + stats->rx_errors++;
> + stats->rx_bytes += cf->can_dlc;
> + netif_rx(skb);
> + return 0;
> +}
[snip]
> +static irqreturn_t ican3_irq(int irq, void *dev_id)
> +{
> + struct ican3_dev *mod = dev_id;
> + u8 stat;
> +
> + /*
> + * The interrupt status register on this device reports interrupts
> + * as zeroes instead of using ones like most other devices
> + */
> + stat = ioread8(&mod->ctrl->int_disable) & (1 << mod->num);
> + if (stat == (1 << mod->num))
> + return IRQ_NONE;
> +
> + dev_dbg(mod->dev, "IRQ: module %d\n", mod->num);
Please remove this dev_dbg() as well.
[snip]
> +/*
> + * Startup an ICAN module, bringing it into fast mode
> + */
> +static int __devinit ican3_startup_module(struct ican3_dev *mod)
> +{
> + int ret;
> +
> + ret = ican3_reset_module(mod);
> + if (ret) {
> + dev_err(mod->dev, "unable to reset module\n");
> + return ret;
> + }
> +
> + /* re-enable interrupts so we can send messages */
> + iowrite8(1 << mod->num, &mod->ctrl->int_enable);
> +
> + ret = ican3_msg_connect(mod);
> + if (ret) {
> + dev_err(mod->dev, "unable to connect to module\n");
> + return ret;
> + }
> +
> + ican3_init_new_host_interface(mod);
> + ret = ican3_msg_newhostif(mod);
> + if (ret) {
> + dev_err(mod->dev, "unable to switch to new-style interface\n");
> + return ret;
> + }
> +
> + ret = ican3_set_termination(mod, true);
> + if (ret) {
> + dev_err(mod->dev, "unable to enable termination\n");
> + return ret;
> + }
Could you please allow the user to disable termination, e.g. via module parameter
"bus_termination=0" in case he uses a cable with built-in termination.
[snip]
> +static int __devinit ican3_probe(struct platform_device *pdev)
> +{
> + struct janz_platform_data *pdata;
> + struct net_device *ndev;
> + struct ican3_dev *mod;
> + struct resource *res;
> + struct device *dev;
> + int ret;
> +
> + pdata = pdev->dev.platform_data;
> + if (!pdata)
> + return -ENXIO;
> +
> + dev_dbg(&pdev->dev, "probe: module number %d\n", pdata->modno);
> +
> + /* save the struct device for printing */
> + dev = &pdev->dev;
> +
> + /* allocate the CAN device and private data */
> + ndev = alloc_candev(sizeof(*mod), 0);
> + if (!ndev) {
> + dev_err(dev, "unable to allocate CANdev\n");
> + ret = -ENOMEM;
> + goto out_return;
> + }
> +
> + platform_set_drvdata(pdev, ndev);
> + mod = netdev_priv(ndev);
> + mod->ndev = ndev;
> + mod->dev = &pdev->dev;
> + mod->num = pdata->modno;
> + netif_napi_add(ndev, &mod->napi, ican3_napi, ICAN3_RX_BUFFERS);
> + spin_lock_init(&mod->lock);
> +
> + /* the first unallocated page in the DPM is 9 */
> + mod->free_page = DPM_FREE_START;
> +
> + ndev->netdev_ops = &ican3_netdev_ops;
> + ndev->flags |= IFF_ECHO;
> + SET_NETDEV_DEV(ndev, &pdev->dev);
> +
> + mod->can.clock.freq = 8000000;
Please use a constant here.
[snip]
Please fix and resubmit with my:
"Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>"
for the SocketCAN part.
Thanks,
Wolfgang.
^ permalink raw reply
* Re: [PATCH] ipv4: check rt_genid in dst_check
From: Herbert Xu @ 2010-03-19 9:00 UTC (permalink / raw)
To: Timo Teras, David S. Miller; +Cc: netdev
In-Reply-To: <20100319085600.GA23705@gondor.apana.org.au>
On Fri, Mar 19, 2010 at 04:56:00PM +0800, Herbert Xu wrote:
>
> I'll send a patch to remove the same check on the IPv6 path.
ipv6: Remove redundant dst NULL check in ip6_dst_check
As the only path leading to ip6_dst_check makes an indirect call
through dst->ops, dst cannot be NULL in ip6_dst_check.
This patch removes this check in case it misleads people who
come across this code.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 52cd3ef..7fcb0e5 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -879,7 +879,7 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
rt = (struct rt6_info *) dst;
- if (rt && rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie))
+ if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie))
return dst;
return NULL;
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* Re: [PATCH] ipv4: check rt_genid in dst_check
From: Herbert Xu @ 2010-03-19 8:56 UTC (permalink / raw)
To: Timo Teras; +Cc: netdev, timo.teras
In-Reply-To: <1268978083-32070-1-git-send-email-timo.teras@iki.fi>
Timo Teras <timo.teras@iki.fi> wrote:
>
> @@ -1726,7 +1726,9 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, u32 mtu)
>
> static struct dst_entry *ipv4_dst_check(struct dst_entry *dst, u32 cookie)
> {
> - return NULL;
> + if (dst && rt_is_expired((struct rtable *)dst))
> + return NULL;
Sorry I didn't spot this the first time around, but the dst check
is redundant too. Since the only path leading to this code is
static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
{
if (dst->obsolete)
dst = dst->ops->check(dst, cookie);
return dst;
}
I'll send a patch to remove the same check on the IPv6 path.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-19 8:47 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
In-Reply-To: <4BA337E6.4010508@iki.fi>
On Fri, Mar 19, 2010 at 10:37:58AM +0200, Timo Teräs wrote:
>
> But I changed that. the flow cache now does *not* call local_bh_enable
> if it returns something. This is deferred until corresponding _put
> call. So bh's are disable while we are touching the lookup results.
I'm sorry but making a function like flow_cache_lookup return with
BH disabled is just wrong!
> It'd probably make sense to remove that. And require _lookup to
> be called with bh disabled so it's more obvious that bh's are
> disabled when touching the cache entry.
That would be better but it's still hacky. Proper reference
counting like we had before would be my preference.
> Not a race. We need to keep bh's disabled while touching fce
> for various reasons.
What are those reasons (apart from this race)?
> Noone. When policy and dst is on cache there's no reference.
> The cache generation id's ensure that the object exists when
> they are in cache. It might make sense to add references to
> both objects and do a BUG_ON if the flow cache flusher would
> need to delete an object. I guess this would be the proper
> way, since that's how the dst stuff works too.
The cache genid is not enough:
CPU1 CPU2
check genid == OK
update genid
kill policy
kfree on policy
use policy == BOOM
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 8:37 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20100319082909.GA23363@gondor.apana.org.au>
Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 09:48:17AM +0200, Timo Teräs wrote:
>> But flow_cache_lookup disables pre-emption until _put is called.
>> So it should work. Would there be a cleaner way?
>
> Previously the flow cache returned a policy directly which works
> because whenever we modify that policy we'd take the appropriate
> lock.
>
> Your patch changes it so that it now returns an fce. But nothing
> is guarding the code that modifies fce. So two CPUs may end up
> modifying the same fce.
But I changed that. the flow cache now does *not* call local_bh_enable
if it returns something. This is deferred until corresponding _put
call. So bh's are disable while we are touching the lookup results.
It'd probably make sense to remove that. And require _lookup to
be called with bh disabled so it's more obvious that bh's are
disabled when touching the cache entry.
> However, it would appear that this race could be harmless, provided
> that you are careful about dereferencing fce->policy and fce->dst.
>
> IOW, this is not OK
>
> if (fce->policy)
> use fce->policy;
>
> and this should work
>
> policy = fce->policy;
> if (policy)
> use policy;
Not a race. We need to keep bh's disabled while touching fce
for various reasons.
> Actually on second tought, even this isn't totally safe. Who
> is taking a reference count on the policy and dst? I see a ref
> count on the fce, but nothing on fce->dst and fce->policy. Do
> you have an implicit reference on them?
Noone. When policy and dst is on cache there's no reference.
The cache generation id's ensure that the object exists when
they are in cache. It might make sense to add references to
both objects and do a BUG_ON if the flow cache flusher would
need to delete an object. I guess this would be the proper
way, since that's how the dst stuff works too.
- Timo
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-19 8:29 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
In-Reply-To: <4BA32C41.2020000@iki.fi>
On Fri, Mar 19, 2010 at 09:48:17AM +0200, Timo Teräs wrote:
>
> But flow_cache_lookup disables pre-emption until _put is called.
> So it should work. Would there be a cleaner way?
Previously the flow cache returned a policy directly which works
because whenever we modify that policy we'd take the appropriate
lock.
Your patch changes it so that it now returns an fce. But nothing
is guarding the code that modifies fce. So two CPUs may end up
modifying the same fce.
However, it would appear that this race could be harmless, provided
that you are careful about dereferencing fce->policy and fce->dst.
IOW, this is not OK
if (fce->policy)
use fce->policy;
and this should work
policy = fce->policy;
if (policy)
use policy;
Actually on second tought, even this isn't totally safe. Who
is taking a reference count on the policy and dst? I see a ref
count on the fce, but nothing on fce->dst and fce->policy. Do
you have an implicit reference on them?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] tcp: Generalized TTL Security Mechanism
From: Pekka Savola @ 2010-03-19 8:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Stephen Hemminger, David Miller, netdev
In-Reply-To: <1268986871.3048.9.camel@edumazet-laptop>
On Fri, 19 Mar 2010, Eric Dumazet wrote:
> This requires that any router in the path between the client and server
> also respects the MINTTL when sending ICMP. Not sure how practical it
> is...
You're correct that with multihop GTSM, ICMP becomes trickier. I'm
not sure how applicable GTSM is really in multihop scenarios, though.
I would not recommend using it to secure e.g. with minttl=250 or
something that uncontrollable. Your comment relates mainly to ICMP
soft/hard errors which are not critical for correct operation.
http://tools.ietf.org/html/draft-ietf-tcpm-icmp-attacks-11 discusses
this.
--
Pekka Savola "You each name yourselves king, yet the
Netcore Oy kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings
^ permalink raw reply
* Re: [PATCH] tcp: Generalized TTL Security Mechanism
From: Eric Dumazet @ 2010-03-19 8:21 UTC (permalink / raw)
To: Pekka Savola; +Cc: Stephen Hemminger, David Miller, netdev
In-Reply-To: <alpine.LRH.2.00.1003190837210.6428@netcore.fi>
Le vendredi 19 mars 2010 à 09:58 +0200, Pekka Savola a écrit :
>
> > But any application using GTSM should be setting IP_TTL socket
> option
> > to set send TTL. But, not sure if Linux TCP ever sends ICMP
> > for existing sessions at all.
>
> Thanks, Stephen! It's nice to see at least one compliant RFC5082
> implementation ;-)
>
> Good point that no one should should probably even be sending ICMP
> messages for TCP sockets, but on receive side the checks are
> important
> :-)
This requires that any router in the path between the client and server
also respects the MINTTL when sending ICMP. Not sure how practical it
is...
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 70df409..a9d3ba5 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -367,6 +367,9 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
if (sock_owned_by_user(sk))
NET_INC_STATS_BH(net, LINUX_MIB_LOCKDROPPEDICMPS);
+ if (iph->ttl < inet_sk(sk)->min_ttl)
+ goto out;
+
if (sk->sk_state == TCP_CLOSE)
goto out;
^ permalink raw reply related
* Re: [PATCH] tcp: Generalized TTL Security Mechanism
From: Pekka Savola @ 2010-03-19 7:58 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
In-Reply-To: <20100318105939.57f8d377@nehalam>
On Thu, 18 Mar 2010, Stephen Hemminger wrote:
>> The experimental, earlier spec (GTSH, RFC3682) did not have this
>> requirement. Most if not all implementations support only GTSH mode.
>> So a backward-compatibility option may be desirable.
>
> The ICMP receive error handling does need to be updated.
>
> But any application using GTSM should be setting IP_TTL socket option
> to set send TTL. But, not sure if Linux TCP ever sends ICMP
> for existing sessions at all.
Thanks, Stephen! It's nice to see at least one compliant RFC5082
implementation ;-)
Good point that no one should should probably even be sending ICMP
messages for TCP sockets, but on receive side the checks are important
:-)
--
Pekka Savola "You each name yourselves king, yet the
Netcore Oy kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 7:48 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20100319072053.GA22913@gondor.apana.org.au>
Herbert Xu wrote:
> On Mon, Mar 15, 2010 at 02:20:10PM +0200, Timo Teras wrote:
>> - policy = flow_cache_lookup(net, fl, dst_orig->ops->family,
>> - dir, xfrm_policy_lookup);
>> - err = PTR_ERR(policy);
>> - if (IS_ERR(policy)) {
>> - XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR);
>> - goto dropdst;
>> + fce = flow_cache_lookup(&net->xfrm.flow_cache,
>> + fl, family, dir);
>> + if (fce == NULL)
>> + goto no_cache;
>> +
>> + xf = container_of(fce, struct xfrm_flow_cache_entry, fce);
>> + xfrm_flow_cache_entry_validate(&net->xfrm.flow_cache, fce);
>
> This doesn't work.
>
> The flow cache operates without locking as it is a per-cpu cache.
> To make this work you must ensure that you stay on the same CPU
> or use some other form of synchronoisation if you write to the
> object returned.
>
> AFAICS there is no synchronisation here and you're writing to fce.
>
> So you'll need to disable preemption around the bit that touches
> fce.
But flow_cache_lookup disables pre-emption until _put is called.
So it should work. Would there be a cleaner way?
However, now I figured that we need to make walk.dead atomic
because it's read some times without taking the policy lock.
- Timo
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 7:27 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <4BA317CE.4050503@iki.fi>
Timo Teräs wrote:
> Herbert Xu wrote:
>> On Fri, Mar 19, 2010 at 07:48:57AM +0200, Timo Teräs wrote:
>>> But it always matches. The caching happens using the inner
>>> flow. Inner flow always matches with the same bundle unless
>>> the bundle expires or goes stale. What happens is that I get
>>> multiple cache entries per-inner flow each referencing to the
>>> same bundle.
>>
>> Sorry for being slow, but if it always matches, doesn't that mean
>> you'll only have a single bundle in the policy bundle list? IOW
>> why do we need this at all?
>
> No. The bundle created for specific flow, matches always later
> that flow.
Just figured that's it's easier to explain with an example.
We have SPD:
10.1.0.0/16 - 10.2.0.0/16 tunnel
1.2.3.4 - 4.3.2.1
Now we get n+1 clients to connect to server in 10.2.0.1.
They each get separate bundle, since the xfrm_dst will be
created and search using flow id's like:
src 10.1.x.x dst 10.2.0.1
So there's one xfrm_policy and xfrm_state, but n+1
xfrm_dst's.
Since the flow cache caches the result of lookups on the
inner flow "10.1.x.x->10.2.0.1" basis, it always returns
matching valid bundle in O(1) time unless the xfrm_dst
expired.
Currently it's looked up with O(n) search in find_bundle.
Same thing happens with wildcard transport mode SPD's.
E.g. SPD:
0.0.0.0/0 - 0.0.0.0/0 proto gre, transport
We are talking with gre to n+1 tunnel destinations.
We get n+1 xfrm_dst's in that xfrm_policy. Flow cache
works on inner flow using flows like:
src 1.2.3.4 dst 4.3.2.1 proto gre
And can keep in cache the right policy always, and
the bundle to use as long as it stays valid.
Hopefully this explains why I think the patch is
useful.
- Timo
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox