Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] tcp: fix ICMP-RTO war
From: Ilpo Järvinen @ 2010-01-27 12:36 UTC (permalink / raw)
  To: Damian Lukowski
  Cc: Denys Fedoryshchenko, Netdev, David Miller, Alexey Kuznetsov
In-Reply-To: <4B5D8ABB.8030906@tvk.rwth-aachen.de>

On Mon, 25 Jan 2010, Damian Lukowski wrote:

> considering Denys' latest tests, I think we should bound
> at TCP_RTO_MIN inside __tcp_set_rto().
> Look at the following piece:
> > [  604.193389] rto: 200 (0 >> 3 + 0, 32) time: 304193 sent: 304091 pen: 1 307291 rem: 98                                                                                         
> > [  604.193518] lower bound violation: 0 code 1 sk_state 1                                                                                                                        
> > [  604.193589] rto: 200 (0 >> 3 + 0, 31) time: 304193 sent: 304091 pen: 1 304291 rem: 98                                                                                         
> > [  604.193706] lower bound violation: 0 code 1 sk_state 1                                                                                                                        
> > [  604.193776] rto: 200 (0 >> 3 + 0, 30) time: 304193 sent: 304091 pen: 1 304291 rem: 98                                                                                         
> > [  607.341327] lower bound violation: 0 code 1 sk_state 1                                                                                                                        
> > [  607.341412] rto: 200 (0 >> 3 + 0, 33) time: 307341 sent: 307091 pen: 1 310291 rem: 0  
> 
> We have a burst of three incoming ICMPs, not triggering retransmissions because
> of rem > 0. Nevertheless, there is an increase of icsk_backoff by four
> within 3100ms, with no ICMPs in between.
> For me, this is explainable by the broken mdev/rtt issue together with
> bursty ICMP replies.

Unless they are for a different connection? We might have to print sk (%p) 
in all those printouts to be sure which maps to which. If a peer becomes 
unreachable, it may well have multiple connections open (this was a 
proxy, iirc?).

-- 
 i.

^ permalink raw reply

* Re: [PATCH] ipv6: Fix possible NULL dereference in icmp6_dst_alloc
From: Atis Elsts @ 2010-01-27 12:35 UTC (permalink / raw)
  To: Jarek Poplawski; +Cc: David Miller, shemminger, djohnson, sakkiped, netdev
In-Reply-To: <20100127121746.GA15611@ff.dom.local>

> On Wed, Jan 27, 2010 at 03:04:16AM -0800, David Miller wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Wed, 27 Jan 2010 10:34:17 +0000
>>
>> > (Btw, maybe it's a sign this might be more readable?)
>>
>> I welcome suggests that, tree wide, would make this harder
>> to misinterpret.

How about adding static inline functions for accessing the dst_entry
field in struct route via a cast? That way it would be immediately
obvious that invalid memory access is not going to happen. Like this:

static inline struct dst_entry *route_dst(struct rtable *rt)
{
	return (struct dst_entry *)rt;
}

static inline struct dst_entry *route6_dst(struct rt6_info *rt)
{
	return (struct dst_entry *)rt;
}

and so on...

Atis

^ permalink raw reply

* Re: [PATCH] ipv6: Fix possible NULL dereference in icmp6_dst_alloc
From: David Miller @ 2010-01-27 12:23 UTC (permalink / raw)
  To: jarkao2; +Cc: shemminger, djohnson, sakkiped, netdev
In-Reply-To: <20100127121746.GA15611@ff.dom.local>

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 27 Jan 2010 12:17:46 +0000

> If there is nothing better I still think this patch is right, except
> better subject (Fix impossible NULL dereference in ... ;-).

Sorry, I'm not applying it, just like I didn't apply Krishna's similar
patch he posted earlier today.

^ permalink raw reply

* Re: [PATCH] ipv6: Fix possible NULL dereference in icmp6_dst_alloc
From: Jarek Poplawski @ 2010-01-27 12:17 UTC (permalink / raw)
  To: David Miller; +Cc: shemminger, djohnson, sakkiped, netdev
In-Reply-To: <20100127.030416.179345234.davem@davemloft.net>

On Wed, Jan 27, 2010 at 03:04:16AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Wed, 27 Jan 2010 10:34:17 +0000
> 
> > (Btw, maybe it's a sign this might be more readable?)
> 
> I welcome suggests that, tree wide, would make this harder
> to misinterpret.

If there is nothing better I still think this patch is right, except
better subject (Fix impossible NULL dereference in ... ;-).

Jarek P.

^ permalink raw reply

* Re: CBQ broken in 2.6
From: David Miller @ 2010-01-27 11:56 UTC (permalink / raw)
  To: anton.ivanov; +Cc: linux-kernel, netdev
In-Reply-To: <1264590347.29501.11.camel@tranquility.sigsegv.cx>

From: Anton Ivanov <anton.ivanov@kot-begemot.co.uk>
Date: Wed, 27 Jan 2010 11:05:47 +0000

> I have been looking at CBQ being broken in 2.6 (Debian BUG 534430).

Please direct your question to netdev@vger.kernel.org which is
where the networking experts hang out.

Added to CC:

> The bug is very simple and it makes it unusable for all practical
> purposes - BOUNDED classes always borrow from their parent. As a result
> it impossible to:
> 
> 1. Set and enforce a bandwidth limit to a class with a CBQ tree
> 2. Use CBQ to do QoS at any place but the actual link. For example - put
> a linux box after behind a DSL modem and tell it to QoS at the bandwidth
> at which the modem is configured.
> 
> I have now looked through the code of cbq_sched.c (in 2.6.33-rc5 and
> 2.6.26 as shipped by Debian).

I think you mean net/sched/sch_cbq.c, there is no cbq_sched.c
anywhere in the source tree.

> In both pieces of code the variable in the class structure for CBQ
> cl->borrow is used to signal if borrowing is allowed. If borrowing is
> not allowed (BOUNDED class) it is set to NULL.
> 
> However the routine which parses the class options and defines the class
> cbq_change_class() sets it to the parent class and never resets it to
> NULL for bounded classes.
> 
> Am I missing something here? 
> 
> Please cc me as I am not subscribed to the list.
> 
> Best Regards,
> 
> -- 
>    Understanding is a three-edged sword:
>             your side, their side, and the truth. --Kosh Naranek
> 
> A. R. Ivanov
> E-mail:  anton.ivanov@kot-begemot.co.uk
> WWW:     http://www.kot-begemot.co.uk/
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply

* Re: [PATCH net-next-2.6] packet: Add GSO/checksum offload support to af_packet sockets
From: Michael S. Tsirkin @ 2010-01-27 11:42 UTC (permalink / raw)
  To: Sridhar Samudrala; +Cc: David Miller, Rusty Russell, Herbert Xu, netdev
In-Reply-To: <1264537819.24933.122.camel@w-sridhar.beaverton.ibm.com>

On Tue, Jan 26, 2010 at 12:30:19PM -0800, Sridhar Samudrala wrote:
> This patch adds GSO/checksum offload to af_packet sockets using
> virtio_net_hdr. Based on Rusty's patch to add this support to tun.
> It allows GSO/checksum offload to be enabled when using raw socket
> backend with virtio_net.
> Adds PACKET_VNET_HDR socket option to prepend virtio_net_hdr in the
> receive path and process/skip virtio_net_hdr in the send path. This
> option is only allowed with SOCK_RAW sockets attached to ethernet
> type devices.
> 
> Signed-off-by: Sridhar Samudrala <sri@us.ibm.com>

So the main issue with this implemenation is that it silently fails for
non-ethernet protocols.  It would be better to detect unsupported
protocols and return an error to user.  This is same issue that was
pointed out by DaveM with my earlier attempt to solve a different
(related) problem:
http://lkml.org/lkml/2010/1/5/474
For an incomplete prototype attempting to solve the issue in a generic way:
http://lkml.org/lkml/2010/1/6/56

A couple of additional comments below.

> diff --git a/include/linux/if_packet.h b/include/linux/if_packet.h
> index 4021d47..aa57a5f 100644
> --- a/include/linux/if_packet.h
> +++ b/include/linux/if_packet.h
> @@ -46,6 +46,7 @@ struct sockaddr_ll {
>  #define PACKET_RESERVE			12
>  #define PACKET_TX_RING			13
>  #define PACKET_LOSS			14
> +#define PACKET_VNET_HDR			15
>  
>  struct tpacket_stats {
>  	unsigned int	tp_packets;
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 53633c5..36d5360 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -80,6 +80,8 @@
>  #include <linux/init.h>
>  #include <linux/mutex.h>
>  #include <linux/if_vlan.h>
> +#include <linux/virtio_net.h>
> +#include <linux/if_arp.h>
>  
>  #ifdef CONFIG_INET
>  #include <net/inet_common.h>
> @@ -193,7 +195,8 @@ struct packet_sock {
>  	struct mutex		pg_vec_lock;
>  	unsigned int		running:1,	/* prot_hook is attached*/
>  				auxdata:1,
> -				origdev:1;
> +				origdev:1,
> +				vnet_hdr:1;
>  	int			ifindex;	/* bound device		*/
>  	__be16			num;
>  	struct packet_mclist	*mclist;
> @@ -1056,6 +1059,30 @@ out:
>  }
>  #endif
>  
> +static inline struct sk_buff *packet_alloc_skb(struct sock *sk, size_t prepad,
> +					       size_t reserve, size_t len,
> +					       size_t linear, int noblock,
> +					       int *err)
> +{
> +	struct sk_buff *skb;
> +
> +	/* Under a page?  Don't bother with paged skb. */
> +	if (prepad + len < PAGE_SIZE || !linear)
> +		linear = len;
> +
> +	skb = sock_alloc_send_pskb(sk, prepad + linear, len - linear, noblock,
> +				   err);
> +	if (!skb)
> +		return NULL;
> +
> +	skb_reserve(skb, reserve);
> +	skb_put(skb, linear);
> +	skb->data_len = len - linear;
> +	skb->len += len - linear;
> +
> +	return skb;
> +}
> +
>  static int packet_snd(struct socket *sock,
>  			  struct msghdr *msg, size_t len)
>  {
> @@ -1066,14 +1093,15 @@ static int packet_snd(struct socket *sock,
>  	__be16 proto;
>  	unsigned char *addr;
>  	int ifindex, err, reserve = 0;
> +	struct virtio_net_hdr vnethdr = { 0 };
> +	int offset = 0;
> +	struct packet_sock *po = pkt_sk(sk);
>  
>  	/*
>  	 *	Get and verify the address.
>  	 */
>  
>  	if (saddr == NULL) {
> -		struct packet_sock *po = pkt_sk(sk);
> -
>  		ifindex	= po->ifindex;
>  		proto	= po->num;
>  		addr	= NULL;
> @@ -1100,25 +1128,52 @@ static int packet_snd(struct socket *sock,
>  	if (!(dev->flags & IFF_UP))
>  		goto out_unlock;
>  
> -	err = -EMSGSIZE;
> -	if (len > dev->mtu+reserve)
> -		goto out_unlock;
> +	if (po->vnet_hdr) {
> +		err = -EINVAL;
> +		if (dev->type != ARPHRD_ETHER)
> +			goto out_unlock;
> +
> +		if (len < sizeof(vnethdr))
> +			goto out_unlock;
>  
> -	skb = sock_alloc_send_skb(sk, len + LL_ALLOCATED_SPACE(dev),
> -				msg->msg_flags & MSG_DONTWAIT, &err);
> +		len -= sizeof(vnethdr);
> +
> +		err = -EFAULT;
> +		if (memcpy_fromiovec((void *)&vnethdr, msg->msg_iov,
> +					sizeof(vnethdr))) 
> +			goto out_unlock;
> +	
> +		if ((vnethdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> +		    (vnethdr.csum_start + vnethdr.csum_offset + 2 >
> +		      vnethdr.hdr_len))
> +			vnethdr.hdr_len = vnethdr.csum_start +
> +						 vnethdr.csum_offset + 2;
> +
> +		err = -EINVAL;
> +		if (vnethdr.hdr_len > len)
> +			goto out_unlock;
> +	} else {
> +		err = -EMSGSIZE;
> +		if (len > dev->mtu+reserve)
> +			goto out_unlock;

IMO we should always perform the length check if GSO is off.

> +	}
> +
> +	err = -ENOBUFS;
> +	skb = packet_alloc_skb(sk, LL_ALLOCATED_SPACE(dev),
> +			       LL_RESERVED_SPACE(dev), len, vnethdr.hdr_len,
> +			       msg->msg_flags & MSG_DONTWAIT, &err);
>  	if (skb == NULL)
>  		goto out_unlock;
>  
> -	skb_reserve(skb, LL_RESERVED_SPACE(dev));
> -	skb_reset_network_header(skb);
> +	skb_set_network_header(skb, reserve);

I think the above is wrong for vlans?

>  
>  	err = -EINVAL;
>  	if (sock->type == SOCK_DGRAM &&
> -	    dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len) < 0)
> +	    (offset = dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len)) < 0)
>  		goto out_free;
>  
>  	/* Returns -EFAULT on error */
> -	err = memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len);
> +	err = skb_copy_datagram_from_iovec(skb, offset, msg->msg_iov, 0, len);
>  	if (err)
>  		goto out_free;
>  
> @@ -1127,6 +1182,51 @@ static int packet_snd(struct socket *sock,
>  	skb->priority = sk->sk_priority;
>  	skb->mark = sk->sk_mark;
>  
> +	if (po->vnet_hdr) {
> +		skb_reset_mac_header(skb);
> +		skb->protocol = eth_hdr(skb)->h_proto;
> +

Is this also broken for vlans?

> +		if (vnethdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> +			if (!skb_partial_csum_set(skb, vnethdr.csum_start,
> +						  vnethdr.csum_offset)) {
> +				err = -EINVAL;
> +				goto out_free;
> +			}
> +		}
> +
> +		if (vnethdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> +			switch (vnethdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> +			case VIRTIO_NET_HDR_GSO_TCPV4:
> +				skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
> +				break;
> +			case VIRTIO_NET_HDR_GSO_TCPV6:
> +				skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
> +				break;
> +			case VIRTIO_NET_HDR_GSO_UDP:
> +				skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> +				break;
> +			default:
> +				err = -EINVAL;
> +				goto out_free;
> +			}
> +
> +			if (vnethdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)
> +				skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;
> +
> +			skb_shinfo(skb)->gso_size = vnethdr.gso_size;
> +			if (skb_shinfo(skb)->gso_size == 0) {
> +				err = -EINVAL;
> +				goto out_free;
> +                	}
> +
> +			/* Header must be checked, and gso_segs computed. */
> +			skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
> +			skb_shinfo(skb)->gso_segs = 0;
> +		}
> +
> +		len += sizeof(vnethdr);
> +	}
> +
>  	/*
>  	 *	Now send it
>  	 */
> @@ -1420,6 +1520,7 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
>  	struct sk_buff *skb;
>  	int copied, err;
>  	struct sockaddr_ll *sll;
> +	int vnet_hdr_len = 0;
>  
>  	err = -EINVAL;
>  	if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT))
> @@ -1451,6 +1552,44 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
>  	if (skb == NULL)
>  		goto out;
>  
> +	if (pkt_sk(sk)->vnet_hdr) {
> +		struct virtio_net_hdr vnethdr = { 0 };
> +
> +		vnet_hdr_len = sizeof(vnethdr);
> +		if ((len -= vnet_hdr_len) < 0)
> +			return -EINVAL;
> +
> +		if (skb_is_gso(skb)) {
> +			struct skb_shared_info *sinfo = skb_shinfo(skb);
> +
> +			/* This is a hint as to how much should be linear. */
> +			vnethdr.hdr_len = skb_headlen(skb);
> +			vnethdr.gso_size = sinfo->gso_size;
> +			if (sinfo->gso_type & SKB_GSO_TCPV4)
> +				vnethdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> +			else if (sinfo->gso_type & SKB_GSO_TCPV6)
> +				vnethdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> +			else if (sinfo->gso_type & SKB_GSO_UDP)
> +				vnethdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
> +			else
> +				BUG();

Is there any chance this can get SKB_GSO_FCOE by binding to
an appropriate interface?  Maybe we don't want to BUG().

> +			if (sinfo->gso_type & SKB_GSO_TCP_ECN)
> +				vnethdr.gso_type |= VIRTIO_NET_HDR_GSO_ECN;
> +		} else
> +			vnethdr.gso_type = VIRTIO_NET_HDR_GSO_NONE;
> +
> +		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +			vnethdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> +			vnethdr.csum_start = skb->csum_start - skb_headroom(skb);
> +			vnethdr.csum_offset = skb->csum_offset;
> +		} /* else everything is zero */
> +
> +		if (unlikely(memcpy_toiovec(msg->msg_iov, (void *)&vnethdr,
> +					    sizeof(vnethdr)))) {
> +			return -EFAULT;
> +		}
> +	}
> +
>  	/*
>  	 *	If the address length field is there to be filled in, we fill
>  	 *	it in now.
> @@ -1502,7 +1641,7 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
>  	 *	Free or return the buffer as appropriate. Again this
>  	 *	hides all the races and re-entrancy issues from us.
>  	 */
> -	err = (flags&MSG_TRUNC) ? skb->len : copied;
> +	err = vnet_hdr_len + ((flags&MSG_TRUNC) ? skb->len : copied);
>  
>  out_free:
>  	skb_free_datagram(sk, skb);
> @@ -1826,6 +1965,22 @@ packet_setsockopt(struct socket *sock, int level, int optname, char __user *optv
>  		po->origdev = !!val;
>  		return 0;
>  	}
> +	case PACKET_VNET_HDR:
> +	{
> +		int val;
> +
> +		if (sock->type != SOCK_RAW)
> +			return -EINVAL;
> +		if (po->rx_ring.pg_vec || po->tx_ring.pg_vec)	
> +			return -EBUSY;

Another way to get a broken ring + vnet hdr configuration
would be to enable vnet hdr first and mmap second.
I think we need to guard against this as well, by checking vnet_hdr
when tx/rx ring is enabled.

> +		if (optlen < sizeof(val))
> +			return -EINVAL;
> +		if (copy_from_user(&val, optval, sizeof(val)))
> +			return -EFAULT;
> +
> +		po->vnet_hdr = !!val;
> +		return 0;
> +	}
>  	default:
>  		return -ENOPROTOOPT;
>  	}
> @@ -1876,6 +2031,13 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
>  
>  		data = &val;
>  		break;
> +	case PACKET_VNET_HDR:
> +		if (len > sizeof(int))
> +			len = sizeof(int);
> +		val = po->vnet_hdr;
> +
> +		data = &val;
> +		break;
>  #ifdef CONFIG_PACKET_MMAP
>  	case PACKET_VERSION:
>  		if (len > sizeof(int))
> 

^ permalink raw reply

* Re: [PATCH 1/1 V4] net: Add Aeroflex Gaisler 10/100/1G Ethernet MAC driver
From: Kristoffer Glembo @ 2010-01-27 11:01 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev, davem
In-Reply-To: <20100125083131.568abc34@nehalam>

Hi,

Stephen Hemminger wrote:
> On Mon, 25 Jan 2010 12:01:12 +0100
> Kristoffer Glembo <kristoffer@gaisler.com> wrote:
> 
>> +/* Accept MAC address of the form macaddr=0x08,0x00,0x20,0x30,0x40,0x50 */
>> +static int macaddr[6];
>> +module_param_array(macaddr, int, NULL, 0);
>> +MODULE_PARM_DESC(macaddr, "GRETH Ethernet MAC address");
> 
> Don't you want an array of uchar here not int?
> 

I copied this approach from the sunhme driver and it works fine. Any special
reason to change it?

I have incorporated the rest of your feedback including ethtool support. 

Thanks a lot for the review!

Best regards,
Kristoffer Glembo

^ permalink raw reply

* Re: [PATCH] ipv6: Fix possible NULL dereference in icmp6_dst_alloc
From: David Miller @ 2010-01-27 11:04 UTC (permalink / raw)
  To: jarkao2; +Cc: shemminger, djohnson, sakkiped, netdev
In-Reply-To: <20100127103417.GA15206@ff.dom.local>

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 27 Jan 2010 10:34:17 +0000

> (Btw, maybe it's a sign this might be more readable?)

I welcome suggests that, tree wide, would make this harder
to misinterpret.

^ permalink raw reply

* Re: [PATCH] ipv6: Fix possible NULL dereference in icmp6_dst_alloc
From: Jarek Poplawski @ 2010-01-27 10:34 UTC (permalink / raw)
  To: David Miller; +Cc: shemminger, djohnson, sakkiped, netdev
In-Reply-To: <20100127.021424.263649631.davem@davemloft.net>

On Wed, Jan 27, 2010 at 02:14:24AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Wed, 27 Jan 2010 09:59:52 +0000
> 
> > While looking at the "Deadlock in IPv6 code while garbage collection
> > on the rwlock protecting the routing tree" reported by Srinivas
> > Akkipeddi I found there is possible a NULL pointer dereference in
> > icmp6_dst_alloc().
> > 
> > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> 
> Jarek, if rt is NULL so is &rt->u.dst
> 
> That's two patches trying to fix this same non-problem in
> two different places today :-)

Yes, great joke! :-\

(Btw, maybe it's a sign this might be more readable?)

Thanks,
Jarek P.

^ permalink raw reply

* Re: [PATCH] ipv6: Fix possible NULL dereference in icmp6_dst_alloc
From: David Miller @ 2010-01-27 10:14 UTC (permalink / raw)
  To: jarkao2; +Cc: shemminger, djohnson, sakkiped, netdev
In-Reply-To: <20100127095952.GA11635@ff.dom.local>

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 27 Jan 2010 09:59:52 +0000

> While looking at the "Deadlock in IPv6 code while garbage collection
> on the rwlock protecting the routing tree" reported by Srinivas
> Akkipeddi I found there is possible a NULL pointer dereference in
> icmp6_dst_alloc().
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Jarek, if rt is NULL so is &rt->u.dst

That's two patches trying to fix this same non-problem in
two different places today :-)

^ permalink raw reply

* [PATCH 1/3] net: maintain namespace isolation between vlan and real device
From: Arnd Bergmann @ 2010-01-27 10:05 UTC (permalink / raw)
  To: David Miller
  Cc: Stephen Hemminger, Patrick McHardy, Michael S. Tsirkin,
	Herbert Xu, Or Gerlitz, netdev, bridge, linux-kernel
In-Reply-To: <201001271104.20607.arnd@arndb.de>

In the vlan and macvlan drivers, the start_xmit function forwards
data to the dev_queue_xmit function for another device, which may
potentially belong to a different namespace.

To make sure that classification stays within a single namespace,
this resets the potentially critical fields.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/macvlan.c     |    2 +-
 include/linux/netdevice.h |    9 +++++++++
 net/8021q/vlan_dev.c      |    2 +-
 net/core/dev.c            |   35 +++++++++++++++++++++++++++++++----
 4 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index bad1303..e0436fd 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -269,7 +269,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 xmit_world:
-	skb->dev = vlan->lowerdev;
+	skb_set_dev(skb, vlan->lowerdev);
 	return dev_queue_xmit(skb);
 }
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 93a32a5..622ba5a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1004,6 +1004,15 @@ static inline bool netdev_uses_dsa_tags(struct net_device *dev)
 	return 0;
 }
 
+#ifndef CONFIG_NET_NS
+static inline void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
+{
+	skb->dev = dev;
+}
+#else /* CONFIG_NET_NS */
+void skb_set_dev(struct sk_buff *skb, struct net_device *dev);
+#endif
+
 static inline bool netdev_uses_trailer_tags(struct net_device *dev)
 {
 #ifdef CONFIG_NET_DSA_TAG_TRAILER
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 77a49ff..95034a8 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -322,7 +322,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
 	}
 
 
-	skb->dev = vlan_dev_info(dev)->real_dev;
+	skb_set_dev(skb, vlan_dev_info(dev)->real_dev);
 	len = skb->len;
 	ret = dev_queue_xmit(skb);
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 2cba5c5..e80403a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1448,13 +1448,10 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
 	if (skb->len > (dev->mtu + dev->hard_header_len))
 		return NET_RX_DROP;
 
-	skb_dst_drop(skb);
+	skb_set_dev(skb, dev);
 	skb->tstamp.tv64 = 0;
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, dev);
-	skb->mark = 0;
-	secpath_reset(skb);
-	nf_reset(skb);
 	return netif_rx(skb);
 }
 EXPORT_SYMBOL_GPL(dev_forward_skb);
@@ -1614,6 +1611,36 @@ static bool dev_can_checksum(struct net_device *dev, struct sk_buff *skb)
 	return false;
 }
 
+/**
+ * skb_dev_set -- assign a buffer to a new device
+ * @skb: buffer for the new device
+ * @dev: network device
+ *
+ * If an skb is owned by a device already, we have to reset
+ * all data private to the namespace a device belongs to
+ * before assigning it a new device.
+ */
+#ifdef CONFIG_NET_NS
+void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
+{
+	skb_dst_drop(skb);
+	if (skb->dev && !net_eq(dev_net(skb->dev), dev_net(dev))) {
+		secpath_reset(skb);
+		nf_reset(skb);
+		skb_init_secmark(skb);
+		skb->mark = 0;
+		skb->priority = 0;
+		skb->nf_trace = 0;
+		skb->ipvs_property = 0;
+#ifdef CONFIG_NET_SCHED
+		skb->tc_index = 0;
+#endif
+	}
+	skb->dev = dev;
+}
+EXPORT_SYMBOL(skb_set_dev);
+#endif /* CONFIG_NET_NS */
+
 /*
  * Invalidate hardware checksum when packet is to be mangled, and
  * complete checksum manually on outgoing path.
-- 
1.6.3.3

^ permalink raw reply related

* [PATCH] ipv6: Fix possible NULL dereference in icmp6_dst_alloc
From: Jarek Poplawski @ 2010-01-27  9:59 UTC (permalink / raw)
  To: David Miller; +Cc: shemminger, djohnson, sakkiped, netdev
In-Reply-To: <20100126103524.GB7780@ff.dom.local>

While looking at the "Deadlock in IPv6 code while garbage collection
on the rwlock protecting the routing tree" reported by Srinivas
Akkipeddi I found there is possible a NULL pointer dereference in
icmp6_dst_alloc().

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 net/ipv6/route.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c2bd74c..93c3b20 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -973,7 +973,7 @@ struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 	rt = ip6_dst_alloc(&net->ipv6.ip6_dst_ops);
 	if (unlikely(rt == NULL)) {
 		in6_dev_put(idev);
-		goto out;
+		return NULL;
 	}
 
 	dev_hold(dev);
@@ -1009,7 +1009,6 @@ struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 
 	fib6_force_start_gc(net);
 
-out:
 	return &rt->u.dst;
 }
 

^ permalink raw reply related

* Re: [RFC] [PATCH] Optimize TCP sendmsg in favour of fast devices?
From: Krishna Kumar2 @ 2010-01-27  9:42 UTC (permalink / raw)
  To: Krishna Kumar2
  Cc: David Miller, eric.dumazet, Herbert Xu, ilpo.jarvinen, netdev
In-Reply-To: <OF7EA723DA.DC2FF4FC-ON652576B8.002064CB-652576B8.00267739@LocalDomain>

> Krishna Kumar2/India/IBM wrote on 01/27/2010 12:42 PM
>
> > Herbert Xu <herbert@gondor.apana.org.au> wrote on 01/21/2010 03:11 PM
>
> Sorry for the late response.
>
> > >> I had to remove the F_SG flag from cxgb3 driver (using ethtool
> > >> didn't show any difference in performance since GSO was enabled
> > >> on the device due to register_netdev setting it). Testing show a
> > >> drop of 25% in performance with this patch for non-SG device,
> > >> the extra alloc/memcpy is showing up.
> > >>
> > >> For the SG driver, I get a good performace gain (not anywhere
> > >> close to 25% though). What do you suggest?
> > >
> > > I don't think we can add your change if it hurts non-SG
> > > devices that much.
> >
> > Wait, we need to be careful when testing this.  Non-SG devices
> > do actually benefit from TSO which they otherwise cannot access.
> >
> > If you unset the F_SG bit, then that would disable TSO too.  So
> > you need to enable GSO to compensate.  So Krishna, did you check
> > with tcpdump to see if GSO was really enabled with SG off?
>
> OK, I unset F_SG and set F_GSO (in driver). With this, tcpdump shows
> GSO is enabled - the tcp packet sizes builds up to 65160 bytes.

I should have mentioned this too - if I unset F_SG in the
cxgb3 driver and nothing else, ethtool -k still shows GSO
is set, and tcpdump shows max packet size is 1448. If I
additionally set GSO in driver, then ethtool still has the
same output, but tcpdump shows max packet size of 65160.

Thanks,

- KK

> I ran 5 serial netperf's with 16K and another 5 serial netperfs
> with 64K I/O sizes, and the aggregate result is:
>
> 0. Driver unsets F_SG but sets F_GSO:
>  Original code with 16K: 19471.65
>  New code with 16K:      19409.70
>  Original code with 64K: 21357.23
>  New code with 64K:      22050.42
>
> To recap the other tests I did today:
>
> 1. Driver unsets F_SG, and with GSO off
>  Original code with 16K: 10123.56
>  New code with 16K:      7111.12
>  Original code with 64K: 11568.99
>  New code with 64K:      7611.37
>
> 2. Driver unsets F_SG and uses ethtool to set GSO:
>  Original code with 16K: 18864.38
>  New code with 16K:      18465.54
>  Original code with 64K: 21005.43
>  New code with 64K:      22529.24
>
> Thanks,
>
> - KK
>
> > IIRC when I did a similar test with e1000 back when I wrote this
> > the performance of GSO with SG off was pretty much the same as
> > no GSO with SG off.


^ permalink raw reply

* Re: [Bug #15042] socket(PF_INET6 hangs when ipv6 not yet initialized
From: Américo Wang @ 2010-01-27  9:10 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux Kernel Mailing List, Kernel Testers List, Marc Haber,
	Linux Kernel Network Developers
In-Reply-To: <q60Z28d_9yH.A.oqH.NZMXLB@chimera>

On Mon, Jan 25, 2010 at 6:23 AM, Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org> wrote:
> This message has been generated automatically as a part of a report
> of regressions introduced between 2.6.31 and 2.6.32.
>
> The following bug entry is on the current list of known regressions
> introduced between 2.6.31 and 2.6.32.  Please verify if it still should
> be listed and let me know (either way).
>
>
> Bug-Entry       : http://bugzilla.kernel.org/show_bug.cgi?id=15042
> Subject         : socket(PF_INET6 hangs when ipv6 not yet initialized
> Submitter       : Marc Haber <mh+linux-kernel@zugschlus.de>
> Date            : 2010-01-10 14:28 (15 days old)
> References      : http://marc.info/?l=linux-kernel&m=126313553029280&w=4
>

(Adding net-dev into Cc)

Hi, Marc,

Sorry, I don't really understand your description of the problem, your mean
your ssh hangs on socket PF_INET6 for 30 seconds?

Have you enabled CONFIG_DETECT_SOFTLOCKUP for your kernel?
If not, please do it.

Thanks.

^ permalink raw reply

* Re: 2.6.33-rc5: (e1000): transmit queue 0 timed out
From: Alexander Beregalov @ 2010-01-27  9:03 UTC (permalink / raw)
  To: Jesse Brandeburg; +Cc: Rafael J. Wysocki, netdev, Jesse Brandeburg, e1000-devel
In-Reply-To: <4807377b1001261712m9d161b6s700cc76b2f2edaf4@mail.gmail.com>

2010/1/27 Jesse Brandeburg <jesse.brandeburg@gmail.com>:
> I also just noticed something else.
>
> On Sat, Jan 23, 2010 at 12:04 PM, Alexander Beregalov
> <a.beregalov@gmail.com> wrote:
>>>> Pid: 5, comm: events/0 Tainted: G        W  2.6.33-rc5 #1
>>>> NF7-S/NF7,NF7-V (nVidia-nForce2)/
>>>> EIP: 0060:[<c1071c51>] EFLAGS: 00010282 CPU: 0
>>>> EIP is at put_page+0x11/0x120
>>>> EAX: 2e8ca4f3 EBX: 2e8ca4f3 ECX: 00000000 EDX: ee960640
>>>> ESI: f6482620 EDI: 000016b0 EBP: f7065ea8 ESP: f7065e98
>>>>  DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
>>>> Process events/0 (pid: 5, ti=f7064000 task=f70553c0 task.ti=f7064000)
>>>> Stack:
>>>>  00000206 00000001 f6482620 000016b0 f7065eb8 c12d3100 f6482620 f71d9f50
>>>> <0> f7065ec4 c12d2e32 f80376b0 f7065ecc c12d2ec5 f7065f00 c1276970 cccccccd
>>>> <0> f7065f00 f711fafc f711fafc f711faa0 00000000 f702b440 000000f2 f702b440
>>>> Call Trace:
>>>>  [<c12d3100>] ? skb_release_data+0x90/0xa0
>>>>  [<c12d2e32>] ? __kfree_skb+0x12/0x90
>>>>  [<c12d2ec5>] ? consume_skb+0x15/0x30
>>>>  [<c1276970>] ? e1000_clean_rx_ring+0x80/0x150
>>>>  [<c127c743>] ? e1000_down+0x1b3/0x1d0
>>>>  [<c127cf60>] ? e1000_reset_task+0x0/0x10
>>>>  [<c127cd3b>] ? e1000_reinit_locked+0x4b/0x70
>>>>  [<c127cf6d>] ? e1000_reset_task+0xd/0x10
>>>>  [<c103a9ea>] ? worker_thread+0x14a/0x230
>>>>  [<c103a989>] ? worker_thread+0xe9/0x230
>>>>  [<c103e160>] ? autoremove_wake_function+0x0/0x40
>>>>  [<c103a8a0>] ? worker_thread+0x0/0x230
>>>>  [<c103de6c>] ? kthread+0x6c/0x80
>>>>  [<c103de00>] ? kthread+0x0/0x80
>>>>  [<c100303a>] ? kernel_thread_helper+0x6/0x1c
>> WARNING: at net/sched/sch_generic.c:255 dev_watchdog+0x1c5/0x1d0()
>> Hardware name:
>> NETDEV WATCHDOG: eth0 (e1000): transmit queue 0 timed out
>
> There are at least two problems here, not sure if they are related yet.
> The first is the WATCHDOG (the kernel will print this only once per
> e1000e driver load), we don't know what is causing that right now,
> maybe we can mock up some ftrace magic to dump the transmit
> descriptors that are formed, or you can run the e1000_dump patch code.
>  Let me know if you want me to generate a version of it for you.

Yes please
>
>> <..>
>> BUG kmalloc-2048: Poison overwritten
>> -----------------------------------------------------------------------------
>>
>> INFO: 0xf4998022-0xf4998607. First byte 0x0 instead of 0x6b
>> INFO: Allocated in __netdev_alloc_skb+0x1e/0x40 age=372 cpu=0 pid=1724
>> INFO: Freed in skb_release_data+0x68/0xa0 age=292 cpu=0 pid=5
>> INFO: Slab 0xc2283300 objects=15 used=0 fp=0xf499b950 flags=0x40004082
>> INFO: Object 0xf4998000 @offset=0 fp=0xf499d1e0
>>
>>  Object 0xf4998000:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>> kkkkkkkkkkkkkkkk
>>  Object 0xf4998010:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>> kkkkkkkkkkkkkkkk
>>  Object 0xf4998020:  6b 6b 00 07 e9 09 d4 79 01 14 2b 09 0b 28 08 00
>> kk..И.тy..+..(..
>>  Object 0xf4998030:  45 20 05 d4 7d 09 40 00 75 06 3c e5 d5 b6 b0 b4
>> E..т}.@.u.<Еу╤╟╢
>>  Object 0xf4998040:  c1 a8 01 02 23 28 af a3 57 1f 82 b2 c7 a5 14 d5
>> а╗..#(╞ёW..╡г╔.у
>> <..>
>
> hey, thats an ethernet/ipv4 packet!  The memory above is typically
> allocated at a 2kB boundary and then the hardware would start DMAing
> at 000+22...  how long was that packet corrupting data in memory (how
> many bytes)?
>
> 00 07 e9 09 d4 79
> dest mac
Yes, this is the mac of the e1000 on the host.
> 01 14 2b 09 0b 28
> src mac address
It looks corrupted. Gateway's mac is 00:14:2b:09:0a:28
> 08 00
> ip header
> 45 20
> header length 20 bytes, ip v4, DSCP = ECN capable
> and on...
>
> so this is the second issue, we've called kfree on a packet but
> hardware still receives into it (probably that we didn't wait long
> enough for receives to quit)
>
>>  Object 0xf49987d0:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>> kkkkkkkkkkkkkkkk
>>  Object 0xf49987e0:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>> kkkkkkkkkkkkkkkk
>>  Object 0xf49987f0:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5
>> kkkkkkkkkkkkkkk╔
>>  Redzone 0xf4998800:  bb bb bb bb                                     ╩╩╩╩
>>  Padding 0xf4998828:  5a 5a 5a 5a 5a 5a 5a 5a                         ZZZZZZZZ
>> Pid: 1719, comm: rtorrent Tainted: G        W  2.6.33-rc5 #1
>> Call Trace:
>>  [<c108d1af>] print_trailer+0xcf/0x120
>>  [<c108d844>] check_bytes_and_report+0xc4/0xf0
>>  [<c108da1f>] check_object+0x1af/0x200
>>  [<c108db4a>] __free_slab+0xda/0x100
>>  [<c108db8c>] discard_slab+0x1c/0x30
>>  [<c108f552>] __slab_free+0xd2/0x280
>>  [<c11d7156>] ? copy_to_user+0x36/0x130
>>  [<c108f9df>] kfree+0xdf/0x110
>>  [<c12d30d8>] ? skb_release_data+0x68/0xa0
>>  [<c12d30d8>] ? skb_release_data+0x68/0xa0
>>  [<c12d30d8>] skb_release_data+0x68/0xa0
>>  [<c12d2e32>] __kfree_skb+0x12/0x90
>>  [<c13006a0>] tcp_recvmsg+0x6c0/0x8d0
>>  [<c102f691>] ? local_bh_enable_ip+0x61/0xc0
>>  [<c1350b75>] ? _raw_spin_unlock_bh+0x25/0x30
>>  [<c10435e5>] ? T.324+0x15/0x1b0
>>  [<c12cdc73>] sock_common_recvmsg+0x43/0x60
>>  [<c12cbc87>] sock_recvmsg+0xb7/0xf0
>>  [<c10435e5>] ? T.324+0x15/0x1b0
>>  [<c12cc589>] sys_recvfrom+0x79/0xe0
>>  [<c104d66b>] ? trace_hardirqs_off+0xb/0x10
>>  [<c104398e>] ? cpu_clock+0x4e/0x60
>>  [<c104d6a7>] ? lock_release_holdtime+0x37/0x1b0
>>  [<c1052121>] ? lock_release_non_nested+0x301/0x340
>>  [<c104d6a7>] ? lock_release_holdtime+0x37/0x1b0
>>  [<c107bd0a>] ? might_fault+0x4a/0xa0
>>  [<c12cc626>] sys_recv+0x36/0x40
>>  [<c12cd74c>] sys_socketcall+0x1ac/0x270
>>  [<c11d68f4>] ? trace_hardirqs_on_thunk+0xc/0x10
>>  [<c1002b10>] sysenter_do_call+0x12/0x36
>> FIX kmalloc-2048: Restoring 0xf4998022-0xf4998607=0x6b
>
> I'll reply here with a patch for checking if the receive unit is still
> not stopped, but I still don't know what is causing the NETDEV
> WATCHDOG and we need some more debug information for that (from the
> e1000_dump patch)

Yes please, I would like to help, but I do not know how to reproduce it.

>
> can you answer any of my other questions too?
>

------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply

* Re: [E1000-devel] 2.6.33-rc5: (e1000): transmit queue 0 timed out
From: Alexander Beregalov @ 2010-01-27  8:55 UTC (permalink / raw)
  To: Brandeburg, Jesse
  Cc: netdev, e1000-devel@lists.sourceforge.net, Rafael J. Wysocki
In-Reply-To: <alpine.WNT.2.00.1001251704310.2536@jbrandeb-desk1.amr.corp.intel.com>

2010/1/26 Brandeburg, Jesse <jesse.brandeburg@intel.com>:
>
>
> On Sat, 23 Jan 2010, Alexander Beregalov wrote:
>> It is x86_32, UP
>>
>> e1000: eth0: e1000_clean_tx_irq: Detected Tx Unit Hang
>>   Tx Queue             <0>
>>   TDH                  <0>
>
> The queue seems to have not been started...  what test are you running?
> what kind of traffic and system?  (lspci -vvv please)

The host just does regular tasks - NFS server and rtorrent client.

01:0a.0 Ethernet controller [0200]: Intel Corporation 82540EM Gigabit
Ethernet Controller [8086:100e] (rev 02)
        Subsystem: Intel Corporation PRO/1000 MT Desktop Adapter [8086:002e]
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
ParErr- Stepping- SERR- FastB2B- DisINTx-
        Status: Cap+ 66MHz+ UDF- FastB2B- ParErr- DEVSEL=medium
>TAbort- <TAbort- <MAbort- >SERR- <PERR+ INTx-
        Latency: 64 (63750ns min)
        Interrupt: pin A routed to IRQ 18
        Region 0: Memory at ec000000 (32-bit, non-prefetchable) [size=128K]
        Region 1: Memory at ec020000 (32-bit, non-prefetchable) [size=128K]
        Region 2: I/O ports at a040 [size=64]
        [virtual] Expansion ROM at 60080000 [disabled] [size=128K]
        Capabilities: [dc] Power Management version 2
                Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA
PME(D0+,D1-,D2-,D3hot+,D3cold+)
                Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=1 PME-
        Capabilities: [e4] PCI-X non-bridge device
                Command: DPERE- ERO+ RBC=512 OST=1
                Status: Dev=00:00.0 64bit- 133MHz- SCD- USC- DC=simple
DMMRBC=2048 DMOST=1 DMCRS=16 RSCEM- 266MHz- 533MHz-
        Capabilities: [f0] MSI: Enable- Count=1/1 Maskable- 64bit+
                Address: 0000000000000000  Data: 0000
        Kernel driver in use: e1000

>
>
>>   TDT                  <1f>
>>   next_to_use          <1f>
>>   next_to_clean        <30>
>> buffer_info[next_to_clean]
>>   time_stamp           <12d519>
>>   next_to_watch        <30>
>>   jiffies              <12da92>
>>   next_to_watch.status <0>
>> WARNING: at net/sched/sch_generic.c:255 dev_watchdog+0x1c5/0x1d0()
>> Hardware name:
>> NETDEV WATCHDOG: eth0 (e1000): transmit queue 0 timed out
>> Modules linked in: hwmon_vid sata_sil i2c_nforce2
>> Pid: 0, comm: swapper Not tainted 2.6.33-rc5 #1
>> Call Trace:
>>  [<c102a49d>] warn_slowpath_common+0x6d/0xa0
>>  [<c12ea885>] ? dev_watchdog+0x1c5/0x1d0
>>  [<c12ea885>] ? dev_watchdog+0x1c5/0x1d0
>>  [<c102a516>] warn_slowpath_fmt+0x26/0x30
>>  [<c12ea885>] dev_watchdog+0x1c5/0x1d0
>>  [<c1033bb7>] ? run_timer_softirq+0xd7/0x240
>>  [<c1033c31>] run_timer_softirq+0x151/0x240
>>  [<c1033bb7>] ? run_timer_softirq+0xd7/0x240
>>  [<c12ea6c0>] ? dev_watchdog+0x0/0x1d0
>>  [<c102f40a>] __do_softirq+0x7a/0x110
>>  [<c102f4ed>] do_softirq+0x4d/0x60
>>  [<c102f625>] irq_exit+0x65/0x70
>>  [<c1015fe7>] smp_apic_timer_interrupt+0x47/0x80
>>  [<c11d6904>] ? trace_hardirqs_off_thunk+0xc/0x18
>>  [<c1350e63>] apic_timer_interrupt+0x2f/0x34
>>  [<c10088fd>] ? default_idle+0x2d/0x60
>>  [<c1001b19>] cpu_idle+0x39/0x60
>>  [<c13451e8>] rest_init+0x48/0x50
>>  [<c16196b4>] start_kernel+0x26d/0x274
>>  [<c1619275>] ? unknown_bootoption+0x0/0x19c
>>  [<c1619068>] i386_start_kernel+0x68/0x6e
>> ---[ end trace 828c510cca9472df ]---
>> BUG: unable to handle kernel paging request at 2e8ca4f3
>> IP: [<c1071c51>] put_page+0x11/0x120
>
> hm, put_page panic, are you running with jumbo frames enabled?  Does your
> network have jumbo frame traffic on it?

Jumbo frames are disabled, no jumbo frame traffic.
>
>> *pde = 00000000
>> Oops: 0000 [#1]
>> last sysfs file: /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
>> Modules linked in: hwmon_vid sata_sil i2c_nforce2
>>
>> Pid: 5, comm: events/0 Tainted: G        W  2.6.33-rc5 #1
>> NF7-S/NF7,NF7-V (nVidia-nForce2)/
>> EIP: 0060:[<c1071c51>] EFLAGS: 00010282 CPU: 0
>> EIP is at put_page+0x11/0x120
>> EAX: 2e8ca4f3 EBX: 2e8ca4f3 ECX: 00000000 EDX: ee960640
>> ESI: f6482620 EDI: 000016b0 EBP: f7065ea8 ESP: f7065e98
>>  DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
>> Process events/0 (pid: 5, ti=f7064000 task=f70553c0 task.ti=f7064000)
>> Stack:
>>  00000206 00000001 f6482620 000016b0 f7065eb8 c12d3100 f6482620 f71d9f50
>> <0> f7065ec4 c12d2e32 f80376b0 f7065ecc c12d2ec5 f7065f00 c1276970 cccccccd
>> <0> f7065f00 f711fafc f711fafc f711faa0 00000000 f702b440 000000f2 f702b440
>> Call Trace:
>>  [<c12d3100>] ? skb_release_data+0x90/0xa0
>>  [<c12d2e32>] ? __kfree_skb+0x12/0x90
>>  [<c12d2ec5>] ? consume_skb+0x15/0x30
>>  [<c1276970>] ? e1000_clean_rx_ring+0x80/0x150
>>  [<c127c743>] ? e1000_down+0x1b3/0x1d0
>>  [<c127cf60>] ? e1000_reset_task+0x0/0x10
>>  [<c127cd3b>] ? e1000_reinit_locked+0x4b/0x70
>>  [<c127cf6d>] ? e1000_reset_task+0xd/0x10
>>  [<c103a9ea>] ? worker_thread+0x14a/0x230
>>  [<c103a989>] ? worker_thread+0xe9/0x230
>>  [<c103e160>] ? autoremove_wake_function+0x0/0x40
>>  [<c103a8a0>] ? worker_thread+0x0/0x230
>>  [<c103de6c>] ? kthread+0x6c/0x80
>>  [<c103de00>] ? kthread+0x0/0x80
>>  [<c100303a>] ? kernel_thread_helper+0x6/0x1c
>> Code: 00 00 00 8d bc 27 00 00 00 00 55 b8 e0 1f 07 c1 89 e5 e8 83 93
>> fc ff c9 c3 90 55 89 e5 83 ec 10 89 5d f4 89 75 f8 89 c3 89 7d fc <66>
>> f7 00 00 c0 0f 85 e4 00 00 00 8b 40 04 85 c0 0f 84 e3 00 00
>> EIP: [<c1071c51>] put_page+0x11/0x120 SS:ESP 0068:f7065e98
>> CR2: 000000002e8ca4f3
>> ---[ end trace 828c510cca9472e0 ]---
>
>
> Thanks for the report, do you believe it to be new to e1000 in 2.6.33-rc5?
> Have you had failure like this before and/or can you see the same failure
> on 2.6.32?

Yes, I believe it is new to 2.6.33-rc5, I have not seen it before.

^ permalink raw reply

* Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver
From: Wolfgang Grandegger @ 2010-01-27  8:13 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Wolfgang Denk, linuxppc-dev, David Miller, dzu, netdev,
	linuxppc-dev, agust, kosmo, Grant Likely
In-Reply-To: <201001270306.22089.arnd@arndb.de>

Arnd Bergmann wrote:
> On Sunday 24 January 2010, Wolfgang Denk wrote:
>> In message <4B5C5BDF.6020001@grandegger.com> you wrote:
>>> You are probably right and your proposal would likely result in more
>>> transparent (less ugly) code. There has been some discussion about
>>> unifying FEC drivers when the patches (with the same subject) have been
>>> submitted for the first time in May last year, but it was not about 512x
>>> and 8xx, IIRC.
>> You can re-read this discussion here:
>>
>> http://patchwork.ozlabs.org/patch/26927/
>>
>> ee especiall Grant's note of 2009-05-21 15:36:11: "If it looks too
>> ugly, then just fork the driver."
> 
> Ok. I fully agree with what Grant said in that thread, especially the
> way the files could be split. Forking the entire driver would work
> as an easy way to get it running at first, and we still have the option
> of reorganizing the duplicate parts later in a saner way if that's seen
> as helpful. I'd assume that at least some parts of it could become a
> lib_fs_enet module that can be shared by all of them.

Yes, I also vote for forking the driver allowing a clean implementation.
 I don't think it makes sense to share a driver with the 8xx for the
reasons you already mentioned. And the 8xx is a dying out arch anyway.

Wolfgang.

^ permalink raw reply

* Re: [PATCH 2/4] ar7: implement clock API
From: Florian Fainelli @ 2010-01-27  8:10 UTC (permalink / raw)
  To: linux-mips; +Cc: Wim Van Sebroeck, ralf, netdev, David Miller
In-Reply-To: <201001121015.27867.florian@openwrt.org>

This patch makes the ar7 clock code implement the
Linux clk API. Drivers using the various clocks
available in the SoC are updated accordingly.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
Acked-by: Wim Van Sebroeck <wim@iguana.be>
---
diff --git a/arch/mips/ar7/clock.c b/arch/mips/ar7/clock.c
index cc65c8e..fc0e715 100644
--- a/arch/mips/ar7/clock.c
+++ b/arch/mips/ar7/clock.c
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2007 Felix Fietkau <nbd@openwrt.org>
  * Copyright (C) 2007 Eugene Konev <ejka@openwrt.org>
+ * Copyright (C) 2009 Florian Fainelli <florian@openwrt.org>
  *
  * 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
@@ -24,6 +25,8 @@
 #include <linux/delay.h>
 #include <linux/gcd.h>
 #include <linux/io.h>
+#include <linux/err.h>
+#include <linux/clk.h>
 
 #include <asm/addrspace.h>
 #include <asm/mach-ar7/ar7.h>
@@ -94,12 +97,16 @@ struct tnetd7200_clocks {
 	struct tnetd7200_clock usb;
 };
 
-int ar7_cpu_clock = 150000000;
-EXPORT_SYMBOL(ar7_cpu_clock);
-int ar7_bus_clock = 125000000;
-EXPORT_SYMBOL(ar7_bus_clock);
-int ar7_dsp_clock;
-EXPORT_SYMBOL(ar7_dsp_clock);
+static struct clk bus_clk = {
+	.rate	= 125000000,
+};
+
+static struct clk cpu_clk = {
+	.rate	= 150000000,
+};
+
+static struct clk dsp_clk;
+static struct clk vbus_clk;
 
 static void approximate(int base, int target, int *prediv,
 			int *postdiv, int *mul)
@@ -185,7 +192,7 @@ static int tnetd7300_get_clock(u32 shift, struct tnetd7300_clock *clock,
 		base_clock = AR7_XTAL_CLOCK;
 		break;
 	case BOOT_PLL_SOURCE_CPU:
-		base_clock = ar7_cpu_clock;
+		base_clock = cpu_clk.rate;
 		break;
 	}
 
@@ -212,11 +219,11 @@ static void tnetd7300_set_clock(u32 shift, struct tnetd7300_clock *clock,
 	u32 *bootcr, u32 frequency)
 {
 	int prediv, postdiv, mul;
-	int base_clock = ar7_bus_clock;
+	int base_clock = bus_clk.rate;
 
 	switch ((*bootcr & (BOOT_PLL_SOURCE_MASK << shift)) >> shift) {
 	case BOOT_PLL_SOURCE_BUS:
-		base_clock = ar7_bus_clock;
+		base_clock = bus_clk.rate;
 		break;
 	case BOOT_PLL_SOURCE_REF:
 		base_clock = AR7_REF_CLOCK;
@@ -225,7 +232,7 @@ static void tnetd7300_set_clock(u32 shift, struct tnetd7300_clock *clock,
 		base_clock = AR7_XTAL_CLOCK;
 		break;
 	case BOOT_PLL_SOURCE_CPU:
-		base_clock = ar7_cpu_clock;
+		base_clock = cpu_clk.rate;
 		break;
 	}
 
@@ -247,18 +254,18 @@ static void __init tnetd7300_init_clocks(void)
 					ioremap_nocache(UR8_REGS_CLOCKS,
 					sizeof(struct tnetd7300_clocks));
 
-	ar7_bus_clock = tnetd7300_get_clock(BUS_PLL_SOURCE_SHIFT,
+	bus_clk.rate = tnetd7300_get_clock(BUS_PLL_SOURCE_SHIFT,
 		&clocks->bus, bootcr, AR7_AFE_CLOCK);
 
 	if (*bootcr & BOOT_PLL_ASYNC_MODE)
-		ar7_cpu_clock = tnetd7300_get_clock(CPU_PLL_SOURCE_SHIFT,
+		cpu_clk.rate = tnetd7300_get_clock(CPU_PLL_SOURCE_SHIFT,
 			&clocks->cpu, bootcr, AR7_AFE_CLOCK);
 	else
-		ar7_cpu_clock = ar7_bus_clock;
+		cpu_clk.rate = bus_clk.rate;
 
-	if (ar7_dsp_clock == 250000000)
+	if (dsp_clk.rate == 250000000)
 		tnetd7300_set_clock(DSP_PLL_SOURCE_SHIFT, &clocks->dsp,
-			bootcr, ar7_dsp_clock);
+			bootcr, dsp_clk.rate);
 
 	iounmap(clocks);
 	iounmap(bootcr);
@@ -343,20 +350,20 @@ static void __init tnetd7200_init_clocks(void)
 		printk(KERN_INFO "Clocks: Setting DSP clock\n");
 		calculate(dsp_base, TNETD7200_DEF_DSP_CLK,
 			&dsp_prediv, &dsp_postdiv, &dsp_mul);
-		ar7_bus_clock =
+		bus_clk.rate =
 			((dsp_base / dsp_prediv) * dsp_mul) / dsp_postdiv;
 		tnetd7200_set_clock(dsp_base, &clocks->dsp,
 			dsp_prediv, dsp_postdiv * 2, dsp_postdiv, dsp_mul * 2,
-			ar7_bus_clock);
+			bus_clk.rate);
 
 		printk(KERN_INFO "Clocks: Setting CPU clock\n");
 		calculate(cpu_base, TNETD7200_DEF_CPU_CLK, &cpu_prediv,
 			&cpu_postdiv, &cpu_mul);
-		ar7_cpu_clock =
+		cpu_clk.rate =
 			((cpu_base / cpu_prediv) * cpu_mul) / cpu_postdiv;
 		tnetd7200_set_clock(cpu_base, &clocks->cpu,
 			cpu_prediv, cpu_postdiv, -1, cpu_mul,
-			ar7_cpu_clock);
+			cpu_clk.rate);
 
 	} else
 		if (*bootcr & BOOT_PLL_2TO1_MODE) {
@@ -365,48 +372,90 @@ static void __init tnetd7200_init_clocks(void)
 			printk(KERN_INFO "Clocks: Setting CPU clock\n");
 			calculate(cpu_base, TNETD7200_DEF_CPU_CLK, &cpu_prediv,
 				&cpu_postdiv, &cpu_mul);
-			ar7_cpu_clock = ((cpu_base / cpu_prediv) * cpu_mul)
+			cpu_clk.rate = ((cpu_base / cpu_prediv) * cpu_mul)
 								/ cpu_postdiv;
 			tnetd7200_set_clock(cpu_base, &clocks->cpu,
 				cpu_prediv, cpu_postdiv, -1, cpu_mul,
-				ar7_cpu_clock);
+				cpu_clk.rate);
 
 			printk(KERN_INFO "Clocks: Setting DSP clock\n");
 			calculate(dsp_base, TNETD7200_DEF_DSP_CLK, &dsp_prediv,
 				&dsp_postdiv, &dsp_mul);
-			ar7_bus_clock = ar7_cpu_clock / 2;
+			bus_clk.rate = cpu_clk.rate / 2;
 			tnetd7200_set_clock(dsp_base, &clocks->dsp,
 				dsp_prediv, dsp_postdiv * 2, dsp_postdiv,
-				dsp_mul * 2, ar7_bus_clock);
+				dsp_mul * 2, bus_clk.rate);
 		} else {
 			printk(KERN_INFO "Clocks: Sync 1:1 mode\n");
 
 			printk(KERN_INFO "Clocks: Setting DSP clock\n");
 			calculate(dsp_base, TNETD7200_DEF_DSP_CLK, &dsp_prediv,
 				&dsp_postdiv, &dsp_mul);
-			ar7_bus_clock = ((dsp_base / dsp_prediv) * dsp_mul)
+			bus_clk.rate = ((dsp_base / dsp_prediv) * dsp_mul)
 								/ dsp_postdiv;
 			tnetd7200_set_clock(dsp_base, &clocks->dsp,
 				dsp_prediv, dsp_postdiv * 2, dsp_postdiv,
-				dsp_mul * 2, ar7_bus_clock);
+				dsp_mul * 2, bus_clk.rate);
 
-			ar7_cpu_clock = ar7_bus_clock;
+			cpu_clk.rate = bus_clk.rate;
 		}
 
 	printk(KERN_INFO "Clocks: Setting USB clock\n");
-	usb_base = ar7_bus_clock;
+	usb_base = bus_clk.rate;
 	calculate(usb_base, TNETD7200_DEF_USB_CLK, &usb_prediv,
 		&usb_postdiv, &usb_mul);
 	tnetd7200_set_clock(usb_base, &clocks->usb,
 		usb_prediv, usb_postdiv, -1, usb_mul,
 		TNETD7200_DEF_USB_CLK);
 
-	ar7_dsp_clock = ar7_cpu_clock;
+	dsp_clk.rate = cpu_clk.rate;
 
 	iounmap(clocks);
 	iounmap(bootcr);
 }
 
+/*
+ * Linux clock API
+ */
+int clk_enable(struct clk *clk)
+{
+	return 0;
+}
+EXPORT_SYMBOL(clk_enable);
+
+void clk_disable(struct clk *clk)
+{
+}
+EXPORT_SYMBOL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	return clk->rate;
+}
+EXPORT_SYMBOL(clk_get_rate);
+
+struct clk *clk_get(struct device *dev, const char *id)
+{
+	if (!strcmp(id, "bus"))
+		return &bus_clk;
+	/* cpmac and vbus share the same rate */
+	if (!strcmp(id, "cpmac"))
+		return &vbus_clk;
+	if (!strcmp(id, "cpu"))
+		return &cpu_clk;
+	if (!strcmp(id, "dsp"));
+		return &dsp_clk;
+	if (!strcmp(id, "vbus"))
+		return &vbus_clk;
+	return ERR_PTR(-ENOENT);
+}
+EXPORT_SYMBOL(clk_get);
+
+void clk_put(struct clk *clk)
+{
+}
+EXPORT_SYMBOL(clk_put);
+
 int __init ar7_init_clocks(void)
 {
 	switch (ar7_chip_id()) {
@@ -415,12 +464,14 @@ int __init ar7_init_clocks(void)
 		tnetd7200_init_clocks();
 		break;
 	case AR7_CHIP_7300:
-		ar7_dsp_clock = tnetd7300_dsp_clock();
+		dsp_clk.rate = tnetd7300_dsp_clock();
 		tnetd7300_init_clocks();
 		break;
 	default:
 		break;
 	}
+	/* adjust vbus clock rate */
+	vbus_clk.rate = bus_clk.rate / 2;
 
 	return 0;
 }
diff --git a/arch/mips/ar7/platform.c b/arch/mips/ar7/platform.c
index acbe147..c591f69 100644
--- a/arch/mips/ar7/platform.c
+++ b/arch/mips/ar7/platform.c
@@ -35,6 +35,7 @@
 #include <linux/phy.h>
 #include <linux/phy_fixed.h>
 #include <linux/gpio.h>
+#include <linux/clk.h>
 
 #include <asm/addrspace.h>
 #include <asm/mach-ar7/ar7.h>
@@ -507,13 +508,18 @@ static int __init ar7_register_devices(void)
 	u32 *bootcr, val;
 #ifdef CONFIG_SERIAL_8250
 	static struct uart_port uart_port[2] __initdata;
+	struct clk *bus_clk;
 
 	memset(uart_port, 0, sizeof(struct uart_port) * 2);
 
+	bus_clk = clk_get(NULL, "bus");
+	if (IS_ERR(bus_clk))
+		panic("unable to get bus clk\n");
+
 	uart_port[0].type = PORT_16550A;
 	uart_port[0].line = 0;
 	uart_port[0].irq = AR7_IRQ_UART0;
-	uart_port[0].uartclk = ar7_bus_freq() / 2;
+	uart_port[0].uartclk = clk_get_rate(bus_clk) / 2;
 	uart_port[0].iotype = UPIO_MEM32;
 	uart_port[0].mapbase = AR7_REGS_UART0;
 	uart_port[0].membase = ioremap(uart_port[0].mapbase, 256);
@@ -528,7 +534,7 @@ static int __init ar7_register_devices(void)
 		uart_port[1].type = PORT_16550A;
 		uart_port[1].line = 1;
 		uart_port[1].irq = AR7_IRQ_UART1;
-		uart_port[1].uartclk = ar7_bus_freq() / 2;
+		uart_port[1].uartclk = clk_get_rate(bus_clk) / 2;
 		uart_port[1].iotype = UPIO_MEM32;
 		uart_port[1].mapbase = UR8_REGS_UART1;
 		uart_port[1].membase = ioremap(uart_port[1].mapbase, 256);
diff --git a/arch/mips/ar7/time.c b/arch/mips/ar7/time.c
index a1fba89..5fb8a01 100644
--- a/arch/mips/ar7/time.c
+++ b/arch/mips/ar7/time.c
@@ -20,11 +20,21 @@
 
 #include <linux/init.h>
 #include <linux/time.h>
+#include <linux/err.h>
+#include <linux/clk.h>
 
 #include <asm/time.h>
 #include <asm/mach-ar7/ar7.h>
 
 void __init plat_time_init(void)
 {
-	mips_hpt_frequency = ar7_cpu_freq() / 2;
+	struct clk *cpu_clk;
+
+	cpu_clk = clk_get(NULL, "cpu");
+	if (IS_ERR(cpu_clk)) {
+		printk(KERN_ERR "unable to get cpu clock\n");
+		return;
+	}
+
+	mips_hpt_frequency = clk_get_rate(cpu_clk) / 2;
 }
diff --git a/arch/mips/include/asm/mach-ar7/ar7.h b/arch/mips/include/asm/mach-ar7/ar7.h
index 21cbbc7..f1cf389 100644
--- a/arch/mips/include/asm/mach-ar7/ar7.h
+++ b/arch/mips/include/asm/mach-ar7/ar7.h
@@ -105,26 +105,9 @@ static inline u8 ar7_chip_rev(void)
 	return (readl((void *)KSEG1ADDR(AR7_REGS_GPIO + 0x14)) >> 16) & 0xff;
 }
 
-static inline int ar7_cpu_freq(void)
-{
-	return ar7_cpu_clock;
-}
-
-static inline int ar7_bus_freq(void)
-{
-	return ar7_bus_clock;
-}
-
-static inline int ar7_vbus_freq(void)
-{
-	return ar7_bus_clock / 2;
-}
-#define ar7_cpmac_freq ar7_vbus_freq
-
-static inline int ar7_dsp_freq(void)
-{
-	return ar7_dsp_clock;
-}
+struct clk {
+	unsigned int	rate;
+};
 
 static inline int ar7_has_high_cpmac(void)
 {
diff --git a/drivers/net/cpmac.c b/drivers/net/cpmac.c
index 8d0be26..bf2072e 100644
--- a/drivers/net/cpmac.c
+++ b/drivers/net/cpmac.c
@@ -36,6 +36,7 @@
 #include <linux/phy_fixed.h>
 #include <linux/platform_device.h>
 #include <linux/dma-mapping.h>
+#include <linux/clk.h>
 #include <asm/gpio.h>
 #include <asm/atomic.h>
 
@@ -294,9 +295,16 @@ static int cpmac_mdio_write(struct mii_bus *bus, int phy_id,
 
 static int cpmac_mdio_reset(struct mii_bus *bus)
 {
+	struct clk *cpmac_clk;
+
+	cpmac_clk = clk_get(&bus->dev, "cpmac");
+	if (IS_ERR(cpmac_clk)) {
+		printk(KERN_ERR "unable to get cpmac clock\n");
+		return -1;
+	}
 	ar7_device_reset(AR7_RESET_BIT_MDIO);
 	cpmac_write(bus->priv, CPMAC_MDIO_CONTROL, MDIOC_ENABLE |
-		    MDIOC_CLKDIV(ar7_cpmac_freq() / 2200000 - 1));
+		    MDIOC_CLKDIV(clk_get_rate(cpmac_clk) / 2200000 - 1));
 	return 0;
 }
 
diff --git a/drivers/watchdog/ar7_wdt.c b/drivers/watchdog/ar7_wdt.c
index 2e94b71..2bb95cd 100644
--- a/drivers/watchdog/ar7_wdt.c
+++ b/drivers/watchdog/ar7_wdt.c
@@ -34,6 +34,7 @@
 #include <linux/ioport.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
+#include <linux/clk.h>
 
 #include <asm/addrspace.h>
 #include <asm/mach-ar7/ar7.h>
@@ -80,6 +81,8 @@ static struct resource *ar7_regs_wdt;
 /* Pointer to the remapped WDT IO space */
 static struct ar7_wdt *ar7_wdt;
 
+static struct clk *vbus_clk;
+
 static void ar7_wdt_kick(u32 value)
 {
 	WRITE_REG(ar7_wdt->kick_lock, 0x5555);
@@ -138,17 +141,19 @@ static void ar7_wdt_disable(u32 value)
 static void ar7_wdt_update_margin(int new_margin)
 {
 	u32 change;
+	u32 vbus_rate;
 
-	change = new_margin * (ar7_vbus_freq() / prescale_value);
+	vbus_rate = clk_get_rate(vbus_clk);
+	change = new_margin * (vbus_rate / prescale_value);
 	if (change < 1)
 		change = 1;
 	if (change > 0xffff)
 		change = 0xffff;
 	ar7_wdt_change(change);
-	margin = change * prescale_value / ar7_vbus_freq();
+	margin = change * prescale_value / vbus_rate;
 	printk(KERN_INFO DRVNAME
 	       ": timer margin %d seconds (prescale %d, change %d, freq %d)\n",
-	       margin, prescale_value, change, ar7_vbus_freq());
+	       margin, prescale_value, change, vbus_rate);
 }
 
 static void ar7_wdt_enable_wdt(void)
@@ -298,6 +303,13 @@ static int __devinit ar7_wdt_probe(struct platform_device *pdev)
 		goto out_mem_region;
 	}
 
+	vbus_clk = clk_get(NULL, "vbus");
+	if (IS_ERR(vbus_clk)) {
+		printk(KERN_ERR DRVNAME ": could not get vbus clock\n");
+		rc = PTR_ERR(vbus_clk);
+		goto out_mem_region;
+	}
+
 	ar7_wdt_disable_wdt();
 	ar7_wdt_prescale(prescale_value);
 	ar7_wdt_update_margin(margin);

^ permalink raw reply related

* Re: [PATCH] ipv6: Fix dst_release of invalid pointer
From: Krishna Kumar2 @ 2010-01-27  7:59 UTC (permalink / raw)
  To: David Miller; +Cc: netdev
In-Reply-To: <20100126.231133.165423271.davem@davemloft.net>

David Miller <davem@davemloft.net> wrote on 01/27/2010 12:41:33 PM:

> From: Krishna Kumar <krkumar2@in.ibm.com>
> Date: Wed, 27 Jan 2010 12:38:02 +0530
>
> > Fix dst_release on a invalid region.
> >
> > Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
>
> NULL, is NULL, is NULL, and always will be NULL.
>
> Even if rt == NULL, &rt->u.dst will be NULL too.
>
> So it's just plain NULL.
>
> You NULL that? :-)

Yes :)

I forgot dst_entry was the first element in rt6_info.

thanks,

- KK


^ permalink raw reply

* Re: Network QoS support in applications
From: Kalle Valo @ 2010-01-27  7:11 UTC (permalink / raw)
  To: Edgar E. Iglesias; +Cc: Dunc, David Miller, kaber, netdev, linux-wireless
In-Reply-To: <20100126215401.GA25095@laped.iglesias.mooo.com>

"Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:

> IMO what apps should be doing is setting the DSCP to a user configurable
> value (config file or cmd line switch etc). This way people can choose DSCP
> to whatever makes sense in their particular network. The default value
> is of less interest.

I have to disagree here. Most of the people are not that interested
configuring their applications, they just want to use them. I see that
having this configurable would be just an excuse for not having a good
default value.

Also it just doesn't scale if every user has to start configuring all
network applications they use. IMHO this all should work "Out of Box".

> WRT L3 vs L2, I think apps should normally be tagging by setting the DSCP
> field. The kernel should provide configurable mappings between DSCP and
> what ever L2 QoS that is available on the egress interface. As the packet
> jumps and gets routed, the DSCP value gets remapped two every links
> particular L2 "QoS" that matches the DSCP. After all, apps shouldn't need to
> know their hooked up to a 802.11, wired ethernet or what ever is on the
> route to the peer...

With this one I fully agree. I think this is the way we should do
this.

> AFAIK, Linux already makes all of this perfectly possible.

Yes, it's possible but not widely used because people don't know about
this. We need to provide documentation and then push applications to
use this.

> My 2 cents..

Thank you very much, much appreciated.

-- 
Kalle Valo

^ permalink raw reply

* Re: [PATCH] ipv6: Fix dst_release of invalid pointer
From: David Miller @ 2010-01-27  7:11 UTC (permalink / raw)
  To: krkumar2; +Cc: netdev
In-Reply-To: <20100127070802.26065.73104.sendpatchset@krkumar2.in.ibm.com>

From: Krishna Kumar <krkumar2@in.ibm.com>
Date: Wed, 27 Jan 2010 12:38:02 +0530

> Fix dst_release on a invalid region.
> 
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>

NULL, is NULL, is NULL, and always will be NULL.

Even if rt == NULL, &rt->u.dst will be NULL too.

So it's just plain NULL.

You NULL that? :-)


^ permalink raw reply

* [PATCH] ipv6: Fix dst_release of invalid pointer
From: Krishna Kumar @ 2010-01-27  7:08 UTC (permalink / raw)
  To: davem; +Cc: netdev, Krishna Kumar

Fix dst_release on a invalid region.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
 net/ipv6/addrconf.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -ruNp org/net/ipv6/addrconf.c new/net/ipv6/addrconf.c
--- org/net/ipv6/addrconf.c	2010-01-27 10:03:28.000000000 +0530
+++ new/net/ipv6/addrconf.c	2010-01-27 10:04:00.000000000 +0530
@@ -823,7 +823,8 @@ static void ipv6_del_addr(struct inet6_i
 				rt->rt6i_flags |= RTF_EXPIRES;
 			}
 		}
-		dst_release(&rt->u.dst);
+		if (rt)
+			dst_release(&rt->u.dst);
 	}
 
 	in6_ifa_put(ifp);

^ permalink raw reply

* Re: Network QoS support in applications
From: Kalle Valo @ 2010-01-27  7:03 UTC (permalink / raw)
  To: Steven Blake; +Cc: David Miller, kaber, netdev, linux-wireless
In-Reply-To: <189614e81eab48b8189ee99236902df3@petri-meat.com>

Steven Blake <slblake@petri-meat.com> writes:

>> This is a local policy decision.
>> 
>> There is no universal way of doing any of this, really.
>
> You are correct.  However, RFC 4594 is an attempt to get applications and
> network operators on the same page.  There is no harm done if apps set DSCP
> values in accordance with this RFC.

Wow, this is great. Somehow I missed this, thank you very much. I will
read it in detail now. 

DiffServ people should have written something like this from the
beginning.

-- 
Kalle Valo

^ permalink raw reply

* Re: [RFC] [PATCH] Optimize TCP sendmsg in favour of fast devices?
From: Krishna Kumar2 @ 2010-01-27  7:12 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David Miller, eric.dumazet, ilpo.jarvinen, netdev
In-Reply-To: <20100121094157.GA15262@gondor.apana.org.au>

Hi Herbert,

> Herbert Xu <herbert@gondor.apana.org.au> wrote on 01/21/2010 03:11 PM

Sorry for the late response.

> >> I had to remove the F_SG flag from cxgb3 driver (using ethtool
> >> didn't show any difference in performance since GSO was enabled
> >> on the device due to register_netdev setting it). Testing show a
> >> drop of 25% in performance with this patch for non-SG device,
> >> the extra alloc/memcpy is showing up.
> >>
> >> For the SG driver, I get a good performace gain (not anywhere
> >> close to 25% though). What do you suggest?
> >
> > I don't think we can add your change if it hurts non-SG
> > devices that much.
>
> Wait, we need to be careful when testing this.  Non-SG devices
> do actually benefit from TSO which they otherwise cannot access.
>
> If you unset the F_SG bit, then that would disable TSO too.  So
> you need to enable GSO to compensate.  So Krishna, did you check
> with tcpdump to see if GSO was really enabled with SG off?

OK, I unset F_SG and set F_GSO (in driver). With this, tcpdump shows
GSO is enabled - the tcp packet sizes builds up to 65160 bytes.

I ran 5 serial netperf's with 16K and another 5 serial netperfs
with 64K I/O sizes, and the aggregate result is:

0. Driver unsets F_SG but sets F_GSO:
      Original code with 16K: 19471.65
      New code with 16K:      19409.70
      Original code with 64K: 21357.23
      New code with 64K:      22050.42

To recap the other tests I did today:

1. Driver unsets F_SG, and with GSO off
      Original code with 16K: 10123.56
      New code with 16K:      7111.12
      Original code with 64K: 11568.99
      New code with 64K:      7611.37

2. Driver unsets F_SG and uses ethtool to set GSO:
      Original code with 16K: 18864.38
      New code with 16K:      18465.54
      Original code with 64K: 21005.43
      New code with 64K:      22529.24

Thanks,

- KK

> IIRC when I did a similar test with e1000 back when I wrote this
> the performance of GSO with SG off was pretty much the same as
> no GSO with SG off.


^ permalink raw reply

* Re: Network QoS support in applications
From: Kalle Valo @ 2010-01-27  6:59 UTC (permalink / raw)
  To: Henning Rogge; +Cc: David Miller, kaber, netdev, linux-wireless
In-Reply-To: <201001261407.04947.henning.rogge@fkie.fraunhofer.de>

Henning Rogge <henning.rogge@fkie.fraunhofer.de> writes:

> On Tue January 26 2010 13:56:50 Kalle Valo wrote:
>> So the idea is that the three left most bits (Class Selector
>> Codepoints from DiffServ RFC 2474) would contain these values from
>> IEEE 802.1d-2004 Annex G:
>> 
>> 0 best effort
>> 1 background
>> 2 (spare)
>> 3 excellent effort
>> 4 controlled load
>> 5 video
>> 6 voice
>> 7 network control
>> 
>> What do people think of this?
>
> The IETF is discussing about a traffic priority below "best effort"
> for bulk traffic at the moment. Something you use to fill up links
> if your can but drop it if you are not sure. So it might be a good
> idea to make "best efford" not zero.

I think 802.1d used zero for best effort to get it as the default. If
no class is set, the field will be zero and best effort is used.

-- 
Kalle Valo

^ 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