Netdev List
 help / color / mirror / Atom feed
* [PATCH 6/9] netdev: bfin_mac: add support for wake-on-lan magic packets
From: Mike Frysinger @ 2010-05-10 15:39 UTC (permalink / raw)
  To: netdev, David S. Miller; +Cc: uclinux-dist-devel, Michael Hennerich
In-Reply-To: <1273505954-32588-1-git-send-email-vapier@gentoo.org>

From: Michael Hennerich <michael.hennerich@analog.com>

Note that WOL works only in PM Suspend Standby Mode (Sleep Mode).

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |   76 +++++++++++++++++++++++++++++++++++++++++++++--
 drivers/net/bfin_mac.h |    3 ++
 2 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index bee5d7a..967d018 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -464,6 +464,14 @@ static int mii_probe(struct net_device *dev)
  * Ethtool support
  */
 
+/*
+ * interrupt routine for magic packet wakeup
+ */
+static irqreturn_t bfin_mac_wake_interrupt(int irq, void *dev_id)
+{
+	return IRQ_HANDLED;
+}
+
 static int
 bfin_mac_ethtool_getsettings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
@@ -498,11 +506,57 @@ static void bfin_mac_ethtool_getdrvinfo(struct net_device *dev,
 	strcpy(info->bus_info, dev_name(&dev->dev));
 }
 
+static void bfin_mac_ethtool_getwol(struct net_device *dev,
+	struct ethtool_wolinfo *wolinfo)
+{
+	struct bfin_mac_local *lp = netdev_priv(dev);
+
+	wolinfo->supported = WAKE_MAGIC;
+	wolinfo->wolopts = lp->wol;
+}
+
+static int bfin_mac_ethtool_setwol(struct net_device *dev,
+	struct ethtool_wolinfo *wolinfo)
+{
+	struct bfin_mac_local *lp = netdev_priv(dev);
+	int rc;
+
+	if (wolinfo->wolopts & (WAKE_MAGICSECURE |
+				WAKE_UCAST |
+				WAKE_MCAST |
+				WAKE_BCAST |
+				WAKE_ARP))
+		return -EOPNOTSUPP;
+
+	lp->wol = wolinfo->wolopts;
+
+	if (lp->wol && !lp->irq_wake_requested) {
+		/* register wake irq handler */
+		rc = request_irq(IRQ_MAC_WAKEDET, bfin_mac_wake_interrupt,
+				 IRQF_DISABLED, "EMAC_WAKE", dev);
+		if (rc)
+			return rc;
+		lp->irq_wake_requested = true;
+	}
+
+	if (!lp->wol && lp->irq_wake_requested) {
+		free_irq(IRQ_MAC_WAKEDET, dev);
+		lp->irq_wake_requested = false;
+	}
+
+	/* Make sure the PHY driver doesn't suspend */
+	device_init_wakeup(&dev->dev, lp->wol);
+
+	return 0;
+}
+
 static const struct ethtool_ops bfin_mac_ethtool_ops = {
 	.get_settings = bfin_mac_ethtool_getsettings,
 	.set_settings = bfin_mac_ethtool_setsettings,
 	.get_link = ethtool_op_get_link,
 	.get_drvinfo = bfin_mac_ethtool_getdrvinfo,
+	.get_wol = bfin_mac_ethtool_getwol,
+	.set_wol = bfin_mac_ethtool_setwol,
 };
 
 /**************************************************************************/
@@ -1477,9 +1531,16 @@ static int __devexit bfin_mac_remove(struct platform_device *pdev)
 static int bfin_mac_suspend(struct platform_device *pdev, pm_message_t mesg)
 {
 	struct net_device *net_dev = platform_get_drvdata(pdev);
+	struct bfin_mac_local *lp = netdev_priv(net_dev);
 
-	if (netif_running(net_dev))
-		bfin_mac_close(net_dev);
+	if (lp->wol) {
+		bfin_write_EMAC_OPMODE((bfin_read_EMAC_OPMODE() & ~TE) | RE);
+		bfin_write_EMAC_WKUP_CTL(MPKE);
+		enable_irq_wake(IRQ_MAC_WAKEDET);
+	} else {
+		if (netif_running(net_dev))
+			bfin_mac_close(net_dev);
+	}
 
 	return 0;
 }
@@ -1487,9 +1548,16 @@ static int bfin_mac_suspend(struct platform_device *pdev, pm_message_t mesg)
 static int bfin_mac_resume(struct platform_device *pdev)
 {
 	struct net_device *net_dev = platform_get_drvdata(pdev);
+	struct bfin_mac_local *lp = netdev_priv(net_dev);
 
-	if (netif_running(net_dev))
-		bfin_mac_open(net_dev);
+	if (lp->wol) {
+		bfin_write_EMAC_OPMODE(bfin_read_EMAC_OPMODE() | TE);
+		bfin_write_EMAC_WKUP_CTL(0);
+		disable_irq_wake(IRQ_MAC_WAKEDET);
+	} else {
+		if (netif_running(net_dev))
+			bfin_mac_open(net_dev);
+	}
 
 	return 0;
 }
diff --git a/drivers/net/bfin_mac.h b/drivers/net/bfin_mac.h
index 87c454f..1ae7b82 100644
--- a/drivers/net/bfin_mac.h
+++ b/drivers/net/bfin_mac.h
@@ -66,6 +66,9 @@ struct bfin_mac_local {
 	unsigned char Mac[6];	/* MAC address of the board */
 	spinlock_t lock;
 
+	int wol;		/* Wake On Lan */
+	int irq_wake_requested;
+
 	/* MII and PHY stuffs */
 	int old_link;          /* used by bf537_adjust_link */
 	int old_speed;
-- 
1.7.1


^ permalink raw reply related

* [PATCH 7/9] netdev: bfin_mac: use promiscuous flag for promiscuous mode
From: Mike Frysinger @ 2010-05-10 15:39 UTC (permalink / raw)
  To: netdev, David S. Miller; +Cc: uclinux-dist-devel, Sonic Zhang
In-Reply-To: <1273505954-32588-1-git-send-email-vapier@gentoo.org>

From: Sonic Zhang <sonic.zhang@analog.com>

Rather than using the Receive All Frames (RAF) bit to enable promiscuous
mode, use the Promiscuous (PR) bit.  This lowers overhead at runtime as
we let the hardware process the packets that should actually be checked.

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 967d018..6f5ee1a 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -1279,7 +1279,7 @@ static void bfin_mac_set_multicast_list(struct net_device *dev)
 	if (dev->flags & IFF_PROMISC) {
 		printk(KERN_INFO "%s: set to promisc mode\n", dev->name);
 		sysctl = bfin_read_EMAC_OPMODE();
-		sysctl |= RAF;
+		sysctl |= PR;
 		bfin_write_EMAC_OPMODE(sysctl);
 	} else if (dev->flags & IFF_ALLMULTI) {
 		/* accept all multicast */
-- 
1.7.1


^ permalink raw reply related

* [PATCH 8/9] netdev: bfin_mac: handle timeouts with the MDIO registers gracefully
From: Mike Frysinger @ 2010-05-10 15:39 UTC (permalink / raw)
  To: netdev, David S. Miller; +Cc: uclinux-dist-devel
In-Reply-To: <1273505954-32588-1-git-send-email-vapier@gentoo.org>

Have the low level MDIO functions pass back up timeout information so we
don't waste time polling them multiple times when there is a problem, and
so we don't let higher layers think the device is available when it isn't.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |   53 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 6f5ee1a..0b62dbf 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -81,9 +81,6 @@ static u16 pin_req[] = P_RMII0;
 static u16 pin_req[] = P_MII0;
 #endif
 
-static void bfin_mac_disable(void);
-static void bfin_mac_enable(void);
-
 static void desc_list_free(void)
 {
 	struct net_dma_desc_rx *r;
@@ -260,7 +257,7 @@ init_error:
  * MII operations
  */
 /* Wait until the previous MDC/MDIO transaction has completed */
-static void bfin_mdio_poll(void)
+static int bfin_mdio_poll(void)
 {
 	int timeout_cnt = MAX_TIMEOUT_CNT;
 
@@ -270,22 +267,30 @@ static void bfin_mdio_poll(void)
 		if (timeout_cnt-- < 0) {
 			printk(KERN_ERR DRV_NAME
 			": wait MDC/MDIO transaction to complete timeout\n");
-			break;
+			return -ETIMEDOUT;
 		}
 	}
+
+	return 0;
 }
 
 /* Read an off-chip register in a PHY through the MDC/MDIO port */
 static int bfin_mdiobus_read(struct mii_bus *bus, int phy_addr, int regnum)
 {
-	bfin_mdio_poll();
+	int ret;
+
+	ret = bfin_mdio_poll();
+	if (ret)
+		return ret;
 
 	/* read mode */
 	bfin_write_EMAC_STAADD(SET_PHYAD((u16) phy_addr) |
 				SET_REGAD((u16) regnum) |
 				STABUSY);
 
-	bfin_mdio_poll();
+	ret = bfin_mdio_poll();
+	if (ret)
+		return ret;
 
 	return (int) bfin_read_EMAC_STADAT();
 }
@@ -294,7 +299,11 @@ static int bfin_mdiobus_read(struct mii_bus *bus, int phy_addr, int regnum)
 static int bfin_mdiobus_write(struct mii_bus *bus, int phy_addr, int regnum,
 			      u16 value)
 {
-	bfin_mdio_poll();
+	int ret;
+
+	ret = bfin_mdio_poll();
+	if (ret)
+		return ret;
 
 	bfin_write_EMAC_STADAT((u32) value);
 
@@ -304,9 +313,7 @@ static int bfin_mdiobus_write(struct mii_bus *bus, int phy_addr, int regnum,
 				STAOP |
 				STABUSY);
 
-	bfin_mdio_poll();
-
-	return 0;
+	return bfin_mdio_poll();
 }
 
 static int bfin_mdiobus_reset(struct mii_bus *bus)
@@ -1181,8 +1188,9 @@ static void bfin_mac_disable(void)
 /*
  * Enable Interrupts, Receive, and Transmit
  */
-static void bfin_mac_enable(void)
+static int bfin_mac_enable(void)
 {
+	int ret;
 	u32 opmode;
 
 	pr_debug("%s: %s\n", DRV_NAME, __func__);
@@ -1192,7 +1200,9 @@ static void bfin_mac_enable(void)
 	bfin_write_DMA1_CONFIG(rx_list_head->desc_a.config);
 
 	/* Wait MII done */
-	bfin_mdio_poll();
+	ret = bfin_mdio_poll();
+	if (ret)
+		return ret;
 
 	/* We enable only RX here */
 	/* ASTP   : Enable Automatic Pad Stripping
@@ -1216,6 +1226,8 @@ static void bfin_mac_enable(void)
 #endif
 	/* Turn on the EMAC rx */
 	bfin_write_EMAC_OPMODE(opmode);
+
+	return 0;
 }
 
 /* Our watchdog timed out. Called by the networking layer */
@@ -1330,7 +1342,7 @@ static void bfin_mac_shutdown(struct net_device *dev)
 static int bfin_mac_open(struct net_device *dev)
 {
 	struct bfin_mac_local *lp = netdev_priv(dev);
-	int retval;
+	int ret;
 	pr_debug("%s: %s\n", dev->name, __func__);
 
 	/*
@@ -1344,18 +1356,21 @@ static int bfin_mac_open(struct net_device *dev)
 	}
 
 	/* initial rx and tx list */
-	retval = desc_list_init();
-
-	if (retval)
-		return retval;
+	ret = desc_list_init();
+	if (ret)
+		return ret;
 
 	phy_start(lp->phydev);
 	phy_write(lp->phydev, MII_BMCR, BMCR_RESET);
 	setup_system_regs(dev);
 	setup_mac_addr(dev->dev_addr);
+
 	bfin_mac_disable();
-	bfin_mac_enable();
+	ret = bfin_mac_enable();
+	if (ret)
+		return ret;
 	pr_debug("hardware init finished\n");
+
 	netif_start_queue(dev);
 	netif_carrier_on(dev);
 
-- 
1.7.1


^ permalink raw reply related

* [PATCH 9/9] netdev: bfin_mac: check for mii_bus platform data
From: Mike Frysinger @ 2010-05-10 15:39 UTC (permalink / raw)
  To: netdev, David S. Miller; +Cc: uclinux-dist-devel, Sonic Zhang
In-Reply-To: <1273505954-32588-1-git-send-email-vapier@gentoo.org>

From: Sonic Zhang <sonic.zhang@analog.com>

If the platform data for the mii_bus is missing, gracefully error out
rather than deference NULL pointers.

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 0b62dbf..0f0aa57 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -1469,6 +1469,11 @@ static int __devinit bfin_mac_probe(struct platform_device *pdev)
 	}
 	pd = pdev->dev.platform_data;
 	lp->mii_bus = platform_get_drvdata(pd);
+	if (!lp->mii_bus) {
+		dev_err(&pdev->dev, "Cannot get mii_bus!\n");
+		rc = -ENODEV;
+		goto out_err_mii_bus_probe;
+	}
 	lp->mii_bus->priv = ndev;
 
 	rc = mii_probe(ndev);
@@ -1514,6 +1519,7 @@ out_err_request_irq:
 out_err_mii_probe:
 	mdiobus_unregister(lp->mii_bus);
 	mdiobus_free(lp->mii_bus);
+out_err_mii_bus_probe:
 	peripheral_free_list(pin_req);
 out_err_probe_mac:
 	platform_set_drvdata(pdev, NULL);
-- 
1.7.1


^ permalink raw reply related

* Re: mmotm 2010-04-28 - RCU whinges
From: Patrick McHardy @ 2010-05-10 15:40 UTC (permalink / raw)
  To: David Miller
  Cc: eric.dumazet, Valdis.Kletnieks, akpm, peterz, linux-kernel,
	netfilter-devel, netdev, paulmck
In-Reply-To: <20100502.225509.233880079.davem@davemloft.net>

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

David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 03 May 2010 07:43:56 +0200
> 
>> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
>>
>>> Oops scratch that, I'll resend a correct version.
>>>
>>>
>> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
>> thought a different mutex was in use in one of the functions.
> 
> Ok, Patrick please review, thanks.

Actually we don't need the rcu_dereference() calls at all since
registration and unregistration are protected by the mutexes.

I queued this patch in nf-next, the only reason why I haven't
submitted it yet is that I was unable to get git to cleanly export
only the proper set of patches meant for -next due to a few merges,
it insists on including 5 patches already merged upstream. If you
don't mind ignoring the first 5 patches in the series, I'll send a
pull request tonight.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3840 bytes --]

commit ed86308f6179d8fa6151c2d0f652aad0091548e2
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Apr 9 16:42:15 2010 +0200

    netfilter: remove invalid rcu_dereference() calls
    
    The CONFIG_PROVE_RCU option discovered a few invalid uses of
    rcu_dereference() in netfilter. In all these cases, the code code
    intends to check whether a pointer is already assigned when
    performing registration or whether the assigned pointer matches
    when performing unregistration. The entire registration/
    unregistration is protected by a mutex, so we don't need the
    rcu_dereference() calls.
    
    Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
    Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index d5a9bcd..849614a 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -81,11 +81,9 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
 int nf_conntrack_register_notifier(struct nf_ct_event_notifier *new)
 {
 	int ret = 0;
-	struct nf_ct_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_conntrack_event_cb);
-	if (notify != NULL) {
+	if (nf_conntrack_event_cb != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
@@ -101,11 +99,8 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
 
 void nf_conntrack_unregister_notifier(struct nf_ct_event_notifier *new)
 {
-	struct nf_ct_event_notifier *notify;
-
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_conntrack_event_cb);
-	BUG_ON(notify != new);
+	BUG_ON(nf_conntrack_event_cb != new);
 	rcu_assign_pointer(nf_conntrack_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);
 }
@@ -114,11 +109,9 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
 int nf_ct_expect_register_notifier(struct nf_exp_event_notifier *new)
 {
 	int ret = 0;
-	struct nf_exp_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_expect_event_cb);
-	if (notify != NULL) {
+	if (nf_expect_event_cb != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
@@ -134,11 +127,8 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
 
 void nf_ct_expect_unregister_notifier(struct nf_exp_event_notifier *new)
 {
-	struct nf_exp_event_notifier *notify;
-
 	mutex_lock(&nf_ct_ecache_mutex);
-	notify = rcu_dereference(nf_expect_event_cb);
-	BUG_ON(notify != new);
+	BUG_ON(nf_expect_event_cb != new);
 	rcu_assign_pointer(nf_expect_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);
 }
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 015725a..908f599 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -35,7 +35,6 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 /* return EEXIST if the same logger is registred, 0 on success. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
-	const struct nf_logger *llog;
 	int i;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
@@ -52,8 +51,7 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 	} else {
 		/* register at end of list to honor first register win */
 		list_add_tail(&logger->list[pf], &nf_loggers_l[pf]);
-		llog = rcu_dereference(nf_loggers[pf]);
-		if (llog == NULL)
+		if (nf_loggers[pf] == NULL)
 			rcu_assign_pointer(nf_loggers[pf], logger);
 	}
 
@@ -65,13 +63,11 @@ EXPORT_SYMBOL(nf_log_register);
 
 void nf_log_unregister(struct nf_logger *logger)
 {
-	const struct nf_logger *c_logger;
 	int i;
 
 	mutex_lock(&nf_log_mutex);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		c_logger = rcu_dereference(nf_loggers[i]);
-		if (c_logger == logger)
+		if (nf_loggers[i] == logger)
 			rcu_assign_pointer(nf_loggers[i], NULL);
 		list_del(&logger->list[i]);
 	}

^ permalink raw reply related

* Re: mmotm 2010-04-28 - RCU whinges
From: Eric Dumazet @ 2010-05-10 15:56 UTC (permalink / raw)
  To: Patrick McHardy
  Cc: David Miller, Valdis.Kletnieks, akpm, peterz, linux-kernel,
	netfilter-devel, netdev, paulmck
In-Reply-To: <4BE8290A.2080707@trash.net>

Le lundi 10 mai 2010 à 17:40 +0200, Patrick McHardy a écrit :
> David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Mon, 03 May 2010 07:43:56 +0200
> > 
> >> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
> >>
> >>> Oops scratch that, I'll resend a correct version.
> >>>
> >>>
> >> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
> >> thought a different mutex was in use in one of the functions.
> > 
> > Ok, Patrick please review, thanks.
> 
> Actually we don't need the rcu_dereference() calls at all since
> registration and unregistration are protected by the mutexes.
> 
> I queued this patch in nf-next, the only reason why I haven't
> submitted it yet is that I was unable to get git to cleanly export
> only the proper set of patches meant for -next due to a few merges,
> it insists on including 5 patches already merged upstream. If you
> don't mind ignoring the first 5 patches in the series, I'll send a
> pull request tonight.
> 


This will clash with upcoming RCU patches, where rcu protected pointer
cannot be directly accessed without lockdep splats.

We will need one day or another a rcu_...(nf_conntrack_event_cb)

> 
> pièce jointe document texte brut (x)
> commit ed86308f6179d8fa6151c2d0f652aad0091548e2
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Fri Apr 9 16:42:15 2010 +0200
> 
>     netfilter: remove invalid rcu_dereference() calls
>     
>     The CONFIG_PROVE_RCU option discovered a few invalid uses of
>     rcu_dereference() in netfilter. In all these cases, the code code
>     intends to check whether a pointer is already assigned when
>     performing registration or whether the assigned pointer matches
>     when performing unregistration. The entire registration/
>     unregistration is protected by a mutex, so we don't need the
>     rcu_dereference() calls.
>     
>     Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>     Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>     Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
> index d5a9bcd..849614a 100644
> --- a/net/netfilter/nf_conntrack_ecache.c
> +++ b/net/netfilter/nf_conntrack_ecache.c
> @@ -81,11 +81,9 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
>  int nf_conntrack_register_notifier(struct nf_ct_event_notifier *new)
>  {
>  	int ret = 0;
> -	struct nf_ct_event_notifier *notify;
>  
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_conntrack_event_cb);
> -	if (notify != NULL) {
> +	if (nf_conntrack_event_cb != NULL) {
>  		ret = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -101,11 +99,8 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
>  
>  void nf_conntrack_unregister_notifier(struct nf_ct_event_notifier *new)
>  {
> -	struct nf_ct_event_notifier *notify;
> -
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_conntrack_event_cb);
> -	BUG_ON(notify != new);
> +	BUG_ON(nf_conntrack_event_cb != new);
>  	rcu_assign_pointer(nf_conntrack_event_cb, NULL);
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  }
> @@ -114,11 +109,9 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
>  int nf_ct_expect_register_notifier(struct nf_exp_event_notifier *new)
>  {
>  	int ret = 0;
> -	struct nf_exp_event_notifier *notify;
>  
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_expect_event_cb);
> -	if (notify != NULL) {
> +	if (nf_expect_event_cb != NULL) {
>  		ret = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -134,11 +127,8 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
>  
>  void nf_ct_expect_unregister_notifier(struct nf_exp_event_notifier *new)
>  {
> -	struct nf_exp_event_notifier *notify;
> -
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_expect_event_cb);
> -	BUG_ON(notify != new);
> +	BUG_ON(nf_expect_event_cb != new);
>  	rcu_assign_pointer(nf_expect_event_cb, NULL);
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  }
> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index 015725a..908f599 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -35,7 +35,6 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
>  /* return EEXIST if the same logger is registred, 0 on success. */
>  int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  {
> -	const struct nf_logger *llog;
>  	int i;
>  
>  	if (pf >= ARRAY_SIZE(nf_loggers))
> @@ -52,8 +51,7 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  	} else {
>  		/* register at end of list to honor first register win */
>  		list_add_tail(&logger->list[pf], &nf_loggers_l[pf]);
> -		llog = rcu_dereference(nf_loggers[pf]);
> -		if (llog == NULL)
> +		if (nf_loggers[pf] == NULL)
>  			rcu_assign_pointer(nf_loggers[pf], logger);
>  	}
>  
> @@ -65,13 +63,11 @@ EXPORT_SYMBOL(nf_log_register);
>  
>  void nf_log_unregister(struct nf_logger *logger)
>  {
> -	const struct nf_logger *c_logger;
>  	int i;
>  
>  	mutex_lock(&nf_log_mutex);
>  	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
> -		c_logger = rcu_dereference(nf_loggers[i]);
> -		if (c_logger == logger)
> +		if (nf_loggers[i] == logger)
>  			rcu_assign_pointer(nf_loggers[i], NULL);
>  		list_del(&logger->list[i]);
>  	}

^ permalink raw reply

* Re: mmotm 2010-04-28 - RCU whinges
From: Eric Dumazet @ 2010-05-10 15:57 UTC (permalink / raw)
  To: Patrick McHardy
  Cc: David Miller, Valdis.Kletnieks, akpm, peterz, linux-kernel,
	netfilter-devel, netdev, paulmck
In-Reply-To: <1273506968.2221.19.camel@edumazet-laptop>

Le lundi 10 mai 2010 à 17:56 +0200, Eric Dumazet a écrit :
> Le lundi 10 mai 2010 à 17:40 +0200, Patrick McHardy a écrit :
> > David Miller wrote:
> > > From: Eric Dumazet <eric.dumazet@gmail.com>
> > > Date: Mon, 03 May 2010 07:43:56 +0200
> > > 
> > >> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
> > >>
> > >>> Oops scratch that, I'll resend a correct version.
> > >>>
> > >>>
> > >> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
> > >> thought a different mutex was in use in one of the functions.
> > > 
> > > Ok, Patrick please review, thanks.
> > 
> > Actually we don't need the rcu_dereference() calls at all since
> > registration and unregistration are protected by the mutexes.
> > 
> > I queued this patch in nf-next, the only reason why I haven't
> > submitted it yet is that I was unable to get git to cleanly export
> > only the proper set of patches meant for -next due to a few merges,
> > it insists on including 5 patches already merged upstream. If you
> > don't mind ignoring the first 5 patches in the series, I'll send a
> > pull request tonight.
> > 
> 
> 
> This will clash with upcoming RCU patches, where rcu protected pointer
> cannot be directly accessed without lockdep splats.
> 

Sorry, I meant sparse here, not lockdep.

> We will need one day or another a rcu_...(nf_conntrack_event_cb)

^ permalink raw reply

* Re: mmotm 2010-04-28 - RCU whinges
From: Paul E. McKenney @ 2010-05-10 15:57 UTC (permalink / raw)
  To: Patrick McHardy
  Cc: David Miller, eric.dumazet, Valdis.Kletnieks, akpm, peterz,
	linux-kernel, netfilter-devel, netdev
In-Reply-To: <4BE8290A.2080707@trash.net>

On Mon, May 10, 2010 at 05:40:58PM +0200, Patrick McHardy wrote:
> David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Mon, 03 May 2010 07:43:56 +0200
> > 
> >> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
> >>
> >>> Oops scratch that, I'll resend a correct version.
> >>>
> >>>
> >> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
> >> thought a different mutex was in use in one of the functions.
> > 
> > Ok, Patrick please review, thanks.
> 
> Actually we don't need the rcu_dereference() calls at all since
> registration and unregistration are protected by the mutexes.

The best approach in that case is rcu_dereference_protected() listing
the lock that must be held.  Of course, your code, so your choice.

						Thanx, Paul

> I queued this patch in nf-next, the only reason why I haven't
> submitted it yet is that I was unable to get git to cleanly export
> only the proper set of patches meant for -next due to a few merges,
> it insists on including 5 patches already merged upstream. If you
> don't mind ignoring the first 5 patches in the series, I'll send a
> pull request tonight.
> 
> 

> commit ed86308f6179d8fa6151c2d0f652aad0091548e2
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Fri Apr 9 16:42:15 2010 +0200
> 
>     netfilter: remove invalid rcu_dereference() calls
>     
>     The CONFIG_PROVE_RCU option discovered a few invalid uses of
>     rcu_dereference() in netfilter. In all these cases, the code code
>     intends to check whether a pointer is already assigned when
>     performing registration or whether the assigned pointer matches
>     when performing unregistration. The entire registration/
>     unregistration is protected by a mutex, so we don't need the
>     rcu_dereference() calls.
>     
>     Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>     Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>     Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
> index d5a9bcd..849614a 100644
> --- a/net/netfilter/nf_conntrack_ecache.c
> +++ b/net/netfilter/nf_conntrack_ecache.c
> @@ -81,11 +81,9 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
>  int nf_conntrack_register_notifier(struct nf_ct_event_notifier *new)
>  {
>  	int ret = 0;
> -	struct nf_ct_event_notifier *notify;
> 
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_conntrack_event_cb);
> -	if (notify != NULL) {
> +	if (nf_conntrack_event_cb != NULL) {
>  		ret = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -101,11 +99,8 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
> 
>  void nf_conntrack_unregister_notifier(struct nf_ct_event_notifier *new)
>  {
> -	struct nf_ct_event_notifier *notify;
> -
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_conntrack_event_cb);
> -	BUG_ON(notify != new);
> +	BUG_ON(nf_conntrack_event_cb != new);
>  	rcu_assign_pointer(nf_conntrack_event_cb, NULL);
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  }
> @@ -114,11 +109,9 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
>  int nf_ct_expect_register_notifier(struct nf_exp_event_notifier *new)
>  {
>  	int ret = 0;
> -	struct nf_exp_event_notifier *notify;
> 
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_expect_event_cb);
> -	if (notify != NULL) {
> +	if (nf_expect_event_cb != NULL) {
>  		ret = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -134,11 +127,8 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
> 
>  void nf_ct_expect_unregister_notifier(struct nf_exp_event_notifier *new)
>  {
> -	struct nf_exp_event_notifier *notify;
> -
>  	mutex_lock(&nf_ct_ecache_mutex);
> -	notify = rcu_dereference(nf_expect_event_cb);
> -	BUG_ON(notify != new);
> +	BUG_ON(nf_expect_event_cb != new);
>  	rcu_assign_pointer(nf_expect_event_cb, NULL);
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  }
> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index 015725a..908f599 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -35,7 +35,6 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
>  /* return EEXIST if the same logger is registred, 0 on success. */
>  int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  {
> -	const struct nf_logger *llog;
>  	int i;
> 
>  	if (pf >= ARRAY_SIZE(nf_loggers))
> @@ -52,8 +51,7 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  	} else {
>  		/* register at end of list to honor first register win */
>  		list_add_tail(&logger->list[pf], &nf_loggers_l[pf]);
> -		llog = rcu_dereference(nf_loggers[pf]);
> -		if (llog == NULL)
> +		if (nf_loggers[pf] == NULL)
>  			rcu_assign_pointer(nf_loggers[pf], logger);
>  	}
> 
> @@ -65,13 +63,11 @@ EXPORT_SYMBOL(nf_log_register);
> 
>  void nf_log_unregister(struct nf_logger *logger)
>  {
> -	const struct nf_logger *c_logger;
>  	int i;
> 
>  	mutex_lock(&nf_log_mutex);
>  	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
> -		c_logger = rcu_dereference(nf_loggers[i]);
> -		if (c_logger == logger)
> +		if (nf_loggers[i] == logger)
>  			rcu_assign_pointer(nf_loggers[i], NULL);
>  		list_del(&logger->list[i]);
>  	}


^ permalink raw reply

* Re: [RFC] net: change bridge/macvlan hook to be be generic
From: Patrick McHardy @ 2010-05-10 15:58 UTC (permalink / raw)
  To: Scott Feldman; +Cc: Stephen Hemminger, David Miller, netdev
In-Reply-To: <C80610E0.2E2EC%scofeldm@cisco.com>

Scott Feldman wrote:
> On 5/4/10 3:37 PM, "Stephen Hemminger" <shemminger@vyatta.com> wrote:
> 
>> The existing macvlan and bridge have special hooks in the packet input
>> path. This patch changes it to a generic hook chain, like the packet type
>> processing. I have been wanting to look into flow based switching, etc...
> 
> Can this be further simplified by saying that a netdev can only be hooked by
> one mux (macvlan, bridge, etc) at any given time, so there is never more
> than one element in the hook chain.  If so, then we just need a single hook,
> not a chain.  
> 
> It seems odd to me that a dev would have both macvlan_port != NULL and
> br_port != NULL.  Can dev be in a macvlan and a bridge at the same time?

Yes, its possible to use the ebtables broute table to have packets
selectively delivered upwards in the stack instead of briding them.
"upwards" could also mean to a macvlan device.

I don't know if anyone is actually doing this, but its a configuration
which currently should work.

^ permalink raw reply

* Re: mmotm 2010-04-28 - RCU whinges
From: Patrick McHardy @ 2010-05-10 16:03 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, Valdis.Kletnieks, akpm, peterz, linux-kernel,
	netfilter-devel, netdev, paulmck
In-Reply-To: <1273506968.2221.19.camel@edumazet-laptop>

Eric Dumazet wrote:
> Le lundi 10 mai 2010 à 17:40 +0200, Patrick McHardy a écrit :
>> David Miller wrote:
>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>> Date: Mon, 03 May 2010 07:43:56 +0200
>>>
>>>> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
>>>>
>>>>> Oops scratch that, I'll resend a correct version.
>>>>>
>>>>>
>>>> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
>>>> thought a different mutex was in use in one of the functions.
>>> Ok, Patrick please review, thanks.
>> Actually we don't need the rcu_dereference() calls at all since
>> registration and unregistration are protected by the mutexes.
>>
>> I queued this patch in nf-next, the only reason why I haven't
>> submitted it yet is that I was unable to get git to cleanly export
>> only the proper set of patches meant for -next due to a few merges,
>> it insists on including 5 patches already merged upstream. If you
>> don't mind ignoring the first 5 patches in the series, I'll send a
>> pull request tonight.
>>
> 
> This will clash with upcoming RCU patches, where rcu protected pointer
> cannot be directly accessed without lockdep splats.
> 
> We will need one day or another a rcu_...(nf_conntrack_event_cb)

Thanks for the information, I didn't realize that when looking at
those patches. So I guess the correct fix once those patches are
merged would be to use rcu_assign_protected() and rcu_access_pointer().
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: mmotm 2010-04-28 - RCU whinges
From: Patrick McHardy @ 2010-05-10 16:04 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, Valdis.Kletnieks, akpm, peterz, linux-kernel,
	netfilter-devel, netdev, paulmck
In-Reply-To: <4BE82E39.6080603@trash.net>

Patrick McHardy wrote:
> Eric Dumazet wrote:
>> Le lundi 10 mai 2010 à 17:40 +0200, Patrick McHardy a écrit :
>>> David Miller wrote:
>>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>>> Date: Mon, 03 May 2010 07:43:56 +0200
>>>>
>>>>> Le lundi 03 mai 2010 à 07:41 +0200, Eric Dumazet a écrit :
>>>>>
>>>>>> Oops scratch that, I'll resend a correct version.
>>>>>>
>>>>>>
>>>>> Sorry, patch _is_ fine, I had one brain collapse when re-reading it, I
>>>>> thought a different mutex was in use in one of the functions.
>>>> Ok, Patrick please review, thanks.
>>> Actually we don't need the rcu_dereference() calls at all since
>>> registration and unregistration are protected by the mutexes.
>>>
>>> I queued this patch in nf-next, the only reason why I haven't
>>> submitted it yet is that I was unable to get git to cleanly export
>>> only the proper set of patches meant for -next due to a few merges,
>>> it insists on including 5 patches already merged upstream. If you
>>> don't mind ignoring the first 5 patches in the series, I'll send a
>>> pull request tonight.
>>>
>> This will clash with upcoming RCU patches, where rcu protected pointer
>> cannot be directly accessed without lockdep splats.
>>
>> We will need one day or another a rcu_...(nf_conntrack_event_cb)
> 
> Thanks for the information, I didn't realize that when looking at
> those patches. So I guess the correct fix once those patches are
> merged would be to use rcu_assign_protected() and rcu_access_pointer().

Ah, and that's what you did. Sorry for the confusion :)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH] drivers/net/usb/asix.c Fix unaligned access
From: Neil Jones @ 2010-05-10 16:06 UTC (permalink / raw)
  To: netdev
In-Reply-To: <s2q91f916b91005060413pe1a1799etdea5ad730841a04e@mail.gmail.com>

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

From b277dbc256de7b1a8c47ca374914c097ff4cdd50 Mon Sep 17 00:00:00 2001
From: Neil Jones <NeilJay@gmail.com>
Date: Thu, 6 May 2010 11:20:53 +0100
Subject: [PATCH] drivers/net/usb/asix.c:        Fix unaligned accesses

Using this driver can cause unaligned accesses in the IP layer
This has been fixed by aligning the skb data correctly using the
spare room left over by the 4 byte header inserted between packets
by the device.

Signed-off-by: Neil Jones <NeilJay@gmail.com>
---
 drivers/net/usb/asix.c |   37 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/drivers/net/usb/asix.c b/drivers/net/usb/asix.c
index a516185..5b4f0df 100644
--- a/drivers/net/usb/asix.c
+++ b/drivers/net/usb/asix.c
@@ -319,16 +319,51 @@ static int asix_rx_fixup(struct usbnet *dev,
struct sk_buff *skb)
               /* get the packet length */
               size = (u16) (header & 0x0000ffff);

-               if ((skb->len) - ((size + 1) & 0xfffe) == 0)
+               if ((skb->len) - ((size + 1) & 0xfffe) == 0) {
+                       u8 alignment = (u32)skb->data & 0x3;
+                       if (alignment != 0x2) {
+                               /*
+                                * not 16bit aligned so use the room provided by
+                                * the 32 bit header to align the data
+                                *
+                                * note we want 16bit alignment as MAC header is
+                                * 14bytes thus ip header will be aligned on
+                                * 32bit boundary so accessing ipheader elements
+                                * using a cast to struct ip header wont cause
+                                * an unaligned accesses.
+                                */
+                               u8 realignment = (alignment + 2) & 0x3;
+                               memmove(skb->data - realignment,
+                                       skb->data,
+                                       size);
+                               skb->data -= realignment;
+                               skb_set_tail_pointer(skb, size);
+                       }
                       return 2;
+               }
+
+
               if (size > ETH_FRAME_LEN) {
                       deverr(dev,"asix_rx_fixup() Bad RX Length %d", size);
                       return 0;
               }
               ax_skb = skb_clone(skb, GFP_ATOMIC);
               if (ax_skb) {
+                       u8 alignment = (u32)packet & 0x3;
                       ax_skb->len = size;
+
+                       if (alignment != 0x2) {
+                               /*
+                                * not 16bit aligned use the room provided by
+                                * the 32 bit header to align the data
+                                */
+                               u8 realignment = (alignment + 2) & 0x3;
+                               memmove(packet - realignment, packet, size);
+                               packet -= realignment;
+                       }
                       ax_skb->data = packet;
+
+
                       skb_set_tail_pointer(ax_skb, size);
                       usbnet_skb_return(dev, ax_skb);
               } else {
--
1.5.5.2

[-- Attachment #2: 0001-drivers-net-usb-asix.c-Fix-unaligned-accesses.patch --]
[-- Type: application/octet-stream, Size: 2284 bytes --]

From b277dbc256de7b1a8c47ca374914c097ff4cdd50 Mon Sep 17 00:00:00 2001
From: Neil Jones <njones@lofty.le.imgtec.org>
Date: Thu, 6 May 2010 11:20:53 +0100
Subject: [PATCH] drivers/net/usb/asix.c:	Fix unaligned accesses

Using this driver can cause unaligned accesses in the IP layer
This has been fixed by aligning the skb data correctly using the
spare room left over by the 4 byte header inserted between packets
by the device.

Signed-off-by: Neil Jones <NeilJay@gmail.com>
---
 drivers/net/usb/asix.c |   37 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/drivers/net/usb/asix.c b/drivers/net/usb/asix.c
index a516185..5b4f0df 100644
--- a/drivers/net/usb/asix.c
+++ b/drivers/net/usb/asix.c
@@ -319,16 +319,51 @@ static int asix_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 		/* get the packet length */
 		size = (u16) (header & 0x0000ffff);
 
-		if ((skb->len) - ((size + 1) & 0xfffe) == 0)
+		if ((skb->len) - ((size + 1) & 0xfffe) == 0) {
+			u8 alignment = (u32)skb->data & 0x3;
+			if (alignment != 0x2) {
+				/*
+				 * not 16bit aligned so use the room provided by
+				 * the 32 bit header to align the data
+				 *
+				 * note we want 16bit alignment as MAC header is
+				 * 14bytes thus ip header will be aligned on
+				 * 32bit boundary so accessing ipheader elements
+				 * using a cast to struct ip header wont cause
+				 * an unaligned accesses.
+				 */
+				u8 realignment = (alignment + 2) & 0x3;
+				memmove(skb->data - realignment,
+					skb->data,
+					size);
+				skb->data -= realignment;
+				skb_set_tail_pointer(skb, size);
+			}
 			return 2;
+		}
+
+
 		if (size > ETH_FRAME_LEN) {
 			deverr(dev,"asix_rx_fixup() Bad RX Length %d", size);
 			return 0;
 		}
 		ax_skb = skb_clone(skb, GFP_ATOMIC);
 		if (ax_skb) {
+			u8 alignment = (u32)packet & 0x3;
 			ax_skb->len = size;
+
+			if (alignment != 0x2) {
+				/*
+				 * not 16bit aligned use the room provided by
+				 * the 32 bit header to align the data
+				 */
+				u8 realignment = (alignment + 2) & 0x3;
+				memmove(packet - realignment, packet, size);
+				packet -= realignment;
+			}
 			ax_skb->data = packet;
+
+
 			skb_set_tail_pointer(ax_skb, size);
 			usbnet_skb_return(dev, ax_skb);
 		} else {
-- 
1.5.5.2


^ permalink raw reply related

* Re: 2.6.34-rc6-git6: Reported regressions from 2.6.33
From: Nick Bowler @ 2010-05-10 16:07 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: DRI, Linux SCSI List, Network Development, Linux Wireless List,
	Linux Kernel Mailing List, Linux ACPI, Andrew Morton,
	Kernel Testers List, Linus Torvalds, Linux PM List,
	Maciej Rutecki
In-Reply-To: <10aqDnTJcLO.A.mLE.48y5LB@chimera>

On 23:13 Sun 09 May     , Rafael J. Wysocki wrote:
> This message contains a list of some regressions from 2.6.33,
> for which there are no fixes in the mainline known to the tracking team.
> If any of them have been fixed already, please let us know.
> 
> If you know of any other unresolved regressions from 2.6.33, please let us
> know either and we'll add them to the list.  Also, please let us know
> if any of the entries below are invalid.

Seems that

  r600 CS checker rejects narrow FBO renderbuffers
  https://bugs.freedesktop.org/show_bug.cgi?id=27609

never got added to the list.  It is still an issue as of 2.6.34-rc7.

-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

^ permalink raw reply

* Re: [PATCH] virtif: initial interface extensions
From: Stefan Berger @ 2010-05-10 15:37 UTC (permalink / raw)
  To: netdev
In-Reply-To: <201005090120.19958.arnd@arndb.de>

Arnd Bergmann <arnd <at> arndb.de> writes:

[...]

> +	if (tb[IFLA_VIRTIF]) {
> +		struct ifla_virtif_port_profile *ivp;
> +		struct nlattr *virtif[IFLA_VIRTIF_MAX+1];
> +		u32 vf;
> +
> +		err = nla_parse_nested(virtif, IFLA_VIRTIF_MAX,
> +				       tb[IFLA_VIRTIF], ifla_virtif_policy);
> +		if (err < 0)
> +			return err;
> +
> +		if (!virtif[IFLA_VIRTIF_VF] || !virtif[IFLA_VIRTIF_PORT_PROFILE])
> +			goto novirtif; /* IFLA_VIRTIF may be directed at user space */


In what case would the IFLA_VIRTIF_PORT_PROFILE be provided? Would libvirt for
example need to be aware of whether the Ethernet device can handle the setup
protocol via its firmware and in this case provide the port profile parameter
and in other cases provide other parameters? Certainly the user or upper layer
management software would have to know it when creating the domain XML and in
fact different types of parameters were needed. Obviously we should have one
common set of (XML) parameters that go into the netlink message and that can be
handled by the kernel driver if the firmware knows how to handle it or by
LLDPAD. Libvirt would send the parameters via netlink message to trigger the
setup protocol and the message may be received by kernel and LLDPAD. From what I
can see LLDPAD also may need a way to probe the kernel driver whether it handled
the setup protocol via firmware on a given interface, which may or may not be
true for all interfaces, but may be necessary to avoid triggering the setup
protocol twice.

   Stefan




^ permalink raw reply

* Re: mmotm 2010-04-28 - RCU whinges
From: David Miller @ 2010-05-10 16:12 UTC (permalink / raw)
  To: kaber
  Cc: eric.dumazet, Valdis.Kletnieks, akpm, peterz, linux-kernel,
	netfilter-devel, netdev, paulmck
In-Reply-To: <4BE8290A.2080707@trash.net>

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 10 May 2010 17:40:58 +0200

> I queued this patch in nf-next, the only reason why I haven't
> submitted it yet is that I was unable to get git to cleanly export
> only the proper set of patches meant for -next due to a few merges,
> it insists on including 5 patches already merged upstream. If you
> don't mind ignoring the first 5 patches in the series, I'll send a
> pull request tonight.

Something like "git format-patch origin" doesn't avoid those upstream
commits?  Weird...

Another trick is to specify a commit range using triple-dot "..."
notation, such as "origin...master"

^ permalink raw reply

* Re: mmotm 2010-04-28 - RCU whinges
From: Patrick McHardy @ 2010-05-10 16:27 UTC (permalink / raw)
  To: David Miller
  Cc: eric.dumazet, Valdis.Kletnieks, akpm, peterz, linux-kernel,
	netfilter-devel, netdev, paulmck
In-Reply-To: <20100510.091257.51273494.davem@davemloft.net>

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Mon, 10 May 2010 17:40:58 +0200
> 
>> I queued this patch in nf-next, the only reason why I haven't
>> submitted it yet is that I was unable to get git to cleanly export
>> only the proper set of patches meant for -next due to a few merges,
>> it insists on including 5 patches already merged upstream. If you
>> don't mind ignoring the first 5 patches in the series, I'll send a
>> pull request tonight.
> 
> Something like "git format-patch origin" doesn't avoid those upstream
> commits?  Weird...

Yeah, it seems to have something to do with me merging the nf-2.6.git
tree a few weeks ago since it had patches queued that were too late
for 2.6.34. Even the --ignore-if-in-upstream option doesn't help.

> Another trick is to specify a commit range using triple-dot "..."
> notation, such as "origin...master"

Thanks, I'll give it another try, the alternative is manually
renumbering the entire patchset.

^ permalink raw reply

* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: Michael S. Tsirkin @ 2010-05-10 16:43 UTC (permalink / raw)
  To: David L Stevens; +Cc: netdev, kvm, virtualization
In-Reply-To: <1272488232.11307.4.camel@w-dls.beaverton.ibm.com>

On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
> @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
>  	use_mm(net->dev.mm);
>  	mutex_lock(&vq->mutex);
>  	vhost_disable_notify(vq);
> -	hdr_size = vq->hdr_size;
> +	vhost_hlen = vq->vhost_hlen;
>  
>  	vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
>  		vq->log : NULL;
>  
> -	for (;;) {
> -		head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> -					 ARRAY_SIZE(vq->iov),
> -					 &out, &in,
> -					 vq_log, &log);
> +	while ((datalen = vhost_head_len(vq, sock->sk))) {
> +		headcount = vhost_get_desc_n(vq, vq->heads,
> +					     datalen + vhost_hlen,
> +					     &in, vq_log, &log);
> +		if (headcount < 0)
> +			break;
>  		/* OK, now we need to know about added descriptors. */
> -		if (head == vq->num) {
> +		if (!headcount) {
>  			if (unlikely(vhost_enable_notify(vq))) {
>  				/* They have slipped one in as we were
>  				 * doing that: check again. */
> @@ -241,46 +272,53 @@ static void handle_rx(struct vhost_net *
>  			break;
>  		}
>  		/* We don't need to be notified again. */
> -		if (out) {
> -			vq_err(vq, "Unexpected descriptor format for RX: "
> -			       "out %d, int %d\n",
> -			       out, in);
> -			break;
> -		}
> -		/* Skip header. TODO: support TSO/mergeable rx buffers. */
> -		s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> +		if (vhost_hlen)
> +			/* Skip header. TODO: support TSO. */
> +			s = move_iovec_hdr(vq->iov, vq->hdr, vhost_hlen, in);
> +		else
> +			s = copy_iovec_hdr(vq->iov, vq->hdr, vq->sock_hlen, in);
>  		msg.msg_iovlen = in;
>  		len = iov_length(vq->iov, in);
>  		/* Sanity check */
>  		if (!len) {
>  			vq_err(vq, "Unexpected header len for RX: "
>  			       "%zd expected %zd\n",
> -			       iov_length(vq->hdr, s), hdr_size);
> +			       iov_length(vq->hdr, s), vhost_hlen);
>  			break;
>  		}
>  		err = sock->ops->recvmsg(NULL, sock, &msg,
>  					 len, MSG_DONTWAIT | MSG_TRUNC);
>  		/* TODO: Check specific error and bomb out unless EAGAIN? */
>  		if (err < 0) {
> -			vhost_discard_vq_desc(vq);
> +			vhost_discard_desc(vq, headcount);
>  			break;
>  		}
> -		/* TODO: Should check and handle checksum. */
> -		if (err > len) {
> -			pr_err("Discarded truncated rx packet: "
> -			       " len %d > %zd\n", err, len);
> -			vhost_discard_vq_desc(vq);
> +		if (err != datalen) {
> +			pr_err("Discarded rx packet: "
> +			       " len %d, expected %zd\n", err, datalen);
> +			vhost_discard_desc(vq, headcount);
>  			continue;
>  		}
>  		len = err;
> -		err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
> -		if (err) {
> -			vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
> -			       vq->iov->iov_base, err);
> +		if (vhost_hlen &&
> +		    memcpy_toiovecend(vq->hdr, (unsigned char *)&hdr, 0,
> +				      vhost_hlen)) {
> +			vq_err(vq, "Unable to write vnet_hdr at addr %p\n",
> +			       vq->iov->iov_base);
>  			break;
>  		}
> -		len += hdr_size;
> -		vhost_add_used_and_signal(&net->dev, vq, head, len);
> +		/* TODO: Should check and handle checksum. */
> +		if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> +		    memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> +				      offsetof(typeof(hdr), num_buffers),
> +				      sizeof(hdr.num_buffers))) {
> +			vq_err(vq, "Failed num_buffers write");
> +			vhost_discard_desc(vq, headcount);
> +			break;
> +		}
> +		len += vhost_hlen;
> +		vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
> +					    headcount);
>  		if (unlikely(vq_log))
>  			vhost_log_write(vq, vq_log, log, len);
>  		total_len += len;

OK I think I see the bug here: vhost_add_used_and_signal_n
does not get the actual length, it gets the iovec length from vhost.
Guest virtio uses this as packet length, with bad results.

So I have applied the follows and it seems to have fixed the problem:

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index c16db02..9d7496d 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -219,7 +219,7 @@ static int peek_head_len(struct vhost_virtqueue *vq, struct sock *sk)
 /* This is a multi-buffer version of vhost_get_desc, that works if
  *	vq has read descriptors only.
  * @vq		- the relevant virtqueue
- * @datalen	- data length we'll be reading
+ * @datalen	- data length we'll be reading. must be > 0
  * @iovcount	- returned count of io vectors we fill
  * @log		- vhost log
  * @log_num	- log offset
@@ -236,9 +236,10 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
 	int seg = 0;
 	int headcount = 0;
 	unsigned d;
+	size_t len;
 	int r, nlogs = 0;
 
-	while (datalen > 0) {
+	for (;;) {
 		if (unlikely(headcount >= VHOST_NET_MAX_SG)) {
 			r = -ENOBUFS;
 			goto err;
@@ -260,16 +261,20 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
 			nlogs += *log_num;
 			log += *log_num;
 		}
+		len = iov_length(vq->iov + seg, in);
+		seg += in;
 		heads[headcount].id = d;
-		heads[headcount].len = iov_length(vq->iov + seg, in);
-		datalen -= heads[headcount].len;
+		if (datalen <= len)
+			break;
+		heads[headcount].len = len;
 		++headcount;
-		seg += in;
+		datalen -= len;
 	}
+	heads[headcount].len = datalen;
 	*iovcount = seg;
 	if (unlikely(log))
 		*log_num = nlogs;
-	return headcount;
+	return headcount + 1;
 err:
 	vhost_discard_desc(vq, headcount);
 	return r;

-- 
MST

^ permalink raw reply related

* Re: mmotm 2010-04-28 - RCU whinges
From: Patrick McHardy @ 2010-05-10 16:53 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Valdis.Kletnieks, Andrew Morton, Peter Zijlstra, David S. Miller,
	linux-kernel, netfilter-devel, netdev, Paul E. McKenney
In-Reply-To: <1272865137.2173.179.camel@edumazet-laptop>

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

Eric Dumazet wrote:
> Le dimanche 02 mai 2010 à 13:46 -0400, Valdis.Kletnieks@vt.edu a écrit :
>> On Wed, 28 Apr 2010 16:53:32 PDT, akpm@linux-foundation.org said:
>>> The mm-of-the-moment snapshot 2010-04-28-16-53 has been uploaded to
>>>
>>>    http://userweb.kernel.org/~akpm/mmotm/
>> I thought we swatted all these, hit another one...
>>
>> [    9.131490] ctnetlink v0.93: registering with nfnetlink.
>> [    9.131535]
>> [    9.131535] ===================================================
>> [    9.131704] [ INFO: suspicious rcu_dereference_check() usage. ]
>> [    9.131794] ---------------------------------------------------
>> [    9.131883] net/netfilter/nf_conntrack_ecache.c:88 invoked rcu_dereference_check() without protection!
>> [    9.131977]
>> [    9.131977] other info that might help us debug this:
>> [    9.131978]
>> [    9.132218]
>> [    9.132219] rcu_scheduler_active = 1, debug_locks = 0
>> [    9.132434] 1 lock held by swapper/1:
>> [    9.132519]  #0:  (nf_ct_ecache_mutex){+.+...}, at: [<ffffffff8148922d>] nf_conntrack_register_notifier+0x1a/0x75
>> [    9.132938]
>> [    9.132939] stack backtrace:
>> [    9.133129] Pid: 1, comm: swapper Tainted: G        W   2.6.34-rc5-mmotm0428 #1
>> [    9.133220] Call Trace:
>> [    9.133319]  [<ffffffff81064832>] lockdep_rcu_dereference+0xaa/0xb2
>> [    9.133410]  [<ffffffff81489250>] nf_conntrack_register_notifier+0x3d/0x75
>> [    9.133521]  [<ffffffff81b5a157>] ctnetlink_init+0x71/0xd5
>> [    9.133627]  [<ffffffff81b5a0e6>] ? ctnetlink_init+0x0/0xd5
>> [    9.133735]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
>> [    9.133843]  [<ffffffff81b2e68a>] kernel_init+0x144/0x1ce
>> [    9.133949]  [<ffffffff81003414>] kernel_thread_helper+0x4/0x10
>> [    9.134060]  [<ffffffff81598a40>] ? restore_args+0x0/0x30
>> [    9.134196]  [<ffffffff81b2e546>] ? kernel_init+0x0/0x1ce
>> [    9.134328]  [<ffffffff81003410>] ? kernel_thread_helper+0x0/0x10
>> [    9.134530] ip_tables: (C) 2000-2006 Netfilter Core Team
>> [    9.134655] TCP bic registered
>>
> 
> Thanks for the report !
> 
> We can use rcu_dereference_protected() in those cases.
> 
> [PATCH] net: Use rcu_dereference_protected in nf_conntrack_ecache
> 
> Writers own nf_ct_ecache_mutex.

I've committed this patch to my tree, which also fixes up the nf_log
changes I already had queued.

I've also figured out how to prevent the false commits from showing
up using the '^' notation, I'll submit everything after some final
testing.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3904 bytes --]

commit b56f2d55c6c22b0c5774b3b22e336fb6cc5f4094
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon May 10 18:47:57 2010 +0200

    netfilter: use rcu_dereference_protected()
    
    Restore the rcu_dereference() calls in conntrack/expectation notifier
    and logger registration/unregistration, but use the _protected variant,
    which will be required by the upcoming __rcu annotations.
    
    Based on patch by Eric Dumazet <eric.dumazet@gmail.com>
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index a94ac3a..cdcc764 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -82,9 +82,12 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
 int nf_conntrack_register_notifier(struct nf_ct_event_notifier *new)
 {
 	int ret = 0;
+	struct nf_ct_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	if (nf_conntrack_event_cb != NULL) {
+	notify = rcu_dereference_protected(nf_conntrack_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
+	if (notify != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
@@ -100,8 +103,12 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
 
 void nf_conntrack_unregister_notifier(struct nf_ct_event_notifier *new)
 {
+	struct nf_ct_event_notifier *notify;
+
 	mutex_lock(&nf_ct_ecache_mutex);
-	BUG_ON(nf_conntrack_event_cb != new);
+	notify = rcu_dereference_protected(nf_conntrack_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
+	BUG_ON(notify != new);
 	rcu_assign_pointer(nf_conntrack_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);
 }
@@ -110,9 +117,12 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
 int nf_ct_expect_register_notifier(struct nf_exp_event_notifier *new)
 {
 	int ret = 0;
+	struct nf_exp_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
-	if (nf_expect_event_cb != NULL) {
+	notify = rcu_dereference_protected(nf_expect_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
+	if (notify != NULL) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
@@ -128,8 +138,12 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
 
 void nf_ct_expect_unregister_notifier(struct nf_exp_event_notifier *new)
 {
+	struct nf_exp_event_notifier *notify;
+
 	mutex_lock(&nf_ct_ecache_mutex);
-	BUG_ON(nf_expect_event_cb != new);
+	notify = rcu_dereference_protected(nf_expect_event_cb,
+					   lockdep_is_held(&nf_ct_ecache_mutex));
+	BUG_ON(notify != new);
 	rcu_assign_pointer(nf_expect_event_cb, NULL);
 	mutex_unlock(&nf_ct_ecache_mutex);
 }
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 908f599..7df37fd 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -35,6 +35,7 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 /* return EEXIST if the same logger is registred, 0 on success. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
+	const struct nf_logger *llog;
 	int i;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
@@ -51,7 +52,9 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 	} else {
 		/* register at end of list to honor first register win */
 		list_add_tail(&logger->list[pf], &nf_loggers_l[pf]);
-		if (nf_loggers[pf] == NULL)
+		llog = rcu_dereference_protected(nf_loggers[pf],
+						 lockdep_is_held(&nf_log_mutex));
+		if (llog == NULL)
 			rcu_assign_pointer(nf_loggers[pf], logger);
 	}
 
@@ -63,11 +66,14 @@ EXPORT_SYMBOL(nf_log_register);
 
 void nf_log_unregister(struct nf_logger *logger)
 {
+	const struct nf_logger *c_logger;
 	int i;
 
 	mutex_lock(&nf_log_mutex);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		if (nf_loggers[i] == logger)
+		c_logger = rcu_dereference_protected(nf_loggers[i],
+						     lockdep_is_held(&nf_log_mutex));
+		if (c_logger == logger)
 			rcu_assign_pointer(nf_loggers[i], NULL);
 		list_del(&logger->list[i]);
 	}

^ permalink raw reply related

* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: David Stevens @ 2010-05-10 17:09 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: kvm, kvm-owner, netdev, virtualization
In-Reply-To: <20100510164303.GA28798@redhat.com>

Since "datalen" carries the difference and will be negative by that amount
from the original loop, what about just adding something like:

        }
        if (headcount)
                heads[headcount-1].len += datalen;
[and really, headcount >0 since datalen > 0, so just:

        heads[headcount-1].len += datalen;

                                                +-DLS


kvm-owner@vger.kernel.org wrote on 05/10/2010 09:43:03 AM:

> On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
> > @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
> >     use_mm(net->dev.mm);
> >     mutex_lock(&vq->mutex);
> >     vhost_disable_notify(vq);
> > -   hdr_size = vq->hdr_size;
> > +   vhost_hlen = vq->vhost_hlen;
> > 
> >     vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> >        vq->log : NULL;
> > 
> > -   for (;;) {
> > -      head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> > -                ARRAY_SIZE(vq->iov),
> > -                &out, &in,
> > -                vq_log, &log);
> > +   while ((datalen = vhost_head_len(vq, sock->sk))) {
> > +      headcount = vhost_get_desc_n(vq, vq->heads,
> > +                    datalen + vhost_hlen,
> > +                    &in, vq_log, &log);
> > +      if (headcount < 0)
> > +         break;
> >        /* OK, now we need to know about added descriptors. */
> > -      if (head == vq->num) {
> > +      if (!headcount) {
> >           if (unlikely(vhost_enable_notify(vq))) {
> >              /* They have slipped one in as we were
> >               * doing that: check again. */
> > @@ -241,46 +272,53 @@ static void handle_rx(struct vhost_net *
> >           break;
> >        }
> >        /* We don't need to be notified again. */
> > -      if (out) {
> > -         vq_err(vq, "Unexpected descriptor format for RX: "
> > -                "out %d, int %d\n",
> > -                out, in);
> > -         break;
> > -      }
> > -      /* Skip header. TODO: support TSO/mergeable rx buffers. */
> > -      s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> > +      if (vhost_hlen)
> > +         /* Skip header. TODO: support TSO. */
> > +         s = move_iovec_hdr(vq->iov, vq->hdr, vhost_hlen, in);
> > +      else
> > +         s = copy_iovec_hdr(vq->iov, vq->hdr, vq->sock_hlen, in);
> >        msg.msg_iovlen = in;
> >        len = iov_length(vq->iov, in);
> >        /* Sanity check */
> >        if (!len) {
> >           vq_err(vq, "Unexpected header len for RX: "
> >                  "%zd expected %zd\n",
> > -                iov_length(vq->hdr, s), hdr_size);
> > +                iov_length(vq->hdr, s), vhost_hlen);
> >           break;
> >        }
> >        err = sock->ops->recvmsg(NULL, sock, &msg,
> >                  len, MSG_DONTWAIT | MSG_TRUNC);
> >        /* TODO: Check specific error and bomb out unless EAGAIN? */
> >        if (err < 0) {
> > -         vhost_discard_vq_desc(vq);
> > +         vhost_discard_desc(vq, headcount);
> >           break;
> >        }
> > -      /* TODO: Should check and handle checksum. */
> > -      if (err > len) {
> > -         pr_err("Discarded truncated rx packet: "
> > -                " len %d > %zd\n", err, len);
> > -         vhost_discard_vq_desc(vq);
> > +      if (err != datalen) {
> > +         pr_err("Discarded rx packet: "
> > +                " len %d, expected %zd\n", err, datalen);
> > +         vhost_discard_desc(vq, headcount);
> >           continue;
> >        }
> >        len = err;
> > -      err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
> > -      if (err) {
> > -         vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
> > -                vq->iov->iov_base, err);
> > +      if (vhost_hlen &&
> > +          memcpy_toiovecend(vq->hdr, (unsigned char *)&hdr, 0,
> > +                  vhost_hlen)) {
> > +         vq_err(vq, "Unable to write vnet_hdr at addr %p\n",
> > +                vq->iov->iov_base);
> >           break;
> >        }
> > -      len += hdr_size;
> > -      vhost_add_used_and_signal(&net->dev, vq, head, len);
> > +      /* TODO: Should check and handle checksum. */
> > +      if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> > +          memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> > +                  offsetof(typeof(hdr), num_buffers),
> > +                  sizeof(hdr.num_buffers))) {
> > +         vq_err(vq, "Failed num_buffers write");
> > +         vhost_discard_desc(vq, headcount);
> > +         break;
> > +      }
> > +      len += vhost_hlen;
> > +      vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
> > +                   headcount);
> >        if (unlikely(vq_log))
> >           vhost_log_write(vq, vq_log, log, len);
> >        total_len += len;
> 
> OK I think I see the bug here: vhost_add_used_and_signal_n
> does not get the actual length, it gets the iovec length from vhost.
> Guest virtio uses this as packet length, with bad results.
> 
> So I have applied the follows and it seems to have fixed the problem:
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index c16db02..9d7496d 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -219,7 +219,7 @@ static int peek_head_len(struct vhost_virtqueue *vq, 

> struct sock *sk)
>  /* This is a multi-buffer version of vhost_get_desc, that works if
>   *   vq has read descriptors only.
>   * @vq      - the relevant virtqueue
> - * @datalen   - data length we'll be reading
> + * @datalen   - data length we'll be reading. must be > 0
>   * @iovcount   - returned count of io vectors we fill
>   * @log      - vhost log
>   * @log_num   - log offset
> @@ -236,9 +236,10 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
>     int seg = 0;
>     int headcount = 0;
>     unsigned d;
> +   size_t len;
>     int r, nlogs = 0;
> 
> -   while (datalen > 0) {
> +   for (;;) {
>        if (unlikely(headcount >= VHOST_NET_MAX_SG)) {
>           r = -ENOBUFS;
>           goto err;
> @@ -260,16 +261,20 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
>           nlogs += *log_num;
>           log += *log_num;
>        }
> +      len = iov_length(vq->iov + seg, in);
> +      seg += in;
>        heads[headcount].id = d;
> -      heads[headcount].len = iov_length(vq->iov + seg, in);
> -      datalen -= heads[headcount].len;
> +      if (datalen <= len)
> +         break;
> +      heads[headcount].len = len;
>        ++headcount;
> -      seg += in;
> +      datalen -= len;
>     }
> +   heads[headcount].len = datalen;
>     *iovcount = seg;
>     if (unlikely(log))
>        *log_num = nlogs;
> -   return headcount;
> +   return headcount + 1;
>  err:
>     vhost_discard_desc(vq, headcount);
>     return r;
> 
> -- 
> MST
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


^ permalink raw reply

* Re: [RFC] net: change bridge/macvlan hook to be be generic
From: Ben Greear @ 2010-05-10 17:14 UTC (permalink / raw)
  To: Scott Feldman; +Cc: Stephen Hemminger, David Miller, Patrick McHardy, netdev
In-Reply-To: <C80610E0.2E2EC%scofeldm@cisco.com>

On 05/04/2010 05:58 PM, Scott Feldman wrote:
> On 5/4/10 3:37 PM, "Stephen Hemminger"<shemminger@vyatta.com>  wrote:
>
>> The existing macvlan and bridge have special hooks in the packet input
>> path. This patch changes it to a generic hook chain, like the packet type
>> processing. I have been wanting to look into flow based switching, etc...
>
> Can this be further simplified by saying that a netdev can only be hooked by
> one mux (macvlan, bridge, etc) at any given time, so there is never more
> than one element in the hook chain.  If so, then we just need a single hook,
> not a chain.
>
> It seems odd to me that a dev would have both macvlan_port != NULL and
> br_port != NULL.  Can dev be in a macvlan and a bridge at the same time?

If we did add the generic hook list, then we could support other hooks, like
a pktgen rx hook to gather latency, pkt-loss, and similar stats, for example.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

^ permalink raw reply

* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: Michael S. Tsirkin @ 2010-05-10 17:25 UTC (permalink / raw)
  To: David Stevens; +Cc: kvm, kvm-owner, netdev, virtualization
In-Reply-To: <OF1F65A110.7C63CDB3-ON8825771F.005DD80E-8825771F.005E3509@us.ibm.com>

On Mon, May 10, 2010 at 10:09:03AM -0700, David Stevens wrote:
> Since "datalen" carries the difference and will be negative by that amount
> from the original loop, what about just adding something like:
> 
>         }
>         if (headcount)
>                 heads[headcount-1].len += datalen;
> [and really, headcount >0 since datalen > 0, so just:
> 
>         heads[headcount-1].len += datalen;
> 
>                                                 +-DLS

This works too, just does more checks and comparisons.
I am still surprised that you were unable to reproduce the problem.

> 
> kvm-owner@vger.kernel.org wrote on 05/10/2010 09:43:03 AM:
> 
> > On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
> > > @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
> > >     use_mm(net->dev.mm);
> > >     mutex_lock(&vq->mutex);
> > >     vhost_disable_notify(vq);
> > > -   hdr_size = vq->hdr_size;
> > > +   vhost_hlen = vq->vhost_hlen;
> > > 
> > >     vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> > >        vq->log : NULL;
> > > 
> > > -   for (;;) {
> > > -      head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> > > -                ARRAY_SIZE(vq->iov),
> > > -                &out, &in,
> > > -                vq_log, &log);
> > > +   while ((datalen = vhost_head_len(vq, sock->sk))) {
> > > +      headcount = vhost_get_desc_n(vq, vq->heads,
> > > +                    datalen + vhost_hlen,
> > > +                    &in, vq_log, &log);
> > > +      if (headcount < 0)
> > > +         break;
> > >        /* OK, now we need to know about added descriptors. */
> > > -      if (head == vq->num) {
> > > +      if (!headcount) {
> > >           if (unlikely(vhost_enable_notify(vq))) {
> > >              /* They have slipped one in as we were
> > >               * doing that: check again. */
> > > @@ -241,46 +272,53 @@ static void handle_rx(struct vhost_net *
> > >           break;
> > >        }
> > >        /* We don't need to be notified again. */
> > > -      if (out) {
> > > -         vq_err(vq, "Unexpected descriptor format for RX: "
> > > -                "out %d, int %d\n",
> > > -                out, in);
> > > -         break;
> > > -      }
> > > -      /* Skip header. TODO: support TSO/mergeable rx buffers. */
> > > -      s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> > > +      if (vhost_hlen)
> > > +         /* Skip header. TODO: support TSO. */
> > > +         s = move_iovec_hdr(vq->iov, vq->hdr, vhost_hlen, in);
> > > +      else
> > > +         s = copy_iovec_hdr(vq->iov, vq->hdr, vq->sock_hlen, in);
> > >        msg.msg_iovlen = in;
> > >        len = iov_length(vq->iov, in);
> > >        /* Sanity check */
> > >        if (!len) {
> > >           vq_err(vq, "Unexpected header len for RX: "
> > >                  "%zd expected %zd\n",
> > > -                iov_length(vq->hdr, s), hdr_size);
> > > +                iov_length(vq->hdr, s), vhost_hlen);
> > >           break;
> > >        }
> > >        err = sock->ops->recvmsg(NULL, sock, &msg,
> > >                  len, MSG_DONTWAIT | MSG_TRUNC);
> > >        /* TODO: Check specific error and bomb out unless EAGAIN? */
> > >        if (err < 0) {
> > > -         vhost_discard_vq_desc(vq);
> > > +         vhost_discard_desc(vq, headcount);
> > >           break;
> > >        }
> > > -      /* TODO: Should check and handle checksum. */
> > > -      if (err > len) {
> > > -         pr_err("Discarded truncated rx packet: "
> > > -                " len %d > %zd\n", err, len);
> > > -         vhost_discard_vq_desc(vq);
> > > +      if (err != datalen) {
> > > +         pr_err("Discarded rx packet: "
> > > +                " len %d, expected %zd\n", err, datalen);
> > > +         vhost_discard_desc(vq, headcount);
> > >           continue;
> > >        }
> > >        len = err;
> > > -      err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
> > > -      if (err) {
> > > -         vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
> > > -                vq->iov->iov_base, err);
> > > +      if (vhost_hlen &&
> > > +          memcpy_toiovecend(vq->hdr, (unsigned char *)&hdr, 0,
> > > +                  vhost_hlen)) {
> > > +         vq_err(vq, "Unable to write vnet_hdr at addr %p\n",
> > > +                vq->iov->iov_base);
> > >           break;
> > >        }
> > > -      len += hdr_size;
> > > -      vhost_add_used_and_signal(&net->dev, vq, head, len);
> > > +      /* TODO: Should check and handle checksum. */
> > > +      if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> > > +          memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> > > +                  offsetof(typeof(hdr), num_buffers),
> > > +                  sizeof(hdr.num_buffers))) {
> > > +         vq_err(vq, "Failed num_buffers write");
> > > +         vhost_discard_desc(vq, headcount);
> > > +         break;
> > > +      }
> > > +      len += vhost_hlen;
> > > +      vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
> > > +                   headcount);
> > >        if (unlikely(vq_log))
> > >           vhost_log_write(vq, vq_log, log, len);
> > >        total_len += len;
> > 
> > OK I think I see the bug here: vhost_add_used_and_signal_n
> > does not get the actual length, it gets the iovec length from vhost.
> > Guest virtio uses this as packet length, with bad results.
> > 
> > So I have applied the follows and it seems to have fixed the problem:
> > 
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index c16db02..9d7496d 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -219,7 +219,7 @@ static int peek_head_len(struct vhost_virtqueue *vq, 
> 
> > struct sock *sk)
> >  /* This is a multi-buffer version of vhost_get_desc, that works if
> >   *   vq has read descriptors only.
> >   * @vq      - the relevant virtqueue
> > - * @datalen   - data length we'll be reading
> > + * @datalen   - data length we'll be reading. must be > 0
> >   * @iovcount   - returned count of io vectors we fill
> >   * @log      - vhost log
> >   * @log_num   - log offset
> > @@ -236,9 +236,10 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> >     int seg = 0;
> >     int headcount = 0;
> >     unsigned d;
> > +   size_t len;
> >     int r, nlogs = 0;
> > 
> > -   while (datalen > 0) {
> > +   for (;;) {
> >        if (unlikely(headcount >= VHOST_NET_MAX_SG)) {
> >           r = -ENOBUFS;
> >           goto err;
> > @@ -260,16 +261,20 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> >           nlogs += *log_num;
> >           log += *log_num;
> >        }
> > +      len = iov_length(vq->iov + seg, in);
> > +      seg += in;
> >        heads[headcount].id = d;
> > -      heads[headcount].len = iov_length(vq->iov + seg, in);
> > -      datalen -= heads[headcount].len;
> > +      if (datalen <= len)
> > +         break;
> > +      heads[headcount].len = len;
> >        ++headcount;
> > -      seg += in;
> > +      datalen -= len;
> >     }
> > +   heads[headcount].len = datalen;
> >     *iovcount = seg;
> >     if (unlikely(log))
> >        *log_num = nlogs;
> > -   return headcount;
> > +   return headcount + 1;
> >  err:
> >     vhost_discard_desc(vq, headcount);
> >     return r;
> > 
> > -- 
> > MST
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: TCP-MD5 checksum failure on x86_64 SMP
From: Bijay Singh @ 2010-05-10 17:27 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Stephen Hemminger, David Miller, <bhaskie@gmail.com>,
	<bhutchings@solarflare.com>, <netdev@vger.kernel.org>
In-Reply-To: <1273504693.2221.17.camel@edumazet-laptop>

Hi Eric,

Didn't intend to mix  the issues. It was a hack intended to calm the nerves. I am going to apply the proper patches asap.

About the latest problem of MD5 not working with MTU set to 4470. I noticed this when i needed to change the MTU for some other  purpose. 

Since it was a production box, i have to first set up my box with the right NIC card to reproduce this and try debugging it. In the meantime any ques will  help.

Thanks,
Bijay
 
On 10-May-2010, at 8:48 PM, Eric Dumazet wrote:

> Le lundi 10 mai 2010 à 14:55 +0000, Bijay Singh a écrit :
>> Hi,
>> I had noticed the corruption in the context and actually did what is mentioned.
>> 
>> I allocated the context on the stack and plugged in the md5.c functions. I was able to temporarily solve the problem, all this before I got a response on this thread.
>> 
>> But now I have seeing another problem, when i change the MTU on the interface from 1500 to 4470 none of the message from the peer get thru and I get hash failed message. I am wondering if this is another bug getting hit in this scenario.
> 
> Thats very fine, but you mix very different problems.
> 
> Step by step resolution is required, and clean patches too, because
> plugging md5.c functions is not an option for stable series :)
> 
> Obviously, nobody seriously used TCP-MD5 on linux, but you...
> 
> 
> 


^ permalink raw reply

* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: Michael S. Tsirkin @ 2010-05-10 17:31 UTC (permalink / raw)
  To: David L Stevens; +Cc: netdev, kvm, virtualization
In-Reply-To: <1272488232.11307.4.camel@w-dls.beaverton.ibm.com>

On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
> @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
>  	use_mm(net->dev.mm);
>  	mutex_lock(&vq->mutex);
>  	vhost_disable_notify(vq);
> -	hdr_size = vq->hdr_size;
> +	vhost_hlen = vq->vhost_hlen;
>  
>  	vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
>  		vq->log : NULL;
>  
> -	for (;;) {
> -		head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> -					 ARRAY_SIZE(vq->iov),
> -					 &out, &in,
> -					 vq_log, &log);
> +	while ((datalen = vhost_head_len(vq, sock->sk))) {
> +		headcount = vhost_get_desc_n(vq, vq->heads,
> +					     datalen + vhost_hlen,
> +					     &in, vq_log, &log);
> +		if (headcount < 0)
> +			break;
>  		/* OK, now we need to know about added descriptors. */
> -		if (head == vq->num) {
> +		if (!headcount) {
>  			if (unlikely(vhost_enable_notify(vq))) {
>  				/* They have slipped one in as we were
>  				 * doing that: check again. */


So I think this breaks handling for a failure mode where
we get an skb that is larger than the max packet guest
can get. The right thing to do in this case is to
drop the skb, we currently do this by passing
truncate flag to recvmsg.

In particular, with mergeable buffers off, if we get an skb
that does not fit in a single packet, this code will
spread it over multiple buffers.

You should be able to reproduce this fairly easily
by disabling both indirect buffers and mergeable buffers
on qemu command line. With current code TCP still
works by falling back on small packets. I think
with your code it will get stuck forever once
we get an skb that is too large for us to handle.

-- 
MST



^ permalink raw reply

* pull request: wireless-2.6 2010-05-10
From: John W. Linville @ 2010-05-10 17:43 UTC (permalink / raw)
  To: davem; +Cc: linux-wireless, netdev, linux-kernel

Dave,

Here are three more candidates for 2.6.34.  I hesitated to push them,
but at least two of them are documented regressions and the other (i.e.
"iwlwifi: work around passive scan issue") avoids some rather annoying
firmware restarts at little or no risk.  I think it would be good to
take these now rather than later.

Please let me know if there are problems!

Thanks,

John

---

The following changes since commit 80ea76bb2575c426154b8d61d324197ee3592baa:
  David S. Miller (1):
        phy: Fix initialization in micrel driver.

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git master

Christian Lamparter (1):
      ar9170: wait for asynchronous firmware loading

Johannes Berg (1):
      iwlwifi: work around passive scan issue

Reinette Chatre (1):
      mac80211: remove association work when processing deauth request

 drivers/net/wireless/ath/ar9170/usb.c       |   11 +++++++++++
 drivers/net/wireless/ath/ar9170/usb.h       |    1 +
 drivers/net/wireless/iwlwifi/iwl-commands.h |    4 +++-
 drivers/net/wireless/iwlwifi/iwl-scan.c     |   23 ++++++++++++++++++-----
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    3 ++-
 net/mac80211/mlme.c                         |    3 ++-
 6 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ar9170/usb.c b/drivers/net/wireless/ath/ar9170/usb.c
index 6b1cb70..24dc555 100644
--- a/drivers/net/wireless/ath/ar9170/usb.c
+++ b/drivers/net/wireless/ath/ar9170/usb.c
@@ -726,12 +726,16 @@ static void ar9170_usb_firmware_failed(struct ar9170_usb *aru)
 {
 	struct device *parent = aru->udev->dev.parent;
 
+	complete(&aru->firmware_loading_complete);
+
 	/* unbind anything failed */
 	if (parent)
 		down(&parent->sem);
 	device_release_driver(&aru->udev->dev);
 	if (parent)
 		up(&parent->sem);
+
+	usb_put_dev(aru->udev);
 }
 
 static void ar9170_usb_firmware_finish(const struct firmware *fw, void *context)
@@ -760,6 +764,8 @@ static void ar9170_usb_firmware_finish(const struct firmware *fw, void *context)
 	if (err)
 		goto err_unrx;
 
+	complete(&aru->firmware_loading_complete);
+	usb_put_dev(aru->udev);
 	return;
 
  err_unrx:
@@ -857,6 +863,7 @@ static int ar9170_usb_probe(struct usb_interface *intf,
 	init_usb_anchor(&aru->tx_pending);
 	init_usb_anchor(&aru->tx_submitted);
 	init_completion(&aru->cmd_wait);
+	init_completion(&aru->firmware_loading_complete);
 	spin_lock_init(&aru->tx_urb_lock);
 
 	aru->tx_pending_urbs = 0;
@@ -876,6 +883,7 @@ static int ar9170_usb_probe(struct usb_interface *intf,
 	if (err)
 		goto err_freehw;
 
+	usb_get_dev(aru->udev);
 	return request_firmware_nowait(THIS_MODULE, 1, "ar9170.fw",
 				       &aru->udev->dev, GFP_KERNEL, aru,
 				       ar9170_usb_firmware_step2);
@@ -895,6 +903,9 @@ static void ar9170_usb_disconnect(struct usb_interface *intf)
 		return;
 
 	aru->common.state = AR9170_IDLE;
+
+	wait_for_completion(&aru->firmware_loading_complete);
+
 	ar9170_unregister(&aru->common);
 	ar9170_usb_cancel_urbs(aru);
 
diff --git a/drivers/net/wireless/ath/ar9170/usb.h b/drivers/net/wireless/ath/ar9170/usb.h
index a2ce3b1..919b060 100644
--- a/drivers/net/wireless/ath/ar9170/usb.h
+++ b/drivers/net/wireless/ath/ar9170/usb.h
@@ -71,6 +71,7 @@ struct ar9170_usb {
 	unsigned int tx_pending_urbs;
 
 	struct completion cmd_wait;
+	struct completion firmware_loading_complete;
 	int readlen;
 	u8 *readbuf;
 
diff --git a/drivers/net/wireless/iwlwifi/iwl-commands.h b/drivers/net/wireless/iwlwifi/iwl-commands.h
index 6383d9f..f4e59ae 100644
--- a/drivers/net/wireless/iwlwifi/iwl-commands.h
+++ b/drivers/net/wireless/iwlwifi/iwl-commands.h
@@ -2621,7 +2621,9 @@ struct iwl_ssid_ie {
 #define PROBE_OPTION_MAX_3945		4
 #define PROBE_OPTION_MAX		20
 #define TX_CMD_LIFE_TIME_INFINITE	cpu_to_le32(0xFFFFFFFF)
-#define IWL_GOOD_CRC_TH			cpu_to_le16(1)
+#define IWL_GOOD_CRC_TH_DISABLED	0
+#define IWL_GOOD_CRC_TH_DEFAULT		cpu_to_le16(1)
+#define IWL_GOOD_CRC_TH_NEVER		cpu_to_le16(0xffff)
 #define IWL_MAX_SCAN_SIZE 1024
 #define IWL_MAX_CMD_SIZE 4096
 #define IWL_MAX_PROBE_REQUEST		200
diff --git a/drivers/net/wireless/iwlwifi/iwl-scan.c b/drivers/net/wireless/iwlwifi/iwl-scan.c
index 5062f4e..2367286 100644
--- a/drivers/net/wireless/iwlwifi/iwl-scan.c
+++ b/drivers/net/wireless/iwlwifi/iwl-scan.c
@@ -812,16 +812,29 @@ static void iwl_bg_request_scan(struct work_struct *data)
 			rate = IWL_RATE_1M_PLCP;
 			rate_flags = RATE_MCS_CCK_MSK;
 		}
-		scan->good_CRC_th = 0;
+		scan->good_CRC_th = IWL_GOOD_CRC_TH_DISABLED;
 	} else if (priv->scan_bands & BIT(IEEE80211_BAND_5GHZ)) {
 		band = IEEE80211_BAND_5GHZ;
 		rate = IWL_RATE_6M_PLCP;
 		/*
-		 * If active scaning is requested but a certain channel
-		 * is marked passive, we can do active scanning if we
-		 * detect transmissions.
+		 * If active scanning is requested but a certain channel is
+		 * marked passive, we can do active scanning if we detect
+		 * transmissions.
+		 *
+		 * There is an issue with some firmware versions that triggers
+		 * a sysassert on a "good CRC threshold" of zero (== disabled),
+		 * on a radar channel even though this means that we should NOT
+		 * send probes.
+		 *
+		 * The "good CRC threshold" is the number of frames that we
+		 * need to receive during our dwell time on a channel before
+		 * sending out probes -- setting this to a huge value will
+		 * mean we never reach it, but at the same time work around
+		 * the aforementioned issue. Thus use IWL_GOOD_CRC_TH_NEVER
+		 * here instead of IWL_GOOD_CRC_TH_DISABLED.
 		 */
-		scan->good_CRC_th = is_active ? IWL_GOOD_CRC_TH : 0;
+		scan->good_CRC_th = is_active ? IWL_GOOD_CRC_TH_DEFAULT :
+						IWL_GOOD_CRC_TH_NEVER;
 
 		/* Force use of chains B and C (0x6) for scan Rx for 4965
 		 * Avoid A (0x1) because of its off-channel reception on A-band.
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index e276f2a..2f47d93 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -2966,7 +2966,8 @@ static void iwl3945_bg_request_scan(struct work_struct *data)
 		 * is marked passive, we can do active scanning if we
 		 * detect transmissions.
 		 */
-		scan->good_CRC_th = is_active ? IWL_GOOD_CRC_TH : 0;
+		scan->good_CRC_th = is_active ? IWL_GOOD_CRC_TH_DEFAULT :
+						IWL_GOOD_CRC_TH_DISABLED;
 		band = IEEE80211_BAND_5GHZ;
 	} else {
 		IWL_WARN(priv, "Invalid scan band count\n");
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 8a96503..6ccd48e 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -2029,7 +2029,8 @@ int ieee80211_mgd_deauth(struct ieee80211_sub_if_data *sdata,
 				continue;
 
 			if (wk->type != IEEE80211_WORK_DIRECT_PROBE &&
-			    wk->type != IEEE80211_WORK_AUTH)
+			    wk->type != IEEE80211_WORK_AUTH &&
+			    wk->type != IEEE80211_WORK_ASSOC)
 				continue;
 
 			if (memcmp(req->bss->bssid, wk->filter_ta, ETH_ALEN))
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

^ permalink raw reply related

* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: David Stevens @ 2010-05-10 17:46 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: kvm, kvm-owner, netdev, netdev-owner, virtualization
In-Reply-To: <20100510172557.GD28798@redhat.com>

netdev-owner@vger.kernel.org wrote on 05/10/2010 10:25:57 AM:

> On Mon, May 10, 2010 at 10:09:03AM -0700, David Stevens wrote:
> > Since "datalen" carries the difference and will be negative by that 
amount
> > from the original loop, what about just adding something like:
> > 
> >         }
> >         if (headcount)
> >                 heads[headcount-1].len += datalen;
> > [and really, headcount >0 since datalen > 0, so just:
> > 
> >         heads[headcount-1].len += datalen;
> > 
> >                                                 +-DLS
> 
> This works too, just does more checks and comparisons.
> I am still surprised that you were unable to reproduce the problem.
> 

I'm sure it happened, and probably had a performance
penalty on my systems too, but not as much as yours.
I didn't see any obvious performance difference running
with the patch, though; not sure why. I'll instrument to
see how often it's happening, I think.
        But fixed now, good catch!

                                                        +-DLS


^ permalink raw reply


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