Netdev List
 help / color / mirror / Atom feed
* Re: question on raw sockets and source IP address validation
From: Chris Friesen @ 2009-09-24 19:37 UTC (permalink / raw)
  To: Neil Horman; +Cc: Linux Network Development list
In-Reply-To: <20090924192651.GC19787@hmsreliant.think-freely.org>

On 09/24/2009 01:26 PM, Neil Horman wrote:

> That said, its not doing source validation, your socket is actually doing a
> route lookup on the flow from your specified source address to your destination
> address.  So you should be able to fool the socket into doing the lookup by
> adding a route to your routing table from your source address to your
> destination address via the interface that you want to send the frames out of.

Hmm...that's an interesting point.  Worth investigating for sure.

Thanks,

Chris


^ permalink raw reply

* Re: [PATCH] inet_peer: Optimize inet_getid()
From: Eric Dumazet @ 2009-09-24 19:57 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <20090924123020.59a2ee6e@s6510>

Stephen Hemminger a écrit :
> On Thu, 24 Sep 2009 21:04:56 +0200
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> While investigating for network latencies, I found inet_getid() was a contention point
>> for some workloads.
>>
>> Fix is straightforward, using cmpxchg() instead of
>> a spin_lock_bh()/spin_unlock_bh() pair on a central lock.
>>
>> Another possibility was to use an atomic_t and atomic_add_return() but
>> the size of struct inet_peer object would had doubled on x86_64 because of
>> SLAB_HWCACHE_ALIGN constraint.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> I thought cmpxchg was not available on all architectures.

Good point Stephen, I forgot about non x86 arches ;)

I'll send an update with cmpxchg() if available, and atomic_t as fallback.

^ permalink raw reply

* Re: [PATCH] net: Fix sock_wfree() race
From: Jarek Poplawski @ 2009-09-24 20:07 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, albcamus, parag.lkml, linux-kernel, netdev
In-Reply-To: <4ABA262F.9040407@gmail.com>

Eric Dumazet wrote, On 09/23/2009 03:44 PM:

...
> Here is the patch for reference :
> 
> [PATCH] net: Fix sock_wfree() race
> 
> Commit 2b85a34e911bf483c27cfdd124aeb1605145dc80
> (net: No more expensive sock_hold()/sock_put() on each tx)
> opens a window in sock_wfree() where another cpu
> might free the socket we are working on.
> 
> A fix is to call sk->sk_write_space(sk) while still
> holding a reference on sk.
> 
> 
> Reported-by: Jike Song <albcamus@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/core/sock.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 30d5446..e1f034e 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1228,17 +1228,22 @@ void __init sk_init(void)
>  void sock_wfree(struct sk_buff *skb)
>  {
>  	struct sock *sk = skb->sk;
> -	int res;
> +	unsigned int len = skb->truesize;
>  
> -	/* In case it might be waiting for more memory. */
> -	res = atomic_sub_return(skb->truesize, &sk->sk_wmem_alloc);
> -	if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE))
> +	if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE)) {
> +		/*
> +		 * Keep a reference on sk_wmem_alloc, this will be released
> +		 * after sk_write_space() call
> +		 */
> +		atomic_sub(len - 1, &sk->sk_wmem_alloc);
>  		sk->sk_write_space(sk);
> +		len = 1;
> +	}
>  	/*
> -	 * if sk_wmem_alloc reached 0, we are last user and should
> -	 * free this sock, as sk_free() call could not do it.
> +	 * if sk_wmem_alloc reaches 0, we must finish what sk_free()
> +	 * could not do because of in-flight packets
>  	 */
> -	if (res == 0)
> +	if (atomic_sub_return(len, &sk->sk_wmem_alloc) == 0)
>  		__sk_free(sk);


Probably atomic_sub_and_test() is more popular for this.

Jarek P.

>  }
>  EXPORT_SYMBOL(sock_wfree);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



^ permalink raw reply

* Re: [PATCH] 3c59x: Get rid of "Trying to free already-free IRQ"
From: Rafael J. Wysocki @ 2009-09-24 20:30 UTC (permalink / raw)
  To: Anton Vorontsov; +Cc: netdev, linux-pm, David Miller
In-Reply-To: <20090924183152.GA30254@oksana.dev.rtsoft.ru>

On Thursday 24 September 2009, Anton Vorontsov wrote:
> Following trace pops up if we try to suspend with 3c59x ethernet NIC
> brought down:

Patch looks good, but IMO it'd be a little effort to convert the driver to
dev_pm_ops while you're at it (please see r8169 for a working example).

Thanks,
Rafael


>   root@b1:~# ifconfig eth16 down
>   root@b1:~# echo mem > /sys/power/state
>   ...
>   3c59x 0000:00:10.0: suspend
>   3c59x 0000:00:10.0: PME# disabled
>   Trying to free already-free IRQ 48
>   ------------[ cut here ]------------
>   Badness at c00554e4 [verbose debug info unavailable]
>   NIP: c00554e4 LR: c00554e4 CTR: c019a098
>   REGS: c7975c60 TRAP: 0700   Not tainted  (2.6.31-rc4)
>   MSR: 00021032 <ME,CE,IR,DR>  CR: 28242422  XER: 20000000
>   TASK = c79cb0c0[1746] 'bash' THREAD: c7974000
>   ...
>   NIP [c00554e4] __free_irq+0x108/0x1b0
>   LR [c00554e4] __free_irq+0x108/0x1b0
>   Call Trace:
>   [c7975d10] [c00554e4] __free_irq+0x108/0x1b0 (unreliable)
>   [c7975d30] [c005559c] free_irq+0x10/0x24
>   [c7975d40] [c01e21ec] vortex_suspend+0x70/0xc4
>   [c7975d60] [c017e584] pci_legacy_suspend+0x58/0x100
> 
> This is because the driver manages interrupts without checking for
> netif_running().
> 
> Though, there are few other issues with suspend/resume in this driver.
> The intention of calling free_irq() in suspend() was to avoid any
> possible spurious interrupts (see commit 5b039e681b8c5f30aac9cc04385
> "3c59x PM fixes"). But,
> 
> - On resume, the driver was requesting IRQ just after pci_set_master(),
>   but before vortex_up() (which actually resets 3c59x chips).
> 
> - Issuing free_irq() on a shared IRQ doesn't guarantee that a buggy
>   HW won't trigger spurious interrupts in another driver that
>   requested the same interrupt. So, if we want to protect from
>   unexpected interrupts, then on suspend we should issue disable_irq(),
>   not free_irq().
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  drivers/net/3c59x.c |   12 +++---------
>  1 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
> index c34aee9..7cdd4b0 100644
> --- a/drivers/net/3c59x.c
> +++ b/drivers/net/3c59x.c
> @@ -807,10 +807,10 @@ static int vortex_suspend(struct pci_dev *pdev, pm_message_t state)
>  		if (netif_running(dev)) {
>  			netif_device_detach(dev);
>  			vortex_down(dev, 1);
> +			disable_irq(dev->irq);
>  		}
>  		pci_save_state(pdev);
>  		pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
> -		free_irq(dev->irq, dev);
>  		pci_disable_device(pdev);
>  		pci_set_power_state(pdev, pci_choose_state(pdev, state));
>  	}
> @@ -833,18 +833,12 @@ static int vortex_resume(struct pci_dev *pdev)
>  			return err;
>  		}
>  		pci_set_master(pdev);
> -		if (request_irq(dev->irq, vp->full_bus_master_rx ?
> -				&boomerang_interrupt : &vortex_interrupt, IRQF_SHARED, dev->name, dev)) {
> -			pr_warning("%s: Could not reserve IRQ %d\n", dev->name, dev->irq);
> -			pci_disable_device(pdev);
> -			return -EBUSY;
> -		}
>  		if (netif_running(dev)) {
>  			err = vortex_up(dev);
>  			if (err)
>  				return err;
> -			else
> -				netif_device_attach(dev);
> +			enable_irq(dev->irq);
> +			netif_device_attach(dev);
>  		}
>  	}
>  	return 0;

^ permalink raw reply

* Re: [PATCH] inet_peer: Optimize inet_getid()
From: Eric Dumazet @ 2009-09-24 20:44 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <20090924123020.59a2ee6e@s6510>

Stephen Hemminger a écrit :
> On Thu, 24 Sep 2009 21:04:56 +0200
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> While investigating for network latencies, I found inet_getid() was a contention point
>> for some workloads.
>>
>> Fix is straightforward, using cmpxchg() instead of
>> a spin_lock_bh()/spin_unlock_bh() pair on a central lock.
>>
>> Another possibility was to use an atomic_t and atomic_add_return() but
>> the size of struct inet_peer object would had doubled on x86_64 because of
>> SLAB_HWCACHE_ALIGN constraint.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> I thought cmpxchg was not available on all architectures.

Here is the second version of the patch, thanks Stephen

[PATCH] inet_peer: Optimize inet_getid()

While investigating for network latencies, I found inet_getid() was a contention point
for some workloads.

If __HAVE_ARCH_CMPXCHG is defined, we can use cmpxchg() instead of
a spin_lock_bh()/spin_unlock_bh() pair on a central lock.

On other arches, we can use an atomic_t instead of a u16,
and atomic_add_return(). This might grow memory usage a bit, unless
someone invents atomic16_t :)

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/net/inetpeer.h |   43 ++++++++++++++++++++++++++++++++-------
 net/ipv4/inetpeer.c    |    5 ----
 net/ipv4/route.c       |    2 -
 3 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h
index 15e1f8f..f1cde51 100644
--- a/include/net/inetpeer.h
+++ b/include/net/inetpeer.h
@@ -19,7 +19,11 @@ struct inet_peer
 	struct inet_peer	*avl_left, *avl_right;
 	__be32			v4daddr;	/* peer's address */
 	__u16			avl_height;
+#if defined(__HAVE_ARCH_CMPXCHG)
 	__u16			ip_id_count;	/* IP ID for the next packet */
+#else
+	atomic_t		ip_id_count;
+#endif
 	struct list_head	unused;
 	__u32			dtime;		/* the time of last use of not
 						 * referenced entries */
@@ -37,17 +41,42 @@ struct inet_peer	*inet_getpeer(__be32 daddr, int create);
 /* can be called from BH context or outside */
 extern void inet_putpeer(struct inet_peer *p);
 
-extern spinlock_t inet_peer_idlock;
 /* can be called with or without local BH being disabled */
 static inline __u16	inet_getid(struct inet_peer *p, int more)
 {
-	__u16 id;
+	__u16 old;
 
-	spin_lock_bh(&inet_peer_idlock);
-	id = p->ip_id_count;
-	p->ip_id_count += 1 + more;
-	spin_unlock_bh(&inet_peer_idlock);
-	return id;
+	more++;
+#if defined(__HAVE_ARCH_CMPXCHG)
+	while (1) {
+		old = p->ip_id_count;
+		if (cmpxchg(&p->ip_id_count, old, old + more) == old)
+			break;
+	}
+#else
+	old = atomic_add_return(more, &p->ip_id_count) - more;
+#endif
+	return old;
 }
 
+static inline void inet_id_set(struct inet_peer *p, int val)
+{
+#if defined(__HAVE_ARCH_CMPXCHG)
+	p->ip_id_count = val;
+#else
+	atomic_set(&p->ip_id_count, val);
+#endif
+}
+
+static inline __u16 inet_id_read(const struct inet_peer *p)
+{
+#if defined(__HAVE_ARCH_CMPXCHG)
+	return p->ip_id_count;
+#else
+	return atomic_read(&p->ip_id_count);
+#endif
+}
+
+
+
 #endif /* _NET_INETPEER_H */
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index b1fbe18..3641831 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -67,9 +67,6 @@
  *		ip_id_count: idlock
  */
 
-/* Exported for inet_getid inline function.  */
-DEFINE_SPINLOCK(inet_peer_idlock);
-
 static struct kmem_cache *peer_cachep __read_mostly;
 
 #define node_height(x) x->avl_height
@@ -390,7 +387,7 @@ struct inet_peer *inet_getpeer(__be32 daddr, int create)
 	n->v4daddr = daddr;
 	atomic_set(&n->refcnt, 1);
 	atomic_set(&n->rid, 0);
-	n->ip_id_count = secure_ip_id(daddr);
+	inet_id_set(n, secure_ip_id(daddr));
 	n->tcp_ts_stamp = 0;
 
 	write_lock_bh(&peer_pool_lock);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index df93473..a090fcf 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2855,7 +2855,7 @@ static int rt_fill_info(struct net *net,
 	error = rt->u.dst.error;
 	expires = rt->u.dst.expires ? rt->u.dst.expires - jiffies : 0;
 	if (rt->peer) {
-		id = rt->peer->ip_id_count;
+		id = inet_id_read(rt->peer);
 		if (rt->peer->tcp_ts_stamp) {
 			ts = rt->peer->tcp_ts;
 			tsage = get_seconds() - rt->peer->tcp_ts_stamp;

^ permalink raw reply related

* Re: [PATCH] net: Fix sock_wfree() race
From: Eric Dumazet @ 2009-09-24 20:49 UTC (permalink / raw)
  To: Jarek Poplawski; +Cc: David Miller, albcamus, parag.lkml, linux-kernel, netdev
In-Reply-To: <4ABBD18A.70208@gmail.com>

Jarek Poplawski a écrit :
> Eric Dumazet wrote, On 09/23/2009 03:44 PM:
> 
> ...
>> Here is the patch for reference :
>>
>> [PATCH] net: Fix sock_wfree() race
>>
>> Commit 2b85a34e911bf483c27cfdd124aeb1605145dc80
>> (net: No more expensive sock_hold()/sock_put() on each tx)
>> opens a window in sock_wfree() where another cpu
>> might free the socket we are working on.
>>
>> A fix is to call sk->sk_write_space(sk) while still
>> holding a reference on sk.
>>
>>
>> Reported-by: Jike Song <albcamus@gmail.com>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>>  net/core/sock.c |   19 ++++++++++++-------
>>  1 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index 30d5446..e1f034e 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -1228,17 +1228,22 @@ void __init sk_init(void)
>>  void sock_wfree(struct sk_buff *skb)
>>  {
>>  	struct sock *sk = skb->sk;
>> -	int res;
>> +	unsigned int len = skb->truesize;
>>  
>> -	/* In case it might be waiting for more memory. */
>> -	res = atomic_sub_return(skb->truesize, &sk->sk_wmem_alloc);
>> -	if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE))
>> +	if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE)) {
>> +		/*
>> +		 * Keep a reference on sk_wmem_alloc, this will be released
>> +		 * after sk_write_space() call
>> +		 */
>> +		atomic_sub(len - 1, &sk->sk_wmem_alloc);
>>  		sk->sk_write_space(sk);
>> +		len = 1;
>> +	}
>>  	/*
>> -	 * if sk_wmem_alloc reached 0, we are last user and should
>> -	 * free this sock, as sk_free() call could not do it.
>> +	 * if sk_wmem_alloc reaches 0, we must finish what sk_free()
>> +	 * could not do because of in-flight packets
>>  	 */
>> -	if (res == 0)
>> +	if (atomic_sub_return(len, &sk->sk_wmem_alloc) == 0)
>>  		__sk_free(sk);
> 
> 
> Probably atomic_sub_and_test() is more popular for this.

Indeed, as x86 can generate faster code (no need of xadd instruction)

Thanks Jarek

[PATCH] net: Fix sock_wfree() race

Commit 2b85a34e911bf483c27cfdd124aeb1605145dc80
(net: No more expensive sock_hold()/sock_put() on each tx)
opens a window in sock_wfree() where another cpu
might free the socket we are working on.

A fix is to call sk->sk_write_space(sk) while still
holding a reference on sk.


Reported-by: Jike Song <albcamus@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/core/sock.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 30d5446..e1f034e 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1228,17 +1228,22 @@ void __init sk_init(void)
 void sock_wfree(struct sk_buff *skb)
 {
 	struct sock *sk = skb->sk;
-	int res;
+	unsigned int len = skb->truesize;
 
-	/* In case it might be waiting for more memory. */
-	res = atomic_sub_return(skb->truesize, &sk->sk_wmem_alloc);
-	if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE))
+	if (!sock_flag(sk, SOCK_USE_WRITE_QUEUE)) {
+		/*
+		 * Keep a reference on sk_wmem_alloc, this will be released
+		 * after sk_write_space() call
+		 */
+		atomic_sub(len - 1, &sk->sk_wmem_alloc);
 		sk->sk_write_space(sk);
+		len = 1;
+	}
 	/*
-	 * if sk_wmem_alloc reached 0, we are last user and should
-	 * free this sock, as sk_free() call could not do it.
+	 * if sk_wmem_alloc reaches 0, we must finish what sk_free()
+	 * could not do because of in-flight packets
 	 */
-	if (res == 0)
+	if (atomic_sub_and_test(len, &sk->sk_wmem_alloc))
 		__sk_free(sk);
 }
 EXPORT_SYMBOL(sock_wfree);


^ permalink raw reply related

* Re: [PATCH] 3c59x: Get rid of "Trying to free already-free IRQ"
From: Anton Vorontsov @ 2009-09-24 21:30 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: David Miller, linux-pm, netdev
In-Reply-To: <200909242230.33881.rjw@sisk.pl>

On Thu, Sep 24, 2009 at 10:30:33PM +0200, Rafael J. Wysocki wrote:
> On Thursday 24 September 2009, Anton Vorontsov wrote:
> > Following trace pops up if we try to suspend with 3c59x ethernet NIC
> > brought down:
> 
> Patch looks good, but IMO it'd be a little effort to convert the driver to
> dev_pm_ops while you're at it (please see r8169 for a working example).

I'd like to avoid putting irrelevant stuff into bugfixes.

Apart from delights as bisecting and revert-only-offending-piece,
keeping bugfixes small and self-sufficient helps to back-port the
fixes to stable/distro kernels. Think of not so old kernels that
don't have dev_pm_ops.

Converting this driver (and others that I'm interested in) to
dev_pm_ops is on my todo list though.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

^ permalink raw reply

* Gigaset & ISDN followup patches
From: Tilman Schmidt @ 2009-09-24 22:03 UTC (permalink / raw)
  To: Karsten Keil, Karsten Keil
  Cc: Hansjoerg Lipp, davem, i4ldeveloper, netdev, linux-kernel

Karsten,

I'm sending you one followup patch each to the ISDN and Gigaset patch
series for kernel 2.6.32 I sent on 2009-09-19, fixing issues I found
after that submission. I've tentatively tacked them on to the end of
each series as "patch 5/4" and "13/12" but if you prefer I can respin
patch 1 of the ISDN series and patch 12 of the Gigaset series to
incorporate the changes.

Looking forward to seeing these merged,
Tilman

^ permalink raw reply

* [PATCH 5/4] Documentation: correction to isdn/INTERFACE.CAPI
From: Tilman Schmidt @ 2009-09-24 22:04 UTC (permalink / raw)
  To: Karsten Keil, Karsten Keil; +Cc: davem, i4ldeveloper, netdev, linux-kernel
In-Reply-To: <20090924-patch-0.tilman@imap.cc>

Correct the paragraphs describing the _cstruct and _cmstruct types.
The _cstruct representation is in fact used for some struct
parameters containing struct subparameters, too.

Impact: documentation
Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 Documentation/isdn/INTERFACE.CAPI |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/isdn/INTERFACE.CAPI b/Documentation/isdn/INTERFACE.CAPI
index e6bb1a7..5fe8de5 100644
--- a/Documentation/isdn/INTERFACE.CAPI
+++ b/Documentation/isdn/INTERFACE.CAPI
@@ -217,18 +217,19 @@ u16         for CAPI parameters of type 'word'
 
 u32         for CAPI parameters of type 'dword'
 
-_cstruct    for CAPI parameters of type 'struct' not containing any
-	    variably-sized (struct) subparameters (eg. 'Called Party Number')
+_cstruct    for CAPI parameters of type 'struct'
 	    The member is a pointer to a buffer containing the parameter in
 	    CAPI encoding (length + content). It may also be NULL, which will
 	    be taken to represent an empty (zero length) parameter.
+	    Subparameters are stored in encoded form within the content part.
 
-_cmstruct   for CAPI parameters of type 'struct' containing 'struct'
-	    subparameters ('Additional Info' and 'B Protocol')
+_cmstruct   alternative representation for CAPI parameters of type 'struct'
+	    (used only for the 'Additional Info' and 'B Protocol' parameters)
 	    The representation is a single byte containing one of the values:
-	    CAPI_DEFAULT: the parameter is empty
-	    CAPI_COMPOSE: the values of the subparameters are stored
-	    individually in the corresponding _cmsg structure members
+	    CAPI_DEFAULT: The parameter is empty/absent.
+	    CAPI_COMPOSE: The parameter is present.
+	    Subparameter values are stored individually in the corresponding
+	    _cmsg structure members.
 
 Functions capi_cmsg2message() and capi_message2cmsg() are provided to convert
 messages between their transport encoding described in the CAPI 2.0 standard
-- 
1.6.2.1.214.ge986c


^ permalink raw reply related

* [PATCH 13/12] gigaset: add some more CAPI message handling
From: Tilman Schmidt @ 2009-09-24 22:04 UTC (permalink / raw)
  To: Karsten Keil, Karsten Keil
  Cc: Hansjoerg Lipp, davem, i4ldeveloper, netdev, linux-kernel
In-Reply-To: <20090924-patch-0.tilman@imap.cc>

Rejecting unsupported CAPI messages with code 0x1102 "illegal command
or subcommand" makes some real-world CAPI apps rather unhappy. This
patch adds code to reply to FACILITY_REQ and RESET_B3_REQ messages
with an appropriate "not supported" message, to accept but ignore
FACILITY_RESP, RESET_B3_RESP and MANUFACTURER messages, and to log
any messages still rejected as illegal.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 drivers/isdn/gigaset/capi.c |  171 +++++++++++++++++++++++++++++++++++++++---
 1 files changed, 158 insertions(+), 13 deletions(-)

diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c
index 6ea2b1d..8afff37 100644
--- a/drivers/isdn/gigaset/capi.c
+++ b/drivers/isdn/gigaset/capi.c
@@ -21,6 +21,7 @@
 #define CapiNcpiNotSupportedByProtocol	0x0001
 #define CapiFlagsNotSupportedByProtocol	0x0002
 #define CapiAlertAlreadySent		0x0003
+#define CapiFacilitySpecificFunctionNotSupported	0x3011
 
 /* missing from capicmd.h */
 #define CAPI_CONNECT_IND_BASELEN	(CAPI_MSG_BASELEN+4+2+8*1)
@@ -31,9 +32,19 @@
 #define CAPI_DATA_B3_CONF_LEN		(CAPI_MSG_BASELEN+4+2+2)
 #define CAPI_DISCONNECT_IND_LEN		(CAPI_MSG_BASELEN+4+2)
 #define CAPI_DISCONNECT_B3_IND_BASELEN	(CAPI_MSG_BASELEN+4+2+1)
+#define CAPI_FACILITY_CONF_BASELEN	(CAPI_MSG_BASELEN+4+2+2+1)
 /* most _CONF messages contain only Controller/PLCI/NCCI and Info parameters */
 #define CAPI_STDCONF_LEN		(CAPI_MSG_BASELEN+4+2)
 
+#define CAPI_FACILITY_HANDSET	0x0000
+#define CAPI_FACILITY_DTMF	0x0001
+#define CAPI_FACILITY_V42BIS	0x0002
+#define CAPI_FACILITY_SUPPSVC	0x0003
+#define CAPI_FACILITY_WAKEUP	0x0004
+#define CAPI_FACILITY_LI	0x0005
+
+#define CAPI_SUPPSVC_GETSUPPORTED	0x0000
+
 /* missing from capiutil.h */
 #define CAPIMSG_PLCI_PART(m)	CAPIMSG_U8(m, 9)
 #define CAPIMSG_NCCI_PART(m)	CAPIMSG_U16(m, 10)
@@ -1044,6 +1055,108 @@ static void send_conf(struct gigaset_capi_ctr *iif,
 }
 
 /*
+ * process FACILITY_REQ message
+ */
+static void do_facility_req(struct gigaset_capi_ctr *iif,
+			    struct gigaset_capi_appl *ap,
+			    struct sk_buff *skb)
+{
+	struct cardstate *cs = iif->ctr.driverdata;
+	struct sk_buff *cskb;
+	u8 *pparam;
+	unsigned int msgsize = CAPI_FACILITY_CONF_BASELEN;
+	u16 function, info;
+	static u8 confparam[10];	/* max. 9 octets + length byte */
+
+	/* decode message */
+	capi_message2cmsg(&iif->acmsg, skb->data);
+	dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+
+	/*
+	 * Facility Request Parameter is not decoded by capi_message2cmsg()
+	 * encoding depends on Facility Selector
+	 */
+	switch (iif->acmsg.FacilitySelector) {
+	case CAPI_FACILITY_DTMF:	/* ToDo */
+		info = CapiFacilityNotSupported;
+		confparam[0] = 2;	/* length */
+		/* DTMF information: Unknown DTMF request */
+		capimsg_setu16(confparam, 1, 2);
+		break;
+
+	case CAPI_FACILITY_V42BIS:	/* not supported */
+		info = CapiFacilityNotSupported;
+		confparam[0] = 2;	/* length */
+		/* V.42 bis information: not available */
+		capimsg_setu16(confparam, 1, 1);
+		break;
+
+	case CAPI_FACILITY_SUPPSVC:
+		/* decode Function parameter */
+		pparam = iif->acmsg.FacilityRequestParameter;
+		if (pparam == NULL || *pparam < 2) {
+			dev_notice(cs->dev, "%s: %s missing\n", "FACILITY_REQ",
+				   "Facility Request Parameter");
+			send_conf(iif, ap, skb, CapiIllMessageParmCoding);
+			return;
+		}
+		function = CAPIMSG_U16(pparam, 1);
+		switch (function) {
+		case CAPI_SUPPSVC_GETSUPPORTED:
+			info = CapiSuccess;
+			/* Supplementary Service specific parameter */
+			confparam[3] = 6;	/* length */
+			/* Supplementary services info: Success */
+			capimsg_setu16(confparam, 4, CapiSuccess);
+			/* Supported Services: none */
+			capimsg_setu32(confparam, 6, 0);
+			break;
+		/* ToDo: add supported services */
+		default:
+			info = CapiFacilitySpecificFunctionNotSupported;
+			/* Supplementary Service specific parameter */
+			confparam[3] = 2;	/* length */
+			/* Supplementary services info: not supported */
+			capimsg_setu16(confparam, 4,
+				       CapiSupplementaryServiceNotSupported);
+		}
+
+		/* Facility confirmation parameter */
+		confparam[0] = confparam[3] + 3;	/* total length */
+		/* Function: copy from _REQ message */
+		capimsg_setu16(confparam, 1, function);
+		/* Supplementary Service specific parameter already set above */
+		break;
+
+	case CAPI_FACILITY_WAKEUP:	/* ToDo */
+		info = CapiFacilityNotSupported;
+		confparam[0] = 2;	/* length */
+		/* Number of accepted awake request parameters: 0 */
+		capimsg_setu16(confparam, 1, 0);
+		break;
+
+	default:
+		info = CapiFacilityNotSupported;
+		confparam[0] = 0;	/* empty struct */
+	}
+
+	/* send FACILITY_CONF with given Info and confirmation parameter */
+	capi_cmsg_answer(&iif->acmsg);
+	iif->acmsg.Info = info;
+	iif->acmsg.FacilityConfirmationParameter = confparam;
+	msgsize += confparam[0];	/* length */
+	cskb = alloc_skb(msgsize, GFP_ATOMIC);
+	if (!cskb) {
+		dev_err(cs->dev, "%s: out of memory\n", __func__);
+		return;
+	}
+	capi_cmsg2message(&iif->acmsg, __skb_put(cskb, msgsize));
+	dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+		capi_ctr_handle_message(&iif->ctr, ap->id, cskb);
+}
+
+
+/*
  * process LISTEN_REQ message
  * just store the masks in the application data structure
  */
@@ -1766,7 +1879,28 @@ static void do_data_b3_req(struct gigaset_capi_ctr *iif,
 }
 
 /*
- * CAPI message handler: just reply "not supported in current state"
+ * process RESET_B3_REQ message
+ * just always reply "not supported by current protocol"
+ */
+static void do_reset_b3_req(struct gigaset_capi_ctr *iif,
+			    struct gigaset_capi_appl *ap,
+			    struct sk_buff *skb)
+{
+	/* decode message */
+	capi_message2cmsg(&iif->acmsg, skb->data);
+	dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+	send_conf(iif, ap, skb,
+		  CapiResetProcedureNotSupportedByCurrentProtocol);
+}
+
+/*
+ * dump unsupported/ignored messages at most twice per minute,
+ * some apps send those very frequently
+ */
+static unsigned long ignored_msg_dump_time;
+
+/*
+ * unsupported CAPI message handler
  */
 static void do_unsupported(struct gigaset_capi_ctr *iif,
 			   struct gigaset_capi_appl *ap,
@@ -1774,7 +1908,8 @@ static void do_unsupported(struct gigaset_capi_ctr *iif,
 {
 	/* decode message */
 	capi_message2cmsg(&iif->acmsg, skb->data);
-	dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+	if (printk_timed_ratelimit(&ignored_msg_dump_time, 30 * 1000))
+		dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
 	send_conf(iif, ap, skb, CapiMessageNotSupportedInCurrentState);
 }
 
@@ -1785,7 +1920,11 @@ static void do_nothing(struct gigaset_capi_ctr *iif,
 		       struct gigaset_capi_appl *ap,
 		       struct sk_buff *skb)
 {
-	dump_rawmsg(DEBUG_CMD, __func__, skb->data);
+	if (printk_timed_ratelimit(&ignored_msg_dump_time, 30 * 1000)) {
+		/* decode message */
+		capi_message2cmsg(&iif->acmsg, skb->data);
+		dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg);
+	}
 	dev_kfree_skb(skb);
 }
 
@@ -1809,6 +1948,7 @@ static struct {
 	/* most frequent messages first for faster lookup */
 	{ CAPI_DATA_B3_REQ, do_data_b3_req },
 	{ CAPI_DATA_B3_RESP, do_data_b3_resp },
+
 	{ CAPI_ALERT_REQ, do_alert_req },
 	{ CAPI_CONNECT_ACTIVE_RESP, do_nothing },
 	{ CAPI_CONNECT_B3_ACTIVE_RESP, do_nothing },
@@ -1821,23 +1961,25 @@ static struct {
 	{ CAPI_DISCONNECT_B3_RESP, do_nothing },
 	{ CAPI_DISCONNECT_REQ, do_disconnect_req },
 	{ CAPI_DISCONNECT_RESP, do_nothing },
-	{ CAPI_INFO_REQ, do_unsupported },
+	{ CAPI_FACILITY_REQ, do_facility_req },
+	{ CAPI_FACILITY_RESP, do_nothing },
+	{ CAPI_LISTEN_REQ, do_listen_req },
+	{ CAPI_SELECT_B_PROTOCOL_REQ, do_unsupported },
+	{ CAPI_RESET_B3_REQ, do_reset_b3_req },
+	{ CAPI_RESET_B3_RESP, do_nothing },
+
 	/*
 	 * ToDo: support overlap sending (requires ev-layer state
 	 * machine extension to generate additional ATD commands)
 	 */
+	{ CAPI_INFO_REQ, do_unsupported },
 	{ CAPI_INFO_RESP, do_nothing },
-	{ CAPI_LISTEN_REQ, do_listen_req },
-	{ CAPI_SELECT_B_PROTOCOL_REQ, do_unsupported },
+
 	/*
-	 * ToDo:
-	 * CAPI_FACILITY_REQ
-	 * CAPI_FACILITY_RESP
-	 * CAPI_MANUFACTURER_REQ
-	 * CAPI_MANUFACTURER_RESP
-	 * CAPI_RESET_B3_REQ
-	 * CAPI_RESET_B3_RESP
+	 * ToDo: what's the proper response for these?
 	 */
+	{ CAPI_MANUFACTURER_REQ, do_nothing },
+	{ CAPI_MANUFACTURER_RESP, do_nothing },
 };
 
 /* look up handler */
@@ -1887,6 +2029,9 @@ static u16 gigaset_send_message(struct capi_ctr *ctr, struct sk_buff *skb)
 	handler = lookup_capi_send_handler(CAPIMSG_CMD(skb->data));
 	if (!handler) {
 		/* unknown/unsupported message type */
+		if (printk_ratelimit())
+			dev_notice(cs->dev, "%s: unsupported message %u\n",
+				   __func__, CAPIMSG_CMD(skb->data));
 		return CAPI_ILLCMDORSUBCMDORMSGTOSMALL;
 	}
 
-- 
1.6.2.1.214.ge986c

^ permalink raw reply related

* [PATCH] icmp: No need to call sk_write_space()
From: Eric Dumazet @ 2009-09-24 22:16 UTC (permalink / raw)
  To: David S. Miller; +Cc: Linux Netdev List

We can make icmp messages tx completion callback a litle bit faster.

Setting SOCK_USE_WRITE_QUEUE sk flag tells sock_wfree() to
not call sk_write_space() on a socket we know no thread is posssibly
waiting for write space. (on per cpu kernel internal icmp sockets only)

This avoids the sock_def_write_space() call and 
read_lock(&sk->sk_callback_lock)/read_unlock(&sk->sk_callback_lock) calls
as well.

We avoid three atomic ops.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 5bc13fe..84adb57 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1165,6 +1165,10 @@ static int __net_init icmp_sk_init(struct net *net)
 		sk->sk_sndbuf =
 			(2 * ((64 * 1024) + sizeof(struct sk_buff)));
 
+		/*
+		 * Speedup sock_wfree()
+		 */
+		sock_set_flag(sk, SOCK_USE_WRITE_QUEUE);
 		inet_sk(sk)->pmtudisc = IP_PMTUDISC_DONT;
 	}
 

^ permalink raw reply related

* Re: [PATCH] 3c59x: Get rid of "Trying to free already-free IRQ"
From: David Miller @ 2009-09-24 22:26 UTC (permalink / raw)
  To: avorontsov; +Cc: rjw, linux-pm, netdev
In-Reply-To: <20090924213039.GA15904@oksana.dev.rtsoft.ru>

From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Fri, 25 Sep 2009 01:30:39 +0400

> On Thu, Sep 24, 2009 at 10:30:33PM +0200, Rafael J. Wysocki wrote:
>> On Thursday 24 September 2009, Anton Vorontsov wrote:
>> > Following trace pops up if we try to suspend with 3c59x ethernet NIC
>> > brought down:
>> 
>> Patch looks good, but IMO it'd be a little effort to convert the driver to
>> dev_pm_ops while you're at it (please see r8169 for a working example).
> 
> I'd like to avoid putting irrelevant stuff into bugfixes.

Agreed.

^ permalink raw reply

* Re: [PATCH 1/2] ipv4: fix do_ip_setsockopt optlen check for IP_MULTICAST_IF
From: David Miller @ 2009-09-24 22:44 UTC (permalink / raw)
  To: shanwei
  Cc: dfeng, kaber, yoshfuji, jmorris, pekkas, kuznet, netdev,
	linux-kernel
In-Reply-To: <4AB97CB6.4000004@cn.fujitsu.com>

From: Shan Wei <shanwei@cn.fujitsu.com>
Date: Wed, 23 Sep 2009 09:41:10 +0800

> [PATCH BUGFIX] ipv4: check optlen for IP_MULTICAST_IF option
> 
> Due to man page of setsockopt, if optlen is not valid, kernel should return
> -EINVAL. But a simple testcase as following, errno is 0, which means setsockopt
> is successful.
> 	addr.s_addr = inet_addr("192.1.2.3");
> 	setsockopt(s, IPPROTO_IP, IP_MULTICAST_IF, &addr, 1);
> 	printf("errno is %d\n", errno);
> 
> Xiaotian Feng(dfeng@redhat.com) caught the bug. We fix it firstly checking
> the availability of optlen and then dealing with the logic like other options. 
> 
> Reported-by: Xiaotian Feng <dfeng@redhat.com> 
> Signed-off-by: Shan Wei <shanwei@cn.fujitsu.com>
> Acked-by: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>

Applied.

^ permalink raw reply

* Re: [PATCH 2/2] pktgen: better scheduler friendliness
From: David Miller @ 2009-09-24 22:44 UTC (permalink / raw)
  To: shemminger; +Cc: netdev
In-Reply-To: <20090923054201.520300835@vyatta.com>

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 22 Sep 2009 22:41:43 -0700

> Previous update did not resched in inner loop causing watchdogs.
> Rewrite inner loop to:
>   * account for delays better with less clock calls
>   * more accurate timing of delay:
>     - only delay if packet was successfully sent
>     - if delay is 100ns and it takes 10ns to build packet then
>       account for that
>   * use wait_event_interruptible_timeout rather than open coding it.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

^ permalink raw reply

* Re: [PATCH 1/2] pktgen: T_TERMINATE flag is unused
From: David Miller @ 2009-09-24 22:44 UTC (permalink / raw)
  To: shemminger; +Cc: netdev
In-Reply-To: <20090923054201.440781719@vyatta.com>

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 22 Sep 2009 22:41:42 -0700

> Get rid of unused flag bit.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

^ permalink raw reply

* Re: [PATCH] Phonet: error on broadcast sending (unimplemented)
From: David Miller @ 2009-09-24 22:45 UTC (permalink / raw)
  To: remi; +Cc: netdev, remi.denis-courmont
In-Reply-To: <1253711831-7947-2-git-send-email-remi@remlab.net>

From: Rémi Denis-Courmont <remi@remlab.net>
Date: Wed, 23 Sep 2009 16:17:11 +0300

> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> If we ever implement this, then we can stop returning an error.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Applied.

^ permalink raw reply

* Re: [PATCH] Phonet: fix race for port number in concurrent bind()
From: David Miller @ 2009-09-24 22:45 UTC (permalink / raw)
  To: remi; +Cc: netdev, remi.denis-courmont
In-Reply-To: <1253711831-7947-1-git-send-email-remi@remlab.net>

From: Rémi Denis-Courmont <remi@remlab.net>
Date: Wed, 23 Sep 2009 16:17:10 +0300

> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> Allocating a port number to a socket and hashing that socket shall be
> an atomic operation with regards to other port allocation. Otherwise,
> we could allocate a port that is already being allocated to another
> socket.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Applied.

^ permalink raw reply

* Re: [PATCH] genetlink: fix netns vs. netlink table locking (2)
From: David Miller @ 2009-09-24 22:45 UTC (permalink / raw)
  To: johannes; +Cc: netdev
In-Reply-To: <1253698470.4458.48.camel@johannes.local>

From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 23 Sep 2009 11:34:30 +0200

> Similar to commit d136f1bd366fdb7e747ca7e0218171e7a00a98a5,
> there's a bug when unregistering a generic netlink family,
> which is caught by the might_sleep() added in that commit:
> 
>     BUG: sleeping function called from invalid context at net/netlink/af_netlink.c:183
>     in_atomic(): 1, irqs_disabled(): 0, pid: 1510, name: rmmod
>     2 locks held by rmmod/1510:
>      #0:  (genl_mutex){+.+.+.}, at: [<ffffffff8138283b>] genl_unregister_family+0x2b/0x130
>      #1:  (rcu_read_lock){.+.+..}, at: [<ffffffff8138270c>] __genl_unregister_mc_group+0x1c/0x120
>     Pid: 1510, comm: rmmod Not tainted 2.6.31-wl #444
>     Call Trace:
>      [<ffffffff81044ff9>] __might_sleep+0x119/0x150
>      [<ffffffff81380501>] netlink_table_grab+0x21/0x100
>      [<ffffffff813813a3>] netlink_clear_multicast_users+0x23/0x60
>      [<ffffffff81382761>] __genl_unregister_mc_group+0x71/0x120
>      [<ffffffff81382866>] genl_unregister_family+0x56/0x130
>      [<ffffffffa0007d85>] nl80211_exit+0x15/0x20 [cfg80211]
>      [<ffffffffa000005a>] cfg80211_exit+0x1a/0x40 [cfg80211]
> 
> Fix in the same way by grabbing the netlink table lock
> before doing rcu_read_lock().
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Applied.

^ permalink raw reply

* Re: [PATCH] net: fix htmldocs sunrpc, clnt.c
From: David Miller @ 2009-09-24 22:46 UTC (permalink / raw)
  To: randy.dunlap
  Cc: jaswinder, Ricardo.Labiaga, bhalevy, andros, Trond.Myklebust,
	linux-nfs, netdev, linux-kernel
In-Reply-To: <4ABB9C17.3020307@oracle.com>

From: Randy Dunlap <randy.dunlap@oracle.com>
Date: Thu, 24 Sep 2009 09:19:35 -0700

> Jaswinder Singh Rajput wrote:
>>   DOCPROC Documentation/DocBook/networking.xml
>>   Warning(net/sunrpc/clnt.c:647): No description found for parameter 'req'
>>   Warning(net/sunrpc/clnt.c:647): No description found for parameter 'tk_ops'
>>   Warning(net/sunrpc/clnt.c:647): Excess function parameter 'ops' description in 'rpc_run_bc_task'
>> 
>> Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> 
> Ack.  Already sent, but possibly lost.

Applied.

^ permalink raw reply

* Re: [PATCH v2] ems_pci: fix size of CAN controllers BAR mapping for CPC-PCI v2
From: David Miller @ 2009-09-24 22:46 UTC (permalink / raw)
  To: wg; +Cc: haas, netdev, socketcan-core
In-Reply-To: <4ABB7F63.2000108@grandegger.com>

From: Wolfgang Grandegger <wg@grandegger.com>
Date: Thu, 24 Sep 2009 16:17:07 +0200

> Sebastian Haas wrote:
>> The driver mapped only 128 bytes of the CAN controller address space when a
>> CPC-PCI v2 was detected (incl. CPC-104P). This patch will fix it by always
>> mapping the whole address space (4096 bytes on all boards) of the
>> corresponding PCI BAR.
>> 
>> Signed-off-by: Sebastian Haas <haas@ems-wuensche.com>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>

Applied.

^ permalink raw reply

* Re: [PATCH] tunnel: eliminate recursion field
From: David Miller @ 2009-09-24 22:47 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev
In-Reply-To: <4ABA84F1.5000802@gmail.com>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 23 Sep 2009 22:28:33 +0200

> Eric Dumazet a écrit :
>> It seems recursion field from "struct ip_tunnel" is not anymore needed.
>> recursion prevention is done at the upper level (in dev_queue_xmit()),
>> since we use HARD_TX_LOCK protection for tunnels.
>> 
>> This avoids a cache line ping pong on "struct ip_tunnel" : This structure
>> should be now mostly read on xmit and receive paths.
> 
> Oops I forgot ipv6 tunnels, silly me, here is an updated version.
> 
> Thanks
> 
> [PATCH] tunnel: eliminate recursion field
> 
> It seems recursion field from "struct ip_tunnel" is not anymore needed.
> recursion prevention is done at the upper level (in dev_queue_xmit()),
> since we use HARD_TX_LOCK protection for tunnels.
> 
> This avoids a cache line ping pong on "struct ip_tunnel" : This structure
> should be now mostly read on xmit and receive paths.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

^ permalink raw reply

* Re: [PATCH] 3c59x: Get rid of "Trying to free already-free IRQ"
From: David Miller @ 2009-09-24 22:47 UTC (permalink / raw)
  To: avorontsov; +Cc: rjw, linux-pm, netdev
In-Reply-To: <20090924183152.GA30254@oksana.dev.rtsoft.ru>

From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 24 Sep 2009 22:31:52 +0400

> Following trace pops up if we try to suspend with 3c59x ethernet NIC
> brought down:
 ...
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Applied.


^ permalink raw reply

* RE: [PATCH] ks8851_ml ethernet network driver - FIXED LINE-WRAPPING ISSUE
From: Choi, David @ 2009-09-24 23:02 UTC (permalink / raw)
  To: David Miller; +Cc: greg, netdev, Li, Charles, Choi, jgarzik, shemminger
In-Reply-To: <20090917.164952.33104590.davem@davemloft.net>

Hello David Miller,

It has taken pretty long time to figure it out to submitting the patch in my environment.
Now I am almost sure I find a workaround solution to submit the patch in my environment.
Anyway I am sorry for making mistakes in submitting the patch and causes to waste your valuable time. 

This patch fixes up as followings;
   .Remove "#define DEBUG" and "#define MALLOC".
   .Remove the compile warning messages from ks_inblk() and ks_outblk().
   .add "return IRQ_NONE" when there is no hardware IRQ indication in ks_irq().
   .remove mutex_lock/unlock from ks_net_open because they are redundancy.

I will appreciate if you send back any comments on my patch.

-------------------------------
--- linux-2.6.31-rc3/drivers/net/ks8851_mll.c.orig	2009-09-17 10:18:56.000000000 -0700
+++ linux-2.6.31-rc3/drivers/net/ks8851_mll.c	2009-09-17 10:09:37.000000000 -0700
@@ -21,8 +21,6 @@
  * KS8851 16bit MLL chip from Micrel Inc.
  */
 
-#define DEBUG
-
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/netdevice.h>
@@ -419,7 +417,6 @@ union ks_tx_hdr {
  * or one of the work queues.
  *
  */
-#define MALLOC(x)		kmalloc(x, GFP_KERNEL)
 
 /* Receive multiplex framer header info */
 struct type_frame_head {
@@ -552,11 +549,9 @@ static void ks_wrreg16(struct ks_net *ks
  */
 static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
 {
-	u32 data_port = (u32)ks->hw_addr;
 	len >>= 1;
-	do {
-		*wptr++ = (u16)ioread16(data_port);
-	} while (--len);
+	while (len--)
+		*wptr++ = (u16)ioread16(ks->hw_addr);
 }
 
 /**
@@ -568,11 +563,9 @@ static inline void ks_inblk(struct ks_ne
  */
 static inline void ks_outblk(struct ks_net *ks, u16 *wptr, u32 len)
 {
-	u32 data_port = (u32)ks->hw_addr;
 	len >>= 1;
-	do {
-		iowrite16(*wptr++, data_port);
-	} while (--len);
+	while (len--)
+		iowrite16(*wptr++, ks->hw_addr);
 }
 
 /**
@@ -818,6 +811,11 @@ static irqreturn_t ks_irq(int irq, void 
 	ks_save_cmd_reg(ks);
 
 	status = ks_rdreg16(ks, KS_ISR);
+	if (unlikely(!status)) {
+		ks_restore_cmd_reg(ks);
+		return IRQ_NONE;
+	}
+
 	ks_wrreg16(ks, KS_ISR, status);
 
 	if (likely(status & IRQ_RXI))
@@ -858,7 +856,6 @@ static int ks_net_open(struct net_device
 	/* lock the card, even if we may not actually do anything
 	 * else at the moment.
 	 */
-	mutex_lock(&ks->lock);
 
 	if (netif_msg_ifup(ks))
 		ks_dbg(ks, "%s - entry\n", __func__);
@@ -875,8 +872,6 @@ static int ks_net_open(struct net_device
 	if (netif_msg_ifup(ks))
 		ks_dbg(ks, "network device %s up\n", netdev->name);
 
-	mutex_unlock(&ks->lock);
-
 	return 0;
 }
 
@@ -1515,12 +1510,13 @@ void ks_enable(struct ks_net *ks)
 
 static int ks_hw_init(struct ks_net *ks)
 {
+#define	MHEADER_SIZE	(sizeof(struct type_frame_head) * MAX_RECV_FRAMES)
 	ks->promiscuous = 0;
 	ks->all_mcast = 0;
 	ks->mcast_lst_size = 0;
 
 	ks->frame_head_info = (struct type_frame_head *) \
-		MALLOC(sizeof(struct type_frame_head) * MAX_RECV_FRAMES);
+		kmalloc(MHEADER_SIZE, GFP_KERNEL);
 	if (!ks->frame_head_info) {
 		printk(KERN_ERR "Error: Fail to allocate frame memory\n");
 		return false;

-------------------------------

Regards,
David J. Choi




-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Thu 9/17/2009 4:49 PM
To: Choi, David
Cc: greg@kroah.com; netdev@vger.kernel.org; Li, Charles; Choi@kroah.com; jgarzik@redhat.com; shemminger@vyatta.com
Subject: Re: [PATCH] ks8851_ml ethernet network driver
 
From: "Choi, David" <David.Choi@Micrel.Com>
Date: Thu, 17 Sep 2009 12:30:27 -0700

> --- linux-2.6.31-rc3/drivers/net/ks8851_mll.c.orig	2009-09-17
> 10:18:56.000000000 -0700
> +++ linux-2.6.31-rc3/drivers/net/ks8851_mll.c	2009-09-17
> 10:09:37.000000000 -0700
> @@ -21,8 +21,6 @@
>   * KS8851 16bit MLL chip from Micrel Inc.

I can't use this patch or even test it, as your email client
has corrupted it by, for example, breaking up long lines.


^ permalink raw reply

* Re: [PATCH] ks8851_ml ethernet network driver - FIXED LINE-WRAPPING ISSUE
From: David Miller @ 2009-09-24 23:39 UTC (permalink / raw)
  To: David.Choi; +Cc: greg, netdev, Charles.Li, Choi, jgarzik, shemminger
In-Reply-To: <C43529A246480145B0A6D0234BDB0F0D02127B@MELANITE.micrel.com>


This is a patch against the driver, not the whole new driver.

We want the whole new driver, with your proper commit log message,
and proper signoffs.

^ permalink raw reply

* Re: TCP stack bug related to F-RTO?
From: Ray Lee @ 2009-09-24 23:39 UTC (permalink / raw)
  To: Joe Cao, Netdev; +Cc: linux-kernel, jcaoco2002
In-Reply-To: <427999.33681.qm@web63406.mail.re1.yahoo.com>

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

[adding netdev cc:]

On Thu, Sep 24, 2009 at 10:43 AM, Joe Cao <caoco2002@yahoo.com> wrote:
>
> Hello,
>
> I have found the following behavior with different versions of linux kernel. The attached pcap trace is collected with server (192.168.0.13) running 2.6.24 and shows the problem. Basically the behavior is like this:
>
> 1. The client opens up a big window,
> 2. the server sends 19 packets in a row (pkt #14- #32 in the trace), but all of them are dropped due to some congestion.
> 3. The server hits RTO and retransmits pkt #14 in #33
> 4. The client immediately acks #33 (=#14), and the server (seems like to enter F-RTO) expends the window and sends *NEW* pkt #35 & #36.=A0 Timeoute is doubled to 2*RTO; The client immediately sends two Dup-ack to #35 and #36.
> 5. after 2*RTO, pkt #15 is retransmitted in #39.
> 6. The client immediately acks #39 (=#15) in #40, and the server continues to expand the window and sends two *NEW* pkt #41 & #42. Now the timeoute is doubled to 4 *RTO.
> 8. After 4*RTO timeout, #16 is retransmitted.
> 9....
> 10. The above steps repeats for retransmitting pkt #16-#32 and each time the timeout is doubled.
> 11. It takes a long long time to retransmit all the lost packets and before that is done, the client sends a RST because of timeout.
>
> The above behavior looks like F-RTO is in effect.  And there seems to be a bug in the TCP's congestion control and retransmission algorithm. Why doesn't the TCP on server (running 2.6.24) enter the slow start? Why should the server take that long to recover from a short period of packet loss?
>
> Has anyone else noticed similar problem before?  If my analysis was wrong, can anyone gives me some pointers to what's really wrong and how to fix it?
>
> Thanks a lot,
> Joe
>
> PS. Please cc me when this message is replied.
>
>
>

[-- Attachment #2: frto.pcap.7 --]
[-- Type: application/octet-stream, Size: 73622 bytes --]

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox