* Re: [PATCH net-next] tcp: avoid expensive pskb_expand_head() calls
From: Eric Dumazet @ 2012-04-18 19:18 UTC (permalink / raw)
To: Neal Cardwell
Cc: David Miller, netdev, Tom Herbert, Maciej Żenczykowski,
Yuchung Cheng
In-Reply-To: <CADVnQy=BkhSBHyN2hyBy=_H64oM8sJvyZZfEjK1x7PYzoLv=5w@mail.gmail.com>
On Wed, 2012-04-18 at 14:40 -0400, Neal Cardwell wrote:
>
> > + prev_packets_acked = tcp_skb_pcount(skb);
>
> FWIW, I'd find old_pcount or prev_pcount a little easier to read than
> prev_packets_acked here (I see "oldpcount" is used in tcp_output
> in a similar context).
>
You're right, I'll change this.
> > TCP_SKB_CB(skb)->seq += len;
> > + TCP_SKB_CB(skb)->header.offset_ack = 0;
>
> If the caller decides to trim a prefix of the skb that does not extend
> to snd_una, then setting offset_ack to 0 here will cause us to forget
> that some prefix is ACKed when we should have remembered this.
> However, the API for the function invites the caller to chop off an
> arbitrary amount (and there's no comment to disuade the caller from
> trying this). This seems to risk bugs in the future.
>
> To attempt to make this API safer and simpler for future generations,
> what do you think about calculating the len inside tcp_trim_head(),
> something like:
>
> static int tcp_trim_head(struct sock *sk, struct sk_buff *skb)
> {
> u32 len = tp->snd_una - TCP_SKB_CB(skb)->seq;
> ...
>
> ...
> if (tcp_trim_head(sk, skb))
> ...
>
Very good point, I'll use this suggestion too in v2.
Thanks !
^ permalink raw reply
* Re: [net-next PATCH v3] bonding: start slaves with link down for ARP monitor
From: Flavio Leitner @ 2012-04-18 19:19 UTC (permalink / raw)
To: Michal Kubecek; +Cc: netdev, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <20120417121135.5F9A740290@alaris.suse.cz>
On Tue, 17 Apr 2012 14:02:06 +0200
Michal Kubecek <mkubecek@suse.cz> wrote:
> Initialize slave device link state as down if ARP monitor is
> active and net_carrier_ok() returns zero. Also shift initial
> value of its last_arp_tx so that it doesn't immediately cause
> fake detection of "up" state.
>
> When ARP monitoring is used, initializing the slave device with
> up link state can cause ARP monitor to detect link failure
> before the device is really up (with igb driver, this can take
> more than two seconds).
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
Signed-off-by: Flavio Leitner <fbl@redhat.com>
fbl
^ permalink raw reply
* Re: [PATCH net-next] bonding,vlan: propagate MAC failover changes to VLANs
From: Ben Hutchings @ 2012-04-18 19:20 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev, David S. Miller, Patrick McHardy, Andy Gospodarek
In-Reply-To: <11643.1334774976@death.nxdomain>
On Wed, 2012-04-18 at 11:49 -0700, Jay Vosburgh wrote:
> Ben Hutchings <bhutchings@solarflare.com> wrote:
>
> >On Wed, 2012-04-18 at 11:02 -0700, Jay Vosburgh wrote:
> >> With bonding's fail_over_mac=active, during failover the MAC
> >> address of the bond itself changes to match that of the slave.
> >>
> >> This patch adds a notifier call to cause VLANs stacked atop the
> >> bonding to also change their MAC addresses to the new address when a
> >> failover occurs.
> >>
> >> While it is legal for a VLAN to have a MAC address that differs
> >> from the underlying device, at least one device (qeth) that requires the
> >> use of fail_over_mac for bonding cannot handle the VLAN's MAC differing
> >> from that of the bond; thus, it needs the MAC change to propagate up
> >> to any VLANs when fail_over_mac is set to active.
> >[...]
> >
> >This doesn't make sense to me. You're applying the behaviour to all
> >VLANs on top of a bond, whether or not the underlying device is driven
> >by qeth, and ignoring any MAC address changes that don't involve the
> >bonding driver.
>
> With the patch, the PROPAGATE event is only generated if bonding
> is set for fail_over_mac=active, which is normally only enabled on those
> devices that require it (some devices for IBM's pseries and zseries
> architectures and Infiniband, which doesn't have VLANs).
Yeah, OK, that makes sense.
> Devices that do not use bonding's fail_over_mac will not have
> VLANs following MAC changes.
I take it that the devices with this limitation on source MAC address
have an essentially unchangeable MAC address? If they are limited to
single address but it's changeable then they should be emitting this
notification too.
> >I think either of these would be better fixes:
> >1. Make VLAN devices follow changes to the parent device's MAC address
> >unless they are assigned an address of their own.
> >2. Add a configuration flag for VLAN devices to follow changes to the
> >parent device's MAC address.
>
> #1 would be a behavior change for all VLAN devices, which I
> sought to avoid.
>
> #2 would be an additional configuration option that would have
> to be enabled just for this case (unless VLANs following MAC changes of
> the parent device is a generally desirable feature).
I don't know whether it is generally desirable. My guess would be that
unless a VLAN device is explicitly configured to use its own address
then it is desirable.
> The patch requires
> no additional option settings beyond what are currently in use.
Right, I understand that this ought to Just Work, if possible.
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] Fix handling of overlength pathname in AF_UNIX sun_path
From: David Miller @ 2012-04-18 19:23 UTC (permalink / raw)
To: carlos
Cc: mtk.manpages, netdev, penguin-kernel, linux-api, yoshfuji,
jengelh, w, alan
In-Reply-To: <CADZpyizwozRjYOvOads0KkqRr2quTo532TC=Ff=cw0sGc3OCQg@mail.gmail.com>
From: "Carlos O'Donell" <carlos@systemhalted.org>
Date: Wed, 18 Apr 2012 14:48:43 -0400
> POSIX doesn't exist in a vacuum, we need to harmonize reality with the
> standard. If an implementation exists where sun_path has no
> null-terminator then it is useful to have POSIX clarify that
> null-termination is implementation defined behaviour, just like it
> says that sun_path's length undefined. Under "Application Usage" or
> "Examples" it's valid to talk about specific implementations.
>
> See: http://pubs.opengroup.org/onlinepubs/007904975/basedefs/sys/un.h.html,
> where it talks about BSD in the "Application Usage." It's about time
> we some "Linux this" and "Linux that" in there.
Ok, thanks for explaining.
^ permalink raw reply
* RE: [PATCH 00/14] usb/net: rndis: first step toward consolidation
From: Haiyang Zhang @ 2012-04-18 19:30 UTC (permalink / raw)
To: Jussi Kivilinna
Cc: Linus Walleij, netdev@vger.kernel.org, linux-usb@vger.kernel.org,
Greg Kroah-Hartman, David S. Miller, Felipe Balbi, Wei Yongjun,
Ben Hutchings
In-Reply-To: <20120418211451.20071p46rovwu5q8@www.81.fi>
> -----Original Message-----
> From: Jussi Kivilinna [mailto:jussi.kivilinna@mbnet.fi]
> Sent: Wednesday, April 18, 2012 2:15 PM
> To: Haiyang Zhang
> Cc: Linus Walleij; netdev@vger.kernel.org; linux-usb@vger.kernel.org; Greg
> Kroah-Hartman; David S. Miller; Felipe Balbi; Wei Yongjun; Ben Hutchings
> Subject: RE: [PATCH 00/14] usb/net: rndis: first step toward consolidation
>
> Quoting Haiyang Zhang <haiyangz@microsoft.com>:
>
> >> -----Original Message-----
> >> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> >> Sent: Wednesday, April 18, 2012 8:26 AM
> >> To: Haiyang Zhang
> >> Cc: netdev@vger.kernel.org; linux-usb@vger.kernel.org; Greg Kroah-
> >> Hartman; David S. Miller; Felipe Balbi; Jussi Kivilinna; Wei Yongjun;
> >> Ben Hutchings
> >> Subject: Re: [PATCH 00/14] usb/net: rndis: first step toward
> >> consolidation
> >>
> >> On Sun, Apr 8, 2012 at 3:58 PM, Haiyang Zhang
> >> <haiyangz@microsoft.com>
> >> wrote:
> >> > _______________________________________
> >> >> From: Linus Walleij [linus.walleij@linaro.org]
> >> >> Sent: Sunday, April 08, 2012 4:46 AM
> >> >> To: netdev@vger.kernel.org; linux-usb@vger.kernel.org; Greg
> >> >> Kroah-Hartman; David S. Miller; Felipe Balbi
> >> >> Cc: Jussi Kivilinna; Haiyang Zhang; Wei Yongjun; Ben Hutchings;
> >> >> Linus Walleij
> >> >> Subject: [PATCH 00/14] usb/net: rndis: first step toward
> >> >> consolidation
> >> >
> >> >> So if you like this, let's proceed, but if you're all in "I wanna
> >> >> keep this one implementation in my corner" I'll just give up right now.
> >> >
> >> >> So: what do you think?
> >> >
> >> > I will test this soon.
> >>
> >> Ping on this... any testing so far?
> >
> > I saw Jussi Kivilinna <jussi.kivilinna@mbnet.fi>'s comments saying it
> > does not work. And your patches are in "changes requested" state. So
> > I'm waiting for your updated patches before testing.
> >
>
> Maybe I should clarify that at least rndis_wlan does not work because of that
> single patch (09/14). By fixing the problem, the patch-set works with
> rndis_wlan fine.
>
I will test after you send out the updated patch set.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove
From: Jonas Gorski @ 2012-04-18 19:30 UTC (permalink / raw)
To: mbizon; +Cc: netdev, Florian Fainelli, Eric Dumazet, David S. Miller
In-Reply-To: <1334753296.5185.3.camel@sakura.staff.proxad.net>
Hi Maxime,
On 18 April 2012 14:48, Maxime Bizon <mbizon@freebox.fr> wrote:
>
> On Wed, 2012-04-18 at 14:02 +0200, Jonas Gorski wrote:
>
>> Only connect/disconnect the phy during probe and remove, not during open
>> and close. The phy seldom changes during the runtime, and disconnecting
>> the phy during close will prevent the phy driver from keeping any
>> configuration over a down/up cycle.
>>
>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>
> please CC me, I wrote this driver
Oops, sorry, that wasn't intentional. I used get_maintainer.pl, which
didn't catch you, and I never checked the actual file. Duly noted for
v2.
>
>> - phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0,
>> - PHY_INTERFACE_MODE_MII);
>
> bcm_enet_adjust_link() may modify some dma registers that are reset by
> bcm_enet_open(), since it can now be called after probe, we may end up
> with broken flow control depending on whatever was called first.
I assume you mean bcm_enet_adjust_phy_link()? bcm_enet_adjust_link()
gets only called if there is no phydev.
I have to admit, I fail to see the race (but feel free to correct me):
On boot:
The phy state machine will start in PHY_READY after phy_connect, which
will result in NOPs, so no call to bcm_enet_adjust_phy_link() after
_probe() and before _open(). The state machine starts doing real work
only after calling phy_start(), which happens only after the dma
register accesses in open(). So no race here.
In case of an down/up cycle:
_close() will call phy_stop() first, which will either block until the
current state machine run is complete if there is one, or will block
the next run until it set the state to PHY_HALTED.
When in PHY_HALTED, bcm_enet_adjust_phy_link() will be called once,
but with phy_dev->link = 0, and only phy_dev->link = 1 can result in
register writes in bcm_enet_adjust_phy_link():
if (phydev->link && phydev->duplex != priv->old_duplex) {
bcm_enet_set_duplex(priv,
(phydev->duplex == DUPLEX_FULL) ? 1 : 0);
...
}
if (phydev->link && phydev->pause != priv->old_pause) {
...
bcm_enet_set_flow(priv, rx_pause_en, tx_pause_en);
...
}
So no problem there either.
There is a theoretical race in writing to priv->old_link in _open()
and the bcm_enet_adjust_phy_link() call right after phy_stop(), but
since both write the same value to priv->old_link I see no problem
here.
Regards
Jonas
^ permalink raw reply
* Re: pull request: wireless 2012-04-18
From: David Miller @ 2012-04-18 19:31 UTC (permalink / raw)
To: linville-2XuSBdqkA4R54TAoqtyWWQ
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20120418184320.GA2436-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
From: "John W. Linville" <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Date: Wed, 18 Apr 2012 14:43:20 -0400
> First up, we have a minor signedness fix for libertas from Amitkumar
> Karwar. Next, Arend gives us a brcm80211 fix for correctly enabling
> Tx FIFOs on channels 12 and 13. Bing Zhao gives us some register
> address corrections for mwifiex. Felix give us a trio of fixes --
> one for ath9k to wake the hardware properly from full sleep, one for
> mac80211 to properly handle packets in cooked monitor mode, and one
> for ensuring that the proper HT mode selection is honored.
>
> Hauke gives us a bcma fix for handling the lack of an sprom. Jonathon
> Bither gives us an ath5k fix for a missing THIS_MODULE build issue,
> and another ath5k fix for an io mapping leak. Lukasz Kucharczyk
> fixes a bitwise check in cfg80211, and Sujith gives us an ath9k fix
> for assigning sequence numbers for fragmented frames. Finally, we
> have a MAINTAINERS change from Wey-Yi Guy -- congrats to Johannes
> Berg for taking the lead on iwlwifi. :-)
:-) Pulled, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] macvlan/macvtap: Fix vlan tagging on user read
From: Basil Gor @ 2012-04-18 19:33 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: netdev, David S. Miller
In-Reply-To: <m1hawglvk3.fsf@fess.ebiederm.org>
On Wed, Apr 18, 2012 at 11:54:52AM -0700, Eric W. Biederman wrote:
> Basil Gor <basil.gor@gmail.com> writes:
>
> > Vlan tag is restored during buffer transmit to a network device (bridge
> > port) in bridging code in case of tun/tap driver. In case of macvtap it
> > has to be done explicitly. Otherwise vlan_tci is ignored and user always
> > gets untagged packets.
> >
> > Scenario tested:
> > kvm guests (that use vlans) migration from bridged network to macvtap
> > revealed that packets delivered to guests are always untagged. Dumping
> > and comparing sk_buff in case of tap and macvtap driver showed that
> > macvtap does not restore vlan_tci.
> >
> > With current patch applied I was able to get working network, kvm guests
> > get correctly tagged packets and can reach each other when macvtap in
> > bridge mode (both with no vlans and through vlan interfaces).
>
> My first impression is that this is the wrong place to add a vlan
> header back.
>
> You need to keep the vlan information in vlan_tci until just
> before the packet is delivered to userspace. Which would suggest
> the best place for these games is macvtap_put_user.
>
> Elsewhere vlan headers should not be explicitly stored in the packet.
>
> At least that was the rule last I looked.
>
> Eric
>
>
This sounds right, and macvtap_put_user was the first place where I
put vlan header adding. But qemu-kvm does smth like get pending data
size and then read, and when I put code in macvtap_put_user qemu
supplied buffer 4 bytes smaller then needed and packets were
truncated. On the other hand tun/tap driver never keeps vlan info in
vlan_tci because you can't do any vlan operations on it I think. So I
decided to restore vlan header just before adding it to macvtap queue.
But I'll try to look deeper in it.
Thanks
> > Signed-off-by: Basil Gor <basilgor@gmail.com>
> > ---
> > drivers/net/macvtap.c | 9 +++++++++
> > 1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> > index 0427c65..a6802b9 100644
> > --- a/drivers/net/macvtap.c
> > +++ b/drivers/net/macvtap.c
> > @@ -1,6 +1,7 @@
> > #include <linux/etherdevice.h>
> > #include <linux/if_macvlan.h>
> > #include <linux/interrupt.h>
> > +#include <linux/if_vlan.h>
> > #include <linux/nsproxy.h>
> > #include <linux/compat.h>
> > #include <linux/if_tun.h>
> > @@ -254,6 +255,14 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
> > if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len)
> > goto drop;
> >
> > + if (vlan_tx_tag_present(skb)) {
> > + skb = __vlan_put_tag(skb, vlan_tx_tag_get(skb));
> > + if (unlikely(!skb))
> > + return NET_RX_DROP;
> > +
> > + skb->vlan_tci = 0;
> > + }
> > +
> > skb_queue_tail(&q->sk.sk_receive_queue, skb);
> > wake_up_interruptible_poll(sk_sleep(&q->sk), POLLIN | POLLRDNORM | POLLRDBAND);
> > return NET_RX_SUCCESS;
^ permalink raw reply
* Re: [PATCH 00/12] atl1c: update hardware settings - v1
From: David Miller @ 2012-04-18 19:37 UTC (permalink / raw)
To: xiong; +Cc: netdev, linux-kernel, qca-linux-team, nic-devel
In-Reply-To: <1334727156-6830-1-git-send-email-xiong@qca.qualcomm.com>
From: xiong <xiong@qca.qualcomm.com>
Date: Wed, 18 Apr 2012 13:32:24 +0800
> This update contains a serial of patches, most of them are hardware
> settings related. register definitions are refined (or removed if
> meaningless) for each patch.
>
> This is the first serial patches, after reviewed/applied the second
> serial will be out.
>
> These patches have addressed all sparse and checkpatch warnings.
>
> Following NICs are tested:
> AR8131/AR8132/AR8151A/AR8152A/AR8152B
> Test item includes:
> build/install/uninstall/dhcp/ping/iperf/wol/reboot/etc.
All applied, thanks.
^ permalink raw reply
* Re: pull request: wireless-next 2012-04-18
From: David Miller @ 2012-04-18 19:36 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev
In-Reply-To: <20120418190521.GB2436@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Wed, 18 Apr 2012 15:05:22 -0400
> Another batch of wireless updates intended for 3.5...
>
> Highlights include some wl12xx refactoring, NFC HCI and SHDLC updates,
> an ath6kl pull, some mesh updates, and the usual updates to iwlwifi,
> ath9k, brcm80211, mwifiex, libertas, and other drivers.
>
> Please let me know if there are problems!
Also pulled, thanks a lot.
^ permalink raw reply
* Re: [PATCH net-next] tcp: avoid expensive pskb_expand_head() calls
From: Vijay Subramanian @ 2012-04-18 19:41 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Tom Herbert, Neal Cardwell,
Maciej Żenczykowski, Yuchung Cheng
In-Reply-To: <1334764184.2472.299.camel@edumazet-glaptop>
> {
> if (skb_cloned(skb) && pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
> return -ENOMEM;
> @@ -1134,6 +1138,7 @@ int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
> __pskb_trim_head(skb, len);
>
> TCP_SKB_CB(skb)->seq += len;
> + TCP_SKB_CB(skb)->header.offset_ack = 0;
> skb->ip_summed = CHECKSUM_PARTIAL;
>
> skb->truesize -= len;
>
Eric,
tcp_trim_head() also updates skb->truesize as above. But is this the
right thing to do when only offsets/pointers are updated. If
__pskb_trim_head() removes len bytes but does not actually free
memory,
should truesize be updated? This could happen if data in the linear
part is acked. This behavior takes place currently even without your
patch, by the way.
Thanks,
Vijay
^ permalink raw reply
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path
From: Alan Cox @ 2012-04-18 19:44 UTC (permalink / raw)
To: Willy Tarreau
Cc: Michael Kerrisk, netdev, Tetsuo Handa, linux-api, yoshfuji,
David Miller, Jan Engelhardt
In-Reply-To: <20120417105107.GA8614@1wt.eu>
I think its pointless.
Yes there may have been a case years ago to nul terminate but thats now
how 4BSD defined the API so that's how the world looks.
It helps nobody on the app side because they'll be defensively coding for
the existing API for another ten years for enterprise distros anyway.
Alan
^ permalink raw reply
* Re: [PATCH net-next] tcp: avoid expensive pskb_expand_head() calls
From: Eric Dumazet @ 2012-04-18 19:49 UTC (permalink / raw)
To: Vijay Subramanian
Cc: David Miller, netdev, Tom Herbert, Neal Cardwell,
Maciej Żenczykowski, Yuchung Cheng
In-Reply-To: <CAGK4HS-jrsy2kxJUeYo+JR9CBfOFqyzBgqa+OzC85yQbaWmFoA@mail.gmail.com>
On Wed, 2012-04-18 at 12:41 -0700, Vijay Subramanian wrote:
> > {
> > if (skb_cloned(skb) && pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
> > return -ENOMEM;
> > @@ -1134,6 +1138,7 @@ int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
> > __pskb_trim_head(skb, len);
> >
> > TCP_SKB_CB(skb)->seq += len;
> > + TCP_SKB_CB(skb)->header.offset_ack = 0;
> > skb->ip_summed = CHECKSUM_PARTIAL;
> >
> > skb->truesize -= len;
> >
>
> Eric,
> tcp_trim_head() also updates skb->truesize as above. But is this the
> right thing to do when only offsets/pointers are updated. If
> __pskb_trim_head() removes len bytes but does not actually free
> memory,
> should truesize be updated? This could happen if data in the linear
> part is acked. This behavior takes place currently even without your
> patch, by the way.
>
Thats ok.
__pskb_trim_head() callers are responsible for taking care of truesize
changes.
By the way, its always complex to guess truesize changes. removing some
bytes in skb head doesnt free memory, so in fact we should not change
truesize in this case.
But only one skb in tcp write queue might have a slightly wrong
truesize, its not a big deal.
Problem with skb->truesize is mostly in input path, when first
skb->truesize is not correctly set by drivers, upper stack can
accumulate errors in socket receive queue.
^ permalink raw reply
* [PATCH v2 net-next] tcp: avoid expensive pskb_expand_head() calls
From: Eric Dumazet @ 2012-04-18 19:51 UTC (permalink / raw)
To: Neal Cardwell
Cc: David Miller, netdev, Tom Herbert, Maciej Żenczykowski,
Yuchung Cheng
In-Reply-To: <1334776707.2472.316.camel@edumazet-glaptop>
From: Eric Dumazet <edumazet@google.com>
While doing netperf sessions on 10Gb Intel nics (ixgbe), I noticed
unexpected profiling results, with pskb_expand_head() being in the top.
After further analysis, I found we hit badly page refcounts,
because when we transmit full size skb (64 KB), we can receive ACK for
the first segments of the frame while skb was not completely sent by
NIC.
It takes ~54 us to send a full TSO packet at 10Gb speed, but with a
close peer, we can receive TCP ACK in less than 50 us rtt.
This is also true on 1Gb links but we were limited by wire speed, not
cpu.
When we try to trim skb, tcp_trim_head() has to call pskb_expand_head(),
because the skb clone we did for transmit is still alive in TX ring
buffer.
pskb_expand_head() is really expensive : It has to make about 16+16
atomic operations on page refcounts, not counting the skb head
reallocation/copy. It increases chances of false sharing.
In fact, we dont really need to trim skb. This costly operation can be
delayed to the point it is really needed : Thats when a retransmit must
happen.
Most of the time, upcoming ACKS will ack the whole packet, and we can
free it with minimal cost (since clone was already freed by TX
completion)
Of course, this means we dont uncharge the acked part from socket limits
until retransmit, but this is hardly a concern with current autotuning
(around 4MB per socket)
Even with small cwnd limit, a single packet can not hold more than half
the window.
Performance results on my Q6600 cpu and 82599EB 10-Gigabit card :
About 3% less cpu used for same workload (single netperf TCP_STREAM),
bounded by x4 PCI-e slots (4660 Mbits).
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tom Herbert <therbert@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Maciej Żenczykowski <maze@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
v2 : added Neal suggestions
include/net/tcp.h | 6 ++++--
net/ipv4/tcp_input.c | 22 +++++++++++-----------
net/ipv4/tcp_output.c | 25 +++++++++++++++++--------
3 files changed, 32 insertions(+), 21 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index d5984e3..0f57706 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -477,7 +477,8 @@ extern int tcp_retransmit_skb(struct sock *, struct sk_buff *);
extern void tcp_retransmit_timer(struct sock *sk);
extern void tcp_xmit_retransmit_queue(struct sock *);
extern void tcp_simple_retransmit(struct sock *);
-extern int tcp_trim_head(struct sock *, struct sk_buff *, u32);
+extern void tcp_set_skb_tso_segs(const struct sock *sk, struct sk_buff *skb,
+ unsigned int mss_now);
extern int tcp_fragment(struct sock *, struct sk_buff *, u32, unsigned int);
extern void tcp_send_probe0(struct sock *);
@@ -640,7 +641,8 @@ struct tcp_skb_cb {
#if IS_ENABLED(CONFIG_IPV6)
struct inet6_skb_parm h6;
#endif
- } header; /* For incoming frames */
+ unsigned int offset_ack; /* part of acked data in this skb */
+ } header;
__u32 seq; /* Starting sequence number */
__u32 end_seq; /* SEQ + FIN + SYN + datalen */
__u32 when; /* used to compute rtt's */
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 99448f0..bdec2e6 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3260,25 +3260,25 @@ static void tcp_rearm_rto(struct sock *sk)
}
}
-/* If we get here, the whole TSO packet has not been acked. */
+/* If we get here, the whole packet has not been acked.
+ * We used to call tcp_trim_head() to remove acked data from skb,
+ * but its expensive with TSO if our previous clone is still in flight.
+ * We thus maintain an offset_ack, and hope no pskb_expand_head()
+ * is needed until whole packet is acked by upcoming ACKs.
+ */
static u32 tcp_tso_acked(struct sock *sk, struct sk_buff *skb)
{
struct tcp_sock *tp = tcp_sk(sk);
- u32 packets_acked;
+ u32 oldpcount = tcp_skb_pcount(skb);
BUG_ON(!after(TCP_SKB_CB(skb)->end_seq, tp->snd_una));
- packets_acked = tcp_skb_pcount(skb);
- if (tcp_trim_head(sk, skb, tp->snd_una - TCP_SKB_CB(skb)->seq))
- return 0;
- packets_acked -= tcp_skb_pcount(skb);
+ TCP_SKB_CB(skb)->header.offset_ack = tp->snd_una - TCP_SKB_CB(skb)->seq;
- if (packets_acked) {
- BUG_ON(tcp_skb_pcount(skb) == 0);
- BUG_ON(!before(TCP_SKB_CB(skb)->seq, TCP_SKB_CB(skb)->end_seq));
- }
+ if (oldpcount > 1)
+ tcp_set_skb_tso_segs(sk, skb, tcp_skb_mss(skb));
- return packets_acked;
+ return oldpcount - tcp_skb_pcount(skb);
}
/* Remove acknowledged frames from the retransmission queue. If our packet
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index de8790c..f66df37 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -927,11 +927,15 @@ static void tcp_queue_skb(struct sock *sk, struct sk_buff *skb)
sk_mem_charge(sk, skb->truesize);
}
-/* Initialize TSO segments for a packet. */
-static void tcp_set_skb_tso_segs(const struct sock *sk, struct sk_buff *skb,
- unsigned int mss_now)
+/* Initialize TSO segments for a packet.
+ * Part of skb (offset_ack) might have been acked.
+ */
+void tcp_set_skb_tso_segs(const struct sock *sk, struct sk_buff *skb,
+ unsigned int mss_now)
{
- if (skb->len <= mss_now || !sk_can_gso(sk) ||
+ unsigned int len = skb->len - TCP_SKB_CB(skb)->header.offset_ack;
+
+ if (len <= mss_now || !sk_can_gso(sk) ||
skb->ip_summed == CHECKSUM_NONE) {
/* Avoid the costly divide in the normal
* non-TSO case.
@@ -940,7 +944,7 @@ static void tcp_set_skb_tso_segs(const struct sock *sk, struct sk_buff *skb,
skb_shinfo(skb)->gso_size = 0;
skb_shinfo(skb)->gso_type = 0;
} else {
- skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss_now);
+ skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len, mss_now);
skb_shinfo(skb)->gso_size = mss_now;
skb_shinfo(skb)->gso_type = sk->sk_gso_type;
}
@@ -1125,15 +1129,20 @@ static void __pskb_trim_head(struct sk_buff *skb, int len)
skb->len = skb->data_len;
}
-/* Remove acked data from a packet in the transmit queue. */
-int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
+/* Remove acked data from a packet in the transmit queue.
+ * Ony called before retransmit.
+ */
+static int tcp_trim_head(struct sock *sk, struct sk_buff *skb)
{
+ u32 len = tcp_sk(sk)->snd_una - TCP_SKB_CB(skb)->seq;
+
if (skb_cloned(skb) && pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
return -ENOMEM;
__pskb_trim_head(skb, len);
TCP_SKB_CB(skb)->seq += len;
+ TCP_SKB_CB(skb)->header.offset_ack = 0;
skb->ip_summed = CHECKSUM_PARTIAL;
skb->truesize -= len;
@@ -2096,7 +2105,7 @@ int tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
if (before(TCP_SKB_CB(skb)->seq, tp->snd_una)) {
if (before(TCP_SKB_CB(skb)->end_seq, tp->snd_una))
BUG();
- if (tcp_trim_head(sk, skb, tp->snd_una - TCP_SKB_CB(skb)->seq))
+ if (tcp_trim_head(sk, skb))
return -ENOMEM;
}
^ permalink raw reply related
* [PATCH] tcp: fix retransmit of partially acked frames
From: Eric Dumazet @ 2012-04-18 20:14 UTC (permalink / raw)
To: Alexander Beregalov, David Miller
Cc: netdev, Linux Kernel Mailing List, Marc MERLIN
In-Reply-To: <1334734196.2472.91.camel@edumazet-glaptop>
From: Eric Dumazet <edumazet@google.com>
Alexander Beregalov reported skb_over_panic errors and provided stack
trace.
I occurs commit a21d45726aca (tcp: avoid order-1 allocations on wifi and
tx path) added a regression, when a retransmit is done after a partial
ACK.
tcp_retransmit_skb() tries to aggregate several frames if the first one
has enough available room to hold the following ones payload. This is
controlled by /proc/sys/net/ipv4/tcp_retrans_collapse tunable (default :
enabled)
Problem is we must make sure _pskb_trim_head() doesnt fool
skb_availroom() when pulling some bytes from skb (this pull is done when
receiver ACK part of the frame).
Reported-by: Alexander Beregalov <a.beregalov@gmail.com>
Cc: Marc MERLIN <marc@merlins.org>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_output.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 376b2cf..7ac6423 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1096,6 +1096,7 @@ static void __pskb_trim_head(struct sk_buff *skb, int len)
eat = min_t(int, len, skb_headlen(skb));
if (eat) {
__skb_pull(skb, eat);
+ skb->avail_size -= eat;
len -= eat;
if (!len)
return;
^ permalink raw reply related
* Re[2]: [PATCH 2/2] IPVS: make failure of netns init more stable
From: Julian Anastasov @ 2012-04-18 20:27 UTC (permalink / raw)
To: Hans Schillstrom
Cc: horms@verge.net.au, Hans Schillstrom, wensong@linux-vs.org,
lvs-devel@vger.kernel.org, netdev@vger.kernel.org,
netfilter-devel@vger.kernel.org
In-Reply-To: <3r6npeu.69e743752dad7d8eb6a19cbbbdfd1ed9@obelix.schillstrom.com>
Hello,
On Wed, 18 Apr 2012, Hans Schillstrom wrote:
> >trigger load error. When I generate ENOMEM on IPVS core init
> >for such case I get ENOENT from register_ip_vs_app when
> >patch 1 and 2 for apps are applied, i.e. net->ipvs is NULL.
> >You can check it with NF_CONNTRACK=y, IP_VS=y and
> >IP_VS_FTP=m. You only need to trigger ENOMEM in __ip_vs_init.
>
>
> I did test this with 4 netns loaded and modprobe ip_vs_ftp
> In the 4:th netns (ipvs->gen >= 4) fire a -ENOMEM
> The result was as expected, ip_vs_ftp was not loaded.
>
> All patches below was loaded. (included the ipvs NULL check)
>
> Just for "fun" I also added a printk in the ipvs NULL check
> but I can't trigger it.
I trigger it in this way (ip_vs in kernel, ip_vs_ftp
as module):
if (1 || ip_vs_app_net_init(net) < 0)
goto app_fail;
This causes ip_vs core to fail. I use NET_NS=n.
> Simon:
> do you have any possibility to test it or give me a hint how to do ?
> (Just to make sure that the patches below will be sufficient)
>
> >
> >> With proper fault handling i.e. all ways returning fault codes to the netns init,
> >> there is no need for checking for "if (!net->ipvs)" or any other action.
> >
> > Probably but one check on load does not hurt much.
>
> I think I have tested all of above now and my conclusion is that we need the following patches
> which also was applied when the tests was run.
> (with a small reservation that I might have missed some..)
>
> [PATCH v3 1/2] netfilter: ipvs: Verify that IP_VS protocol has been registered, Sasha Levin
> [PATCH v3 2/2] netfilter: ipvs: use GFP_KERNEL allocation where possible, Sasha Levin
>
> [PATCH 0/6] Convert some GFP_ATOMIC allocations, Julian Anastasov
> [PATCH 1/6] ipvs: timeout tables do not need GFP_ATOMIC allocation, Julian Anastasov
> [PATCH 2/6] ipvs: SH scheduler does not need GFP_ATOMIC allocation, Julian Anastasov
> [PATCH 5/6] ipvs: LBLCR scheduler does not need GFP_ATOMIC allocation on init, Julian Anastasov
> [PATCH 6/6] ipvs: WRR scheduler does not need GFP_ATOMIC allocation, Julian Anastasov
> [PATCH 3/6] ipvs: DH scheduler does not need GFP_ATOMIC allocation, Julian Anastasov
> [PATCH 4/6] ipvs: LBLC scheduler does not need GFP_ATOMIC allocation on init, Julian Anastasov
>
> [PATCH] ipvs: fix crash in ip_vs_control_net_cleanup on unload, Julian Anastasov
>
> [PATCH 1/2] ipvs: reset ipvs pointer in netns, Julian Anastasov
> [PATCH 1/2] IPVS: take care of return value from protocol init_netns, Hans Schillstrom
>
> To be safe, add this to [PATCH 1/2] ipvs: reset ipvs pointer in netns or make a new patch
The ip_vs_ftp part is fixed by:
"[PATCH 2/2] ipvs: fix app registration in netns". So, this check
for ip_vs_ftp is not needed anymore:
> diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
> index 538d74e..c757359 100644
> --- a/net/netfilter/ipvs/ip_vs_ftp.c
> +++ b/net/netfilter/ipvs/ip_vs_ftp.c
> @@ -439,6 +439,9 @@ static int __net_init __ip_vs_ftp_init(struct net *net)
> struct ip_vs_app *app;
> struct netns_ipvs *ipvs = net_ipvs(net);
>
> + if (!ipvs)
> + return ERR_PTR(-ENOENT);
> +
> app = kmemdup(&ip_vs_ftp, sizeof(struct ip_vs_app), GFP_KERNEL);
> if (!app)
> return -ENOMEM;
>
> diff --git a/net/netfilter/ipvs/ip_vs_lblc.c b/net/netfilter/ipvs/ip_vs_lblc.c
> index 74c7278..1d74996 100644
> --- a/net/netfilter/ipvs/ip_vs_lblc.c
> +++ b/net/netfilter/ipvs/ip_vs_lblc.c
> @@ -549,6 +549,9 @@ static int __net_init __ip_vs_lblc_init(struct net *net)
> {
> struct netns_ipvs *ipvs = net_ipvs(net);
>
> + if (!ipvs)
> + return ERR_PTR(-ENOENT);
You can post additional patches for lblc and lblcr but
use 'return -ENOENT;', ERR_PTR is not appropriate here, it
is for pointers.
> +
> if (!net_eq(net, &init_net)) {
> ipvs->lblc_ctl_table = kmemdup(vs_vars_table,
> sizeof(vs_vars_table),
> diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
> index 8620c68..c328ee0 100644
> --- a/net/netfilter/ipvs/ip_vs_lblcr.c
> +++ b/net/netfilter/ipvs/ip_vs_lblcr.c
> @@ -743,6 +743,9 @@ static int __net_init __ip_vs_lblcr_init(struct net *net)
> {
> struct netns_ipvs *ipvs = net_ipvs(net);
>
> + if (!ipvs)
> + return ERR_PTR(-ENOENT);
> +
> if (!net_eq(net, &init_net)) {
> ipvs->lblcr_ctl_table = kmemdup(vs_vars_table,
> sizeof(vs_vars_table),
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH] tcp: fix retransmit of partially acked frames
From: David Miller @ 2012-04-18 20:54 UTC (permalink / raw)
To: eric.dumazet; +Cc: a.beregalov, netdev, linux-kernel, marc
In-Reply-To: <1334780063.2472.352.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 18 Apr 2012 22:14:23 +0200
> From: Eric Dumazet <edumazet@google.com>
>
> Alexander Beregalov reported skb_over_panic errors and provided stack
> trace.
>
> I occurs commit a21d45726aca (tcp: avoid order-1 allocations on wifi and
> tx path) added a regression, when a retransmit is done after a partial
> ACK.
>
> tcp_retransmit_skb() tries to aggregate several frames if the first one
> has enough available room to hold the following ones payload. This is
> controlled by /proc/sys/net/ipv4/tcp_retrans_collapse tunable (default :
> enabled)
>
> Problem is we must make sure _pskb_trim_head() doesnt fool
> skb_availroom() when pulling some bytes from skb (this pull is done when
> receiver ACK part of the frame).
>
> Reported-by: Alexander Beregalov <a.beregalov@gmail.com>
> Cc: Marc MERLIN <marc@merlins.org>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, thanks.
^ permalink raw reply
* Re[3]: [PATCH 2/2] IPVS: make failure of netns init more stable
From: Hans Schillstrom @ 2012-04-18 22:33 UTC (permalink / raw)
To: Julian Anastasov
Cc: horms@verge.net.au, Hans Schillstrom, wensong@linux-vs.org,
lvs-devel@vger.kernel.org, netdev@vger.kernel.org,
netfilter-devel@vger.kernel.org
Hello,
>
>On Wed, 18 Apr 2012, Hans Schillstrom wrote:
>
>> >trigger load error. When I generate ENOMEM on IPVS core init
>> >for such case I get ENOENT from register_ip_vs_app when
>> >patch 1 and 2 for apps are applied, i.e. net->ipvs is NULL.
>> >You can check it with NF_CONNTRACK=y, IP_VS=y and
>> >IP_VS_FTP=m. You only need to trigger ENOMEM in __ip_vs_init.
>>
>>
>> I did test this with 4 netns loaded and modprobe ip_vs_ftp
>> In the 4:th netns (ipvs->gen >= 4) fire a -ENOMEM
>> The result was as expected, ip_vs_ftp was not loaded.
>>
>> All patches below was loaded. (included the ipvs NULL check)
>>
>> Just for "fun" I also added a printk in the ipvs NULL check
>> but I can't trigger it.
>
> I trigger it in this way (ip_vs in kernel, ip_vs_ftp
>as module):
>
> if (1 || ip_vs_app_net_init(net) < 0)
> goto app_fail;
>
> This causes ip_vs core to fail. I use NET_NS=n.
Thanks, It's a really odd case when you don't have enough memory to start the kernel
I think it's cured now, it doesn't trigger any dumps now.
Tested with and without CONFIG_NET_NS
>
>> Simon:
>> do you have any possibility to test it or give me a hint how to do ?
>> (Just to make sure that the patches below will be sufficient)
>>
>> >
>> >> With proper fault handling i.e. all ways returning fault codes to the netns init,
>> >> there is no need for checking for "if (!net->ipvs)" or any other action.
>> >
>> > Probably but one check on load does not hurt much.
>>
>> I think I have tested all of above now and my conclusion is that we need the following patches
>> which also was applied when the tests was run.
>> (with a small reservation that I might have missed some..)
>>
>> [PATCH v3 1/2] netfilter: ipvs: Verify that IP_VS protocol has been registered, Sasha Levin
>> [PATCH v3 2/2] netfilter: ipvs: use GFP_KERNEL allocation where possible, Sasha Levin
>>
>> [PATCH 0/6] Convert some GFP_ATOMIC allocations, Julian Anastasov
>> [PATCH 1/6] ipvs: timeout tables do not need GFP_ATOMIC allocation, Julian Anastasov
>> [PATCH 2/6] ipvs: SH scheduler does not need GFP_ATOMIC allocation, Julian Anastasov
>> [PATCH 5/6] ipvs: LBLCR scheduler does not need GFP_ATOMIC allocation on init, Julian Anastasov
>> [PATCH 6/6] ipvs: WRR scheduler does not need GFP_ATOMIC allocation, Julian Anastasov
>> [PATCH 3/6] ipvs: DH scheduler does not need GFP_ATOMIC allocation, Julian Anastasov
>> [PATCH 4/6] ipvs: LBLC scheduler does not need GFP_ATOMIC allocation on init, Julian Anastasov
>>
>> [PATCH] ipvs: fix crash in ip_vs_control_net_cleanup on unload, Julian Anastasov
>>
>> [PATCH 1/2] ipvs: reset ipvs pointer in netns, Julian Anastasov
>> [PATCH 1/2] IPVS: take care of return value from protocol init_netns, Hans Schillstrom
>>
>> To be safe, add this to [PATCH 1/2] ipvs: reset ipvs pointer in netns or make a new patch
>
> The ip_vs_ftp part is fixed by:
>"[PATCH 2/2] ipvs: fix app registration in netns". So, this check
>for ip_vs_ftp is not needed anymore:
Well, I don't think "[PATCH 2/2] ipvs: fix app registration in netns" is needed if the null check is there.
I have only used the NULL check (without ERR_PTR() :-) not the entire patch in my tests.
If you think it's needed it is OK for me.
>
>> diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
>> index 538d74e..c757359 100644
>> --- a/net/netfilter/ipvs/ip_vs_ftp.c
>> +++ b/net/netfilter/ipvs/ip_vs_ftp.c
>> @@ -439,6 +439,9 @@ static int __net_init __ip_vs_ftp_init(struct net *net)
>> struct ip_vs_app *app;
>> struct netns_ipvs *ipvs = net_ipvs(net);
>>
>> + if (!ipvs)
>> + return ERR_PTR(-ENOENT);
>> +
>> app = kmemdup(&ip_vs_ftp, sizeof(struct ip_vs_app), GFP_KERNEL);
>> if (!app)
>> return -ENOMEM;
>>
>> diff --git a/net/netfilter/ipvs/ip_vs_lblc.c b/net/netfilter/ipvs/ip_vs_lblc.c
>> index 74c7278..1d74996 100644
>> --- a/net/netfilter/ipvs/ip_vs_lblc.c
>> +++ b/net/netfilter/ipvs/ip_vs_lblc.c
>> @@ -549,6 +549,9 @@ static int __net_init __ip_vs_lblc_init(struct net *net)
>> {
>> struct netns_ipvs *ipvs = net_ipvs(net);
>>
>> + if (!ipvs)
>> + return ERR_PTR(-ENOENT);
>
> You can post additional patches for lblc and lblcr but
>use 'return -ENOENT;', ERR_PTR is not appropriate here, it
>is for pointers.
Oops, cut & paste
>
>> +
>> if (!net_eq(net, &init_net)) {
>> ipvs->lblc_ctl_table = kmemdup(vs_vars_table,
>> sizeof(vs_vars_table),
>> diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
>> index 8620c68..c328ee0 100644
>> --- a/net/netfilter/ipvs/ip_vs_lblcr.c
>> +++ b/net/netfilter/ipvs/ip_vs_lblcr.c
>> @@ -743,6 +743,9 @@ static int __net_init __ip_vs_lblcr_init(struct net *net)
>> {
>> struct netns_ipvs *ipvs = net_ipvs(net);
>>
>> + if (!ipvs)
>> + return ERR_PTR(-ENOENT);
>> +
>> if (!net_eq(net, &init_net)) {
>> ipvs->lblcr_ctl_table = kmemdup(vs_vars_table,
>> sizeof(vs_vars_table),
>
>Regards
>
>--
>Julian Anastasov <ja@ssi.bg>
--
Regards
Hans Schillstrom <hans@schillstrom.com>
^ permalink raw reply
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path
From: Michael Kerrisk (man-pages) @ 2012-04-18 22:50 UTC (permalink / raw)
To: David Miller
Cc: carlos-v2tUB8YBRSi3e3T8WW9gsA, netdev-u79uwXL29TY76Z2rM5mHXA,
penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp,
linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g,
jengelh-nopoi9nDyk+ELgA04lAiVw, w,
alan-qBU/x9rampVanCEyBjwyrvXRex20P6io
In-Reply-To: <20120418.001650.1042781402985153056.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Wed, Apr 18, 2012 at 4:16 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org>
> Date: Wed, 18 Apr 2012 00:08:47 -0400
>
>> I don't clearly understand your position here, and perhaps that's my
>> own ignorance, but could you please clarify, with examples, exactly
>> why the change is not acceptable?
>
> My position is that since millions upon millions of Linux systems, in
> fact every single Linux system, exists right now with the current
> behavior we are not helping application writers at all by changing
> behavior now after it's been this way for nearly 20 years.
>
> Because if an application writer wants his code to work on systems
> that actually exist he has to accomodate the non-NULL termination
> situation if he wants to inspect or print out an AF_UNIX path.
>
> Because every system in existence right now allows the non-NULL
> terminated AF_UNIX paths, therefore it's possible on every system
> in existence right now.
>
> Catch my drift?
>
> The very thing the patch claims to help, it doesn't. We install this
> kernel patch now and then tell application writers that they can just
> assume all AF_UNIX paths are NULL terminated when they want to print
> it out, because such code will not actually be guarenteed to work on
> all deployed Linux machines out there.
Hang on a moment. I did not suggest that we can just tell users they
can forget about the past. Obviously, users will need to program to
past kernel behavior here for a good long time yet. (As Alan says
elsewhere in the thread "they'll be defensively coding for
the existing API for another ten years for enterprise distros
anyway".) However, this is about longer-term improvement of the
quality of implementation; in X years (choose your X) time, a lot of
new application may not need to care about the old broken behavior.
See some related examples below.
And you skipped past my other two points. Even if my understanding
about POSIX mandates is correct, I can understand how we might ignore
that point. But the last one is still germane:
[[
3. Considering these two sets:
(a) [applications that rely on the assumption that there
is a null terminator inside sizeof(sun_path) bytes]
(b) [applications that would break if the kernel behavior changed]
I suspect that set (a) is rather larger than set (b)--or, more
likely still, applications ensure they go for the lowest common
denominator limit of 92 (HP-UX) or 104 (historical BSD limit)
bytes, and so avoid this issue completely.
]]
There may well be potential breakages out there in set (a), and
improving the QOI would help them. (To put things in terms of Alan's
response: I suspect that there may well be existing applications that
are *not* defensively handling the existing API).
Taking the logic you've posed (my reading: "we shouldn't fix old
brokenness because applications will still need to code to the
brokenness") to the extreme, we'd *never* fix old pieces of
brokenness. However, we certainly have precedents for doing exactly
that:
After nearly 15 years of brokenness (stretching back to the first
kernels), commit 69be8f189653cd81aae5a74e26615b12871bb72e fixed this
(sigaction(2)):
BUGS
In kernels up to and including 2.6.13, specifying SA_NODEFER in
sa_flags prevents not only the delivered signal from being
masked during execution of the handler, but also the signals
specified in sa_mask. This bug was fixed in kernel 2.6.14.
Similarly, after brokenness that had run through the entire preceding
2.4.x kernel series, Linux 2.6.4 fixed this:
BUGS
In kernel 2.4 (and earlier) there is some strangeness in the
handling of X_OK tests for superuser. If all categories of
execute permission are disabled for a nondirectory file, then
the only access() test that returns -1 is when mode is speci‐
fied as just X_OK; if R_OK or W_OK is also specified in mode,
then access() returns 0 for such files. Early 2.6 kernels (up
to and including 2.6.3) also behaved in the same way as kernel
2.4.
(A little background here:
http://thread.gmane.org/gmane.linux.kernel/158814, and the fix
eventually went in with
http://thread.gmane.org/gmane.linux.kernel/178719)
> You cannot just ignore 20 years of precedence and say "oh let's change
> this in the kernel now, and that way application writers don't have to
> worry about that lack of NULL termination any more." It simply
> doesn't work like that.
As should be clear from the above, I agree. But still, I don't think
the logic "it's broken, and even if we fix it, users will still have
to code to the old brokenness" is a sufficient argument against
improving the QOI long-term.
> All of this talk about whether applications actually create non-NULL
> terminated AF_UNIX paths don't even factor into the conversation.
>
> So the value proposition for this patch simply does not exist.
Of course, it's your call in the end, but I don't think things are as
cut-and-dried as your response suggests.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
^ permalink raw reply
* [PATCH v5 0/3] netdev/of/phy: MDIO bus multiplexer support.
From: David Daney @ 2012-04-18 23:20 UTC (permalink / raw)
To: Grant Likely, Rob Herring,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David S. Miller,
netdev-u79uwXL29TY76Z2rM5mHXA
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA,
afleming-Re5JQEeQqe8AvxtiuMwx3w, David Daney,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
This code has been working well for about six months on a couple of
different configurations (boards), so I thought it would be a good
time to send it out again, and I hope get it on the path towards
merging.
v5: Correct Kconfig depends in 3/3 as noticed by David Miller.
v4: Correct some comment text and rename a couple of variables to
better reflect their purpose.
v3: Update binding to use "mdio-mux-gpio" compatible property.
Cleanups suggested by Grant Likely. Now uses the driver probe
deferral mechanism if GPIOs or parent bus not available.
v2: Update bindings to use "reg" and "mdio-parent-bus" instead of
"cell-index" and "parent-bus"
v1:
We have several different boards with a multiplexer in the MDIO bus.
There is an MDIO bus controller connected to a switching device with
several child MDIO busses.
Everything is wired up using device tree bindings.
1/3 - New of_mdio_find_bus() function used to help configuring the
driver topology.
2/3 - MDIO bus multiplexer framework.
3/3 - A driver for a GPIO controlled multiplexer.
David Daney (3):
netdev/of/phy: New function: of_mdio_find_bus().
netdev/of/phy: Add MDIO bus multiplexer support.
netdev/of/phy: Add MDIO bus multiplexer driven by GPIO lines.
.../devicetree/bindings/net/mdio-mux-gpio.txt | 127 +++++++++++++
Documentation/devicetree/bindings/net/mdio-mux.txt | 136 ++++++++++++++
drivers/net/phy/Kconfig | 18 ++
drivers/net/phy/Makefile | 2 +
drivers/net/phy/mdio-mux-gpio.c | 142 +++++++++++++++
drivers/net/phy/mdio-mux.c | 192 ++++++++++++++++++++
drivers/net/phy/mdio_bus.c | 32 ++++
drivers/of/of_mdio.c | 2 +
include/linux/mdio-mux.h | 21 ++
include/linux/of_mdio.h | 2 +
10 files changed, 674 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/mdio-mux-gpio.txt
create mode 100644 Documentation/devicetree/bindings/net/mdio-mux.txt
create mode 100644 drivers/net/phy/mdio-mux-gpio.c
create mode 100644 drivers/net/phy/mdio-mux.c
create mode 100644 include/linux/mdio-mux.h
--
1.7.2.3
^ permalink raw reply
* [PATCH v5 1/3] netdev/of/phy: New function: of_mdio_find_bus().
From: David Daney @ 2012-04-18 23:20 UTC (permalink / raw)
To: Grant Likely, Rob Herring, devicetree-discuss, David S. Miller,
netdev
Cc: linux-kernel, linux-mips, afleming, galak, David Daney,
Grant Likely, David S. Miller
In-Reply-To: <1334791254-15987-1-git-send-email-ddaney.cavm@gmail.com>
From: David Daney <david.daney@cavium.com>
Add of_mdio_find_bus() which allows an mii_bus to be located given its
associated the device tree node.
This is needed by the follow-on patch to add a driver for MDIO bus
multiplexers.
The of_mdiobus_register() function is modified so that the device tree
node is recorded in the mii_bus. Then we can find it again by
iterating over all mdio_bus_class devices.
Because the OF device tree has now become an integral part of the
kernel, this can live in mdio_bus.c (which contains the needed
mdio_bus_class structure) instead of of_mdio.c.
Signed-off-by: David Daney <david.daney@cavium.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: "David S. Miller" <davem@davemloft.net>
---
drivers/net/phy/mdio_bus.c | 32 ++++++++++++++++++++++++++++++++
drivers/of/of_mdio.c | 2 ++
include/linux/of_mdio.h | 2 ++
3 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 8985cc6..83d5c9f 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -88,6 +88,38 @@ static struct class mdio_bus_class = {
.dev_release = mdiobus_release,
};
+#ifdef CONFIG_OF_MDIO
+/* Helper function for of_mdio_find_bus */
+static int of_mdio_bus_match(struct device *dev, void *mdio_bus_np)
+{
+ return dev->of_node == mdio_bus_np;
+}
+/**
+ * of_mdio_find_bus - Given an mii_bus node, find the mii_bus.
+ * @mdio_np: Pointer to the mii_bus.
+ *
+ * Returns a pointer to the mii_bus, or NULL if none found.
+ *
+ * Because the association of a device_node and mii_bus is made via
+ * of_mdiobus_register(), the mii_bus cannot be found before it is
+ * registered with of_mdiobus_register().
+ *
+ */
+struct mii_bus *of_mdio_find_bus(struct device_node *mdio_bus_np)
+{
+ struct device *d;
+
+ if (!mdio_bus_np)
+ return NULL;
+
+ d = class_find_device(&mdio_bus_class, NULL, mdio_bus_np,
+ of_mdio_bus_match);
+
+ return d ? to_mii_bus(d) : NULL;
+}
+EXPORT_SYMBOL(of_mdio_find_bus);
+#endif
+
/**
* mdiobus_register - bring up all the PHYs on a given bus and attach them to bus
* @bus: target mii_bus
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 483c0ad..2574abd 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -45,6 +45,8 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
for (i=0; i<PHY_MAX_ADDR; i++)
mdio->irq[i] = PHY_POLL;
+ mdio->dev.of_node = np;
+
/* Register the MDIO bus */
rc = mdiobus_register(mdio);
if (rc)
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 53b94e0..912c27a 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -22,4 +22,6 @@ extern struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
void (*hndlr)(struct net_device *),
phy_interface_t iface);
+extern struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
+
#endif /* __LINUX_OF_MDIO_H */
--
1.7.2.3
^ permalink raw reply related
* [PATCH v5 2/3] netdev/of/phy: Add MDIO bus multiplexer support.
From: David Daney @ 2012-04-18 23:20 UTC (permalink / raw)
To: Grant Likely, Rob Herring, devicetree-discuss, David S. Miller,
netdev
Cc: linux-kernel, linux-mips, afleming, galak, David Daney
In-Reply-To: <1334791254-15987-1-git-send-email-ddaney.cavm@gmail.com>
From: David Daney <david.daney@cavium.com>
This patch adds a somewhat generic framework for MDIO bus
multiplexers. It is modeled on the I2C multiplexer.
The multiplexer is needed if there are multiple PHYs with the same
address connected to the same MDIO bus adepter, or if there is
insufficient electrical drive capability for all the connected PHY
devices.
Conceptually it could look something like this:
------------------
| Control Signal |
--------+---------
|
--------------- --------+------
| MDIO MASTER |---| Multiplexer |
--------------- --+-------+----
| |
C C
h h
i i
l l
d d
| |
--------- A B ---------
| | | | | |
| PHY@1 +-------+ +---+ PHY@1 |
| | | | | |
--------- | | ---------
--------- | | ---------
| | | | | |
| PHY@2 +-------+ +---+ PHY@2 |
| | | |
--------- ---------
This framework configures the bus topology from device tree data. The
mechanics of switching the multiplexer is left to device specific
drivers.
The follow-on patch contains a multiplexer driven by GPIO lines.
Signed-off-by: David Daney <david.daney@cavium.com>
---
Documentation/devicetree/bindings/net/mdio-mux.txt | 136 ++++++++++++++
drivers/net/phy/Kconfig | 8 +
drivers/net/phy/Makefile | 1 +
drivers/net/phy/mdio-mux.c | 192 ++++++++++++++++++++
include/linux/mdio-mux.h | 21 ++
5 files changed, 358 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/mdio-mux.txt
create mode 100644 drivers/net/phy/mdio-mux.c
create mode 100644 include/linux/mdio-mux.h
diff --git a/Documentation/devicetree/bindings/net/mdio-mux.txt b/Documentation/devicetree/bindings/net/mdio-mux.txt
new file mode 100644
index 0000000..f65606f
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio-mux.txt
@@ -0,0 +1,136 @@
+Common MDIO bus multiplexer/switch properties.
+
+An MDIO bus multiplexer/switch will have several child busses that are
+numbered uniquely in a device dependent manner. The nodes for an MDIO
+bus multiplexer/switch will have one child node for each child bus.
+
+Required properties:
+- mdio-parent-bus : phandle to the parent MDIO bus.
+- #address-cells = <1>;
+- #size-cells = <0>;
+
+Optional properties:
+- Other properties specific to the multiplexer/switch hardware.
+
+Required properties for child nodes:
+- #address-cells = <1>;
+- #size-cells = <0>;
+- reg : The sub-bus number.
+
+
+Example :
+
+ /* The parent MDIO bus. */
+ smi1: mdio@1180000001900 {
+ compatible = "cavium,octeon-3860-mdio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x11800 0x00001900 0x0 0x40>;
+ };
+
+ /*
+ An NXP sn74cbtlv3253 dual 1-of-4 switch controlled by a
+ pair of GPIO lines. Child busses 2 and 3 populated with 4
+ PHYs each.
+ */
+ mdio-mux {
+ compatible = "mdio-mux-gpio";
+ gpios = <&gpio1 3 0>, <&gpio1 4 0>;
+ mdio-parent-bus = <&smi1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ mdio@2 {
+ reg = <2>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ phy11: ethernet-phy@1 {
+ reg = <1>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <10 8>; /* Pin 10, active low */
+ };
+ phy12: ethernet-phy@2 {
+ reg = <2>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <10 8>; /* Pin 10, active low */
+ };
+ phy13: ethernet-phy@3 {
+ reg = <3>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <10 8>; /* Pin 10, active low */
+ };
+ phy14: ethernet-phy@4 {
+ reg = <4>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <10 8>; /* Pin 10, active low */
+ };
+ };
+
+ mdio@3 {
+ reg = <3>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ phy21: ethernet-phy@1 {
+ reg = <1>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <12 8>; /* Pin 12, active low */
+ };
+ phy22: ethernet-phy@2 {
+ reg = <2>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <12 8>; /* Pin 12, active low */
+ };
+ phy23: ethernet-phy@3 {
+ reg = <3>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <12 8>; /* Pin 12, active low */
+ };
+ phy24: ethernet-phy@4 {
+ reg = <4>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <12 8>; /* Pin 12, active low */
+ };
+ };
+ };
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 0e01f4e..222b06b 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -135,6 +135,14 @@ config MDIO_OCTEON
If in doubt, say Y.
+config MDIO_BUS_MUX
+ tristate
+ help
+ This module provides a driver framework for MDIO bus
+ multiplexers which connect one of several child MDIO busses
+ to a parent bus. Switching between child busses is done by
+ device specific drivers.
+
endif # PHYLIB
config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index b7438b1..a6b50e7 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -25,3 +25,4 @@ obj-$(CONFIG_MICREL_PHY) += micrel.o
obj-$(CONFIG_MDIO_OCTEON) += mdio-octeon.o
obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
obj-$(CONFIG_AMD_PHY) += amd.o
+obj-$(CONFIG_MDIO_BUS_MUX) += mdio-mux.o
diff --git a/drivers/net/phy/mdio-mux.c b/drivers/net/phy/mdio-mux.c
new file mode 100644
index 0000000..39ea067
--- /dev/null
+++ b/drivers/net/phy/mdio-mux.c
@@ -0,0 +1,192 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2011, 2012 Cavium, Inc.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/mdio-mux.h>
+#include <linux/of_mdio.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+
+#define DRV_VERSION "1.0"
+#define DRV_DESCRIPTION "MDIO bus multiplexer driver"
+
+struct mdio_mux_child_bus;
+
+struct mdio_mux_parent_bus {
+ struct mii_bus *mii_bus;
+ int current_child;
+ int parent_id;
+ void *switch_data;
+ int (*switch_fn)(int current_child, int desired_child, void *data);
+
+ /* List of our children linked through their next fields. */
+ struct mdio_mux_child_bus *children;
+};
+
+struct mdio_mux_child_bus {
+ struct mii_bus *mii_bus;
+ struct mdio_mux_parent_bus *parent;
+ struct mdio_mux_child_bus *next;
+ int bus_number;
+ int phy_irq[PHY_MAX_ADDR];
+};
+
+/*
+ * The parent bus' lock is used to order access to the switch_fn.
+ */
+static int mdio_mux_read(struct mii_bus *bus, int phy_id, int regnum)
+{
+ struct mdio_mux_child_bus *cb = bus->priv;
+ struct mdio_mux_parent_bus *pb = cb->parent;
+ int r;
+
+ mutex_lock(&pb->mii_bus->mdio_lock);
+ r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
+ if (r)
+ goto out;
+
+ pb->current_child = cb->bus_number;
+
+ r = pb->mii_bus->read(pb->mii_bus, phy_id, regnum);
+out:
+ mutex_unlock(&pb->mii_bus->mdio_lock);
+
+ return r;
+}
+
+/*
+ * The parent bus' lock is used to order access to the switch_fn.
+ */
+static int mdio_mux_write(struct mii_bus *bus, int phy_id,
+ int regnum, u16 val)
+{
+ struct mdio_mux_child_bus *cb = bus->priv;
+ struct mdio_mux_parent_bus *pb = cb->parent;
+
+ int r;
+
+ mutex_lock(&pb->mii_bus->mdio_lock);
+ r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
+ if (r)
+ goto out;
+
+ pb->current_child = cb->bus_number;
+
+ r = pb->mii_bus->write(pb->mii_bus, phy_id, regnum, val);
+out:
+ mutex_unlock(&pb->mii_bus->mdio_lock);
+
+ return r;
+}
+
+static int parent_count;
+
+int mdio_mux_init(struct device *dev,
+ int (*switch_fn)(int cur, int desired, void *data),
+ void **mux_handle,
+ void *data)
+{
+ struct device_node *parent_bus_node;
+ struct device_node *child_bus_node;
+ int r, ret_val;
+ struct mii_bus *parent_bus;
+ struct mdio_mux_parent_bus *pb;
+ struct mdio_mux_child_bus *cb;
+
+ if (!dev->of_node)
+ return -ENODEV;
+
+ parent_bus_node = of_parse_phandle(dev->of_node, "mdio-parent-bus", 0);
+
+ if (!parent_bus_node)
+ return -ENODEV;
+
+ parent_bus = of_mdio_find_bus(parent_bus_node);
+ if (parent_bus == NULL) {
+ ret_val = -EPROBE_DEFER;
+ goto err_parent_bus;
+ }
+
+ pb = devm_kzalloc(dev, sizeof(*pb), GFP_KERNEL);
+ if (pb == NULL) {
+ ret_val = -ENOMEM;
+ goto err_parent_bus;
+ }
+
+ pb->switch_data = data;
+ pb->switch_fn = switch_fn;
+ pb->current_child = -1;
+ pb->parent_id = parent_count++;
+ pb->mii_bus = parent_bus;
+
+ ret_val = -ENODEV;
+ for_each_child_of_node(dev->of_node, child_bus_node) {
+ u32 v;
+
+ r = of_property_read_u32(child_bus_node, "reg", &v);
+ if (r)
+ continue;
+
+ cb = devm_kzalloc(dev, sizeof(*cb), GFP_KERNEL);
+ if (cb == NULL) {
+ dev_err(dev,
+ "Error: Failed to allocate memory for child\n");
+ ret_val = -ENOMEM;
+ break;
+ }
+ cb->bus_number = v;
+ cb->parent = pb;
+ cb->mii_bus = mdiobus_alloc();
+ cb->mii_bus->priv = cb;
+
+ cb->mii_bus->irq = cb->phy_irq;
+ cb->mii_bus->name = "mdio_mux";
+ snprintf(cb->mii_bus->id, MII_BUS_ID_SIZE, "%x.%x",
+ pb->parent_id, v);
+ cb->mii_bus->parent = dev;
+ cb->mii_bus->read = mdio_mux_read;
+ cb->mii_bus->write = mdio_mux_write;
+ r = of_mdiobus_register(cb->mii_bus, child_bus_node);
+ if (r) {
+ mdiobus_free(cb->mii_bus);
+ devm_kfree(dev, cb);
+ } else {
+ of_node_get(child_bus_node);
+ cb->next = pb->children;
+ pb->children = cb;
+ }
+ }
+ if (pb->children) {
+ *mux_handle = pb;
+ dev_info(dev, "Version " DRV_VERSION "\n");
+ return 0;
+ }
+err_parent_bus:
+ of_node_put(parent_bus_node);
+ return ret_val;
+}
+EXPORT_SYMBOL_GPL(mdio_mux_init);
+
+void mdio_mux_uninit(void *mux_handle)
+{
+ struct mdio_mux_parent_bus *pb = mux_handle;
+ struct mdio_mux_child_bus *cb = pb->children;
+
+ while (cb) {
+ mdiobus_unregister(cb->mii_bus);
+ mdiobus_free(cb->mii_bus);
+ cb = cb->next;
+ }
+}
+EXPORT_SYMBOL_GPL(mdio_mux_uninit);
+
+MODULE_DESCRIPTION(DRV_DESCRIPTION);
+MODULE_VERSION(DRV_VERSION);
+MODULE_AUTHOR("David Daney");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mdio-mux.h b/include/linux/mdio-mux.h
new file mode 100644
index 0000000..a243dbb
--- /dev/null
+++ b/include/linux/mdio-mux.h
@@ -0,0 +1,21 @@
+/*
+ * MDIO bus multiplexer framwork.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2011, 2012 Cavium, Inc.
+ */
+#ifndef __LINUX_MDIO_MUX_H
+#define __LINUX_MDIO_MUX_H
+#include <linux/device.h>
+
+int mdio_mux_init(struct device *dev,
+ int (*switch_fn) (int cur, int desired, void *data),
+ void **mux_handle,
+ void *data);
+
+void mdio_mux_uninit(void *mux_handle);
+
+#endif /* __LINUX_MDIO_MUX_H */
--
1.7.2.3
^ permalink raw reply related
* [PATCH v5 3/3] netdev/of/phy: Add MDIO bus multiplexer driven by GPIO lines.
From: David Daney @ 2012-04-18 23:20 UTC (permalink / raw)
To: Grant Likely, Rob Herring,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David S. Miller,
netdev-u79uwXL29TY76Z2rM5mHXA
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA,
afleming-Re5JQEeQqe8AvxtiuMwx3w, David Daney,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1334791254-15987-1-git-send-email-ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
The GPIO pins select which sub bus is connected to the master.
Initially tested with an sn74cbtlv3253 switch device wired into the
MDIO bus.
Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
---
.../devicetree/bindings/net/mdio-mux-gpio.txt | 127 +++++++++++++++++
drivers/net/phy/Kconfig | 10 ++
drivers/net/phy/Makefile | 1 +
drivers/net/phy/mdio-mux-gpio.c | 142 ++++++++++++++++++++
4 files changed, 280 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/mdio-mux-gpio.txt
create mode 100644 drivers/net/phy/mdio-mux-gpio.c
diff --git a/Documentation/devicetree/bindings/net/mdio-mux-gpio.txt b/Documentation/devicetree/bindings/net/mdio-mux-gpio.txt
new file mode 100644
index 0000000..7938411
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio-mux-gpio.txt
@@ -0,0 +1,127 @@
+Properties for an MDIO bus multiplexer/switch controlled by GPIO pins.
+
+This is a special case of a MDIO bus multiplexer. One or more GPIO
+lines are used to control which child bus is connected.
+
+Required properties in addition to the generic multiplexer properties:
+
+- compatible : mdio-mux-gpio.
+- gpios : GPIO specifiers for each GPIO line. One or more must be specified.
+
+
+Example :
+
+ /* The parent MDIO bus. */
+ smi1: mdio@1180000001900 {
+ compatible = "cavium,octeon-3860-mdio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x11800 0x00001900 0x0 0x40>;
+ };
+
+ /*
+ An NXP sn74cbtlv3253 dual 1-of-4 switch controlled by a
+ pair of GPIO lines. Child busses 2 and 3 populated with 4
+ PHYs each.
+ */
+ mdio-mux {
+ compatible = "mdio-mux-gpio";
+ gpios = <&gpio1 3 0>, <&gpio1 4 0>;
+ mdio-parent-bus = <&smi1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ mdio@2 {
+ reg = <2>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ phy11: ethernet-phy@1 {
+ reg = <1>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <10 8>; /* Pin 10, active low */
+ };
+ phy12: ethernet-phy@2 {
+ reg = <2>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <10 8>; /* Pin 10, active low */
+ };
+ phy13: ethernet-phy@3 {
+ reg = <3>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <10 8>; /* Pin 10, active low */
+ };
+ phy14: ethernet-phy@4 {
+ reg = <4>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <10 8>; /* Pin 10, active low */
+ };
+ };
+
+ mdio@3 {
+ reg = <3>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ phy21: ethernet-phy@1 {
+ reg = <1>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <12 8>; /* Pin 12, active low */
+ };
+ phy22: ethernet-phy@2 {
+ reg = <2>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <12 8>; /* Pin 12, active low */
+ };
+ phy23: ethernet-phy@3 {
+ reg = <3>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <12 8>; /* Pin 12, active low */
+ };
+ phy24: ethernet-phy@4 {
+ reg = <4>;
+ compatible = "marvell,88e1149r";
+ marvell,reg-init = <3 0x10 0 0x5777>,
+ <3 0x11 0 0x00aa>,
+ <3 0x12 0 0x4105>,
+ <3 0x13 0 0x0a60>;
+ interrupt-parent = <&gpio>;
+ interrupts = <12 8>; /* Pin 12, active low */
+ };
+ };
+ };
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 222b06b..32e9be0 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -143,6 +143,16 @@ config MDIO_BUS_MUX
to a parent bus. Switching between child busses is done by
device specific drivers.
+config MDIO_BUS_MUX_GPIO
+ tristate "Support for GPIO controlled MDIO bus multiplexers"
+ depends on OF_GPIO && OF_MDIO
+ select MDIO_BUS_MUX
+ help
+ This module provides a driver for MDIO bus multiplexers that
+ are controlled via GPIO lines. The multiplexer connects one of
+ several child MDIO busses to a parent bus. Child bus
+ selection is under the control of GPIO lines.
+
endif # PHYLIB
config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a6b50e7..f51af68 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -26,3 +26,4 @@ obj-$(CONFIG_MDIO_OCTEON) += mdio-octeon.o
obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
obj-$(CONFIG_AMD_PHY) += amd.o
obj-$(CONFIG_MDIO_BUS_MUX) += mdio-mux.o
+obj-$(CONFIG_MDIO_BUS_MUX_GPIO) += mdio-mux-gpio.o
diff --git a/drivers/net/phy/mdio-mux-gpio.c b/drivers/net/phy/mdio-mux-gpio.c
new file mode 100644
index 0000000..e0cc4ef
--- /dev/null
+++ b/drivers/net/phy/mdio-mux-gpio.c
@@ -0,0 +1,142 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2011, 2012 Cavium, Inc.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/of_mdio.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/phy.h>
+#include <linux/mdio-mux.h>
+#include <linux/of_gpio.h>
+
+#define DRV_VERSION "1.0"
+#define DRV_DESCRIPTION "GPIO controlled MDIO bus multiplexer driver"
+
+#define MDIO_MUX_GPIO_MAX_BITS 8
+
+struct mdio_mux_gpio_state {
+ int gpio[MDIO_MUX_GPIO_MAX_BITS];
+ unsigned int num_gpios;
+ void *mux_handle;
+};
+
+static int mdio_mux_gpio_switch_fn(int current_child, int desired_child,
+ void *data)
+{
+ int change;
+ unsigned int n;
+ struct mdio_mux_gpio_state *s = data;
+
+ if (current_child == desired_child)
+ return 0;
+
+ change = current_child == -1 ? -1 : current_child ^ desired_child;
+
+ for (n = 0; n < s->num_gpios; n++) {
+ if (change & 1)
+ gpio_set_value_cansleep(s->gpio[n],
+ (desired_child & 1) != 0);
+ change >>= 1;
+ desired_child >>= 1;
+ }
+
+ return 0;
+}
+
+static int __devinit mdio_mux_gpio_probe(struct platform_device *pdev)
+{
+ enum of_gpio_flags f;
+ struct mdio_mux_gpio_state *s;
+ unsigned int num_gpios;
+ unsigned int n;
+ int r;
+
+ if (!pdev->dev.of_node)
+ return -ENODEV;
+
+ num_gpios = of_gpio_count(pdev->dev.of_node);
+ if (num_gpios == 0 || num_gpios > MDIO_MUX_GPIO_MAX_BITS)
+ return -ENODEV;
+
+ s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
+ if (!s)
+ return -ENOMEM;
+
+ s->num_gpios = num_gpios;
+
+ for (n = 0; n < num_gpios; ) {
+ int gpio = of_get_gpio_flags(pdev->dev.of_node, n, &f);
+ if (gpio < 0) {
+ r = (gpio == -ENODEV) ? -EPROBE_DEFER : gpio;
+ goto err;
+ }
+ s->gpio[n] = gpio;
+
+ n++;
+
+ r = gpio_request(gpio, "mdio_mux_gpio");
+ if (r)
+ goto err;
+
+ r = gpio_direction_output(gpio, 0);
+ if (r)
+ goto err;
+ }
+
+ r = mdio_mux_init(&pdev->dev,
+ mdio_mux_gpio_switch_fn, &s->mux_handle, s);
+
+ if (r == 0) {
+ pdev->dev.platform_data = s;
+ return 0;
+ }
+err:
+ while (n) {
+ n--;
+ gpio_free(s->gpio[n]);
+ }
+ devm_kfree(&pdev->dev, s);
+ return r;
+}
+
+static int __devexit mdio_mux_gpio_remove(struct platform_device *pdev)
+{
+ struct mdio_mux_gpio_state *s = pdev->dev.platform_data;
+ mdio_mux_uninit(s->mux_handle);
+ return 0;
+}
+
+static struct of_device_id mdio_mux_gpio_match[] = {
+ {
+ .compatible = "mdio-mux-gpio",
+ },
+ {
+ /* Legacy compatible property. */
+ .compatible = "cavium,mdio-mux-sn74cbtlv3253",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mdio_mux_gpio_match);
+
+static struct platform_driver mdio_mux_gpio_driver = {
+ .driver = {
+ .name = "mdio-mux-gpio",
+ .owner = THIS_MODULE,
+ .of_match_table = mdio_mux_gpio_match,
+ },
+ .probe = mdio_mux_gpio_probe,
+ .remove = __devexit_p(mdio_mux_gpio_remove),
+};
+
+module_platform_driver(mdio_mux_gpio_driver);
+
+MODULE_DESCRIPTION(DRV_DESCRIPTION);
+MODULE_VERSION(DRV_VERSION);
+MODULE_AUTHOR("David Daney");
+MODULE_LICENSE("GPL");
--
1.7.2.3
^ permalink raw reply related
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path
From: David Miller @ 2012-04-18 23:31 UTC (permalink / raw)
To: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w
Cc: carlos-v2tUB8YBRSi3e3T8WW9gsA, netdev-u79uwXL29TY76Z2rM5mHXA,
penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp,
linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g,
jengelh-nopoi9nDyk+ELgA04lAiVw, w,
alan-qBU/x9rampVanCEyBjwyrvXRex20P6io
In-Reply-To: <CAKgNAkh46EMDWpessyi0n-EyNoRid-iW1O1RfUpTtzKDv0mZFw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
From: "Michael Kerrisk (man-pages)" <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Thu, 19 Apr 2012 10:50:40 +1200
> However, this is about longer-term improvement of the quality of
> implementation; in X years (choose your X) time, a lot of new
> application may not need to care about the old broken behavior.
There is really no value to this, the AF_UNIX NULL termination issue
is significantly different from the signal examples you mention.
If we're going to, like Carlos will, make mention in POSIX documents
that one must account for possible lack of NULL termination, there
is absolutely ZERO value in changing things because we are telling
application writers the state of reality which is that they have
to allot for this.
Please drop this issue, the discussion was over a long time ago, thank
you very much.
^ permalink raw reply
* RE: [PATCH] net/hyperv: RX packets may keep on increasing although the NIC is down
From: wenqi_ma @ 2012-04-19 0:45 UTC (permalink / raw)
To: haiyangz, netdev; +Cc: davem, kys
In-Reply-To: <A1F3067C9B68744AA19F6802BAB8FFDC0DD331FB@TK5EX14MBXC221.redmond.corp.microsoft.com>
Thanks for your comments and suggestion, I will resubmit the revised patch later.
Best Regards,
Wenqi Ma
-----Original Message-----
From: Haiyang Zhang [mailto:haiyangz@microsoft.com]
Sent: Wednesday, April 18, 2012 10:20 PM
To: Wenqi Ma (RD-CN); netdev@vger.kernel.org
Cc: davem@davemloft.net; KY Srinivasan
Subject: RE: [PATCH] net/hyperv: RX packets may keep on increasing although the NIC is down
> -----Original Message-----
> From: Haiyang Zhang
> Sent: Wednesday, April 18, 2012 10:11 AM
> To: 'Wenqi Ma'; netdev@vger.kernel.org
> Cc: davem@davemloft.net; KY Srinivasan
> Subject: RE: [PATCH] net/hyperv: RX packets may keep on increasing
> although the NIC is down
>
>
>
> > -----Original Message-----
> > From: Wenqi Ma [mailto:wenqi_ma@trendmicro.com.cn]
> > Sent: Wednesday, April 18, 2012 10:02 AM
> > To: netdev@vger.kernel.org
> > Cc: davem@davemloft.net; Haiyang Zhang; Wenqi Ma
> > Subject: [PATCH] net/hyperv: RX packets may keep on increasing although
> > the NIC is down
> >
> > Although the network interface is down, the RX packets number which
> > could be observed by ifconfig may keep on increasing.
> >
> > This is because the WORK scheduled in netvsc_set_multicast_list()
> > may be executed after netvsc_close(). That means the rndis filter
> > may be re-enabled by do_set_multicast() even if it was closed by
> > netvsc_close().
> >
> > By canceling possible WORK before close the rndis filter, the issue
> > could be never happened.
> >
> > Signed-off-by: Wenqi Ma <wenqi_ma@trendmicro.com.cn>
> > ---
> > drivers/net/hyperv/netvsc_drv.c | 32 +++++++++++++-------------------
> > 1 files changed, 13 insertions(+), 19 deletions(-)
>
> Thanks for patching it. I have some comments --
> The existing "struct set_multicast_work" is no longer in use, so it should be
> removed now.
> You should also add cancel_work into netvsc_change_mtu().
Also the patch subject should say what fixes/changes you made, not the bug behavior.
Recommend something like, "Adding cancellation to set promiscuous mode work".
Thanks,
- Haiyang
TREND MICRO EMAIL NOTICE
The information contained in this email and any attachments is confidential and may be subject to copyright or other intellectual property protection. If you are not the intended recipient, you are not authorized to use or disclose this information, and we request that you notify us by reply mail or telephone and delete the original message from your mail system.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox