Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] xen/netfront: improve truesize tracking
From: Eric Dumazet @ 2012-12-20 15:39 UTC (permalink / raw)
  To: Sander Eikelenboom
  Cc: Ian Campbell, netdev@vger.kernel.org, Konrad Rzeszutek Wilk,
	annie li, xen-devel@lists.xensource.com
In-Reply-To: <1797374383.20121220135139@eikelenboom.it>

On Thu, 2012-12-20 at 13:51 +0100, Sander Eikelenboom wrote:

> Eric:
>      From the warn_on_once, delta should be smaller than len, but probably they should be as close together as possible.
>      When you say "accurate estimation", what would be a acceptable difference between DELTA and LEN ?

I would use the most exact value, which is :

   skb->truesize += nr_frags * PAGE_SIZE;

Then, if we can spot later a regression in some stacks, adapt the
limiting parameters. I did a lot of work in GRO and TCP stack to reduce
the memory, and further changes are possible.

We really want to account memory, because we want to control how memory
is used on our machines and don't let some users use more than the
amount that was allowed to them.

^ permalink raw reply

* Re: [PATCH net-next V4 03/13] bridge: Validate that vlan is permitted on ingress
From: Vlad Yasevich @ 2012-12-20 15:41 UTC (permalink / raw)
  To: Shmulik Ladkani
  Cc: netdev, shemminger, davem, or.gerlitz, jhs, mst, erdnetdev, jiri
In-Reply-To: <20121220160713.30cdfc05@pixies.home.jungo.com>

On 12/20/2012 09:07 AM, Shmulik Ladkani wrote:
> Hi Vlad,
>
> On Wed, 19 Dec 2012 12:48:14 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote:
>> +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb)
>> +{
>> +	struct net_port_vlan *pve;
>> +	u16 vid;
>> +
>> +	/* If there are no vlan in the permitted list, all packets are
>> +	 * permitted.
>> +	 */
>> +	if (list_empty(&p->vlan_list))
>> +		return true;
>
> I assumed the default policy would be Drop in such case, otherwise
> leaking between vlan domains is possible.
> Or maybe, ingress policy when port isn't a member of ingress VID should
> be configurable (drop/allow).

We have have to default to allow since we want to retain original bridge 
functionality if there is no configuration.

>
>> +	vid = br_get_vlan(skb);
>> +	pve = nbp_vlan_find(p, vid);
>
> Why search by iterating through NBP's vlan_list?
> You know the VID (hence may fetch the net_bridge_vlan from the hash), so
> why don't you directly consult the net_bridge_vlan's port_bitmap?

It's an alternative...  I am betting that this port isn't in too many 
vlans and that searching the list might be faster.

>
>> @@ -54,6 +74,9 @@ int br_handle_frame_finish(struct sk_buff *skb)
>>   	if (!p || p->state == BR_STATE_DISABLED)
>>   		goto drop;
>>
>> +	if (!br_allowed_ingress(p, skb))
>> +		goto drop;
>> +
>
> This condition should be also encorporated upon "ingress" at the "bridge
> master port" (that is, early at br_dev_xmit).
> Think of the "bridge master port" as yet another port:
> upon "ingress" (meaning, tx packets from the ip stack), we should
> also enforce any ingress permission rules.
>

I've tried that before and now can't think of a reason why I rejected 
it.  I'll try to remember...

Thanks
-vlad

> Regards,
> Shmulik
>

^ permalink raw reply

* Re: [PATCH net-next V4 03/13] bridge: Validate that vlan is permitted on ingress
From: Shmulik Ladkani @ 2012-12-20 16:24 UTC (permalink / raw)
  To: vyasevic; +Cc: netdev, shemminger, davem, or.gerlitz, jhs, mst, erdnetdev, jiri
In-Reply-To: <50D331A8.3080206@redhat.com>

On Thu, 20 Dec 2012 10:41:28 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote:
> >> +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb)
> >> +{
> >> +	struct net_port_vlan *pve;
> >> +	u16 vid;
> >> +
> >> +	/* If there are no vlan in the permitted list, all packets are
> >> +	 * permitted.
> >> +	 */
> >> +	if (list_empty(&p->vlan_list))
> >> +		return true;
> >
> > I assumed the default policy would be Drop in such case, otherwise
> > leaking between vlan domains is possible.
> > Or maybe, ingress policy when port isn't a member of ingress VID should
> > be configurable (drop/allow).
> 
> We have have to default to allow since we want to retain original bridge 
> functionality if there is no configuration.

Ok; so having the port not a member of ANY vlan is a "port vlan
disabled" configuration knob, and as such, it is a member of ANY vlan,
meaning that:

(1) every "non-vlan port" is connected to any other "non-vlan port"
(2) frame ingress on a "non-vlan" port may egress on a "vlan enabled"
  port, depending on the ingress VID and the port-membership map of the
  egress port
  (and thus, PVID should be defined even to "non-vlan" ports, for the
  case where untagged frame is received on the non-vlan port)
(3) frame ingress on a "vlan-enabled" port would always egress on
  "non-vlan" ports

Seems ok.
However this is an additional nuance that might not be expected by the
user configuring the bridge; maybe this needs some clarification.

> >> +	vid = br_get_vlan(skb);
> >> +	pve = nbp_vlan_find(p, vid);
> >
> > Why search by iterating through NBP's vlan_list?
> > You know the VID (hence may fetch the net_bridge_vlan from the hash), so
> > why don't you directly consult the net_bridge_vlan's port_bitmap?
> 
> It's an alternative...  I am betting that this port isn't in too many 
> vlans and that searching the list might be faster.

I assumed the opposite: finding the hash bucket is just a bitwise mask,
and number of items in a bucket would rarely be grater than 1.
I expect such code to be shorter, but this needs to be verified.

Regards,
Shmulik

^ permalink raw reply

* Re: [PATCH net] net/vxlan: Use the underlying device index when joining/leaving multicast groups
From: Stephen Hemminger @ 2012-12-20 16:26 UTC (permalink / raw)
  To: Yan Burman; +Cc: netdev, ogerlitz
In-Reply-To: <1356010568-21644-1-git-send-email-yanb@mellanox.com>

On Thu, 20 Dec 2012 15:36:08 +0200
Yan Burman <yanb@mellanox.com> wrote:

> The socket calls from vxlan to join/leave multicast group aren't
> using the index of the underlying device, as a result the stack uses
> the first interface that is up. This results in vxlan being non functional
> over a device which isn't the 1st to be up.
> Fix this by providing the iflink field to the vxlan instance
> to the multicast calls.
> 
> Signed-off-by: Yan Burman <yanb@mellanox.com>
> ---
>  drivers/net/vxlan.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 3b3fdf6..40f2cc1 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -505,7 +505,8 @@ static int vxlan_join_group(struct net_device *dev)
>  	struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
>  	struct sock *sk = vn->sock->sk;
>  	struct ip_mreqn mreq = {
> -		.imr_multiaddr.s_addr = vxlan->gaddr,
> +		.imr_multiaddr.s_addr	= vxlan->gaddr,
> +		.imr_ifindex		= vxlan->link,
>  	};
>  	int err;
>  
> @@ -532,7 +533,8 @@ static int vxlan_leave_group(struct net_device *dev)
>  	int err = 0;
>  	struct sock *sk = vn->sock->sk;
>  	struct ip_mreqn mreq = {
> -		.imr_multiaddr.s_addr = vxlan->gaddr,
> +		.imr_multiaddr.s_addr	= vxlan->gaddr,
> +		.imr_ifindex		= vxlan->link,
>  	};
>  
>  	/* Only leave group when last vxlan is done. */

Acked-by: Stephen Hemminger <shemminger@vyatta.com>

^ permalink raw reply

* Re: Lockdep warning in vxlan
From: Stephen Hemminger @ 2012-12-20 16:34 UTC (permalink / raw)
  To: Yan Burman; +Cc: netdev
In-Reply-To: <50D31A00.7060905@mellanox.com>

On Thu, 20 Dec 2012 16:00:32 +0200
Yan Burman <yanb@mellanox.com> wrote:

> Hi.
> 
> When working with vxlan from current net-next, I got a lockdep warning 
> (below).
> It seems to happen when I have host B pinging host A and while the pings 
> continue,
> I do "ip link del" on the vxlan interface on host A. The lockdep warning 
> is on host A.
> Tell me if you need some more info.
> 

Looks like the case of nested ARP requests, the initial request is coming
from neigh_timer (ARP retransmit), but inside neigh_probe the lock
is dropped?

^ permalink raw reply

* Re: [PATCH net-next] af_unix: MSG_TRUNC support for dgram sockets
From: Michael Kerrisk (man-pages) @ 2012-12-20 16:50 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, piergiorgio.beruto, netdev
In-Reply-To: <1329902695.18384.101.camel@edumazet-laptop>

On Wed, Feb 22, 2012 at 10:24 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Piergiorgio Beruto expressed the need to fetch size of first datagram in
> queue for AF_UNIX sockets and suggested a patch against SIOCINQ ioctl.
>
> I suggested instead to implement MSG_TRUNC support as a recv() input
> flag, as already done for RAW, UDP & NETLINK sockets.
>
> len = recv(fd, &byte, 1, MSG_PEEK | MSG_TRUNC);
>
> MSG_TRUNC asks recv() to return the real length of the packet, even when
> is was longer than the passed buffer.
>
> There is risk that a userland application used MSG_TRUNC by accident
> (since it had no effect on af_unix sockets) and this might break after
> this patch.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Tested-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
> CC: Michael Kerrisk <mtk.manpages@gmail.com>

Patch below to man-pages applied.

Thanks for CCing me, Eric.

Cheers,

Michael

--- a/man2/recv.2
+++ b/man2/recv.2
@@ -264,7 +264,7 @@ subsequent receive call will return the same data.
 For raw
 .RB ( AF_PACKET ),
 Internet datagram (since Linux 2.4.27/2.6.8),
-and netlink (since Linux 2.6.22) sockets:
+netlink (since Linux 2.6.22) and UNIX datagram (since Linux 3.4) sockets:
 return the real length of the packet or datagram,
 even when it was longer than the passed buffer.
 Not implemented for UNIX domain

^ permalink raw reply

* Re: [PATCH net-next V4 03/13] bridge: Validate that vlan is permitted on ingress
From: Vlad Yasevich @ 2012-12-20 16:54 UTC (permalink / raw)
  To: Shmulik Ladkani
  Cc: netdev, shemminger, davem, or.gerlitz, jhs, mst, erdnetdev, jiri
In-Reply-To: <20121220182402.6143fcb1@pixies.home.jungo.com>

On 12/20/2012 11:24 AM, Shmulik Ladkani wrote:
> On Thu, 20 Dec 2012 10:41:28 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote:
>>>> +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb)
>>>> +{
>>>> +	struct net_port_vlan *pve;
>>>> +	u16 vid;
>>>> +
>>>> +	/* If there are no vlan in the permitted list, all packets are
>>>> +	 * permitted.
>>>> +	 */
>>>> +	if (list_empty(&p->vlan_list))
>>>> +		return true;
>>>
>>> I assumed the default policy would be Drop in such case, otherwise
>>> leaking between vlan domains is possible.
>>> Or maybe, ingress policy when port isn't a member of ingress VID should
>>> be configurable (drop/allow).
>>
>> We have have to default to allow since we want to retain original bridge
>> functionality if there is no configuration.
>
> Ok; so having the port not a member of ANY vlan is a "port vlan
> disabled" configuration knob, and as such, it is a member of ANY vlan,
> meaning that:
>
> (1) every "non-vlan port" is connected to any other "non-vlan port"

Technically, it would be connected to every over port.

> (2) frame ingress on a "non-vlan" port may egress on a "vlan enabled"
>    port, depending on the ingress VID and the port-membership map of the
>    egress port
>    (and thus, PVID should be defined even to "non-vlan" ports, for the
>    case where untagged frame is received on the non-vlan port)

Sort of.  The way I did it (testing now), is like this:
    if there is egress policy
	apply policy and forward.
    else if there was ingress policy (pvid)
	apply it and forward
    else
	forward as is (old bridge behavior).

This way if there was a pvid on an ingress port and nothing on egress,
then pvid applies.  If there was nothing configured on ingress port,
but we have a egress policy, we'll apply any vlan information from
the frame to egress policy.  In this case, ingress untagged traffic 
would be assigned vlan 0.


> (3) frame ingress on a "vlan-enabled" port would always egress on
>    "non-vlan" ports

yes and they would egress based on their ingress policy.

>
> Seems ok.
> However this is an additional nuance that might not be expected by the
> user configuring the bridge; maybe this needs some clarification.

I'll try to document things sufficiently.  This hybrid approach may
produce some unintended results.  We could always remove it or introduce
the tunable to change default policy to drop once vlan configuration is
in effect.


>
>>>> +	vid = br_get_vlan(skb);
>>>> +	pve = nbp_vlan_find(p, vid);
>>>
>>> Why search by iterating through NBP's vlan_list?
>>> You know the VID (hence may fetch the net_bridge_vlan from the hash), so
>>> why don't you directly consult the net_bridge_vlan's port_bitmap?
>>
>> It's an alternative...  I am betting that this port isn't in too many
>> vlans and that searching the list might be faster.
>
> I assumed the opposite: finding the hash bucket is just a bitwise mask,
> and number of items in a bucket would rarely be grater than 1.
> I expect such code to be shorter, but this needs to be verified.

I'll try to set something up, but that will probably be next year...

-vlad
>
> Regards,
> Shmulik
>

^ permalink raw reply

* Re: [PATCH net-next V4 00/13] Add basic VLAN support to bridges
From: Stephen Hemminger @ 2012-12-20 17:07 UTC (permalink / raw)
  To: Vitalii Demianets
  Cc: Andrew Collins, Vlad Yasevich, netdev, davem, or.gerlitz, jhs,
	mst, erdnetdev, jiri
In-Reply-To: <201212201208.14204.vitas@nppfactor.kiev.ua>

On Thu, 20 Dec 2012 12:08:13 +0200
Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote:

> On Thursday 20 December 2012 00:54:27 Andrew Collins wrote:
> > On Wed, Dec 19, 2012 at 10:48 AM, Vlad Yasevich <vyasevic@redhat.com> wrote:
> > > This series of patches provides an ability to add VLANs to the bridge
> > > ports.  This is similar to what can be found in most switches.  The
> > > bridge port may have any number of VLANs added to it including vlan 0
> > > priority tagged traffic.  When vlans are added to the port, only traffic
> > > tagged with particular vlan will forwarded over this port.  Additionally,
> > > vlan ids are added to FDB entries and become part of the lookup.  This
> > > way we correctly identify the FDB entry.
> >
> > This is likely well beyond the scope of this change, but I figured I'd
> > throw out the question anyway.  This changeset looks to bring the
> > Linux bridging code closer to the 802.1Q-2005 definition of a bridge,
> > which is nice to see, I'm curious if this changeset also opens up the
> > possibility of supporting MSTP in the future?  The big thing I see
> > missing is per-VLAN port state, although I'm not very familiar with
> > the current STP/bridge interactions.  Has anyone put any thought into
> > what other necessary bridge pieces might be missing for MSTP support?
> 
> I think, to be compatible with 802.1Q-2005 we need the following pieces:
> 1) Multiple FIDs (it is 802.1Q term for FDB) support. It means that kernel 
> should support several independent FDBs on a single bridge. The 802.1Q-2005 
> standard requires the number of supported FDBs to be no less than the number 
> of different MSTIs the implementation supports;
> 2) VLAN-to-FDB mapping should be introduced;
> 3) Support of Multiple Spanning Tree Instances (MSTIs);
> 4) FDB-to-MSTI mapping should be introduced;
> 5) And finally, per-MST port states should be implemented.
> 
> > obviously something to handle the MSTP protocol itself would need to exist 
> as well
> 
> Please look here: http://sourceforge.net/projects/mstpd/

A couple of points:
* How does this compare with features/functionality of commercial
  hardware bridges?
* Is this as simple as possible? It looks like there is creeping-featurism
  here. I am all for a simple extension to allow bridge vlan filtering, but
  not the added complexity of "let's teach bridges all about all possible
  things any user might want to do with vlan.s"


 

^ permalink raw reply

* Re: at91sam9260 MACB problem with IP fragmentation
From: Nicolas Ferre @ 2012-12-20 17:51 UTC (permalink / raw)
  To: Erwin Rol
  Cc: linux-kernel, Havard Skinnemoen, linux-arm-kernel, matteo.fortini,
	netdev
In-Reply-To: <50D2D7BD.3030801@erwinrol.com>

On 12/20/2012 10:17 AM, Erwin Rol :
> Hallo Nicolas,
> 
> On 6-12-2012 14:27, Nicolas Ferre wrote:
>> Erwin,
>>
>> On 12/06/2012 12:32 PM, Erwin Rol :
>>> Hello Nicolas, Havard, all,
>>>
>>> I have a very obscure problem with a at91sam9260 board (almost 1 to 1
>>> copy of the Atmel EK).
>>>
>>> The MACB seems to stall when I use large (>2 * MTU) UDP datagrams. The
>>> test case is that a udp echo client (PC) sends datagrams with increasing
>>> length to the AT91 until the max length of the UDP datagram is reached.
>>> When there is no IP fragmentation everything is fine, but when the
>>> datagrams are starting to get fragmented the AT91 will not reply
>>> anymore. But as soon as some network traffic happens it goes on again,
>>> and non of the data is lost.
> 
> <snip>
> 
>>> I tried several kernels including the test version from Nicolas that he
>>> posted on LKML in October. They all show the same effect.
>>
>> [..]
>>
>> It seems that Matteo has the same behavior: check here:
>> http://www.spinics.net/lists/netdev/msg218951.html
> 
> I tried Matteo's patch and it seems to work. But I don't know if the
> patch is really the right solution. I checked again with wireshark and
> it really seems the sending that stalls not the receiving. But as soon
> as a ethernet frame is received the sending "un-stalls". So maybe the
> patch just causes an MACB IRQ at certain moments that causes the sending
> to continue?

Any digging is interesting for me.


>> I am working on the macb driver right now, so I will try to reproduce
>> and track this issue on my side.
> 
> Any luck reproducing it ?

Yes, I see unexpected things happening but as I am connected to a whole
company network so maybe some broadcast packets are unlocking the
interface...
Anyway, I am continuing to investigate.

Best regards,--
Nicolas Ferre

^ permalink raw reply

* Re: Lockdep warning in vxlan
From: Eric Dumazet @ 2012-12-20 18:16 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Yan Burman, netdev
In-Reply-To: <20121220083436.0c7fc33f@nehalam.linuxnetplumber.net>

On Thu, 2012-12-20 at 08:34 -0800, Stephen Hemminger wrote:
> On Thu, 20 Dec 2012 16:00:32 +0200
> Yan Burman <yanb@mellanox.com> wrote:
> 
> > Hi.
> > 
> > When working with vxlan from current net-next, I got a lockdep warning 
> > (below).
> > It seems to happen when I have host B pinging host A and while the pings 
> > continue,
> > I do "ip link del" on the vxlan interface on host A. The lockdep warning 
> > is on host A.
> > Tell me if you need some more info.
> > 
> 
> Looks like the case of nested ARP requests, the initial request is coming
> from neigh_timer (ARP retransmit), but inside neigh_probe the lock
> is dropped?

Bug is from arp_solicit(), releasing the lock after arp_send()

Its used to protect neigh->ha

We could instead copy neigh->ha, without taking n->lock but ha_lock
seqlock, using neigh_ha_snapshot() helper

Yan, could you test the following patch ?

Thanks
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index ce6fbdf..1169ed4 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -321,7 +321,7 @@ static void arp_error_report(struct neighbour *neigh, struct sk_buff *skb)
 static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
 {
 	__be32 saddr = 0;
-	u8  *dst_ha = NULL;
+	u8 dst_ha[MAX_ADDR_LEN];
 	struct net_device *dev = neigh->dev;
 	__be32 target = *(__be32 *)neigh->primary_key;
 	int probes = atomic_read(&neigh->probes);
@@ -363,9 +363,9 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
 	if (probes < 0) {
 		if (!(neigh->nud_state & NUD_VALID))
 			pr_debug("trying to ucast probe in NUD_INVALID\n");
-		dst_ha = neigh->ha;
-		read_lock_bh(&neigh->lock);
+		neigh_ha_snapshot(dst_ha, neigh, dev);
 	} else {
+		memset(dst_ha, 0, dev->addr_len);
 		probes -= neigh->parms->app_probes;
 		if (probes < 0) {
 #ifdef CONFIG_ARPD
@@ -377,8 +377,6 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
 
 	arp_send(ARPOP_REQUEST, ETH_P_ARP, target, dev, saddr,
 		 dst_ha, dev->dev_addr, NULL);
-	if (dst_ha)
-		read_unlock_bh(&neigh->lock);
 }
 
 static int arp_ignore(struct in_device *in_dev, __be32 sip, __be32 tip)

^ permalink raw reply related

* Re: Lockdep warning in vxlan
From: Stephen Hemminger @ 2012-12-20 18:22 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Yan Burman, netdev
In-Reply-To: <1356027360.21834.2973.camel@edumazet-glaptop>

On Thu, 20 Dec 2012 10:16:00 -0800
Eric Dumazet <erdnetdev@gmail.com> wrote:

> On Thu, 2012-12-20 at 08:34 -0800, Stephen Hemminger wrote:
> > On Thu, 20 Dec 2012 16:00:32 +0200
> > Yan Burman <yanb@mellanox.com> wrote:
> >   
> > > Hi.
> > > 
> > > When working with vxlan from current net-next, I got a lockdep warning 
> > > (below).
> > > It seems to happen when I have host B pinging host A and while the pings 
> > > continue,
> > > I do "ip link del" on the vxlan interface on host A. The lockdep warning 
> > > is on host A.
> > > Tell me if you need some more info.
> > >   
> > 
> > Looks like the case of nested ARP requests, the initial request is coming
> > from neigh_timer (ARP retransmit), but inside neigh_probe the lock
> > is dropped?  
> 
> Bug is from arp_solicit(), releasing the lock after arp_send()
> 
> Its used to protect neigh->ha
> 
> We could instead copy neigh->ha, without taking n->lock but ha_lock
> seqlock, using neigh_ha_snapshot() helper
> 
> Yan, could you test the following patch ?
> 
> Thanks
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index ce6fbdf..1169ed4 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -321,7 +321,7 @@ static void arp_error_report(struct neighbour *neigh, struct sk_buff *skb)
>  static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  {
>  	__be32 saddr = 0;
> -	u8  *dst_ha = NULL;
> +	u8 dst_ha[MAX_ADDR_LEN];
>  	struct net_device *dev = neigh->dev;
>  	__be32 target = *(__be32 *)neigh->primary_key;
>  	int probes = atomic_read(&neigh->probes);
> @@ -363,9 +363,9 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  	if (probes < 0) {
>  		if (!(neigh->nud_state & NUD_VALID))
>  			pr_debug("trying to ucast probe in NUD_INVALID\n");
> -		dst_ha = neigh->ha;
> -		read_lock_bh(&neigh->lock);
> +		neigh_ha_snapshot(dst_ha, neigh, dev);
>  	} else {
> +		memset(dst_ha, 0, dev->addr_len);
>  		probes -= neigh->parms->app_probes;
>  		if (probes < 0) {
>  #ifdef CONFIG_ARPD
> @@ -377,8 +377,6 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  
>  	arp_send(ARPOP_REQUEST, ETH_P_ARP, target, dev, saddr,
>  		 dst_ha, dev->dev_addr, NULL);
> -	if (dst_ha)
> -		read_unlock_bh(&neigh->lock);
>  }
>  
>  static int arp_ignore(struct in_device *in_dev, __be32 sip, __be32 tip)

I like this. Getting rid of yet another read lock

^ permalink raw reply

* Re: [PATCH 3/3] iproute2: make `bridge mdb` output consistent with input
From: Stephen Hemminger @ 2012-12-20 18:58 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, bridge
In-Reply-To: <1356013915-20835-3-git-send-email-amwang@redhat.com>

On Thu, 20 Dec 2012 22:31:55 +0800
Cong Wang <amwang@redhat.com> wrote:

> bridge -> dev
> group -> grp
> 

All three patches accepted for next version of iproute2.

^ permalink raw reply

* [PATCH] bnx2x: use ARRAY_SIZE where possible
From: Sasha Levin @ 2012-12-20 19:11 UTC (permalink / raw)
  To: Eilon Greenstein, netdev, linux-kernel; +Cc: Sasha Levin
In-Reply-To: <1356030701-16284-1-git-send-email-sasha.levin@oracle.com>

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
index 09096b4..cb41f54 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
@@ -3659,7 +3659,7 @@ static void bnx2x_warpcore_enable_AN_KR2(struct bnx2x_phy *phy,
 	bnx2x_cl45_read_or_write(bp, phy, MDIO_WC_DEVAD,
 				 MDIO_WC_REG_CL49_USERB0_CTRL, (3<<6));
 
-	for (i = 0; i < sizeof(reg_set)/sizeof(struct bnx2x_reg_set); i++)
+	for (i = 0; i < ARRAY_SIZE(reg_set); i++)
 		bnx2x_cl45_write(bp, phy, reg_set[i].devad, reg_set[i].reg,
 				 reg_set[i].val);
 
@@ -3713,7 +3713,7 @@ static void bnx2x_warpcore_enable_AN_KR(struct bnx2x_phy *phy,
 	};
 	DP(NETIF_MSG_LINK, "Enable Auto Negotiation for KR\n");
 	/* Set to default registers that may be overriden by 10G force */
-	for (i = 0; i < sizeof(reg_set)/sizeof(struct bnx2x_reg_set); i++)
+	for (i = 0; i < ARRAY_SIZE(reg_set); i++)
 		bnx2x_cl45_write(bp, phy, reg_set[i].devad, reg_set[i].reg,
 				 reg_set[i].val);
 
@@ -3854,7 +3854,7 @@ static void bnx2x_warpcore_set_10G_KR(struct bnx2x_phy *phy,
 		{MDIO_PMA_DEVAD, MDIO_WC_REG_PMD_KR_CONTROL, 0x2}
 	};
 
-	for (i = 0; i < sizeof(reg_set)/sizeof(struct bnx2x_reg_set); i++)
+	for (i = 0; i < ARRAY_SIZE(reg_set); i++)
 		bnx2x_cl45_write(bp, phy, reg_set[i].devad, reg_set[i].reg,
 				 reg_set[i].val);
 
@@ -4242,7 +4242,7 @@ static void bnx2x_warpcore_clear_regs(struct bnx2x_phy *phy,
 	bnx2x_cl45_read_or_write(bp, phy, MDIO_WC_DEVAD,
 				 MDIO_WC_REG_RX66_CONTROL, (3<<13));
 
-	for (i = 0; i < sizeof(wc_regs)/sizeof(struct bnx2x_reg_set); i++)
+	for (i = 0; i < ARRAY_SIZE(wc_regs); i++)
 		bnx2x_cl45_write(bp, phy, wc_regs[i].devad, wc_regs[i].reg,
 				 wc_regs[i].val);
 
@@ -9520,7 +9520,7 @@ static void bnx2x_save_848xx_spirom_version(struct bnx2x_phy *phy,
 	} else {
 		/* For 32-bit registers in 848xx, access via MDIO2ARM i/f. */
 		/* (1) set reg 0xc200_0014(SPI_BRIDGE_CTRL_2) to 0x03000000 */
-		for (i = 0; i < sizeof(reg_set)/sizeof(struct bnx2x_reg_set);
+		for (i = 0; i < ARRAY_SIZE(reg_set);
 		      i++)
 			bnx2x_cl45_write(bp, phy, reg_set[i].devad,
 					 reg_set[i].reg, reg_set[i].val);
@@ -9592,7 +9592,7 @@ static void bnx2x_848xx_set_led(struct bnx2x *bp,
 			 MDIO_PMA_DEVAD,
 			 MDIO_PMA_REG_8481_LINK_SIGNAL, val);
 
-	for (i = 0; i < sizeof(reg_set)/sizeof(struct bnx2x_reg_set); i++)
+	for (i = 0; i < ARRAY_SIZE(reg_set); i++)
 		bnx2x_cl45_write(bp, phy, reg_set[i].devad, reg_set[i].reg,
 				 reg_set[i].val);
 
@@ -13395,7 +13395,7 @@ static void bnx2x_disable_kr2(struct link_params *params,
 	};
 	DP(NETIF_MSG_LINK, "Disabling 20G-KR2\n");
 
-	for (i = 0; i < sizeof(reg_set)/sizeof(struct bnx2x_reg_set); i++)
+	for (i = 0; i < ARRAY_SIZE(reg_set); i++)
 		bnx2x_cl45_write(bp, phy, reg_set[i].devad, reg_set[i].reg,
 				 reg_set[i].val);
 	vars->link_attr_sync &= ~LINK_ATTR_SYNC_KR2_ENABLE;
-- 
1.8.0

^ permalink raw reply related

* Re: [PATCH 4/4] net/smsc911x: Provide common clock functionality
From: Linus Walleij @ 2012-12-20 19:12 UTC (permalink / raw)
  To: Lee Jones
  Cc: linux-arm-kernel, linux-kernel, arnd, linus.walleij,
	Steve Glendinning, netdev, Robert Marklund
In-Reply-To: <1355937587-31730-4-git-send-email-lee.jones@linaro.org>

On Wed, Dec 19, 2012 at 6:19 PM, Lee Jones <lee.jones@linaro.org> wrote:

> Some platforms provide clocks which require enabling before the
> SMSC911x chip will power on. This patch uses the new common clk
> framework to do just that. If no clock is provided, it will just
> be ignored and the driver will continue to assume that no clock
> is required for the chip to run successfully.
>
> Cc: Steve Glendinning <steve.glendinning@shawell.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Seems to me like it'll do the trick.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply

* [PATCH] wireless: mwifiex: remove unreachable paths
From: Sasha Levin @ 2012-12-20 19:11 UTC (permalink / raw)
  To: Bing Zhao, John W. Linville, linux-wireless, netdev, linux-kernel
  Cc: Sasha Levin
In-Reply-To: <1356030701-16284-1-git-send-email-sasha.levin@oracle.com>

We know 'firmware' is non-NULL from the beginning of mwifiex_prog_fw_w_helper,
remove all !firmware paths from the rest of the function.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 drivers/net/wireless/mwifiex/usb.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/usb.c b/drivers/net/wireless/mwifiex/usb.c
index 63ac9f2..8bd7098 100644
--- a/drivers/net/wireless/mwifiex/usb.c
+++ b/drivers/net/wireless/mwifiex/usb.c
@@ -836,23 +836,14 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter,
 			dlen = 0;
 		} else {
 			/* copy the header of the fw_data to get the length */
-			if (firmware)
-				memcpy(&fwdata->fw_hdr, &firmware[tlen],
-				       sizeof(struct fw_header));
-			else
-				mwifiex_get_fw_data(adapter, tlen,
-						    sizeof(struct fw_header),
-						    (u8 *)&fwdata->fw_hdr);
+			memcpy(&fwdata->fw_hdr, &firmware[tlen],
+			       sizeof(struct fw_header));
 
 			dlen = le32_to_cpu(fwdata->fw_hdr.data_len);
 			dnld_cmd = le32_to_cpu(fwdata->fw_hdr.dnld_cmd);
 			tlen += sizeof(struct fw_header);
 
-			if (firmware)
-				memcpy(fwdata->data, &firmware[tlen], dlen);
-			else
-				mwifiex_get_fw_data(adapter, tlen, dlen,
-						    (u8 *)fwdata->data);
+			memcpy(fwdata->data, &firmware[tlen], dlen);
 
 			fwdata->seq_num = cpu_to_le32(fw_seqnum);
 			tlen += dlen;
-- 
1.8.0

^ permalink raw reply related

* Re: [PATCH 4/4] net/smsc911x: Provide common clock functionality
From: Russell King - ARM Linux @ 2012-12-20 19:24 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Lee Jones, Steve Glendinning, Robert Marklund, linus.walleij,
	arnd, netdev, linux-kernel, linux-arm-kernel
In-Reply-To: <CACRpkda79O_b3Z8g7Sy7vMtW9neZU4x-Z=iEQgjqu4X5tFKyhw@mail.gmail.com>

On Thu, Dec 20, 2012 at 08:12:08PM +0100, Linus Walleij wrote:
> On Wed, Dec 19, 2012 at 6:19 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > Some platforms provide clocks which require enabling before the
> > SMSC911x chip will power on. This patch uses the new common clk
> > framework to do just that. If no clock is provided, it will just
> > be ignored and the driver will continue to assume that no clock
> > is required for the chip to run successfully.
> >
> > Cc: Steve Glendinning <steve.glendinning@shawell.net>
> > Cc: netdev@vger.kernel.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Seems to me like it'll do the trick.
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

This looks fairly dangerous.  What about those platforms which use this
driver, but don't provide a clock for it?

It looks like this will result in those platforms losing their ethernet
support.  There's at least a bunch of the ARM evaluation boards which
make use of this driver...

^ permalink raw reply

* NAPI documentation needed
From: Rafał Miłecki @ 2012-12-20 19:39 UTC (permalink / raw)
  To: netdev; +Cc: David S. Miller

I wanted to report some problem I've encouraged during bgmac driver development.

At the very beginning I've implemented IRQ using threaded IRQ
(request_threaded_irq). I didn't know about NAPI until someone pointed
me that mistake. So I decided to rewrite IRQs handling to use NAPI.
I've found following documents:
http://www.linuxfoundation.org/collaborate/workgroups/networking/napi
ftp://robur.slu.se/pub/Linux/net-development/NAPI/README
ftp://robur.slu.se/pub/Linux/net-development/NAPI/NAPI_HOWTO.txt
ftp://robur.slu.se/pub/Linux/net-development/NAPI/converting-to-NAPI.txt~
but nothing really official sitting in kernel's Documentation dir.

So I started to using found documents, but then noticed they are quite outdated.

1) We don't have netif_rx_schedule and netif_rx_complete anymore.
2) We don't set poll and weight manually anymore but use netif_napi_add
3) Return type and arguments has changed in poll. None of the
following is up-to-date:
static void my_poll (struct net_device *dev, int *budget)
int (*poll)(struct net_device *dev, int *budget);

It would be great if someone with NAPI knowledge could document it in
a kernel. Would be really helpful for new network drivers developers.

-- 
Rafał

^ permalink raw reply

* Re: NAPI documentation needed
From: Eric Dumazet @ 2012-12-20 20:04 UTC (permalink / raw)
  To: Rafał Miłecki; +Cc: netdev, David S. Miller
In-Reply-To: <CACna6rzG3KcMYUpMZv-wv6e62J9j1NLuZpTRSQHLpyfTTbKyUg@mail.gmail.com>

On Thu, 2012-12-20 at 20:39 +0100, Rafał Miłecki wrote:
> I wanted to report some problem I've encouraged during bgmac driver development.
> 
> At the very beginning I've implemented IRQ using threaded IRQ
> (request_threaded_irq). I didn't know about NAPI until someone pointed
> me that mistake. So I decided to rewrite IRQs handling to use NAPI.
> I've found following documents:
> http://www.linuxfoundation.org/collaborate/workgroups/networking/napi
> ftp://robur.slu.se/pub/Linux/net-development/NAPI/README
> ftp://robur.slu.se/pub/Linux/net-development/NAPI/NAPI_HOWTO.txt
> ftp://robur.slu.se/pub/Linux/net-development/NAPI/converting-to-NAPI.txt~
> but nothing really official sitting in kernel's Documentation dir.
> 
> So I started to using found documents, but then noticed they are quite outdated.
> 
> 1) We don't have netif_rx_schedule and netif_rx_complete anymore.
> 2) We don't set poll and weight manually anymore but use netif_napi_add
> 3) Return type and arguments has changed in poll. None of the
> following is up-to-date:
> static void my_poll (struct net_device *dev, int *budget)
> int (*poll)(struct net_device *dev, int *budget);
> 
> It would be great if someone with NAPI knowledge could document it in
> a kernel. Would be really helpful for new network drivers developers.

I think you might be the one to update/create the official NAPI
documentation, now ideas are clear for you.

That would be really great indeed.

^ permalink raw reply

* Re: NAPI documentation needed
From: Ben Hutchings @ 2012-12-20 20:09 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Rafał Miłecki, netdev, David S. Miller
In-Reply-To: <1356033878.21834.3163.camel@edumazet-glaptop>

On Thu, 2012-12-20 at 12:04 -0800, Eric Dumazet wrote:
> On Thu, 2012-12-20 at 20:39 +0100, Rafał Miłecki wrote:
> > I wanted to report some problem I've encouraged during bgmac driver development.
> > 
> > At the very beginning I've implemented IRQ using threaded IRQ
> > (request_threaded_irq). I didn't know about NAPI until someone pointed
> > me that mistake. So I decided to rewrite IRQs handling to use NAPI.
> > I've found following documents:
> > http://www.linuxfoundation.org/collaborate/workgroups/networking/napi
> > ftp://robur.slu.se/pub/Linux/net-development/NAPI/README
> > ftp://robur.slu.se/pub/Linux/net-development/NAPI/NAPI_HOWTO.txt
> > ftp://robur.slu.se/pub/Linux/net-development/NAPI/converting-to-NAPI.txt~
> > but nothing really official sitting in kernel's Documentation dir.
> > 
> > So I started to using found documents, but then noticed they are quite outdated.
> > 
> > 1) We don't have netif_rx_schedule and netif_rx_complete anymore.
> > 2) We don't set poll and weight manually anymore but use netif_napi_add
> > 3) Return type and arguments has changed in poll. None of the
> > following is up-to-date:
> > static void my_poll (struct net_device *dev, int *budget)
> > int (*poll)(struct net_device *dev, int *budget);
> > 
> > It would be great if someone with NAPI knowledge could document it in
> > a kernel. Would be really helpful for new network drivers developers.
> 
> I think you might be the one to update/create the official NAPI
> documentation, now ideas are clear for you.
> 
> That would be really great indeed.

It would.  Last time someone asked, my answer was:

> The initial change to napi_struct is explained in
> <http://lwn.net/Articles/244640/>.
> 
> Since then there have been further changes:
> 
> - netif_napi_del() has been added.  You must call it to clean up NAPI
> contexts before freeing the associated net device(s).
> 
> - Instead of netif_rx_schedule(), netif_rx_complete(), etc. you must use
> napi_schedule(), napi_complete() etc. which just take a napi_struct
> pointer.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply

* RE: [PATCH] wireless: mwifiex: remove unreachable paths
From: Bing Zhao @ 2012-12-20 20:13 UTC (permalink / raw)
  To: Sasha Levin
  Cc: John W. Linville, linux-wireless@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1356030701-16284-17-git-send-email-sasha.levin@oracle.com>

Hi Sasha,

Thanks for the patch.

> We know 'firmware' is non-NULL from the beginning of mwifiex_prog_fw_w_helper,
> remove all !firmware paths from the rest of the function.

After removing all !firmware paths the function 'mwifiex_get_fw_data' becomes an orphan.

Could you please remove that function as well and resend a v2 patch?

Thanks,
Bing

> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>  drivers/net/wireless/mwifiex/usb.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/usb.c b/drivers/net/wireless/mwifiex/usb.c
> index 63ac9f2..8bd7098 100644
> --- a/drivers/net/wireless/mwifiex/usb.c
> +++ b/drivers/net/wireless/mwifiex/usb.c
> @@ -836,23 +836,14 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter,
>  			dlen = 0;
>  		} else {
>  			/* copy the header of the fw_data to get the length */
> -			if (firmware)
> -				memcpy(&fwdata->fw_hdr, &firmware[tlen],
> -				       sizeof(struct fw_header));
> -			else
> -				mwifiex_get_fw_data(adapter, tlen,
> -						    sizeof(struct fw_header),
> -						    (u8 *)&fwdata->fw_hdr);
> +			memcpy(&fwdata->fw_hdr, &firmware[tlen],
> +			       sizeof(struct fw_header));
> 
>  			dlen = le32_to_cpu(fwdata->fw_hdr.data_len);
>  			dnld_cmd = le32_to_cpu(fwdata->fw_hdr.dnld_cmd);
>  			tlen += sizeof(struct fw_header);
> 
> -			if (firmware)
> -				memcpy(fwdata->data, &firmware[tlen], dlen);
> -			else
> -				mwifiex_get_fw_data(adapter, tlen, dlen,
> -						    (u8 *)fwdata->data);
> +			memcpy(fwdata->data, &firmware[tlen], dlen);
> 
>  			fwdata->seq_num = cpu_to_le32(fw_seqnum);
>  			tlen += dlen;
> --
> 1.8.0

^ permalink raw reply

* Re: [PATCH net-next V4 03/13] bridge: Validate that vlan is permitted on ingress
From: Shmulik Ladkani @ 2012-12-20 20:13 UTC (permalink / raw)
  To: vyasevic; +Cc: netdev, shemminger, davem, or.gerlitz, jhs, mst, erdnetdev, jiri
In-Reply-To: <50D342B7.9010901@redhat.com>

Hi Vlad,

On Thu, 20 Dec 2012 11:54:15 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote:
> > (2) frame ingress on a "non-vlan" port may egress on a "vlan enabled"
> >    port, depending on the ingress VID and the port-membership map of the
> >    egress port
> >    (and thus, PVID should be defined even to "non-vlan" ports, for the
> >    case where untagged frame is received on the non-vlan port)
> 
> Sort of.  The way I did it (testing now), is like this:
>     if there is egress policy
> 	apply policy and forward.
>     else if there was ingress policy (pvid)
> 	apply it and forward
>     else
> 	forward as is (old bridge behavior).
> 
> This way if there was a pvid on an ingress port and nothing on egress,
> then pvid applies.  If there was nothing configured on ingress port,
> but we have a egress policy, we'll apply any vlan information from
> the frame to egress policy.  In this case, ingress untagged traffic 
> would be assigned vlan 0.

Sorry, got too cryptic too me ;)
We're probably aligned, but if you don't mind I'd like to make sure I
got it right.

I'd expect the following logic if the bridge is a vlan bridge:

1. Frame ingress on a port
  Frame's VID is collected: if frame was tagged, its the VID found in
  the tag; if frame was untagged (or prio-tagged), the VID would be
  port's PVID.
2. Ingress membership verification
  Verify the ingress port is a member of the frame's VID vlan (collected
  on step 1).
  (Usually policy is 'drop' in case port is not a member).
  Easily calculated by testing if the port bit is set in vlan's port
  membership map.
3. Switching logic
  Consult FDB for a forward/flood/drop decision, resulting in a map of
  candidate ports the frame might egress upon (e.g. in the common case,
  a valid existing unicast entry, the result is just one candidate
  port).
4. Egress membership verification
  For each candidate port found on step 3, verify it is a member of the
  frame's VID vlan.
  (Usually, candidate ports that aren't members of the vlan will not be
  selected for actual egress).
  This can be easily calculated by masking the candidates port map
  (found on step 3) with the vlan's port membership map. The resulting
  masked map is final egress portmap.
5. Frame tag construction and egress 
  For each final egress port (found on step 4), verify its
  tagged/untagged policy in the vlan's egress-policy map.
  Properly add/remove the vlan tag (if needed) according to port's
  egress policy, and transmit.

To my best understanding, if all the ports are "vlan-enabled" (having a
non-empty vid list, i.e. belonging to at least one vlan), the behavior
of the implemented bridge is aligned with the above scheme.

For "non-vlan" ports (having en empty vid list), we treat them as if
they belong to ANY POSSIBLE vlan (as if their bit is always set in every
vlan port membeship map). Meaning, in step (2) verification always
suceeds for such ports, and in step (4) such ports will never be masked
out of the egress candidates portmap.

Please let me know if the implementation fits this.

> I'll try to document things sufficiently.  This hybrid approach may
> produce some unintended results.  We could always remove it or introduce
> the tunable to change default policy to drop once vlan configuration is
> in effect.

Ok.

Regards,
Shmulik

^ permalink raw reply

* Re: [PATCH 4/4] net/smsc911x: Provide common clock functionality
From: Lee Jones @ 2012-12-20 20:35 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Linus Walleij, Steve Glendinning, Robert Marklund, linus.walleij,
	arnd, netdev, linux-kernel, linux-arm-kernel
In-Reply-To: <20121220192441.GC14363@n2100.arm.linux.org.uk>

On Thu, 20 Dec 2012, Russell King - ARM Linux wrote:

> On Thu, Dec 20, 2012 at 08:12:08PM +0100, Linus Walleij wrote:
> > On Wed, Dec 19, 2012 at 6:19 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > 
> > > Some platforms provide clocks which require enabling before the
> > > SMSC911x chip will power on. This patch uses the new common clk
> > > framework to do just that. If no clock is provided, it will just
> > > be ignored and the driver will continue to assume that no clock
> > > is required for the chip to run successfully.
> > >
> > > Cc: Steve Glendinning <steve.glendinning@shawell.net>
> > > Cc: netdev@vger.kernel.org
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > 
> > Seems to me like it'll do the trick.
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> This looks fairly dangerous.  What about those platforms which use this
> driver, but don't provide a clock for it?
> 
> It looks like this will result in those platforms losing their ethernet
> support.  There's at least a bunch of the ARM evaluation boards which
> make use of this driver...

Right, but nothing should regress. If no clock is provided the driver
moves on during the request and will refuse to prepare, enable and
disable there after. 

Unless I've made a mistake somewhere? If so, I'd be happy to fixup.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply

* bonding driver - how to recognize the active slave
From: Erez Shitrit @ 2012-12-20 20:38 UTC (permalink / raw)
  To: netdev@vger.kernel.org

Hi,
Is there any way to know who is the active slave in active-passive mode?

Is there any indication for that in netdevice flags? Or some other way?
(I would like to get in my network interface driver which can be a slave of the bonding interface)

I saw that there is the flag IFF_SLAVE_INACTIVE but it is not used as far as I can see.

Thanks, Erez

^ permalink raw reply

* Re: [PATCH 4/4] net/smsc911x: Provide common clock functionality
From: Russell King - ARM Linux @ 2012-12-20 20:51 UTC (permalink / raw)
  To: Lee Jones
  Cc: Linus Walleij, Steve Glendinning, Robert Marklund, linus.walleij,
	arnd, netdev, linux-kernel, linux-arm-kernel
In-Reply-To: <20121220203514.GN2691@gmail.com>

On Thu, Dec 20, 2012 at 08:35:14PM +0000, Lee Jones wrote:
> On Thu, 20 Dec 2012, Russell King - ARM Linux wrote:
> 
> > On Thu, Dec 20, 2012 at 08:12:08PM +0100, Linus Walleij wrote:
> > > On Wed, Dec 19, 2012 at 6:19 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > > 
> > > > Some platforms provide clocks which require enabling before the
> > > > SMSC911x chip will power on. This patch uses the new common clk
> > > > framework to do just that. If no clock is provided, it will just
> > > > be ignored and the driver will continue to assume that no clock
> > > > is required for the chip to run successfully.
> > > >
> > > > Cc: Steve Glendinning <steve.glendinning@shawell.net>
> > > > Cc: netdev@vger.kernel.org
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > 
> > > Seems to me like it'll do the trick.
> > > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > 
> > This looks fairly dangerous.  What about those platforms which use this
> > driver, but don't provide a clock for it?
> > 
> > It looks like this will result in those platforms losing their ethernet
> > support.  There's at least a bunch of the ARM evaluation boards which
> > make use of this driver...
> 
> Right, but nothing should regress. If no clock is provided the driver
> moves on during the request and will refuse to prepare, enable and
> disable there after. 
> 
> Unless I've made a mistake somewhere? If so, I'd be happy to fixup.

No, but... don't use NULL for that.  Use IS_ERR(pdata->clk) instead.

^ permalink raw reply

* Re: bonding driver - how to recognize the active slave
From: Eric Dumazet @ 2012-12-20 21:19 UTC (permalink / raw)
  To: Erez Shitrit; +Cc: netdev@vger.kernel.org
In-Reply-To: <BA0B6857ABCA474FB07F0DA6F50A3077918B2450@MTRDAG01.mtl.com>

On Thu, 2012-12-20 at 20:38 +0000, Erez Shitrit wrote:
> Hi,
> Is there any way to know who is the active slave in active-passive mode?
> 
> Is there any indication for that in netdevice flags? Or some other way?
> (I would like to get in my network interface driver which can be a slave of the bonding interface)
> 
> I saw that there is the flag IFF_SLAVE_INACTIVE but it is not used as far as I can see.


cat /sys/class/net/bond0/bonding/active_slave

^ 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