* [PATCH] bonding: If IP route look-up to send an ARP fails, mark in bonding structure as no ARP sent.
From: rama nichanamatlu @ 2013-11-21 0:53 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 4588 bytes --]
During the creation of VLAN's atop bonding the underlying interfaces are
made part of VLAN's, and at the same bonding driver gets aware that
VLAN's exists above it and hence would consult IP routing for every ARP
to be sent to determine the route which tells bonding driver the
correct VLAN tag to attach to the outgoing ARP packet. But, during the
VLAN creation when vlan driver puts the underlying interface into
default vlan and then actual vlan, in-between this if bonding driver
consults the IP for a route, IP fails to provide a correct route and
upon which bonding driver drops the ARP packet. ARP monitor when it
comes around next time, sees no ARP response and fails-over to the next
available slave. Consulting for a IP route, ip_route_output(),happens in
bond_arp_send_all().
To prevent this false fail-over, when bonding driver fails to send an
ARP out it marks in its private structure, bonding{}, not to expect an
ARP response, when ARP monitor comes around next time ARP sending will
be tried again.
Extensively tested in a VM environment; sr-iov intf->bonding intf->vlan
intf. All virtual interfaces created at boot time.
Orabug: 17172660
Signed-off-by: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
Signed-off-by: Rama Nichanamatlu <rama.nichanamatlu@oracle.com>
---
drivers/net/bonding/bond_main.c | 13 ++++++++-----
drivers/net/bonding/bonding.h | 1 +
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c
b/drivers/net/bonding/bond_main.c
index dde6b4a..d475161 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2661,7 +2661,7 @@ static int bond_has_this_ip(struct bonding *bond,
__be32 ip)
* switches in VLAN mode (especially if ports are configured as
* "native" to a VLAN) might not pass non-tagged frames.
*/
-static void bond_arp_send(struct net_device *slave_dev, int arp_op,
__be32 dest_ip, __be32 src_ip, unsigned short vlan_id)
+static void bond_arp_send(struct bonding *bond, struct net_device
*slave_dev, int arp_op, __be32 dest_ip, __be32 src_ip, unsigned short
vlan_id)
{
struct sk_buff *skb;
@@ -2683,6 +2683,7 @@ static void bond_arp_send(struct net_device
*slave_dev, int arp_op, __be32 dest_
}
}
arp_xmit(skb);
+ bond->arp_sent=true;
}
@@ -2700,7 +2701,7 @@ static void bond_arp_send_all(struct bonding
*bond, struct slave *slave)
pr_debug("basa: target %x\n", targets[i]);
if (!bond->vlgrp) {
pr_debug("basa: empty vlan: arp_send\n");
- bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
+ bond_arp_send(bond, slave->dev, ARPOP_REQUEST, targets[i],
bond->master_ip, 0);
continue;
}
@@ -2726,7 +2727,7 @@ static void bond_arp_send_all(struct bonding
*bond, struct slave *slave)
if (rt->dst.dev == bond->dev) {
ip_rt_put(rt);
pr_debug("basa: rtdev == bond->dev: arp_send\n");
- bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
+ bond_arp_send(bond, slave->dev, ARPOP_REQUEST, targets[i],
bond->master_ip, 0);
continue;
}
@@ -2744,7 +2745,7 @@ static void bond_arp_send_all(struct bonding
*bond, struct slave *slave)
if (vlan_id) {
ip_rt_put(rt);
- bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
+ bond_arp_send(bond, slave->dev, ARPOP_REQUEST, targets[i],
vlan->vlan_ip, vlan_id);
continue;
}
@@ -3206,7 +3207,7 @@ void bond_activebackup_arp_mon(struct work_struct
*work)
should_notify_peers = bond_should_notify_peers(bond);
- if (bond_ab_arp_inspect(bond, delta_in_ticks)) {
+ if (bond->arp_sent && bond_ab_arp_inspect(bond, delta_in_ticks)) {
read_unlock(&bond->lock);
rtnl_lock();
read_lock(&bond->lock);
@@ -3218,6 +3219,7 @@ void bond_activebackup_arp_mon(struct work_struct
*work)
read_lock(&bond->lock);
}
+ bond->arp_sent=false;
bond_ab_arp_probe(bond);
re_arm:
@@ -4425,6 +4427,7 @@ static void bond_setup(struct net_device *bond_dev)
bond_dev->hw_features &= ~(NETIF_F_ALL_CSUM & ~NETIF_F_NO_CSUM);
bond_dev->features |= bond_dev->hw_features;
+ bond->arp_sent=false;
}
static void bond_work_cancel_all(struct bonding *bond)
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index e9a3c56..3878bbd 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -253,6 +253,7 @@ struct bonding {
/* debugging suport via debugfs */
struct dentry *debug_dir;
#endif /* CONFIG_DEBUG_FS */
+ bool arp_sent;
};
#define bond_slave_get_rcu(dev) \
--
1.8.2.1
[-- Attachment #2: Attached Message Part --]
[-- Type: text/plain, Size: 0 bytes --]
^ permalink raw reply related
* Re: [BUG,REGRESSION?] 3.11.6+,3.12: GbE iface rate drops to few KB/s
From: Willy Tarreau @ 2013-11-21 0:44 UTC (permalink / raw)
To: Arnaud Ebalard
Cc: Thomas Petazzoni, Florian Fainelli, simon.guinot, Eric Dumazet,
netdev, edumazet, Cong Wang, linux-arm-kernel
In-Reply-To: <20131120215435.GT8581@1wt.eu>
[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]
Hi Arnaud,
On Wed, Nov 20, 2013 at 10:54:35PM +0100, Willy Tarreau wrote:
> I'm currently trying to implement TX IRQ handling. I found the registers
> description in the neta driver that is provided in Marvell's LSP kernel
> that is shipped with some devices using their CPUs. This code is utterly
> broken (eg: splice fails with -EBADF) but I think the register descriptions
> could be trusted.
>
> I'd rather have real IRQ handling than just relying on mvneta_poll(), so
> that we can use it for asymmetric traffic/routing/whatever.
OK it paid off. And very well :-)
I did it at once and it worked immediately. I generally don't like this
because I always fear that some bug was left there hidden in the code. I have
only tested it on the Mirabox, so I'll have to try on the OpenBlocks AX3-4 and
on the XP-GP board for some SMP stress tests.
I upgraded my Mirabox to latest Linus' git (commit 5527d151) and compared
with and without the patch.
without :
- need at least 12 streams to reach gigabit.
- 60% of idle CPU remains at 1 Gbps
- HTTP connection rate on empty objects is 9950 connections/s
- cumulated outgoing traffic on two ports reaches 1.3 Gbps
with the patch :
- a single stream easily saturates the gigabit
- 87% of idle CPU at 1 Gbps (12 streams, 90% idle at 1 stream)
- HTTP connection rate on empty objects is 10250 connections/s
- I saturate the two gig ports at 99% CPU, so 2 Gbps sustained output.
BTW I must say I was impressed to see that big an improvement in CPU
usage between 3.10 and 3.13, I suspect some of the Tx queue improvements
that Eric has done in between account for this.
I cut the patch in 3 parts :
- one which reintroduces the hidden bits of the driver
- one which replaces the timer with the IRQ
- one which changes the default Tx coalesce from 16 to 4 packets
(larger was preferred with the timer, but less is better now).
I'm attaching them, please test them on your device.
Note that this is *not* for inclusion at the moment as it has not been
tested on the SMP CPUs.
Cheers,
Willy
[-- Attachment #2: 0001-net-mvneta-add-missing-bit-descriptions-for-interrup.patch --]
[-- Type: text/plain, Size: 3902 bytes --]
>From b77b32dbffdfffc6aa21fa230054e09e2a4cd227 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Wed, 20 Nov 2013 23:58:30 +0100
Subject: net: mvneta: add missing bit descriptions for interrupt masks and
causes
Marvell has not published the chip's datasheet yet, so it's very hard
to find the relevant bits to manipulate to change the IRQ behaviour.
Fortunately, these bits are described in the proprietary LSP patch set
which is publicly available here :
http://www.plugcomputer.org/downloads/mirabox/
So let's put them back in the driver in order to reduce the burden of
current and future maintenance.
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 44 +++++++++++++++++++++++++++++++++--
1 file changed, 42 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index b8e232b..6630690 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -101,16 +101,56 @@
#define MVNETA_CPU_RXQ_ACCESS_ALL_MASK 0x000000ff
#define MVNETA_CPU_TXQ_ACCESS_ALL_MASK 0x0000ff00
#define MVNETA_RXQ_TIME_COAL_REG(q) (0x2580 + ((q) << 2))
+
+/* Exception Interrupt Port/Queue Cause register */
+
#define MVNETA_INTR_NEW_CAUSE 0x25a0
-#define MVNETA_RX_INTR_MASK(nr_rxqs) (((1 << nr_rxqs) - 1) << 8)
#define MVNETA_INTR_NEW_MASK 0x25a4
+
+/* bits 0..7 = TXQ SENT, one bit per queue.
+ * bits 8..15 = RXQ OCCUP, one bit per queue.
+ * bits 16..23 = RXQ FREE, one bit per queue.
+ * bit 29 = OLD_REG_SUM, see old reg ?
+ * bit 30 = TX_ERR_SUM, one bit for 4 ports
+ * bit 31 = MISC_SUM, one bit for 4 ports
+ */
+#define MVNETA_TX_INTR_MASK(nr_txqs) (((1 << nr_txqs) - 1) << 0)
+#define MVNETA_TX_INTR_MASK_ALL (0xff << 0)
+#define MVNETA_RX_INTR_MASK(nr_rxqs) (((1 << nr_rxqs) - 1) << 8)
+#define MVNETA_RX_INTR_MASK_ALL (0xff << 8)
+
#define MVNETA_INTR_OLD_CAUSE 0x25a8
#define MVNETA_INTR_OLD_MASK 0x25ac
+
+/* Data Path Port/Queue Cause Register */
#define MVNETA_INTR_MISC_CAUSE 0x25b0
#define MVNETA_INTR_MISC_MASK 0x25b4
+
+#define MVNETA_CAUSE_PHY_STATUS_CHANGE BIT(0)
+#define MVNETA_CAUSE_LINK_CHANGE BIT(1)
+#define MVNETA_CAUSE_PTP BIT(4)
+
+#define MVNETA_CAUSE_INTERNAL_ADDR_ERR BIT(7)
+#define MVNETA_CAUSE_RX_OVERRUN BIT(8)
+#define MVNETA_CAUSE_RX_CRC_ERROR BIT(9)
+#define MVNETA_CAUSE_RX_LARGE_PKT BIT(10)
+#define MVNETA_CAUSE_TX_UNDERUN BIT(11)
+#define MVNETA_CAUSE_PRBS_ERR BIT(12)
+#define MVNETA_CAUSE_PSC_SYNC_CHANGE BIT(13)
+#define MVNETA_CAUSE_SERDES_SYNC_ERR BIT(14)
+
+#define MVNETA_CAUSE_BMU_ALLOC_ERR_SHIFT 16
+#define MVNETA_CAUSE_BMU_ALLOC_ERR_ALL_MASK (0xF << MVNETA_CAUSE_BMU_ALLOC_ERR_SHIFT)
+#define MVNETA_CAUSE_BMU_ALLOC_ERR_MASK(pool) (1 << (MVNETA_CAUSE_BMU_ALLOC_ERR_SHIFT + (pool)))
+
+#define MVNETA_CAUSE_TXQ_ERROR_SHIFT 24
+#define MVNETA_CAUSE_TXQ_ERROR_ALL_MASK (0xFF << MVNETA_CAUSE_TXQ_ERROR_SHIFT)
+#define MVNETA_CAUSE_TXQ_ERROR_MASK(q) (1 << (MVNETA_CAUSE_TXQ_ERROR_SHIFT + (q)))
+
#define MVNETA_INTR_ENABLE 0x25b8
#define MVNETA_TXQ_INTR_ENABLE_ALL_MASK 0x0000ff00
-#define MVNETA_RXQ_INTR_ENABLE_ALL_MASK 0xff000000
+#define MVNETA_RXQ_INTR_ENABLE_ALL_MASK 0xff000000 // note: neta says it's 0x000000FF
+
#define MVNETA_RXQ_CMD 0x2680
#define MVNETA_RXQ_DISABLE_SHIFT 8
#define MVNETA_RXQ_ENABLE_MASK 0x000000ff
--
1.7.12.1
[-- Attachment #3: 0002-net-mvneta-replace-Tx-timer-with-a-real-interrupt.patch --]
[-- Type: text/plain, Size: 6447 bytes --]
>From 741bc1bfccbfe33cebaceb2e854539946e8ec9fa Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Wed, 20 Nov 2013 19:47:11 +0100
Subject: net: mvneta: replace Tx timer with a real interrupt
Right now the mvneta driver doesn't handle Tx IRQ, and solely relies on a
timer to flush Tx descriptors. This causes jerky output traffic with bursts
and pauses, making it difficult to reach line rate with very few streams.
It seems that this feature was inherited from the original driver but
nothing there mentions any reason for not using the interrupt instead,
which the chip supports.
Thus, this patch enables Tx interrupts and removes the timer. It does the
two at once because it's not really possible to make the two mechanisms
coexist, so a split patch doesn't make sense.
First tests performed on a Mirabox (Armada 370) show that much less CPU
is used when sending traffic. One reason might be that we now call the
mvneta_tx_done_gbe() with a mask indicating which queues have been done
instead of looping over all of them.
The results are quite good, a single TCP stream is now capable of saturating
a gigabit link, while a minimum of 12 concurrent streams are needed without
the patch. At 1 Gbps with 12 concurrent streams, 60% of the CPU remains idle
without the patch, and it grows to 87% with the patch. The connection rate
has also increased from 9950 to 10150 connections per second (HTTP requests
of empty objects).
Signed-off-by: Willy Tarreau <w@1wt.eu>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnaud Ebalard <arno@natisbad.org>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---
drivers/net/ethernet/marvell/mvneta.c | 71 ++++++-----------------------------
1 file changed, 12 insertions(+), 59 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 6630690..def32a8 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -216,9 +216,6 @@
#define MVNETA_RX_COAL_PKTS 32
#define MVNETA_RX_COAL_USEC 100
-/* Timer */
-#define MVNETA_TX_DONE_TIMER_PERIOD 10
-
/* Napi polling weight */
#define MVNETA_RX_POLL_WEIGHT 64
@@ -272,16 +269,11 @@ struct mvneta_port {
void __iomem *base;
struct mvneta_rx_queue *rxqs;
struct mvneta_tx_queue *txqs;
- struct timer_list tx_done_timer;
struct net_device *dev;
u32 cause_rx_tx;
struct napi_struct napi;
- /* Flags */
- unsigned long flags;
-#define MVNETA_F_TX_DONE_TIMER_BIT 0
-
/* Napi weight */
int weight;
@@ -1140,17 +1132,6 @@ static void mvneta_tx_done_pkts_coal_set(struct mvneta_port *pp,
txq->done_pkts_coal = value;
}
-/* Trigger tx done timer in MVNETA_TX_DONE_TIMER_PERIOD msecs */
-static void mvneta_add_tx_done_timer(struct mvneta_port *pp)
-{
- if (test_and_set_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags) == 0) {
- pp->tx_done_timer.expires = jiffies +
- msecs_to_jiffies(MVNETA_TX_DONE_TIMER_PERIOD);
- add_timer(&pp->tx_done_timer);
- }
-}
-
-
/* Handle rx descriptor fill by setting buf_cookie and buf_phys_addr */
static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
u32 phys_addr, u32 cookie)
@@ -1632,15 +1613,6 @@ out:
dev_kfree_skb_any(skb);
}
- if (txq->count >= MVNETA_TXDONE_COAL_PKTS)
- mvneta_txq_done(pp, txq);
-
- /* If after calling mvneta_txq_done, count equals
- * frags, we need to set the timer
- */
- if (txq->count == frags && frags > 0)
- mvneta_add_tx_done_timer(pp);
-
return NETDEV_TX_OK;
}
@@ -1916,14 +1888,22 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
/* Read cause register */
cause_rx_tx = mvreg_read(pp, MVNETA_INTR_NEW_CAUSE) &
- MVNETA_RX_INTR_MASK(rxq_number);
+ (MVNETA_RX_INTR_MASK(rxq_number) | MVNETA_TX_INTR_MASK(txq_number));
+
+ /* Release Tx descriptors */
+ if (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL) {
+ int tx_todo = 0;
+
+ mvneta_tx_done_gbe(pp, (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL), &tx_todo);
+ cause_rx_tx &= ~MVNETA_TX_INTR_MASK_ALL;
+ }
/* For the case where the last mvneta_poll did not process all
* RX packets
*/
cause_rx_tx |= pp->cause_rx_tx;
if (rxq_number > 1) {
- while ((cause_rx_tx != 0) && (budget > 0)) {
+ while ((cause_rx_tx & MVNETA_RX_INTR_MASK_ALL) && (budget > 0)) {
int count;
struct mvneta_rx_queue *rxq;
/* get rx queue number from cause_rx_tx */
@@ -1955,7 +1935,7 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
napi_complete(napi);
local_irq_save(flags);
mvreg_write(pp, MVNETA_INTR_NEW_MASK,
- MVNETA_RX_INTR_MASK(rxq_number));
+ MVNETA_RX_INTR_MASK(rxq_number) | MVNETA_TX_INTR_MASK(txq_number));
local_irq_restore(flags);
}
@@ -1963,26 +1943,6 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
return rx_done;
}
-/* tx done timer callback */
-static void mvneta_tx_done_timer_callback(unsigned long data)
-{
- struct net_device *dev = (struct net_device *)data;
- struct mvneta_port *pp = netdev_priv(dev);
- int tx_done = 0, tx_todo = 0;
-
- if (!netif_running(dev))
- return ;
-
- clear_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags);
-
- tx_done = mvneta_tx_done_gbe(pp,
- (((1 << txq_number) - 1) &
- MVNETA_CAUSE_TXQ_SENT_DESC_ALL_MASK),
- &tx_todo);
- if (tx_todo > 0)
- mvneta_add_tx_done_timer(pp);
-}
-
/* Handle rxq fill: allocates rxq skbs; called when initializing a port */
static int mvneta_rxq_fill(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
int num)
@@ -2232,7 +2192,7 @@ static void mvneta_start_dev(struct mvneta_port *pp)
/* Unmask interrupts */
mvreg_write(pp, MVNETA_INTR_NEW_MASK,
- MVNETA_RX_INTR_MASK(rxq_number));
+ MVNETA_RX_INTR_MASK(rxq_number) | MVNETA_TX_INTR_MASK(txq_number));
phy_start(pp->phy_dev);
netif_tx_start_all_queues(pp->dev);
@@ -2518,8 +2478,6 @@ static int mvneta_stop(struct net_device *dev)
free_irq(dev->irq, pp);
mvneta_cleanup_rxqs(pp);
mvneta_cleanup_txqs(pp);
- del_timer(&pp->tx_done_timer);
- clear_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags);
return 0;
}
@@ -2868,11 +2826,6 @@ static int mvneta_probe(struct platform_device *pdev)
}
}
- pp->tx_done_timer.data = (unsigned long)dev;
- pp->tx_done_timer.function = mvneta_tx_done_timer_callback;
- init_timer(&pp->tx_done_timer);
- clear_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags);
-
pp->tx_ring_size = MVNETA_MAX_TXD;
pp->rx_ring_size = MVNETA_MAX_RXD;
--
1.7.12.1
[-- Attachment #4: 0003-net-mvneta-reduce-Tx-coalesce-from-16-to-4-packets.patch --]
[-- Type: text/plain, Size: 1012 bytes --]
>From 04a4891c4f9a77052e5aea7d2ade25a3f8da5436 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Thu, 21 Nov 2013 00:13:06 +0100
Subject: net: mvneta: reduce Tx coalesce from 16 to 4 packets
I'm getting slightly better performance with a smaller Tx coalesce setting,
both with large and short packets. Since it was used differently with the
timer, it is possible that the previous value was more suited for use with
a slow timer.
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index def32a8..d188828 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -212,7 +212,7 @@
/* Various constants */
/* Coalescing */
-#define MVNETA_TXDONE_COAL_PKTS 16
+#define MVNETA_TXDONE_COAL_PKTS 4
#define MVNETA_RX_COAL_PKTS 32
#define MVNETA_RX_COAL_USEC 100
--
1.7.12.1
[-- Attachment #5: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Issue with gratuitous arps when new addr is different from cached addr
From: Salam Noureddine @ 2013-11-21 0:40 UTC (permalink / raw)
To: David S. Miller, Daniel Borkmann, Willem de Bruijn, Phil Sutter,
Eric Dumazet, netdev
Hi,
It seems to me that neigh_update is not handling correctly the case
when the new address is different from the cached one and
NEIGH_UPDATE_F_OVERRIDE is not set. When we receive a gratuitous arp
request we check jiffies against the neigh->updated + locktime in
arp_process. If we're passed that time then the flag is set.
In neigh_update, we set neigh->updated before checking for the case
where we have a new address and the override flag is not set. This
means, that we "extend the life of the old address". By setting
locktime to 2 sec and sending an arp with a new address every 1 sec, I
was able to perpetuate the old entry for as long as I wanted.
To fix this, we can just move setting neigh->updated to after the
check for new address and override flag not present,
--- linux-3.4.orig/net/core/neighbour.c
+++ linux-3.4/net/core/neighbour.c
@@ -1206,10 +1206,6 @@ int neigh_update(struct neighbour *neigh
lladdr = neigh->ha;
}
- if (new & NUD_CONNECTED)
- neigh->confirmed = jiffies;
- neigh->updated = jiffies;
-
/* If entry was valid and address is not changed,
do not change entry state, if new one is STALE.
*/
@@ -1233,6 +1229,10 @@ int neigh_update(struct neighbour *neigh
}
}
+ if (new & NUD_CONNECTED)
+ neigh->confirmed = jiffies;
+ neigh->updated = jiffies;
+
if (new != old) {
neigh_del_timer(neigh);
if (new & NUD_IN_TIMER)
If that seems like a an acceptable solution, I would post a patch shortly.
Thanks,
Salam
^ permalink raw reply
* [PATCH] net: rework recvmsg handler msg_name and msg_namelen logic
From: Hannes Frederic Sowa @ 2013-11-21 0:38 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet
This patch now always passes msg->msg_namelen as 0. recvmsg handlers must
set msg_namelen to the proper size <= sizeof(struct sockaddr_storage)
to return msg_name to the user.
This prevents numerous uninitialized memory leaks we had in the
recvmsg handlers and makes it harder for new code to accidentally leak
uninitialized memory.
Optimize for the case recvfrom is called with NULL as address. We don't
need to copy the address at all, so set it to NULL before invoking the
recvmsg handler. We can do so, because all the recvmsg handlers must
cope with the case a plain read() is called on them. read() also sets
msg_name to NULL.
Also document these changes in include/linux/net.h as suggested by David
Miller.
=== Changes since RFC: ===
Set msg->msg_name = NULL if user specified a NULL in msg_name but had a
non-null msg_namelen in verify_iovec/verify_compat_iovec. This doesn't
affect sendto as it would bail out earlier while trying to copy-in
the address (it more naturally reflects the logic by the callers of
verify_iovec).
With this change in place I could remove "
if (!uaddr || msg_sys->msg_namelen == 0)
msg->msg_name = NULL
".
This change does not alter the user visible error logic as we ignore
msg_namelen as long as msg_name is NULL.
Also remove two unnecessary curly brackets in ___sys_recvmsg and change
comments to netdev style.
Cc: David Miller <davem@davemloft.net>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
crypto/algif_hash.c | 2 --
crypto/algif_skcipher.c | 1 -
drivers/isdn/mISDN/socket.c | 14 +++++---------
drivers/net/ppp/pppoe.c | 2 --
include/linux/net.h | 8 ++++++++
net/appletalk/ddp.c | 17 ++++++++---------
net/atm/common.c | 2 --
net/ax25/af_ax25.c | 5 +++--
net/bluetooth/af_bluetooth.c | 9 ++-------
net/bluetooth/hci_sock.c | 2 --
net/bluetooth/rfcomm/sock.c | 1 -
net/bluetooth/sco.c | 1 -
net/caif/caif_socket.c | 4 ----
net/compat.c | 2 +-
net/core/iovec.c | 2 +-
net/ipx/af_ipx.c | 3 +--
net/irda/af_irda.c | 4 ----
net/iucv/af_iucv.c | 2 --
net/key/af_key.c | 1 -
net/l2tp/l2tp_ppp.c | 2 --
net/llc/af_llc.c | 2 --
net/netlink/af_netlink.c | 2 --
net/netrom/af_netrom.c | 3 +--
net/nfc/llcp_sock.c | 2 --
net/nfc/rawsock.c | 2 --
net/packet/af_packet.c | 32 +++++++++++++++-----------------
net/rds/recv.c | 2 --
net/rose/af_rose.c | 8 +++++---
net/rxrpc/ar-recvmsg.c | 9 ++++++---
net/socket.c | 19 +++++++++++--------
net/tipc/socket.c | 6 ------
net/unix/af_unix.c | 5 -----
net/vmw_vsock/af_vsock.c | 2 --
net/vmw_vsock/vmci_transport.c | 2 --
net/x25/af_x25.c | 3 +--
35 files changed, 68 insertions(+), 115 deletions(-)
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 0262210..ef5356c 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -161,8 +161,6 @@ static int hash_recvmsg(struct kiocb *unused, struct socket *sock,
else if (len < ds)
msg->msg_flags |= MSG_TRUNC;
- msg->msg_namelen = 0;
-
lock_sock(sk);
if (ctx->more) {
ctx->more = 0;
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index a1c4f0a..6a6dfc0 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -432,7 +432,6 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
long copied = 0;
lock_sock(sk);
- msg->msg_namelen = 0;
for (iov = msg->msg_iov, iovlen = msg->msg_iovlen; iovlen > 0;
iovlen--, iov++) {
unsigned long seglen = iov->iov_len;
diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c
index e47dcb9..f31c17c 100644
--- a/drivers/isdn/mISDN/socket.c
+++ b/drivers/isdn/mISDN/socket.c
@@ -117,7 +117,6 @@ mISDN_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
{
struct sk_buff *skb;
struct sock *sk = sock->sk;
- struct sockaddr_mISDN *maddr;
int copied, err;
@@ -135,9 +134,10 @@ mISDN_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
if (!skb)
return err;
- if (msg->msg_namelen >= sizeof(struct sockaddr_mISDN)) {
- msg->msg_namelen = sizeof(struct sockaddr_mISDN);
- maddr = (struct sockaddr_mISDN *)msg->msg_name;
+ if (msg->msg_name) {
+ struct sockaddr_mISDN *maddr =
+ (struct sockaddr_mISDN *)msg->msg_name;
+
maddr->family = AF_ISDN;
maddr->dev = _pms(sk)->dev->id;
if ((sk->sk_protocol == ISDN_P_LAPD_TE) ||
@@ -150,11 +150,7 @@ mISDN_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
maddr->sapi = _pms(sk)->ch.addr & 0xFF;
maddr->tei = (_pms(sk)->ch.addr >> 8) & 0xFF;
}
- } else {
- if (msg->msg_namelen)
- printk(KERN_WARNING "%s: too small namelen %d\n",
- __func__, msg->msg_namelen);
- msg->msg_namelen = 0;
+ msg->msg_namelen = sizeof(*maddr);
}
copied = skb->len + MISDN_HEADER_LEN;
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 5f66e30..82ee6ed 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -979,8 +979,6 @@ static int pppoe_recvmsg(struct kiocb *iocb, struct socket *sock,
if (error < 0)
goto end;
- m->msg_namelen = 0;
-
if (skb) {
total_len = min_t(size_t, total_len, skb->len);
error = skb_copy_datagram_iovec(skb, 0, m->msg_iov, total_len);
diff --git a/include/linux/net.h b/include/linux/net.h
index b292a04..4bcee94 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -164,6 +164,14 @@ struct proto_ops {
#endif
int (*sendmsg) (struct kiocb *iocb, struct socket *sock,
struct msghdr *m, size_t total_len);
+ /* Notes for implementing recvmsg:
+ * ===============================
+ * msg->msg_namelen should get updated by the recvmsg handlers
+ * iff msg_name != NULL. It is by default 0 to prevent
+ * returning uninitialized memory to user space. The recvfrom
+ * handlers can assume that msg.msg_name is either NULL or has
+ * a minimum size of sizeof(struct sockaddr_storage).
+ */
int (*recvmsg) (struct kiocb *iocb, struct socket *sock,
struct msghdr *m, size_t total_len,
int flags);
diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 7fee50d..20f6e3b 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -1735,7 +1735,6 @@ static int atalk_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr
size_t size, int flags)
{
struct sock *sk = sock->sk;
- struct sockaddr_at *sat = (struct sockaddr_at *)msg->msg_name;
struct ddpehdr *ddp;
int copied = 0;
int offset = 0;
@@ -1764,14 +1763,14 @@ static int atalk_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr
}
err = skb_copy_datagram_iovec(skb, offset, msg->msg_iov, copied);
- if (!err) {
- if (sat) {
- sat->sat_family = AF_APPLETALK;
- sat->sat_port = ddp->deh_sport;
- sat->sat_addr.s_node = ddp->deh_snode;
- sat->sat_addr.s_net = ddp->deh_snet;
- }
- msg->msg_namelen = sizeof(*sat);
+ if (!err && msg->msg_name) {
+ struct sockaddr_at *sat =
+ (struct sockaddr_at *)msg->msg_name;
+ sat->sat_family = AF_APPLETALK;
+ sat->sat_port = ddp->deh_sport;
+ sat->sat_addr.s_node = ddp->deh_snode;
+ sat->sat_addr.s_net = ddp->deh_snet;
+ msg->msg_namelen = sizeof(*sat);
}
skb_free_datagram(sk, skb); /* Free the datagram. */
diff --git a/net/atm/common.c b/net/atm/common.c
index 737bef5..7b49100 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -531,8 +531,6 @@ int vcc_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg,
struct sk_buff *skb;
int copied, error = -EINVAL;
- msg->msg_namelen = 0;
-
if (sock->state != SS_CONNECTED)
return -ENOTCONN;
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index a00123e..b64aea5 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1636,11 +1636,12 @@ static int ax25_recvmsg(struct kiocb *iocb, struct socket *sock,
skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);
- if (msg->msg_namelen != 0) {
- struct sockaddr_ax25 *sax = (struct sockaddr_ax25 *)msg->msg_name;
+ if (msg->msg_name) {
ax25_digi digi;
ax25_address src;
const unsigned char *mac = skb_mac_header(skb);
+ struct sockaddr_ax25 *sax =
+ (struct sockaddr_ax25 *)msg->msg_name;
memset(sax, 0, sizeof(struct full_sockaddr_ax25));
ax25_addr_parse(mac + 1, skb->data - mac - 1, &src, NULL,
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index f6a1671..56ca494 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -224,10 +224,9 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
skb = skb_recv_datagram(sk, flags, noblock, &err);
if (!skb) {
- if (sk->sk_shutdown & RCV_SHUTDOWN) {
- msg->msg_namelen = 0;
+ if (sk->sk_shutdown & RCV_SHUTDOWN)
return 0;
- }
+
return err;
}
@@ -245,8 +244,6 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
if (bt_sk(sk)->skb_msg_name)
bt_sk(sk)->skb_msg_name(skb, msg->msg_name,
&msg->msg_namelen);
- else
- msg->msg_namelen = 0;
}
skb_free_datagram(sk, skb);
@@ -295,8 +292,6 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
if (flags & MSG_OOB)
return -EOPNOTSUPP;
- msg->msg_namelen = 0;
-
BT_DBG("sk %p size %zu", sk, size);
lock_sock(sk);
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 71f0be1..6a6c8bb 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -856,8 +856,6 @@ static int hci_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
if (!skb)
return err;
- msg->msg_namelen = 0;
-
copied = skb->len;
if (len < copied) {
msg->msg_flags |= MSG_TRUNC;
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index c4d3d42..c80766f 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -615,7 +615,6 @@ static int rfcomm_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
if (test_and_clear_bit(RFCOMM_DEFER_SETUP, &d->flags)) {
rfcomm_dlc_accept(d);
- msg->msg_namelen = 0;
return 0;
}
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 12a0e51..24fa396 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -711,7 +711,6 @@ static int sco_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
sco_conn_defer_accept(pi->conn->hcon, pi->setting);
sk->sk_state = BT_CONFIG;
- msg->msg_namelen = 0;
release_sock(sk);
return 0;
diff --git a/net/caif/caif_socket.c b/net/caif/caif_socket.c
index 05a41c7..d6be3ed 100644
--- a/net/caif/caif_socket.c
+++ b/net/caif/caif_socket.c
@@ -286,8 +286,6 @@ static int caif_seqpkt_recvmsg(struct kiocb *iocb, struct socket *sock,
if (m->msg_flags&MSG_OOB)
goto read_error;
- m->msg_namelen = 0;
-
skb = skb_recv_datagram(sk, flags, 0 , &ret);
if (!skb)
goto read_error;
@@ -361,8 +359,6 @@ static int caif_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
if (flags&MSG_OOB)
goto out;
- msg->msg_namelen = 0;
-
/*
* Lock the socket to prevent queue disordering
* while sleeps in memcpy_tomsg
diff --git a/net/compat.c b/net/compat.c
index 8903258..08cd1c8 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -93,7 +93,7 @@ int verify_compat_iovec(struct msghdr *kern_msg, struct iovec *kern_iov,
if (err < 0)
return err;
}
- kern_msg->msg_name = kern_address;
+ kern_msg->msg_name = (kern_msg->msg_name) ? kern_address : NULL;
} else
kern_msg->msg_name = NULL;
diff --git a/net/core/iovec.c b/net/core/iovec.c
index 4cdb7c4..5b0c48e 100644
--- a/net/core/iovec.c
+++ b/net/core/iovec.c
@@ -48,7 +48,7 @@ int verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr_storage *a
if (err < 0)
return err;
}
- m->msg_name = address;
+ m->msg_name = (m->msg_name) ? address : NULL;
} else {
m->msg_name = NULL;
}
diff --git a/net/ipx/af_ipx.c b/net/ipx/af_ipx.c
index 7a1e0fc..e096025 100644
--- a/net/ipx/af_ipx.c
+++ b/net/ipx/af_ipx.c
@@ -1823,8 +1823,6 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
if (skb->tstamp.tv64)
sk->sk_stamp = skb->tstamp;
- msg->msg_namelen = sizeof(*sipx);
-
if (sipx) {
sipx->sipx_family = AF_IPX;
sipx->sipx_port = ipx->ipx_source.sock;
@@ -1832,6 +1830,7 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
sipx->sipx_network = IPX_SKB_CB(skb)->ipx_source_net;
sipx->sipx_type = ipx->ipx_type;
sipx->sipx_zero = 0;
+ msg->msg_namelen = sizeof(*sipx);
}
rc = copied;
diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c
index 0f67690..de7db23 100644
--- a/net/irda/af_irda.c
+++ b/net/irda/af_irda.c
@@ -1385,8 +1385,6 @@ static int irda_recvmsg_dgram(struct kiocb *iocb, struct socket *sock,
IRDA_DEBUG(4, "%s()\n", __func__);
- msg->msg_namelen = 0;
-
skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
flags & MSG_DONTWAIT, &err);
if (!skb)
@@ -1451,8 +1449,6 @@ static int irda_recvmsg_stream(struct kiocb *iocb, struct socket *sock,
target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
timeo = sock_rcvtimeo(sk, noblock);
- msg->msg_namelen = 0;
-
do {
int chunk;
struct sk_buff *skb = skb_dequeue(&sk->sk_receive_queue);
diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 168aff5..c4b7218 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1324,8 +1324,6 @@ static int iucv_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
int err = 0;
u32 offset;
- msg->msg_namelen = 0;
-
if ((sk->sk_state == IUCV_DISCONN) &&
skb_queue_empty(&iucv->backlog_skb_q) &&
skb_queue_empty(&sk->sk_receive_queue) &&
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 911ef03..545f047 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -3616,7 +3616,6 @@ static int pfkey_recvmsg(struct kiocb *kiocb,
if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT))
goto out;
- msg->msg_namelen = 0;
skb = skb_recv_datagram(sk, flags, flags & MSG_DONTWAIT, &err);
if (skb == NULL)
goto out;
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index ffda81e..be5fadf 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -197,8 +197,6 @@ static int pppol2tp_recvmsg(struct kiocb *iocb, struct socket *sock,
if (sk->sk_state & PPPOX_BOUND)
goto end;
- msg->msg_namelen = 0;
-
err = 0;
skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
flags & MSG_DONTWAIT, &err);
diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index 6cba486..7b01b9f 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -720,8 +720,6 @@ static int llc_ui_recvmsg(struct kiocb *iocb, struct socket *sock,
int target; /* Read at least this many bytes */
long timeo;
- msg->msg_namelen = 0;
-
lock_sock(sk);
copied = -ENOTCONN;
if (unlikely(sk->sk_type == SOCK_STREAM && sk->sk_state == TCP_LISTEN))
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index f0176e1..bca50b9 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2335,8 +2335,6 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
}
#endif
- msg->msg_namelen = 0;
-
copied = data_skb->len;
if (len < copied) {
msg->msg_flags |= MSG_TRUNC;
diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
index 698814b..53c19a3 100644
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -1179,10 +1179,9 @@ static int nr_recvmsg(struct kiocb *iocb, struct socket *sock,
sax->sax25_family = AF_NETROM;
skb_copy_from_linear_data_offset(skb, 7, sax->sax25_call.ax25_call,
AX25_ADDR_LEN);
+ msg->msg_namelen = sizeof(*sax);
}
- msg->msg_namelen = sizeof(*sax);
-
skb_free_datagram(sk, skb);
release_sock(sk);
diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index d308402..824c605 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -807,8 +807,6 @@ static int llcp_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
pr_debug("%p %zu\n", sk, len);
- msg->msg_namelen = 0;
-
lock_sock(sk);
if (sk->sk_state == LLCP_CLOSED &&
diff --git a/net/nfc/rawsock.c b/net/nfc/rawsock.c
index cd958b3..66bcd2e 100644
--- a/net/nfc/rawsock.c
+++ b/net/nfc/rawsock.c
@@ -244,8 +244,6 @@ static int rawsock_recvmsg(struct kiocb *iocb, struct socket *sock,
if (!skb)
return rc;
- msg->msg_namelen = 0;
-
copied = skb->len;
if (len < copied) {
msg->msg_flags |= MSG_TRUNC;
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2e8286b..61bd50a 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2660,7 +2660,6 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
struct sock *sk = sock->sk;
struct sk_buff *skb;
int copied, err;
- struct sockaddr_ll *sll;
int vnet_hdr_len = 0;
err = -EINVAL;
@@ -2744,22 +2743,10 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
goto out_free;
}
- /*
- * If the address length field is there to be filled in, we fill
- * it in now.
- */
-
- sll = &PACKET_SKB_CB(skb)->sa.ll;
- if (sock->type == SOCK_PACKET)
- msg->msg_namelen = sizeof(struct sockaddr_pkt);
- else
- msg->msg_namelen = sll->sll_halen + offsetof(struct sockaddr_ll, sll_addr);
-
- /*
- * You lose any data beyond the buffer you gave. If it worries a
- * user program they can ask the device for its MTU anyway.
+ /* You lose any data beyond the buffer you gave. If it worries
+ * a user program they can ask the device for its MTU
+ * anyway.
*/
-
copied = skb->len;
if (copied > len) {
copied = len;
@@ -2772,9 +2759,20 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
sock_recv_ts_and_drops(msg, sk, skb);
- if (msg->msg_name)
+ if (msg->msg_name) {
+ /* If the address length field is there to be filled
+ * in, we fill it in now.
+ */
+ if (sock->type == SOCK_PACKET) {
+ msg->msg_namelen = sizeof(struct sockaddr_pkt);
+ } else {
+ struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll;
+ msg->msg_namelen = sll->sll_halen +
+ offsetof(struct sockaddr_ll, sll_addr);
+ }
memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa,
msg->msg_namelen);
+ }
if (pkt_sk(sk)->auxdata) {
struct tpacket_auxdata aux;
diff --git a/net/rds/recv.c b/net/rds/recv.c
index 9f0f17c..de339b2 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -410,8 +410,6 @@ int rds_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg,
rdsdebug("size %zu flags 0x%x timeo %ld\n", size, msg_flags, timeo);
- msg->msg_namelen = 0;
-
if (msg_flags & MSG_OOB)
goto out;
diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index e98fcfb..2b491bf 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -1216,7 +1216,6 @@ static int rose_recvmsg(struct kiocb *iocb, struct socket *sock,
{
struct sock *sk = sock->sk;
struct rose_sock *rose = rose_sk(sk);
- struct sockaddr_rose *srose = (struct sockaddr_rose *)msg->msg_name;
size_t copied;
unsigned char *asmptr;
struct sk_buff *skb;
@@ -1252,8 +1251,11 @@ static int rose_recvmsg(struct kiocb *iocb, struct socket *sock,
skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);
- if (srose != NULL) {
- memset(srose, 0, msg->msg_namelen);
+ if (msg->msg_name) {
+ struct sockaddr_rose *srose;
+
+ memset(msg->msg_name, 0, sizeof(struct full_sockaddr_rose));
+ srose = (struct sockaddr_rose *)msg->msg_name;
srose->srose_family = AF_ROSE;
srose->srose_addr = rose->dest_addr;
srose->srose_call = rose->dest_call;
diff --git a/net/rxrpc/ar-recvmsg.c b/net/rxrpc/ar-recvmsg.c
index 4b48687..898492a 100644
--- a/net/rxrpc/ar-recvmsg.c
+++ b/net/rxrpc/ar-recvmsg.c
@@ -143,10 +143,13 @@ int rxrpc_recvmsg(struct kiocb *iocb, struct socket *sock,
/* copy the peer address and timestamp */
if (!continue_call) {
- if (msg->msg_name && msg->msg_namelen > 0)
+ if (msg->msg_name) {
+ size_t len =
+ sizeof(call->conn->trans->peer->srx);
memcpy(msg->msg_name,
- &call->conn->trans->peer->srx,
- sizeof(call->conn->trans->peer->srx));
+ &call->conn->trans->peer->srx, len);
+ msg->msg_namelen = len;
+ }
sock_recv_ts_and_drops(msg, &rx->sk, skb);
}
diff --git a/net/socket.c b/net/socket.c
index c226ace..fc28556 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1840,8 +1840,10 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
msg.msg_iov = &iov;
iov.iov_len = size;
iov.iov_base = ubuf;
- msg.msg_name = (struct sockaddr *)&address;
- msg.msg_namelen = sizeof(address);
+ /* Save some cycles and don't copy the address if not needed */
+ msg.msg_name = addr ? (struct sockaddr *)&address : NULL;
+ /* We assume all kernel code knows the size of sockaddr_storage */
+ msg.msg_namelen = 0;
if (sock->file->f_flags & O_NONBLOCK)
flags |= MSG_DONTWAIT;
err = sock_recvmsg(sock, &msg, size, flags);
@@ -2221,16 +2223,14 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg,
goto out;
}
- /*
- * Save the user-mode address (verify_iovec will change the
- * kernel msghdr to use the kernel address space)
+ /* Save the user-mode address (verify_iovec will change the
+ * kernel msghdr to use the kernel address space)
*/
-
uaddr = (__force void __user *)msg_sys->msg_name;
uaddr_len = COMPAT_NAMELEN(msg);
- if (MSG_CMSG_COMPAT & flags) {
+ if (MSG_CMSG_COMPAT & flags)
err = verify_compat_iovec(msg_sys, iov, &addr, VERIFY_WRITE);
- } else
+ else
err = verify_iovec(msg_sys, iov, &addr, VERIFY_WRITE);
if (err < 0)
goto out_freeiov;
@@ -2239,6 +2239,9 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg,
cmsg_ptr = (unsigned long)msg_sys->msg_control;
msg_sys->msg_flags = flags & (MSG_CMSG_CLOEXEC|MSG_CMSG_COMPAT);
+ /* We assume all kernel code knows the size of sockaddr_storage */
+ msg_sys->msg_namelen = 0;
+
if (sock->file->f_flags & O_NONBLOCK)
flags |= MSG_DONTWAIT;
err = (nosec ? sock_recvmsg_nosec : sock_recvmsg)(sock, msg_sys,
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 3906527..3b61851 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -980,9 +980,6 @@ static int recv_msg(struct kiocb *iocb, struct socket *sock,
goto exit;
}
- /* will be updated in set_orig_addr() if needed */
- m->msg_namelen = 0;
-
timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
restart:
@@ -1091,9 +1088,6 @@ static int recv_stream(struct kiocb *iocb, struct socket *sock,
goto exit;
}
- /* will be updated in set_orig_addr() if needed */
- m->msg_namelen = 0;
-
target = sock_rcvlowat(sk, flags & MSG_WAITALL, buf_len);
timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index c1f403b..01625cc 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1754,7 +1754,6 @@ static void unix_copy_addr(struct msghdr *msg, struct sock *sk)
{
struct unix_sock *u = unix_sk(sk);
- msg->msg_namelen = 0;
if (u->addr) {
msg->msg_namelen = u->addr->len;
memcpy(msg->msg_name, u->addr->name, u->addr->len);
@@ -1778,8 +1777,6 @@ static int unix_dgram_recvmsg(struct kiocb *iocb, struct socket *sock,
if (flags&MSG_OOB)
goto out;
- msg->msg_namelen = 0;
-
err = mutex_lock_interruptible(&u->readlock);
if (err) {
err = sock_intr_errno(sock_rcvtimeo(sk, noblock));
@@ -1924,8 +1921,6 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
target = sock_rcvlowat(sk, flags&MSG_WAITALL, size);
timeo = sock_rcvtimeo(sk, flags&MSG_DONTWAIT);
- msg->msg_namelen = 0;
-
/* Lock the socket to prevent queue disordering
* while sleeps in memcpy_tomsg
*/
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 545c08b..5adfd94 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1662,8 +1662,6 @@ vsock_stream_recvmsg(struct kiocb *kiocb,
vsk = vsock_sk(sk);
err = 0;
- msg->msg_namelen = 0;
-
lock_sock(sk);
if (sk->sk_state != SS_CONNECTED) {
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 9d69866..687360d 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -1746,8 +1746,6 @@ static int vmci_transport_dgram_dequeue(struct kiocb *kiocb,
if (flags & MSG_OOB || flags & MSG_ERRQUEUE)
return -EOPNOTSUPP;
- msg->msg_namelen = 0;
-
/* Retrieve the head sk_buff from the socket's receive queue. */
err = 0;
skb = skb_recv_datagram(&vsk->sk, flags, noblock, &err);
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 45a3ab5..7622789 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -1340,10 +1340,9 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock,
if (sx25) {
sx25->sx25_family = AF_X25;
sx25->sx25_addr = x25->dest_addr;
+ msg->msg_namelen = sizeof(*sx25);
}
- msg->msg_namelen = sizeof(struct sockaddr_x25);
-
x25_check_rbuf(sk);
rc = copied;
out_free_dgram:
--
1.8.3.1
^ permalink raw reply related
* Re: Get rxhash fixes and RFS support in tun
From: David Miller @ 2013-11-21 0:02 UTC (permalink / raw)
To: therbert; +Cc: netdev, edumazet, hkchu
In-Reply-To: <alpine.DEB.2.02.1311201220510.19725@tomh.mtv.corp.google.com>
From: Tom Herbert <therbert@google.com>
Date: Wed, 20 Nov 2013 12:25:48 -0800 (PST)
> This patch series fixes some subtle bugs in tun use of skb->rxhash, all
> rxhash hash not be cleared appropraitely, and adds support for tun flows
> to work with RFS.
>
> Testing, in particular with tun, hasn't been completed yet.
I think this needs to be reworked slightly.
We really only have two boolean states:
1) Is the rxhash value in this SKB valid?
2) Is it a full L4 tuple hash?
You are adding a "this is a SW computed hash" boolean state but I do
not think you should distinguish sw vs. hw especially. If the
hardware computed the rxhash on a tunneled packet in the
pre-decapsulated state, we very much want to recompute it, in
software, upon tunnel decapsulation in ip_tunnel_core.c
This is actually implemented in this patch set, by testing two states.
Both the "l4_rxhash" and "sw_rxhash".
Why don't we just do everything in a straightforward manner, where
nothing directly sets rxhash values. Only helper routines do.
1) skb_set_rxhash(struct sk_buff *skb, __u32 rxhash, bool l4_rxhash)
Update all drivers to call this.
2) Add "rxhash_valid" boolean to sk_buff, set it in skb_set_rxhash,
test it in skb_get_rxhash(), propagate it in SKB copies.
3) Your skb_clear_rxhash() now just clears rxhash_valid.
Now, if the issue is that HW computed hashes sometimes do the tunnel
demux and look into the inner L4 headers to compute the hash, you'll
need a boolean to indicate _that_ rather than unconditionally treating
hardware that way. Because not all of them will do this, and for
those that do not you do want to compute the hash in SW after tunnel
decapsulation.
Thoughts?
^ permalink raw reply
* Facebook End Of Year Promotional Award
From: Facebook Award @ 2013-11-20 19:38 UTC (permalink / raw)
To: Recipients
You Have Been Awarded £100,000 Pounds By Facebook End Of Year Promotional Award. Contact Our Agent:
Le ha concedido £ 100.000 libras por Facebook Fin Año Premio Promocional Of. Póngase en contacto con nuestro agente:
^ permalink raw reply
* FINAL RESULT
From: NOKIA @ 2013-11-20 23:22 UTC (permalink / raw)
To: Recipients
YOUR MAIL WAS SELECTED FOR CLAIMS OF $2,500,000.00 IN NOKIA UK PROMOTION. SEND NN013/009/DC13 WITH NAME,ADDRESS,OCCUPATION,AGE,MOBILE,TO NOKIAUKOFFICE@NOKIAMAIL.COM
^ permalink raw reply
* [PATCH v3 2/4] net/phy: Add VSC8574 support
From: Shruti @ 2013-11-20 22:38 UTC (permalink / raw)
To: davem, netdev; +Cc: shaohui xie, Andy Fleming, Shruti Kanetkar
In-Reply-To: <1384987099-4518-1-git-send-email-Shruti@freescale.com>
From: shaohui xie <Shaohui.Xie@freescale.com>
The VSC8574 is a quad-port Gigabit Ethernet transceiver with four SerDes
interfaces for quad-port dual media capability.
Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
Signed-off-by: Andy Fleming <afleming@gmail.com>
Signed-off-by: Shruti Kanetkar <Shruti@freescale.com>
---
drivers/net/phy/vitesse.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 59e3b58..31b3148 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -56,6 +56,7 @@
#define PHY_ID_VSC8234 0x000fc620
#define PHY_ID_VSC8244 0x000fc6c0
+#define PHY_ID_VSC8574 0x000704a0
#define PHY_ID_VSC8221 0x000fc550
#define PHY_ID_VSC8211 0x000fc4b0
@@ -120,7 +121,8 @@ static int vsc82xx_config_intr(struct phy_device *phydev)
if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
err = phy_write(phydev, MII_VSC8244_IMASK,
(phydev->drv->phy_id == PHY_ID_VSC8234 ||
- phydev->drv->phy_id == PHY_ID_VSC8244) ?
+ phydev->drv->phy_id == PHY_ID_VSC8244 ||
+ phydev->drv->phy_id == PHY_ID_VSC8574) ?
MII_VSC8244_IMASK_MASK :
MII_VSC8221_IMASK_MASK);
else {
@@ -178,6 +180,18 @@ static struct phy_driver vsc82xx_driver[] = {
.config_intr = &vsc82xx_config_intr,
.driver = { .owner = THIS_MODULE,},
}, {
+ .phy_id = PHY_ID_VSC8574,
+ .name = "Vitesse VSC8574",
+ .phy_id_mask = 0x000ffff0,
+ .features = PHY_GBIT_FEATURES,
+ .flags = PHY_HAS_INTERRUPT,
+ .config_init = &vsc824x_config_init,
+ .config_aneg = &genphy_config_aneg,
+ .read_status = &genphy_read_status,
+ .ack_interrupt = &vsc824x_ack_interrupt,
+ .config_intr = &vsc82xx_config_intr,
+ .driver = { .owner = THIS_MODULE,},
+}, {
/* Vitesse 8221 */
.phy_id = PHY_ID_VSC8221,
.phy_id_mask = 0x000ffff0,
@@ -223,6 +237,7 @@ module_exit(vsc82xx_exit);
static struct mdio_device_id __maybe_unused vitesse_tbl[] = {
{ PHY_ID_VSC8234, 0x000ffff0 },
{ PHY_ID_VSC8244, 0x000fffc0 },
+ { PHY_ID_VSC8574, 0x000ffff0 },
{ PHY_ID_VSC8221, 0x000ffff0 },
{ PHY_ID_VSC8211, 0x000ffff0 },
{ }
--
1.7.9.7
^ permalink raw reply related
* [PATCH v3 3/4] net/phy: Add VSC8662 support
From: Shruti @ 2013-11-20 22:38 UTC (permalink / raw)
To: davem, netdev; +Cc: Sandeep Singh, Sandeep Singh, Andy Fleming, Shruti Kanetkar
In-Reply-To: <1384987099-4518-1-git-send-email-Shruti@freescale.com>
From: Sandeep Singh <sandeep@freescale.com>
Vitesse VSC8662 is Dual Port 10/100/1000Base-T Phy
Its register set and features are similar to other Vitesse Phys.
Signed-off-by: Sandeep Singh <Sandeep@freescale.com>
Signed-off-by: Andy Fleming <afleming@gmail.com>
Signed-off-by: Shruti Kanetkar <Shruti@Freescale.com>
---
drivers/net/phy/vitesse.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 31b3148..40406bc 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -57,6 +57,7 @@
#define PHY_ID_VSC8234 0x000fc620
#define PHY_ID_VSC8244 0x000fc6c0
#define PHY_ID_VSC8574 0x000704a0
+#define PHY_ID_VSC8662 0x00070660
#define PHY_ID_VSC8221 0x000fc550
#define PHY_ID_VSC8211 0x000fc4b0
@@ -192,6 +193,18 @@ static struct phy_driver vsc82xx_driver[] = {
.config_intr = &vsc82xx_config_intr,
.driver = { .owner = THIS_MODULE,},
}, {
+ .phy_id = PHY_ID_VSC8662,
+ .name = "Vitesse VSC8662",
+ .phy_id_mask = 0x000ffff0,
+ .features = PHY_GBIT_FEATURES,
+ .flags = PHY_HAS_INTERRUPT,
+ .config_init = &vsc824x_config_init,
+ .config_aneg = &genphy_config_aneg,
+ .read_status = &genphy_read_status,
+ .ack_interrupt = &vsc824x_ack_interrupt,
+ .config_intr = &vsc82xx_config_intr,
+ .driver = { .owner = THIS_MODULE,},
+}, {
/* Vitesse 8221 */
.phy_id = PHY_ID_VSC8221,
.phy_id_mask = 0x000ffff0,
@@ -238,6 +251,7 @@ static struct mdio_device_id __maybe_unused vitesse_tbl[] = {
{ PHY_ID_VSC8234, 0x000ffff0 },
{ PHY_ID_VSC8244, 0x000fffc0 },
{ PHY_ID_VSC8574, 0x000ffff0 },
+ { PHY_ID_VSC8662, 0x000ffff0 },
{ PHY_ID_VSC8221, 0x000ffff0 },
{ PHY_ID_VSC8211, 0x000ffff0 },
{ }
--
1.7.9.7
^ permalink raw reply related
* [PATCH v3 4/4] net/phy: Add the autocross feature for forced links on VSC82x4
From: Shruti @ 2013-11-20 22:38 UTC (permalink / raw)
To: davem, netdev; +Cc: Madalin Bucur, Shruti Kanetkar
In-Reply-To: <1384987099-4518-1-git-send-email-Shruti@freescale.com>
From: Madalin Bucur <madalin.bucur@freescale.com>
Add auto-MDI/MDI-X capability for forced (autonegotiation disabled)
10/100 Mbps speeds on Vitesse VSC82x4 PHYs. Exported previously static
function genphy_setup_forced() required by the new config_aneg handler
in the Vitesse PHY module.
Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
Signed-off-by: Shruti Kanetkar <Shruti@freescale.com>
---
drivers/net/phy/phy_device.c | 4 +--
drivers/net/phy/vitesse.c | 75 +++++++++++++++++++++++++++++++++++++++---
include/linux/phy.h | 1 +
3 files changed, 73 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74630e9..d6447b3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -697,7 +697,7 @@ static int genphy_config_advert(struct phy_device *phydev)
* to the values in phydev. Assumes that the values are valid.
* Please see phy_sanitize_settings().
*/
-static int genphy_setup_forced(struct phy_device *phydev)
+int genphy_setup_forced(struct phy_device *phydev)
{
int err;
int ctl = 0;
@@ -716,7 +716,7 @@ static int genphy_setup_forced(struct phy_device *phydev)
return err;
}
-
+EXPORT_SYMBOL(genphy_setup_forced);
/**
* genphy_restart_aneg - Enable and Restart Autonegotiation
diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 40406bc..508e435 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -3,7 +3,7 @@
*
* Author: Kriston Carson
*
- * Copyright (c) 2005, 2009 Freescale Semiconductor, Inc.
+ * Copyright (c) 2005, 2009, 2011 Freescale Semiconductor, Inc.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License as published by the
@@ -18,6 +18,11 @@
#include <linux/ethtool.h>
#include <linux/phy.h>
+/* Vitesse Extended Page Magic Register(s) */
+#define MII_VSC82X4_EXT_PAGE_16E 0x10
+#define MII_VSC82X4_EXT_PAGE_17E 0x11
+#define MII_VSC82X4_EXT_PAGE_18E 0x12
+
/* Vitesse Extended Control Register 1 */
#define MII_VSC8244_EXT_CON1 0x17
#define MII_VSC8244_EXTCON1_INIT 0x0000
@@ -54,6 +59,9 @@
#define MII_VSC8221_AUXCONSTAT_INIT 0x0004 /* need to set this bit? */
#define MII_VSC8221_AUXCONSTAT_RESERVED 0x0004
+/* Vitesse Extended Page Access Register */
+#define MII_VSC82X4_EXT_PAGE_ACCESS 0x1f
+
#define PHY_ID_VSC8234 0x000fc620
#define PHY_ID_VSC8244 0x000fc6c0
#define PHY_ID_VSC8574 0x000704a0
@@ -154,6 +162,63 @@ static int vsc8221_config_init(struct phy_device *phydev)
*/
}
+/* vsc82x4_config_autocross_enable - Enable auto MDI/MDI-X for forced links
+ * @phydev: target phy_device struct
+ *
+ * Enable auto MDI/MDI-X when in 10/100 forced link speeds by writing
+ * special values in the VSC8234/VSC8244 extended reserved registers
+ */
+static int vsc82x4_config_autocross_enable(struct phy_device *phydev)
+{
+ int ret;
+
+ if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed > SPEED_100)
+ return 0;
+
+ /* map extended registers set 0x10 - 0x1e */
+ ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_ACCESS, 0x52b5);
+ if (ret >= 0)
+ ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_18E, 0x0012);
+ if (ret >= 0)
+ ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_17E, 0x2803);
+ if (ret >= 0)
+ ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_16E, 0x87fa);
+ /* map standard registers set 0x10 - 0x1e */
+ if (ret >= 0)
+ ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_ACCESS, 0x0000);
+ else
+ phy_write(phydev, MII_VSC82X4_EXT_PAGE_ACCESS, 0x0000);
+
+ return ret;
+}
+
+/* vsc82x4_config_aneg - restart auto-negotiation or write BMCR
+ * @phydev: target phy_device struct
+ *
+ * Description: If auto-negotiation is enabled, we configure the
+ * advertising, and then restart auto-negotiation. If it is not
+ * enabled, then we write the BMCR and also start the auto
+ * MDI/MDI-X feature
+ */
+static int vsc82x4_config_aneg(struct phy_device *phydev)
+{
+ int ret;
+
+ /* Enable auto MDI/MDI-X when in 10/100 forced link speeds by
+ * writing special values in the VSC8234 extended reserved registers
+ */
+ if (phydev->autoneg != AUTONEG_ENABLE && phydev->speed <= SPEED_100) {
+ ret = genphy_setup_forced(phydev);
+
+ if (ret < 0) /* error */
+ return ret;
+
+ return vsc82x4_config_autocross_enable(phydev);
+ }
+
+ return genphy_config_aneg(phydev);
+}
+
/* Vitesse 82xx */
static struct phy_driver vsc82xx_driver[] = {
{
@@ -163,7 +228,7 @@ static struct phy_driver vsc82xx_driver[] = {
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.config_init = &vsc824x_config_init,
- .config_aneg = &genphy_config_aneg,
+ .config_aneg = &vsc82x4_config_aneg,
.read_status = &genphy_read_status,
.ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
@@ -175,7 +240,7 @@ static struct phy_driver vsc82xx_driver[] = {
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.config_init = &vsc824x_config_init,
- .config_aneg = &genphy_config_aneg,
+ .config_aneg = &vsc82x4_config_aneg,
.read_status = &genphy_read_status,
.ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
@@ -187,7 +252,7 @@ static struct phy_driver vsc82xx_driver[] = {
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.config_init = &vsc824x_config_init,
- .config_aneg = &genphy_config_aneg,
+ .config_aneg = &vsc82x4_config_aneg,
.read_status = &genphy_read_status,
.ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
@@ -199,7 +264,7 @@ static struct phy_driver vsc82xx_driver[] = {
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.config_init = &vsc824x_config_init,
- .config_aneg = &genphy_config_aneg,
+ .config_aneg = &vsc82x4_config_aneg,
.read_status = &genphy_read_status,
.ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 64ab823..48a4dc3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -559,6 +559,7 @@ static inline int phy_read_status(struct phy_device *phydev) {
return phydev->drv->read_status(phydev);
}
+int genphy_setup_forced(struct phy_device *phydev);
int genphy_restart_aneg(struct phy_device *phydev);
int genphy_config_aneg(struct phy_device *phydev);
int genphy_update_link(struct phy_device *phydev);
--
1.7.9.7
^ permalink raw reply related
* [PATCH v3 1/4] net/phy: Add VSC8234 support
From: Shruti @ 2013-11-20 22:38 UTC (permalink / raw)
To: davem, netdev; +Cc: Andy Fleming, Kumar Gala, Shruti Kanetkar
From: Andy Fleming <afleming@gmail.com>
Vitesse VSC8234 is quad port 10/100/1000BASE-T PHY
with SGMII and SERDES MAC interfaces.
Signed-off-by: Andy Fleming <afleming@gmail.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
Signed-off-by: Shruti Kanetkar <Shruti@freescale.com>
---
drivers/net/phy/vitesse.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
v3: No content change. v1 & 2 didn't get picked up by Patchwork.
No idea what else to do so trying to re-submit from a different
machine (git version, etc.)
diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 69b482b..59e3b58 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -54,6 +54,7 @@
#define MII_VSC8221_AUXCONSTAT_INIT 0x0004 /* need to set this bit? */
#define MII_VSC8221_AUXCONSTAT_RESERVED 0x0004
+#define PHY_ID_VSC8234 0x000fc620
#define PHY_ID_VSC8244 0x000fc6c0
#define PHY_ID_VSC8221 0x000fc550
#define PHY_ID_VSC8211 0x000fc4b0
@@ -118,7 +119,8 @@ static int vsc82xx_config_intr(struct phy_device *phydev)
if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
err = phy_write(phydev, MII_VSC8244_IMASK,
- phydev->drv->phy_id == PHY_ID_VSC8244 ?
+ (phydev->drv->phy_id == PHY_ID_VSC8234 ||
+ phydev->drv->phy_id == PHY_ID_VSC8244) ?
MII_VSC8244_IMASK_MASK :
MII_VSC8221_IMASK_MASK);
else {
@@ -149,9 +151,21 @@ static int vsc8221_config_init(struct phy_device *phydev)
*/
}
-/* Vitesse 824x */
+/* Vitesse 82xx */
static struct phy_driver vsc82xx_driver[] = {
{
+ .phy_id = PHY_ID_VSC8234,
+ .name = "Vitesse VSC8234",
+ .phy_id_mask = 0x000ffff0,
+ .features = PHY_GBIT_FEATURES,
+ .flags = PHY_HAS_INTERRUPT,
+ .config_init = &vsc824x_config_init,
+ .config_aneg = &genphy_config_aneg,
+ .read_status = &genphy_read_status,
+ .ack_interrupt = &vsc824x_ack_interrupt,
+ .config_intr = &vsc82xx_config_intr,
+ .driver = { .owner = THIS_MODULE,},
+}, {
.phy_id = PHY_ID_VSC8244,
.name = "Vitesse VSC8244",
.phy_id_mask = 0x000fffc0,
@@ -207,6 +221,7 @@ module_init(vsc82xx_init);
module_exit(vsc82xx_exit);
static struct mdio_device_id __maybe_unused vitesse_tbl[] = {
+ { PHY_ID_VSC8234, 0x000ffff0 },
{ PHY_ID_VSC8244, 0x000fffc0 },
{ PHY_ID_VSC8221, 0x000ffff0 },
{ PHY_ID_VSC8211, 0x000ffff0 },
--
1.7.9.7
^ permalink raw reply related
* Re: [PATCH RFC v1 4/7] net: phy: suspend unused PHYs on mdio_bus in late_initcall
From: Florian Fainelli @ 2013-11-20 22:36 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: netdev, linux-kernel@vger.kernel.org, David S. Miller,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <528D2403.5040204@gmail.com>
2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> On 11/20/2013 09:58 PM, Florian Fainelli wrote:
>>
>> 2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
>>>
>>> Since phy_attach ensures PHYs are resumed, we can now suspend all
>>> PHYs that have no attached netdev after initcalls.
>>
>>
>> I do like the idea, but I think you might want to make sure that the
>> MDIO bus suspend policy was set to "auto" (which is the default afair)
>> not to expose unexpected behavior.
>
>
> Ok, TBH I haven't looked through all of phy internals. If we are fine
> with the overall approach, I could use some guidance in the individual
> patches and policies.
>
>
>>>
>>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> ---
>>> Cc: David S. Miller <davem@davemloft.net>
>>> Cc: netdev@vger.kernel.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>> drivers/net/phy/mdio_bus.c | 27 +++++++++++++++++++++++++++
>>> 1 files changed, 27 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>>> index 5617876..10eba58 100644
>>> --- a/drivers/net/phy/mdio_bus.c
>>> +++ b/drivers/net/phy/mdio_bus.c
>>> @@ -320,6 +320,33 @@ static int mdio_bus_match(struct device *dev, struct
>>> device_driver *drv)
>>> (phydev->phy_id & phydrv->phy_id_mask));
>>> }
>>>
>>> +static int mdio_bus_suspend_unused(struct device *busdev, void *data)
>>> +{
>>> + struct mii_bus *bus = to_mii_bus(busdev);
>>> + struct phy_device *phydev;
>>> + struct phy_driver *phydrv;
>>> + int i;
>>> +
>>> + for (i = 0; i < PHY_MAX_ADDR; i++) {
>>> + if (!bus->phy_map[i])
>>> + continue;
>>> +
>>> + phydev = to_phy_device(&bus->phy_map[i]->dev);
>>> + phydrv = to_phy_driver(phydev->dev.driver);
>>> + if (!phydev->attached_dev && phydrv && phydrv->suspend)
>>> + phy_suspend(phydev);
>>> + }
>>> +
>>> + return 0;
>>
>>
>> You might want to reuse mdio_bus_phy_may_suspend() here to have a
>> central place checking for phydev->attached_dev and phydrv->suspend
>> just in case we need to add more callbacks in the future or implicit
>> PHY state machine hooks. That might also take care of my concern
>> expressed above.
>
>
> Unfortunately, mdio_bus_phy_may_suspend() doesn't help here. It will
> correctly tell that unconnected PHYs may_suspend but also that PHYs
> connected to PM aware drivers may_suspend.
I agree for the first part which is why I was suggesting it, we are
sure we will return early because of the if (!netdev) branch.
>
> I tried it and it will also suspend the PHY taken by mv643xx_eth.
>
> But actually, as phy_suspend/resume check for phydrv->suspend
> themselves, we can just call it on !phydev->attached_dev and remove
> the additional checks.
Sure, sounds good.
--
Florian
^ permalink raw reply
* Re: [PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization
From: David Miller @ 2013-11-20 22:15 UTC (permalink / raw)
To: sebastian.hesselbarth; +Cc: netdev, linux-arm-kernel, linux-kernel
In-Reply-To: <528D217B.5020500@gmail.com>
From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Wed, 20 Nov 2013 21:54:19 +0100
> Right, as far as I understand BMCR powerdown, i.e. what is called in
> genphy_suspend/resume, powers down the PHY but _does_ retain PHY
> config.
> It is not resetting the device.
>
> I haven't checked a lot of datasheets but [1] notes that "registers
> will
> preserve their configuration". Even if we have PHYs that do not
> preserve
> it, they should have a device specific callback for suspend/resume
> that
> takes care of preserving it.
>
> [1] http://www.ti.com/lit/an/snoa463a/snoa463a.pdf
Thanks, that addresses my concerns.
^ permalink raw reply
* FINAL RESULT
From: NOKIA @ 2013-11-20 22:03 UTC (permalink / raw)
To: Recipients
YOUR MAIL WAS SELECTED FOR CLAIMS OF $2,500,000.00 IN NOKIA UK PROMOTION. SEND NN013/009/DC13 WITH NAME,ADDRESS,OCCUPATION,AGE,MOBILE,TO NOKIAUKOFFICE@NOKIAMAIL.COM
^ permalink raw reply
* Re: [RFC net-next PATCH] macvtap: Add packet capture support
From: Vlad Yasevich @ 2013-11-20 22:01 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev
In-Reply-To: <20131120211228.GB11452@redhat.com>
On 11/20/2013 04:12 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 20, 2013 at 03:35:22PM -0500, Vlad Yasevich wrote:
>> On 11/20/2013 03:30 PM, Michael S. Tsirkin wrote:
>>> On Wed, Nov 20, 2013 at 03:19:01PM -0500, Vlad Yasevich wrote:
>>>> On 11/20/2013 03:06 PM, Michael S. Tsirkin wrote:
>>>>> On Wed, Nov 20, 2013 at 02:36:40PM -0500, Vlad Yasevich wrote:
>>>>>> On 11/20/2013 01:19 PM, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Nov 20, 2013 at 01:04:09PM -0500, Vlad Yasevich wrote:
>>>>>>>> Currently it is impossible to capture traffic when using a macvtap
>>>>>>>> device. The reason is that all capture handling is done either in
>>>>>>>> dev_hard_start_xmit() or in __netif_receive_skb_core(). Macvtap
>>>>>>>> currenlty doesn't use dev_hard_start_xmit(), and at the time the
>>>>>>>> packet traverses __netif_receive_skb_core, the skb->dev is set to
>>>>>>>> the lower-level device that doesn't end up matching macvtap.
>>>>>>>>
>>>>>>>> To solve the issue, use dev_hard_start_xmit() on the output path.
>>>>>>>> On the input path, it is toughter to solve since macvtap ends up
>>>>>>>> consuming the skb so there is nothing more left for
>>>>>>>> __netif_receive_skb_core() to do.
>>>>>
>>>>> Actually I thought I understand what you are saying here, but now I
>>>>> don't. bridge installs rx handler exactly in the same way.
>>>>> packet handlers seem to be called before the rx handlers so
>>>>> everything should just work.
>>>>>
>>>>> Is this about the bridge mode again?
>>>>
>>>> No. It has to do with bridge submitting the frame back to the
>>>> network stack to forward it to the ports, but macvtap ends up
>>>> stealing it.
>>>
>>> Confused.
>>>
>>> rx_handler = rcu_dereference(skb->dev->rx_handler);
>>> if (rx_handler) {
>>> if (pt_prev) {
>>> ret = deliver_skb(skb, pt_prev, orig_dev);
>>> pt_prev = NULL;
>>> }
>>> switch (rx_handler(&skb)) {
>>>
>>> ....
>>>
>>> so packet handlers (including packet socket)
>>> seem to be invoked before rx handlers (including bridge and macvtap).
>>>
>>> What's the issue then?
>>>
>>> I guess I'm missing something obvious.
>>
>> In case of macvtap0@eth0, before rx_handler is invoked skb->dev == eth0.
>> During the macvlan rx_handler, skb->dev = macvtap0, and the packet is
>> stolen and delivered to the socket, thus no capture.
>
> Aha. I get what the issue is now. So tcpdump -i eth0 works, you want to make
> tcpdump -i macvtap0@eth0 work too.
>
>> In the case of the bridge (eth0, eth1), during the rx_handler
>> skb->dev = bridge. Bridge calls netif_receive_skb() again, when in
>> promisc mode, and now packet is captured.
>>
>> -vlad
>
> Well but not in non promisc mode correct?
>
> I guess a closer model is tun, you can always
> do tcpdump on it and see whatever application sees.
>
> I guess one other option is a new RX_HANDLER_DEV_CHANGED -
> same as consumed but invokes taps after we changed the skb dev
>
> case RX_HANDLER_DEV_CHANGED:
> if (!list_empty(&ptype_all))
> goto another_round;
> ret = NET_RX_SUCCESS;
> kfree_skb(skb);
> goto unlock;
>
This is going to have a problem in that by doing another_round, you not
only do the taps, but you also attempt to deliver locally if no
rx_handlers consume the skb. With the current code, there will be
no rx_handlers on the macvtap device and you'll attempt to deliver
locally to the host. Not what we want...
-vlad
>
>>>
>>>
>>>
>>>> Also, bridge, if running in promisc mode will hand the frame up
>>>> (as if it received it). See the IFF_PROMISC code in
>>>> br_handle_frame_finish().
>>>> So, if someone is capturing on the bridge itself, this lets it
>>>> see the packets even though they are not destined for the bridge
>>>> device.
>>>
>>> Yes, that's quite expected: same thing will happen with
>>> real hardware: set it to promisc, see lots of packets
>>> destined at others.
>>>
>>>>>
>>>>>> A simple solution is to
>>>>>>>> pull the code that delivers things to the taps/captures into
>>>>>>>> its own function and allow macvtap to call it from its receive
>>>>>>>> routine.
>>>>>>>>
>>>>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>>>>>> ---
>>>>>>>> This is only an RFC. I'd like to solicit comments on this simple
>>>>>>>> approach.
>>>>>>>
>>>>>>> I'm kind of worried about this. What worries me is that normally
>>>>>>> if you have a packet socket bound to all interfaces, what it shows is
>>>>>>> traffic to/from the box.
>>>>>>>
>>>>>>> This might be a bug for someone, but I suspect at this point this
>>>>>>> is part of the ABI.
>>>>>>>
>>>>>>> But macvtap bypasses most of the host networking stack,
>>>>>>> So I worry that suddenly showing these packets would be confusing.
>>>>>>
>>>>>> Is it really different from using bridge and tap? If someone
>>>>>> does 'tcpdump -i any', they will see packets sent by the guest with
>>>>>> bridge+tap.
>>>>>> It makes sense to do that same thing for macvtap, no?
>>>>>
>>>>> I was going by your comments not the code.
>>>>> Assuming we never showed macvtap traffic this might
>>>>> be part of ABI.
>>>>
>>>> BTW, if we end up doing it with a new rx_handler, it will end up
>>>> showing exactly the same traffic is this patch does because the
>>>> common path in __netif_receive_skb_core() will run and deliver
>>>> to all registered eligible entries in ptype_all.
>>>>
>>>> -vlad
>>>
>>> Dave here thinks it's not a problem, I trust him ...
>>>
>>>>>
>>>>>>>
>>>>>>> Assuming we want to show this traffic, I think it's preferable to
>>>>>>> - if (!ptype->dev || ptype->dev == skb->dev) {
>>>>>>> + if (ptype->dev == skb->dev) {
>>>>>>> so you have to bind to macvtap explicitly to see the traffic.
>>>>>>>
>>>>>>>
>>>>>>> Of course when you start binding to specific macvtaps
>>>>>>> it doesn't scale well because we'll have a long list
>>>>>>> on ptype_all suddenly.
>>>>>>
>>>>>> How likely is that really? ptype_all doesn't scale as it.
>>>>>> Any time you don't set the protocol field, the entry goes
>>>>>> into ptype_all. Macvtap doesn't change that. You can
>>>>>> create a long list in ptype all if you have lots of guests and
>>>>>> you want to snoop on each guest separately thus binding to tap
>>>>>> device.
>>>>>>
>>>>>> -vlad
>>>>>>
>>>>>>> This might mean we need to keep this list per-device ...
>>>>>>>
>>>>>>>> A more complicated solution would have been to give
>>>>>>>> macvtap its own rx_handler and return RX_HANDLER_ANOTHER during
>>>>>>>> receive operation to make the packet go through another round of
>>>>>>>> capturing and hit the macvtap rx_handler. I thought this would
>>>>>>>> hurt performance too much for no real gain.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> -vlad
>>>>>>>>
>>>>>>>> drivers/net/macvtap.c | 4 +++-
>>>>>>>> include/linux/netdevice.h | 2 ++
>>>>>>>> net/core/dev.c | 33 ++++++++++++++++++++++++++-------
>>>>>>>> 3 files changed, 31 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>>>>>> index dc76670..0ed8fae 100644
>>>>>>>> --- a/drivers/net/macvtap.c
>>>>>>>> +++ b/drivers/net/macvtap.c
>>>>>>>> @@ -334,6 +334,7 @@ drop:
>>>>>>>> */
>>>>>>>> static int macvtap_receive(struct sk_buff *skb)
>>>>>>>> {
>>>>>>>> + netif_receive_skb_taps(skb, true);
>>>>>>>> skb_push(skb, ETH_HLEN);
>>>>>>>> return macvtap_forward(skb->dev, skb);
>>>>>>>> }
>>>>>>>> @@ -727,8 +728,9 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
>>>>>>>> skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
>>>>>>>> }
>>>>>>>> if (vlan) {
>>>>>>>> + skb->dev = vlan->dev;
>>>>>>>> local_bh_disable();
>>>>>>>> - macvlan_start_xmit(skb, vlan->dev);
>>>>>>>> + dev_hard_start_xmit(skb, vlan->dev, NULL, NULL);
>>>>>>>> local_bh_enable();
>>>>>>>> } else {
>>>>>>>> kfree_skb(skb);
>>>>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>>>>> index 8b3de7c..84880eb 100644
>>>>>>>> --- a/include/linux/netdevice.h
>>>>>>>> +++ b/include/linux/netdevice.h
>>>>>>>> @@ -2383,6 +2383,8 @@ void dev_kfree_skb_any(struct sk_buff *skb);
>>>>>>>> int netif_rx(struct sk_buff *skb);
>>>>>>>> int netif_rx_ni(struct sk_buff *skb);
>>>>>>>> int netif_receive_skb(struct sk_buff *skb);
>>>>>>>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
>>>>>>>> + bool last_deliver);
>>>>>>>> gro_result_t napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb);
>>>>>>>> void napi_gro_flush(struct napi_struct *napi, bool flush_old);
>>>>>>>> struct sk_buff *napi_get_frags(struct napi_struct *napi);
>>>>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>>>>>> index da9c5e1..50f0ac4 100644
>>>>>>>> --- a/net/core/dev.c
>>>>>>>> +++ b/net/core/dev.c
>>>>>>>> @@ -3484,6 +3484,31 @@ static bool skb_pfmemalloc_protocol(struct sk_buff *skb)
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
>>>>>>>> + bool last_deliver)
>>>>>>>> +{
>>>>>>>> + struct packet_type *ptype, *pt_prev = NULL;
>>>>>>>> + struct net_device *orig_dev;
>>>>>>>> +
>>>>>>>> + if (list_empty(&ptype_all))
>>>>>>>> + return NULL;
>>>>>>>> +
>>>>>>>> + orig_dev = skb->dev;
>>>>>>>> + list_for_each_entry_rcu(ptype, &ptype_all, list) {
>>>>>>>> + if (!ptype->dev || ptype->dev == skb->dev) {
>>>>>>>> + if (pt_prev)
>>>>>>>> + deliver_skb(skb, pt_prev, orig_dev);
>>>>>>>> + pt_prev = ptype;
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + if (last_deliver && pt_prev)
>>>>>>>> + deliver_skb(skb, pt_prev, orig_dev);
>>>>>>>> +
>>>>>>>> + return pt_prev;
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL_GPL(netif_receive_skb_taps);
>>>>>>>> +
>>>>>>>> static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
>>>>>>>> {
>>>>>>>> struct packet_type *ptype, *pt_prev;
>>>>>>>> @@ -3535,13 +3560,7 @@ another_round:
>>>>>>>> if (pfmemalloc)
>>>>>>>> goto skip_taps;
>>>>>>>>
>>>>>>>> - list_for_each_entry_rcu(ptype, &ptype_all, list) {
>>>>>>>> - if (!ptype->dev || ptype->dev == skb->dev) {
>>>>>>>> - if (pt_prev)
>>>>>>>> - ret = deliver_skb(skb, pt_prev, orig_dev);
>>>>>>>> - pt_prev = ptype;
>>>>>>>> - }
>>>>>>>> - }
>>>>>>>> + pt_prev = netif_receive_skb_taps(skb, false);
>>>>>>>>
>>>>>>>> skip_taps:
>>>>>>>> #ifdef CONFIG_NET_CLS_ACT
>>>>>>>> --
>>>>>>>> 1.8.4.2
^ permalink raw reply
* Re: [BUG,REGRESSION?] 3.11.6+,3.12: GbE iface rate drops to few KB/s
From: Willy Tarreau @ 2013-11-20 21:54 UTC (permalink / raw)
To: Arnaud Ebalard
Cc: Eric Dumazet, Thomas Petazzoni, Florian Fainelli, simon.guinot,
netdev, edumazet, Cong Wang, linux-arm-kernel
In-Reply-To: <87txf692zx.fsf@natisbad.org>
Hi Arnaud,
On Wed, Nov 20, 2013 at 10:28:50PM +0100, Arnaud Ebalard wrote:
> With current Linus tree (head being b4789b8e: aacraid: prevent invalid
> pointer dereference), as a baseline here is what I get:
>
> w/ tcp_wmem left w/ default values (4096 16384 4071360)
>
> via netperf (TCP_MAERTS/TCP_STREAM): 151.13 / 935.50 Mbits/s
> via wget against apache: 15.4 MB/s
> via wget against nginx: 104 MB/s
>
> w/ tcp_wmem set to 4096 16384 262144:
>
> via netperf (TCP_MAERTS/TCP_STREAM): 919.89 / 935.50 Mbits/s
> via wget against apache: 63.3 MB/s
> via wget against nginx: 104 MB/s
>
> With your patch on top of it (and tcp_wmem kept at its default value):
>
> via netperf: 939.16 / 935.44 Mbits/s
> via wget against apache: 65.9 MB/s (top reports 69.5 sy, 30.1 si
> and 72% CPU for apache2)
> via wget against nginx: 106 MB/s
>
>
> With your patch and MVNETA_TX_DONE_TIMER_PERIOD set to 1 instead of 10
> (still w/ and tcp_wmem kept at its default value):
>
> via netperf: 939.12 / 935.84 Mbits/s
> via wget against apache: 63.7 MB/s
> via wget against nginx: 108 MB/s
>
> So:
>
> - First, Eric's patch sitting in Linus tree does fix the regression
> I had on 3.11.7 and early 3.12 (15.4 MB/s vs 256KB/s).
>
> - As can be seen in the results of first test, Eric's patch still
> requires some additional tweaking of tcp_wmem to get netperf and
> apache somewhat happy w/ perfectible drivers (63.3 MB/s instead of
> 15.4MB/s by setting max tcp send buffer space to 256KB for apache).
>
> - For unknown reasons, nginx manages to provide a 104MB/s download rate
> even with a tcp_wmem set to default and no specific patch of mvneta.
>
> - Now, Willy's patch seems to makes netperf happy (link saturated from
> server to client), w/o tweaking tcp_wmem.
>
> - Again with Willy's patch I guess the "limitations" of the platform
> (1.2GHz CPU w/ 512MB of RAM) somehow prevent Apache to saturate the
> link. All I can say is that the same test some months ago on a 1.6GHz
> ARMv5TE (kirkwood 88f6282) w/ 256MB of RAM gave me 108MB/s. I do not
> know if it is some apache regression, some mvneta vs mv63xx_eth
> difference or some CPU frequency issue but having netperf and nginx
> happy make me wonder about Apache.
>
> - Willy, setting MVNETA_TX_DONE_TIMER_PERIOD to 1 instead of 10 w/ your
> patch does not improve the already good value I get w/ your patch.
Great, thanks for your detailed tests! Concerning Apache, it's common to
see it consume more CPU than others, which makes it more sensible to small
devices like these ones (which BTW have a very small cache and only a 16bit
RAM bus). Please still note that there could be a number of other differences
such as Apache always doing TCP_NODELAY resulting in sending incomplete
segments at the end of each buffer, which consume slightly more descriptors.
> In the end if you iterate on your work to push a version of your patch
> upstream, I'll be happy to test it. And thanks for the time you already
> spent!
I'm currently trying to implement TX IRQ handling. I found the registers
description in the neta driver that is provided in Marvell's LSP kernel
that is shipped with some devices using their CPUs. This code is utterly
broken (eg: splice fails with -EBADF) but I think the register descriptions
could be trusted.
I'd rather have real IRQ handling than just relying on mvneta_poll(), so
that we can use it for asymmetric traffic/routing/whatever.
Regards,
Willy
^ permalink raw reply
* Re: [BUG,REGRESSION?] 3.11.6+,3.12: GbE iface rate drops to few KB/s
From: Arnaud Ebalard @ 2013-11-20 21:28 UTC (permalink / raw)
To: Willy Tarreau
Cc: Eric Dumazet, Thomas Petazzoni, Florian Fainelli, simon.guinot,
netdev, edumazet, Cong Wang, linux-arm-kernel
In-Reply-To: <20131120191145.GP8581@1wt.eu>
Hi,
Willy Tarreau <w@1wt.eu> writes:
> From d1a00e593841223c7d871007b1e1fc528afe8e4d Mon Sep 17 00:00:00 2001
> From: Willy Tarreau <w@1wt.eu>
> Date: Wed, 20 Nov 2013 19:47:11 +0100
> Subject: EXP: net: mvneta: try to flush Tx descriptor queue upon Rx
> interrupts
>
> Right now the mvneta driver doesn't handle Tx IRQ, and solely relies on a
> timer to flush Tx descriptors. This causes jerky output traffic with bursts
> and pauses, making it difficult to reach line rate with very few streams.
> This patch tries to improve the situation which is complicated by the lack
> of public datasheet from Marvell. The workaround consists in trying to flush
> pending buffers during the Rx polling. The idea is that for symmetric TCP
> traffic, ACKs received in response to the packets sent will trigger the Rx
> interrupt and will anticipate the flushing of the descriptors.
>
> The results are quite good, a single TCP stream is now capable of saturating
> a gigabit.
>
> This is only a workaround, it doesn't address asymmetric traffic nor datagram
> based traffic.
>
> Signed-off-by: Willy Tarreau <w@1wt.eu>
> ---
> drivers/net/ethernet/marvell/mvneta.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 5aed8ed..59e1c86 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -2013,6 +2013,26 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
> }
>
> pp->cause_rx_tx = cause_rx_tx;
> +
> + /* Try to flush pending Tx buffers if any */
> + if (test_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags)) {
> + int tx_todo = 0;
> +
> + mvneta_tx_done_gbe(pp,
> + (((1 << txq_number) - 1) &
> + MVNETA_CAUSE_TXQ_SENT_DESC_ALL_MASK),
> + &tx_todo);
> +
> + if (tx_todo > 0) {
> + mod_timer(&pp->tx_done_timer,
> + jiffies + msecs_to_jiffies(MVNETA_TX_DONE_TIMER_PERIOD));
> + }
> + else {
> + clear_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags);
> + del_timer(&pp->tx_done_timer);
> + }
> + }
> +
> return rx_done;
> }
With current Linus tree (head being b4789b8e: aacraid: prevent invalid
pointer dereference), as a baseline here is what I get:
w/ tcp_wmem left w/ default values (4096 16384 4071360)
via netperf (TCP_MAERTS/TCP_STREAM): 151.13 / 935.50 Mbits/s
via wget against apache: 15.4 MB/s
via wget against nginx: 104 MB/s
w/ tcp_wmem set to 4096 16384 262144:
via netperf (TCP_MAERTS/TCP_STREAM): 919.89 / 935.50 Mbits/s
via wget against apache: 63.3 MB/s
via wget against nginx: 104 MB/s
With your patch on top of it (and tcp_wmem kept at its default value):
via netperf: 939.16 / 935.44 Mbits/s
via wget against apache: 65.9 MB/s (top reports 69.5 sy, 30.1 si
and 72% CPU for apache2)
via wget against nginx: 106 MB/s
With your patch and MVNETA_TX_DONE_TIMER_PERIOD set to 1 instead of 10
(still w/ and tcp_wmem kept at its default value):
via netperf: 939.12 / 935.84 Mbits/s
via wget against apache: 63.7 MB/s
via wget against nginx: 108 MB/s
So:
- First, Eric's patch sitting in Linus tree does fix the regression
I had on 3.11.7 and early 3.12 (15.4 MB/s vs 256KB/s).
- As can be seen in the results of first test, Eric's patch still
requires some additional tweaking of tcp_wmem to get netperf and
apache somewhat happy w/ perfectible drivers (63.3 MB/s instead of
15.4MB/s by setting max tcp send buffer space to 256KB for apache).
- For unknown reasons, nginx manages to provide a 104MB/s download rate
even with a tcp_wmem set to default and no specific patch of mvneta.
- Now, Willy's patch seems to makes netperf happy (link saturated from
server to client), w/o tweaking tcp_wmem.
- Again with Willy's patch I guess the "limitations" of the platform
(1.2GHz CPU w/ 512MB of RAM) somehow prevent Apache to saturate the
link. All I can say is that the same test some months ago on a 1.6GHz
ARMv5TE (kirkwood 88f6282) w/ 256MB of RAM gave me 108MB/s. I do not
know if it is some apache regression, some mvneta vs mv63xx_eth
difference or some CPU frequency issue but having netperf and nginx
happy make me wonder about Apache.
- Willy, setting MVNETA_TX_DONE_TIMER_PERIOD to 1 instead of 10 w/ your
patch does not improve the already good value I get w/ your patch.
In the end if you iterate on your work to push a version of your patch
upstream, I'll be happy to test it. And thanks for the time you already
spent!
Cheers,
a+
^ permalink raw reply
* RE: [PATCH] rds: Error on offset mismatch if not loopback
From: Venkat Venkatsubra @ 2013-11-20 21:28 UTC (permalink / raw)
To: David Miller; +Cc: honli, joshhunt00, jjolly, linux-kernel, netdev
In-Reply-To: <20131120.135457.154965902030101577.davem@davemloft.net>
> Why are you posting this message a second time?
Reposting just the contents of the second message in case it got missed the previous time.
Looks like the fix pointed to by the previous link is for a panic on a PPC system with a PAGE_SIZE of 64Kbytes.
I think the sequence it was going through before that fix was:
/* Do not send cong updates to IB loopback */
if (conn->c_loopback
&& rm->m_inc.i_hdr.h_flags & RDS_FLAG_CONG_BITMAP) {
rds_cong_map_updated(conn->c_fcong, ~(u64) 0);
return sizeof(struct rds_header) + RDS_CONG_MAP_BYTES;
}
rds_ib_xmit returns 8240
rds_send_xmit : c_xmit_data_off = 0 + 8240 - 48 (rds header the first time) = 8196
c_xmit_data_off < 65536 (sg->length)
calls rds_ib_xmit again
rds_ib_xmit returns 8240
rds_send_xmit: c_xmit_data_off = 8192+8240 = 16432 and calls rds_ib_xmit
rds_ib_xmit : returns 8240
rds_send_xmit: c_xmit_data_off 24672 and calls rds_ib_xmit ...
...
and so on till
rds_send_xmit: c_xmit_data_off 57632 and calls rds_ib_xmit
rds_ib_xmit: returns 8240
On the last iteration it hits the below BUG_ON in rds_send_xmit.
while (ret) {
tmp = min_t(int, ret, sg->length -
conn->c_xmit_data_off);
[tmp = 7904]
conn->c_xmit_data_off += tmp;
[c_xmit_data_off = 65536]
ret -= tmp;
[ret = 8240-7904 = 336]
if (conn->c_xmit_data_off == sg->length) {
conn->c_xmit_data_off = 0;
sg++;
conn->c_xmit_sg++;
BUG_ON(ret != 0 &&
conn->c_xmit_sg == rm->data.op_nents);
}
}
Since the congestion update over loopback is not actually transmitted as a message,
the multiple iterations we see in the case of ppc is unnecessary.
All that rds_ib_xmit needs to do is return a number of bytes that will tell the caller
that we are done with this message.
This might fix the original problem without introducing the current panic:
/* Do not send cong updates to IB loopback */
if (conn->c_loopback
&& rm->m_inc.i_hdr.h_flags & RDS_FLAG_CONG_BITMAP) {
rds_cong_map_updated(conn->c_fcong, ~(u64) 0);
scat = &rm->data.op_sg[sg];
ret = max_t(int, RDS_CONG_MAP_BYTES, scat->length);
return ret + sizeof(struct rds_header);
}
It will return 8240 when PAGE_SIZE is 4k and 64k+48 in case of ppc when scat->length is 64k and
be done with one iteration of rds_send_xmit/rds_ib_xmit loop.
Venkat
^ permalink raw reply
* Re: [RFC net-next PATCH] macvtap: Add packet capture support
From: Vlad Yasevich @ 2013-11-20 20:35 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev
In-Reply-To: <20131120203027.GA11386@redhat.com>
On 11/20/2013 03:30 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 20, 2013 at 03:19:01PM -0500, Vlad Yasevich wrote:
>> On 11/20/2013 03:06 PM, Michael S. Tsirkin wrote:
>>> On Wed, Nov 20, 2013 at 02:36:40PM -0500, Vlad Yasevich wrote:
>>>> On 11/20/2013 01:19 PM, Michael S. Tsirkin wrote:
>>>>> On Wed, Nov 20, 2013 at 01:04:09PM -0500, Vlad Yasevich wrote:
>>>>>> Currently it is impossible to capture traffic when using a macvtap
>>>>>> device. The reason is that all capture handling is done either in
>>>>>> dev_hard_start_xmit() or in __netif_receive_skb_core(). Macvtap
>>>>>> currenlty doesn't use dev_hard_start_xmit(), and at the time the
>>>>>> packet traverses __netif_receive_skb_core, the skb->dev is set to
>>>>>> the lower-level device that doesn't end up matching macvtap.
>>>>>>
>>>>>> To solve the issue, use dev_hard_start_xmit() on the output path.
>>>>>> On the input path, it is toughter to solve since macvtap ends up
>>>>>> consuming the skb so there is nothing more left for
>>>>>> __netif_receive_skb_core() to do.
>>>
>>> Actually I thought I understand what you are saying here, but now I
>>> don't. bridge installs rx handler exactly in the same way.
>>> packet handlers seem to be called before the rx handlers so
>>> everything should just work.
>>>
>>> Is this about the bridge mode again?
>>
>> No. It has to do with bridge submitting the frame back to the
>> network stack to forward it to the ports, but macvtap ends up
>> stealing it.
>
> Confused.
>
> rx_handler = rcu_dereference(skb->dev->rx_handler);
> if (rx_handler) {
> if (pt_prev) {
> ret = deliver_skb(skb, pt_prev, orig_dev);
> pt_prev = NULL;
> }
> switch (rx_handler(&skb)) {
>
> ....
>
> so packet handlers (including packet socket)
> seem to be invoked before rx handlers (including bridge and macvtap).
>
> What's the issue then?
>
> I guess I'm missing something obvious.
In case of macvtap0@eth0, before rx_handler is invoked skb->dev == eth0.
During the macvlan rx_handler, skb->dev = macvtap0, and the packet is
stolen and delivered to the socket, thus no capture.
In the case of the bridge (eth0, eth1), during the rx_handler
skb->dev = bridge. Bridge calls netif_receive_skb() again, when in
promisc mode, and now packet is captured.
-vlad
>
>
>
>> Also, bridge, if running in promisc mode will hand the frame up
>> (as if it received it). See the IFF_PROMISC code in
>> br_handle_frame_finish().
>> So, if someone is capturing on the bridge itself, this lets it
>> see the packets even though they are not destined for the bridge
>> device.
>
> Yes, that's quite expected: same thing will happen with
> real hardware: set it to promisc, see lots of packets
> destined at others.
>
>>>
>>>> A simple solution is to
>>>>>> pull the code that delivers things to the taps/captures into
>>>>>> its own function and allow macvtap to call it from its receive
>>>>>> routine.
>>>>>>
>>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>>>> ---
>>>>>> This is only an RFC. I'd like to solicit comments on this simple
>>>>>> approach.
>>>>>
>>>>> I'm kind of worried about this. What worries me is that normally
>>>>> if you have a packet socket bound to all interfaces, what it shows is
>>>>> traffic to/from the box.
>>>>>
>>>>> This might be a bug for someone, but I suspect at this point this
>>>>> is part of the ABI.
>>>>>
>>>>> But macvtap bypasses most of the host networking stack,
>>>>> So I worry that suddenly showing these packets would be confusing.
>>>>
>>>> Is it really different from using bridge and tap? If someone
>>>> does 'tcpdump -i any', they will see packets sent by the guest with
>>>> bridge+tap.
>>>> It makes sense to do that same thing for macvtap, no?
>>>
>>> I was going by your comments not the code.
>>> Assuming we never showed macvtap traffic this might
>>> be part of ABI.
>>
>> BTW, if we end up doing it with a new rx_handler, it will end up
>> showing exactly the same traffic is this patch does because the
>> common path in __netif_receive_skb_core() will run and deliver
>> to all registered eligible entries in ptype_all.
>>
>> -vlad
>
> Dave here thinks it's not a problem, I trust him ...
>
>>>
>>>>>
>>>>> Assuming we want to show this traffic, I think it's preferable to
>>>>> - if (!ptype->dev || ptype->dev == skb->dev) {
>>>>> + if (ptype->dev == skb->dev) {
>>>>> so you have to bind to macvtap explicitly to see the traffic.
>>>>>
>>>>>
>>>>> Of course when you start binding to specific macvtaps
>>>>> it doesn't scale well because we'll have a long list
>>>>> on ptype_all suddenly.
>>>>
>>>> How likely is that really? ptype_all doesn't scale as it.
>>>> Any time you don't set the protocol field, the entry goes
>>>> into ptype_all. Macvtap doesn't change that. You can
>>>> create a long list in ptype all if you have lots of guests and
>>>> you want to snoop on each guest separately thus binding to tap
>>>> device.
>>>>
>>>> -vlad
>>>>
>>>>> This might mean we need to keep this list per-device ...
>>>>>
>>>>>> A more complicated solution would have been to give
>>>>>> macvtap its own rx_handler and return RX_HANDLER_ANOTHER during
>>>>>> receive operation to make the packet go through another round of
>>>>>> capturing and hit the macvtap rx_handler. I thought this would
>>>>>> hurt performance too much for no real gain.
>>>>>>
>>>>>> Thanks
>>>>>> -vlad
>>>>>>
>>>>>> drivers/net/macvtap.c | 4 +++-
>>>>>> include/linux/netdevice.h | 2 ++
>>>>>> net/core/dev.c | 33 ++++++++++++++++++++++++++-------
>>>>>> 3 files changed, 31 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>>>> index dc76670..0ed8fae 100644
>>>>>> --- a/drivers/net/macvtap.c
>>>>>> +++ b/drivers/net/macvtap.c
>>>>>> @@ -334,6 +334,7 @@ drop:
>>>>>> */
>>>>>> static int macvtap_receive(struct sk_buff *skb)
>>>>>> {
>>>>>> + netif_receive_skb_taps(skb, true);
>>>>>> skb_push(skb, ETH_HLEN);
>>>>>> return macvtap_forward(skb->dev, skb);
>>>>>> }
>>>>>> @@ -727,8 +728,9 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
>>>>>> skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
>>>>>> }
>>>>>> if (vlan) {
>>>>>> + skb->dev = vlan->dev;
>>>>>> local_bh_disable();
>>>>>> - macvlan_start_xmit(skb, vlan->dev);
>>>>>> + dev_hard_start_xmit(skb, vlan->dev, NULL, NULL);
>>>>>> local_bh_enable();
>>>>>> } else {
>>>>>> kfree_skb(skb);
>>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>>> index 8b3de7c..84880eb 100644
>>>>>> --- a/include/linux/netdevice.h
>>>>>> +++ b/include/linux/netdevice.h
>>>>>> @@ -2383,6 +2383,8 @@ void dev_kfree_skb_any(struct sk_buff *skb);
>>>>>> int netif_rx(struct sk_buff *skb);
>>>>>> int netif_rx_ni(struct sk_buff *skb);
>>>>>> int netif_receive_skb(struct sk_buff *skb);
>>>>>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
>>>>>> + bool last_deliver);
>>>>>> gro_result_t napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb);
>>>>>> void napi_gro_flush(struct napi_struct *napi, bool flush_old);
>>>>>> struct sk_buff *napi_get_frags(struct napi_struct *napi);
>>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>>>> index da9c5e1..50f0ac4 100644
>>>>>> --- a/net/core/dev.c
>>>>>> +++ b/net/core/dev.c
>>>>>> @@ -3484,6 +3484,31 @@ static bool skb_pfmemalloc_protocol(struct sk_buff *skb)
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
>>>>>> + bool last_deliver)
>>>>>> +{
>>>>>> + struct packet_type *ptype, *pt_prev = NULL;
>>>>>> + struct net_device *orig_dev;
>>>>>> +
>>>>>> + if (list_empty(&ptype_all))
>>>>>> + return NULL;
>>>>>> +
>>>>>> + orig_dev = skb->dev;
>>>>>> + list_for_each_entry_rcu(ptype, &ptype_all, list) {
>>>>>> + if (!ptype->dev || ptype->dev == skb->dev) {
>>>>>> + if (pt_prev)
>>>>>> + deliver_skb(skb, pt_prev, orig_dev);
>>>>>> + pt_prev = ptype;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + if (last_deliver && pt_prev)
>>>>>> + deliver_skb(skb, pt_prev, orig_dev);
>>>>>> +
>>>>>> + return pt_prev;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(netif_receive_skb_taps);
>>>>>> +
>>>>>> static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
>>>>>> {
>>>>>> struct packet_type *ptype, *pt_prev;
>>>>>> @@ -3535,13 +3560,7 @@ another_round:
>>>>>> if (pfmemalloc)
>>>>>> goto skip_taps;
>>>>>>
>>>>>> - list_for_each_entry_rcu(ptype, &ptype_all, list) {
>>>>>> - if (!ptype->dev || ptype->dev == skb->dev) {
>>>>>> - if (pt_prev)
>>>>>> - ret = deliver_skb(skb, pt_prev, orig_dev);
>>>>>> - pt_prev = ptype;
>>>>>> - }
>>>>>> - }
>>>>>> + pt_prev = netif_receive_skb_taps(skb, false);
>>>>>>
>>>>>> skip_taps:
>>>>>> #ifdef CONFIG_NET_CLS_ACT
>>>>>> --
>>>>>> 1.8.4.2
^ permalink raw reply
* Re: [PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization
From: Sebastian Hesselbarth @ 2013-11-20 21:20 UTC (permalink / raw)
To: Florian Fainelli
Cc: David Miller, netdev, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAGVrzcZ5wV2eHPNfr-xToOEc6jbBJXiNQ+OtZv2j+yGbS9YW-w@mail.gmail.com>
On 11/20/2013 10:10 PM, Florian Fainelli wrote:
> 2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
>> On 11/20/2013 09:36 PM, David Miller wrote:
>>>
>>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> Date: Wed, 20 Nov 2013 21:21:46 +0100
>>>
>>>> Ethernet PHYs consume a significant amount of power when link is
>>>> detected.
>>>> Especially, for embedded systems it can be easily 20-40% of total system
>>>> power. Now, currently most likely all ethernet drivers leave PHYs powered
>>>> on, even if the device is taken down. Also, some stupid boot loaders
>>>> power
>>>> on all PHYs available.
>>>>
>>>> This RFC deals with saving power consumed by ethernet PHYs, that have no
>>>> corresponding ethernet device driver or are attached to ethernet devices
>>>> which are taken down by user request, i.e. ifconfig ethN down. Ports with
>>>> no link, i.e. cable removed, are already quite good at power saving due
>>>> to
>>>> PHY internal link detection.
>>>
>>>
>>> The idea is sound and the goal is of course valuable, but it brings up
>>> a chronically reoccurring issue as of late.
>>>
>>> You cannot reset the PHY or take it down without somehow retaining the
>>> settings the PHY had when you bring it back up.
>>
>>
>> Right, as far as I understand BMCR powerdown, i.e. what is called in
>> genphy_suspend/resume, powers down the PHY but _does_ retain PHY config.
>> It is not resetting the device.
>
> Right that's also my understanding of how BMCR powerdown works. That
> said, I am relatively sure that we can find PHY devices for which this
> is not true.
No doubt.
> As for the PHY state machine, I think we need a new state
> PHY_SUSPENDED and upon calling phy_start() we make sure that we treat
> PHY_SUSPENDED just like we treat PHY_HALTED today since that would
> make sure that the PHY parameters are applied correctly upon resume
> (interrupt configuration, autoneg and and such).
>
> There is still some discussion on how we should deal with
> auto-suspending the PHY when phy_stop() is called, and how does that
> differ from the PHY_HALTED state? So this also raises the question of
> whether PHY_HALTED is really different from PHY_SUSPENDED. The only
> difference with your patches would be that we have put the PHY into a
> low-power mode.
Well, I haven't thought about WoL and stuff and how the driver should
leave the PHY for that to work. Suspend support isn't really spread
among ARM SoCs :P
But if you can run some tests on non-ARM platforms you might already
been testing with, I am sure we can work it out. Maybe, we just have
PHY_SUSPENDED and deal with it differently if any regressions pop up?
Also, suspend_unused and auto-suspend on phy_stop can be disabled by
default for some kernel versions until we have enough coverage?
>> I haven't checked a lot of datasheets but [1] notes that "registers will
>> preserve their configuration". Even if we have PHYs that do not preserve
>> it, they should have a device specific callback for suspend/resume that
>> takes care of preserving it.
>
> Right, but the PHY driver should only take care of restoring "state
> less" PHY context, while the PHY state machine has to restore a "state
> aware" PHY device context. So for quirky PHY chips, or those having
> advanced power management features, we definitively need the two to be
> helping each other.
I see it was a good idea to send the RFC early. The savings are
impressive but I was already quite sure that it has the potential to
break (at least) the quirky PHYs.
Sebastian
>>
>> [1] http://www.ti.com/lit/an/snoa463a/snoa463a.pdf
>>
>>
>>> If I ifdown/ifup a device, my ethtool link configuration better be
>>> retained.
>>>
>>> This means the PHY layer must have a way to reprogram the device when
>>> it is brought back up, with whatever settings the software state
>>> things are there.
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Paul Zimmerman @ 2013-11-20 21:11 UTC (permalink / raw)
To: David Laight, Sarah Sharp
Cc: Alan Stern, Ben Hutchings, netdev@vger.kernel.org,
linux-usb@vger.kernel.org
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B743D@saturn3.aculab.com>
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of David Laight
> Sent: Wednesday, November 20, 2013 9:16 AM
>
> > Ok, so the networking layer should be fine. However, with the current
> > patch, if the mass storage driver sends down a scatter-gather list
> > that's bigger than a ring segment, or needs to be split up so it doesn't
> > cross 64K boundaries, then the URB submission will fail. We don't want
> > that to happen.
>
> My suspicion is that long SG lists are unusual - otherwise the
> ring expansion code would have been needed much earlier.
>
> Can anyone remember whether that was needed because of long SG lists
> or because of large numbers of outstanding requests?
>
> I've seen it for network cards - but only because usbnet sends
> down far too many tx buffers.
usb-storage limits the maximum transfer size to 120K. That is a max of
31 page-size segments if my math is right. That's probably why mass-storage
never saw a problem.
--
Paul
^ permalink raw reply
* Re: [PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization
From: Florian Fainelli @ 2013-11-20 21:10 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: David Miller, netdev, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <528D217B.5020500@gmail.com>
2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> On 11/20/2013 09:36 PM, David Miller wrote:
>>
>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Date: Wed, 20 Nov 2013 21:21:46 +0100
>>
>>> Ethernet PHYs consume a significant amount of power when link is
>>> detected.
>>> Especially, for embedded systems it can be easily 20-40% of total system
>>> power. Now, currently most likely all ethernet drivers leave PHYs powered
>>> on, even if the device is taken down. Also, some stupid boot loaders
>>> power
>>> on all PHYs available.
>>>
>>> This RFC deals with saving power consumed by ethernet PHYs, that have no
>>> corresponding ethernet device driver or are attached to ethernet devices
>>> which are taken down by user request, i.e. ifconfig ethN down. Ports with
>>> no link, i.e. cable removed, are already quite good at power saving due
>>> to
>>> PHY internal link detection.
>>
>>
>> The idea is sound and the goal is of course valuable, but it brings up
>> a chronically reoccurring issue as of late.
>>
>> You cannot reset the PHY or take it down without somehow retaining the
>> settings the PHY had when you bring it back up.
>
>
> Right, as far as I understand BMCR powerdown, i.e. what is called in
> genphy_suspend/resume, powers down the PHY but _does_ retain PHY config.
> It is not resetting the device.
Right that's also my understanding of how BMCR powerdown works. That
said, I am relatively sure that we can find PHY devices for which this
is not true.
As for the PHY state machine, I think we need a new state
PHY_SUSPENDED and upon calling phy_start() we make sure that we treat
PHY_SUSPENDED just like we treat PHY_HALTED today since that would
make sure that the PHY parameters are applied correctly upon resume
(interrupt configuration, autoneg and and such).
There is still some discussion on how we should deal with
auto-suspending the PHY when phy_stop() is called, and how does that
differ from the PHY_HALTED state? So this also raises the question of
whether PHY_HALTED is really different from PHY_SUSPENDED. The only
difference with your patches would be that we have put the PHY into a
low-power mode.
>
> I haven't checked a lot of datasheets but [1] notes that "registers will
> preserve their configuration". Even if we have PHYs that do not preserve
> it, they should have a device specific callback for suspend/resume that
> takes care of preserving it.
Right, but the PHY driver should only take care of restoring "state
less" PHY context, while the PHY state machine has to restore a "state
aware" PHY device context. So for quirky PHY chips, or those having
advanced power management features, we definitively need the two to be
helping each other.
>
> [1] http://www.ti.com/lit/an/snoa463a/snoa463a.pdf
>
>
>> If I ifdown/ifup a device, my ethtool link configuration better be
>> retained.
>>
>> This means the PHY layer must have a way to reprogram the device when
>> it is brought back up, with whatever settings the software state
>> things are there.
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Florian
^ permalink raw reply
* Re: [RFC net-next PATCH] macvtap: Add packet capture support
From: Michael S. Tsirkin @ 2013-11-20 21:12 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <528D1D0A.3010505@redhat.com>
On Wed, Nov 20, 2013 at 03:35:22PM -0500, Vlad Yasevich wrote:
> On 11/20/2013 03:30 PM, Michael S. Tsirkin wrote:
> > On Wed, Nov 20, 2013 at 03:19:01PM -0500, Vlad Yasevich wrote:
> >> On 11/20/2013 03:06 PM, Michael S. Tsirkin wrote:
> >>> On Wed, Nov 20, 2013 at 02:36:40PM -0500, Vlad Yasevich wrote:
> >>>> On 11/20/2013 01:19 PM, Michael S. Tsirkin wrote:
> >>>>> On Wed, Nov 20, 2013 at 01:04:09PM -0500, Vlad Yasevich wrote:
> >>>>>> Currently it is impossible to capture traffic when using a macvtap
> >>>>>> device. The reason is that all capture handling is done either in
> >>>>>> dev_hard_start_xmit() or in __netif_receive_skb_core(). Macvtap
> >>>>>> currenlty doesn't use dev_hard_start_xmit(), and at the time the
> >>>>>> packet traverses __netif_receive_skb_core, the skb->dev is set to
> >>>>>> the lower-level device that doesn't end up matching macvtap.
> >>>>>>
> >>>>>> To solve the issue, use dev_hard_start_xmit() on the output path.
> >>>>>> On the input path, it is toughter to solve since macvtap ends up
> >>>>>> consuming the skb so there is nothing more left for
> >>>>>> __netif_receive_skb_core() to do.
> >>>
> >>> Actually I thought I understand what you are saying here, but now I
> >>> don't. bridge installs rx handler exactly in the same way.
> >>> packet handlers seem to be called before the rx handlers so
> >>> everything should just work.
> >>>
> >>> Is this about the bridge mode again?
> >>
> >> No. It has to do with bridge submitting the frame back to the
> >> network stack to forward it to the ports, but macvtap ends up
> >> stealing it.
> >
> > Confused.
> >
> > rx_handler = rcu_dereference(skb->dev->rx_handler);
> > if (rx_handler) {
> > if (pt_prev) {
> > ret = deliver_skb(skb, pt_prev, orig_dev);
> > pt_prev = NULL;
> > }
> > switch (rx_handler(&skb)) {
> >
> > ....
> >
> > so packet handlers (including packet socket)
> > seem to be invoked before rx handlers (including bridge and macvtap).
> >
> > What's the issue then?
> >
> > I guess I'm missing something obvious.
>
> In case of macvtap0@eth0, before rx_handler is invoked skb->dev == eth0.
> During the macvlan rx_handler, skb->dev = macvtap0, and the packet is
> stolen and delivered to the socket, thus no capture.
Aha. I get what the issue is now. So tcpdump -i eth0 works, you want to make
tcpdump -i macvtap0@eth0 work too.
> In the case of the bridge (eth0, eth1), during the rx_handler
> skb->dev = bridge. Bridge calls netif_receive_skb() again, when in
> promisc mode, and now packet is captured.
>
> -vlad
Well but not in non promisc mode correct?
I guess a closer model is tun, you can always
do tcpdump on it and see whatever application sees.
I guess one other option is a new RX_HANDLER_DEV_CHANGED -
same as consumed but invokes taps after we changed the skb dev
case RX_HANDLER_DEV_CHANGED:
if (!list_empty(&ptype_all))
goto another_round;
ret = NET_RX_SUCCESS;
kfree_skb(skb);
goto unlock;
> >
> >
> >
> >> Also, bridge, if running in promisc mode will hand the frame up
> >> (as if it received it). See the IFF_PROMISC code in
> >> br_handle_frame_finish().
> >> So, if someone is capturing on the bridge itself, this lets it
> >> see the packets even though they are not destined for the bridge
> >> device.
> >
> > Yes, that's quite expected: same thing will happen with
> > real hardware: set it to promisc, see lots of packets
> > destined at others.
> >
> >>>
> >>>> A simple solution is to
> >>>>>> pull the code that delivers things to the taps/captures into
> >>>>>> its own function and allow macvtap to call it from its receive
> >>>>>> routine.
> >>>>>>
> >>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>>>>> ---
> >>>>>> This is only an RFC. I'd like to solicit comments on this simple
> >>>>>> approach.
> >>>>>
> >>>>> I'm kind of worried about this. What worries me is that normally
> >>>>> if you have a packet socket bound to all interfaces, what it shows is
> >>>>> traffic to/from the box.
> >>>>>
> >>>>> This might be a bug for someone, but I suspect at this point this
> >>>>> is part of the ABI.
> >>>>>
> >>>>> But macvtap bypasses most of the host networking stack,
> >>>>> So I worry that suddenly showing these packets would be confusing.
> >>>>
> >>>> Is it really different from using bridge and tap? If someone
> >>>> does 'tcpdump -i any', they will see packets sent by the guest with
> >>>> bridge+tap.
> >>>> It makes sense to do that same thing for macvtap, no?
> >>>
> >>> I was going by your comments not the code.
> >>> Assuming we never showed macvtap traffic this might
> >>> be part of ABI.
> >>
> >> BTW, if we end up doing it with a new rx_handler, it will end up
> >> showing exactly the same traffic is this patch does because the
> >> common path in __netif_receive_skb_core() will run and deliver
> >> to all registered eligible entries in ptype_all.
> >>
> >> -vlad
> >
> > Dave here thinks it's not a problem, I trust him ...
> >
> >>>
> >>>>>
> >>>>> Assuming we want to show this traffic, I think it's preferable to
> >>>>> - if (!ptype->dev || ptype->dev == skb->dev) {
> >>>>> + if (ptype->dev == skb->dev) {
> >>>>> so you have to bind to macvtap explicitly to see the traffic.
> >>>>>
> >>>>>
> >>>>> Of course when you start binding to specific macvtaps
> >>>>> it doesn't scale well because we'll have a long list
> >>>>> on ptype_all suddenly.
> >>>>
> >>>> How likely is that really? ptype_all doesn't scale as it.
> >>>> Any time you don't set the protocol field, the entry goes
> >>>> into ptype_all. Macvtap doesn't change that. You can
> >>>> create a long list in ptype all if you have lots of guests and
> >>>> you want to snoop on each guest separately thus binding to tap
> >>>> device.
> >>>>
> >>>> -vlad
> >>>>
> >>>>> This might mean we need to keep this list per-device ...
> >>>>>
> >>>>>> A more complicated solution would have been to give
> >>>>>> macvtap its own rx_handler and return RX_HANDLER_ANOTHER during
> >>>>>> receive operation to make the packet go through another round of
> >>>>>> capturing and hit the macvtap rx_handler. I thought this would
> >>>>>> hurt performance too much for no real gain.
> >>>>>>
> >>>>>> Thanks
> >>>>>> -vlad
> >>>>>>
> >>>>>> drivers/net/macvtap.c | 4 +++-
> >>>>>> include/linux/netdevice.h | 2 ++
> >>>>>> net/core/dev.c | 33 ++++++++++++++++++++++++++-------
> >>>>>> 3 files changed, 31 insertions(+), 8 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>>>>> index dc76670..0ed8fae 100644
> >>>>>> --- a/drivers/net/macvtap.c
> >>>>>> +++ b/drivers/net/macvtap.c
> >>>>>> @@ -334,6 +334,7 @@ drop:
> >>>>>> */
> >>>>>> static int macvtap_receive(struct sk_buff *skb)
> >>>>>> {
> >>>>>> + netif_receive_skb_taps(skb, true);
> >>>>>> skb_push(skb, ETH_HLEN);
> >>>>>> return macvtap_forward(skb->dev, skb);
> >>>>>> }
> >>>>>> @@ -727,8 +728,9 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
> >>>>>> skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
> >>>>>> }
> >>>>>> if (vlan) {
> >>>>>> + skb->dev = vlan->dev;
> >>>>>> local_bh_disable();
> >>>>>> - macvlan_start_xmit(skb, vlan->dev);
> >>>>>> + dev_hard_start_xmit(skb, vlan->dev, NULL, NULL);
> >>>>>> local_bh_enable();
> >>>>>> } else {
> >>>>>> kfree_skb(skb);
> >>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >>>>>> index 8b3de7c..84880eb 100644
> >>>>>> --- a/include/linux/netdevice.h
> >>>>>> +++ b/include/linux/netdevice.h
> >>>>>> @@ -2383,6 +2383,8 @@ void dev_kfree_skb_any(struct sk_buff *skb);
> >>>>>> int netif_rx(struct sk_buff *skb);
> >>>>>> int netif_rx_ni(struct sk_buff *skb);
> >>>>>> int netif_receive_skb(struct sk_buff *skb);
> >>>>>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
> >>>>>> + bool last_deliver);
> >>>>>> gro_result_t napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb);
> >>>>>> void napi_gro_flush(struct napi_struct *napi, bool flush_old);
> >>>>>> struct sk_buff *napi_get_frags(struct napi_struct *napi);
> >>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
> >>>>>> index da9c5e1..50f0ac4 100644
> >>>>>> --- a/net/core/dev.c
> >>>>>> +++ b/net/core/dev.c
> >>>>>> @@ -3484,6 +3484,31 @@ static bool skb_pfmemalloc_protocol(struct sk_buff *skb)
> >>>>>> }
> >>>>>> }
> >>>>>>
> >>>>>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
> >>>>>> + bool last_deliver)
> >>>>>> +{
> >>>>>> + struct packet_type *ptype, *pt_prev = NULL;
> >>>>>> + struct net_device *orig_dev;
> >>>>>> +
> >>>>>> + if (list_empty(&ptype_all))
> >>>>>> + return NULL;
> >>>>>> +
> >>>>>> + orig_dev = skb->dev;
> >>>>>> + list_for_each_entry_rcu(ptype, &ptype_all, list) {
> >>>>>> + if (!ptype->dev || ptype->dev == skb->dev) {
> >>>>>> + if (pt_prev)
> >>>>>> + deliver_skb(skb, pt_prev, orig_dev);
> >>>>>> + pt_prev = ptype;
> >>>>>> + }
> >>>>>> + }
> >>>>>> +
> >>>>>> + if (last_deliver && pt_prev)
> >>>>>> + deliver_skb(skb, pt_prev, orig_dev);
> >>>>>> +
> >>>>>> + return pt_prev;
> >>>>>> +}
> >>>>>> +EXPORT_SYMBOL_GPL(netif_receive_skb_taps);
> >>>>>> +
> >>>>>> static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> >>>>>> {
> >>>>>> struct packet_type *ptype, *pt_prev;
> >>>>>> @@ -3535,13 +3560,7 @@ another_round:
> >>>>>> if (pfmemalloc)
> >>>>>> goto skip_taps;
> >>>>>>
> >>>>>> - list_for_each_entry_rcu(ptype, &ptype_all, list) {
> >>>>>> - if (!ptype->dev || ptype->dev == skb->dev) {
> >>>>>> - if (pt_prev)
> >>>>>> - ret = deliver_skb(skb, pt_prev, orig_dev);
> >>>>>> - pt_prev = ptype;
> >>>>>> - }
> >>>>>> - }
> >>>>>> + pt_prev = netif_receive_skb_taps(skb, false);
> >>>>>>
> >>>>>> skip_taps:
> >>>>>> #ifdef CONFIG_NET_CLS_ACT
> >>>>>> --
> >>>>>> 1.8.4.2
^ permalink raw reply
* Re: [PATCH RFC v1 4/7] net: phy: suspend unused PHYs on mdio_bus in late_initcall
From: Sebastian Hesselbarth @ 2013-11-20 21:05 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, linux-kernel@vger.kernel.org, David S. Miller,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAGVrzcYBexBWrh2Nz8tu0KRLQ4YPjtJux9N-TC07-Hxf3On1FQ@mail.gmail.com>
On 11/20/2013 09:58 PM, Florian Fainelli wrote:
> 2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
>> Since phy_attach ensures PHYs are resumed, we can now suspend all
>> PHYs that have no attached netdev after initcalls.
>
> I do like the idea, but I think you might want to make sure that the
> MDIO bus suspend policy was set to "auto" (which is the default afair)
> not to expose unexpected behavior.
Ok, TBH I haven't looked through all of phy internals. If we are fine
with the overall approach, I could use some guidance in the individual
patches and policies.
>>
>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> ---
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> drivers/net/phy/mdio_bus.c | 27 +++++++++++++++++++++++++++
>> 1 files changed, 27 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index 5617876..10eba58 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>> @@ -320,6 +320,33 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
>> (phydev->phy_id & phydrv->phy_id_mask));
>> }
>>
>> +static int mdio_bus_suspend_unused(struct device *busdev, void *data)
>> +{
>> + struct mii_bus *bus = to_mii_bus(busdev);
>> + struct phy_device *phydev;
>> + struct phy_driver *phydrv;
>> + int i;
>> +
>> + for (i = 0; i < PHY_MAX_ADDR; i++) {
>> + if (!bus->phy_map[i])
>> + continue;
>> +
>> + phydev = to_phy_device(&bus->phy_map[i]->dev);
>> + phydrv = to_phy_driver(phydev->dev.driver);
>> + if (!phydev->attached_dev && phydrv && phydrv->suspend)
>> + phy_suspend(phydev);
>> + }
>> +
>> + return 0;
>
> You might want to reuse mdio_bus_phy_may_suspend() here to have a
> central place checking for phydev->attached_dev and phydrv->suspend
> just in case we need to add more callbacks in the future or implicit
> PHY state machine hooks. That might also take care of my concern
> expressed above.
Unfortunately, mdio_bus_phy_may_suspend() doesn't help here. It will
correctly tell that unconnected PHYs may_suspend but also that PHYs
connected to PM aware drivers may_suspend.
I tried it and it will also suspend the PHY taken by mv643xx_eth.
But actually, as phy_suspend/resume check for phydrv->suspend
themselves, we can just call it on !phydev->attached_dev and remove
the additional checks.
>> +}
>> +
>> +static int mdio_bus_class_suspend_unused(void)
>> +{
>> + return class_for_each_device(&mdio_bus_class, NULL, NULL,
>> + mdio_bus_suspend_unused);
>> +}
>> +late_initcall_sync(mdio_bus_class_suspend_unused);
>> +
>> #ifdef CONFIG_PM
>>
>> static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
>> --
>> 1.7.2.5
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
^ permalink raw reply
* Re: [PATCH RFC v1 4/7] net: phy: suspend unused PHYs on mdio_bus in late_initcall
From: Florian Fainelli @ 2013-11-20 20:58 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: netdev, linux-kernel@vger.kernel.org, David S. Miller,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1384978913-8052-5-git-send-email-sebastian.hesselbarth@gmail.com>
2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> Since phy_attach ensures PHYs are resumed, we can now suspend all
> PHYs that have no attached netdev after initcalls.
I do like the idea, but I think you might want to make sure that the
MDIO bus suspend policy was set to "auto" (which is the default afair)
not to expose unexpected behavior.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Cc: David S. Miller <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/net/phy/mdio_bus.c | 27 +++++++++++++++++++++++++++
> 1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 5617876..10eba58 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -320,6 +320,33 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
> (phydev->phy_id & phydrv->phy_id_mask));
> }
>
> +static int mdio_bus_suspend_unused(struct device *busdev, void *data)
> +{
> + struct mii_bus *bus = to_mii_bus(busdev);
> + struct phy_device *phydev;
> + struct phy_driver *phydrv;
> + int i;
> +
> + for (i = 0; i < PHY_MAX_ADDR; i++) {
> + if (!bus->phy_map[i])
> + continue;
> +
> + phydev = to_phy_device(&bus->phy_map[i]->dev);
> + phydrv = to_phy_driver(phydev->dev.driver);
> + if (!phydev->attached_dev && phydrv && phydrv->suspend)
> + phy_suspend(phydev);
> + }
> +
> + return 0;
You might want to reuse mdio_bus_phy_may_suspend() here to have a
central place checking for phydev->attached_dev and phydrv->suspend
just in case we need to add more callbacks in the future or implicit
PHY state machine hooks. That might also take care of my concern
expressed above.
> +}
> +
> +static int mdio_bus_class_suspend_unused(void)
> +{
> + return class_for_each_device(&mdio_bus_class, NULL, NULL,
> + mdio_bus_suspend_unused);
> +}
> +late_initcall_sync(mdio_bus_class_suspend_unused);
> +
> #ifdef CONFIG_PM
>
> static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
> --
> 1.7.2.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Florian
^ 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