* [PATCH] [resend] 3c509: convert to isa_driver and pnp_driver v5
From: Ondrej Zary @ 2008-02-13 21:28 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Stephen Hemminger, linux-kernel, netdev
In-Reply-To: <20080210051855.GA22064@infradead.org>
Hello,
this patch converts 3c509 driver to isa_driver and pnp_driver. The result is
that autoloading using udev and hibernation works with ISA PnP cards. It also
adds hibernation support for non-PnP ISA cards.
xcvr module parameter was removed as its value was not used.
Tested using 3 ISA cards in various combinations of PnP and non-PnP modes.
EISA and MCA only compile-tested.
Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
--- linux-2.6.24-orig/drivers/net/3c509.c 2008-01-27 19:48:19.000000000 +0100
+++ linux-2.6.24-pentium/drivers/net/3c509.c 2008-02-10 21:52:04.000000000 +0100
@@ -54,25 +54,24 @@
v1.19a 28Oct2002 Davud Ruggiero <jdr@farfalle.com>
- Increase *read_eeprom udelay to workaround oops with 2 cards.
v1.19b 08Nov2002 Marc Zyngier <maz@wild-wind.fr.eu.org>
- - Introduce driver model for EISA cards.
+ - Introduce driver model for EISA cards.
+ v1.20 04Feb2008 Ondrej Zary <linux@rainbow-software.org>
+ - convert to isa_driver and pnp_driver and some cleanups
*/
#define DRV_NAME "3c509"
-#define DRV_VERSION "1.19b"
-#define DRV_RELDATE "08Nov2002"
+#define DRV_VERSION "1.20"
+#define DRV_RELDATE "04Feb2008"
/* A few values that may be tweaked. */
/* Time in jiffies before concluding the transmitter is hung. */
#define TX_TIMEOUT (400*HZ/1000)
-/* Maximum events (Rx packets, etc.) to handle at each interrupt. */
-static int max_interrupt_work = 10;
#include <linux/module.h>
-#ifdef CONFIG_MCA
#include <linux/mca.h>
-#endif
-#include <linux/isapnp.h>
+#include <linux/isa.h>
+#include <linux/pnp.h>
#include <linux/string.h>
#include <linux/interrupt.h>
#include <linux/errno.h>
@@ -97,10 +96,6 @@
static char version[] __initdata = DRV_NAME ".c:" DRV_VERSION " " DRV_RELDATE " becker@scyld.com\n";
-#if defined(CONFIG_PM) && (defined(CONFIG_MCA) || defined(CONFIG_EISA))
-#define EL3_SUSPEND
-#endif
-
#ifdef EL3_DEBUG
static int el3_debug = EL3_DEBUG;
#else
@@ -111,6 +106,7 @@
* a global variable so that the mca/eisa probe routines can increment
* it */
static int el3_cards = 0;
+#define EL3_MAX_CARDS 8
/* To minimize the size of the driver source I only define operating
constants if they are used several times. You'll need the manual
@@ -119,7 +115,7 @@
#define EL3_DATA 0x00
#define EL3_CMD 0x0e
#define EL3_STATUS 0x0e
-#define EEPROM_READ 0x80
+#define EEPROM_READ 0x80
#define EL3_IO_EXTENT 16
@@ -168,23 +164,31 @@
*/
#define SKB_QUEUE_SIZE 64
+enum el3_cardtype { EL3_ISA, EL3_PNP, EL3_MCA, EL3_EISA };
+
struct el3_private {
struct net_device_stats stats;
- struct net_device *next_dev;
spinlock_t lock;
/* skb send-queue */
int head, size;
struct sk_buff *queue[SKB_QUEUE_SIZE];
- enum {
- EL3_MCA,
- EL3_PNP,
- EL3_EISA,
- } type; /* type of device */
- struct device *dev;
+ enum el3_cardtype type;
};
-static int id_port __initdata = 0x110; /* Start with 0x110 to avoid new sound cards.*/
-static struct net_device *el3_root_dev;
+static int id_port;
+static int current_tag;
+static struct net_device *el3_devs[EL3_MAX_CARDS];
+
+/* Parameters that may be passed into the module. */
+static int debug = -1;
+static int irq[] = {-1, -1, -1, -1, -1, -1, -1, -1};
+/* Maximum events (Rx packets, etc.) to handle at each interrupt. */
+static int max_interrupt_work = 10;
+#ifdef CONFIG_PNP
+static int nopnp;
+#endif
+static int __init el3_common_init(struct net_device *dev);
+static void el3_common_remove(struct net_device *dev);
static ushort id_read_eeprom(int index);
static ushort read_eeprom(int ioaddr, int index);
static int el3_open(struct net_device *dev);
@@ -199,23 +203,279 @@
static void el3_down(struct net_device *dev);
static void el3_up(struct net_device *dev);
static const struct ethtool_ops ethtool_ops;
-#ifdef EL3_SUSPEND
+#ifdef CONFIG_PM
static int el3_suspend(struct device *, pm_message_t);
static int el3_resume(struct device *);
-#else
-#define el3_suspend NULL
-#define el3_resume NULL
#endif
/* generic device remove for all device types */
-#if defined(CONFIG_EISA) || defined(CONFIG_MCA)
static int el3_device_remove (struct device *device);
-#endif
#ifdef CONFIG_NET_POLL_CONTROLLER
static void el3_poll_controller(struct net_device *dev);
#endif
+/* Return 0 on success, 1 on error, 2 when found already detected PnP card */
+static int el3_isa_id_sequence(__be16 *phys_addr)
+{
+ short lrs_state = 0xff;
+ int i;
+
+ /* ISA boards are detected by sending the ID sequence to the
+ ID_PORT. We find cards past the first by setting the 'current_tag'
+ on cards as they are found. Cards with their tag set will not
+ respond to subsequent ID sequences. */
+
+ outb(0x00, id_port);
+ outb(0x00, id_port);
+ for (i = 0; i < 255; i++) {
+ outb(lrs_state, id_port);
+ lrs_state <<= 1;
+ lrs_state = lrs_state & 0x100 ? lrs_state ^ 0xcf : lrs_state;
+ }
+ /* For the first probe, clear all board's tag registers. */
+ if (current_tag == 0)
+ outb(0xd0, id_port);
+ else /* Otherwise kill off already-found boards. */
+ outb(0xd8, id_port);
+ if (id_read_eeprom(7) != 0x6d50)
+ return 1;
+ /* Read in EEPROM data, which does contention-select.
+ Only the lowest address board will stay "on-line".
+ 3Com got the byte order backwards. */
+ for (i = 0; i < 3; i++)
+ phys_addr[i] = htons(id_read_eeprom(i));
+#ifdef CONFIG_PNP
+ if (!nopnp) {
+ /* The ISA PnP 3c509 cards respond to the ID sequence too.
+ This check is needed in order not to register them twice. */
+ for (i = 0; i < el3_cards; i++) {
+ struct el3_private *lp = netdev_priv(el3_devs[i]);
+ if (lp->type == EL3_PNP
+ && !memcmp(phys_addr, el3_devs[i]->dev_addr,
+ ETH_ALEN)) {
+ if (el3_debug > 3)
+ printk(KERN_DEBUG "3c509 with address %02x %02x %02x %02x %02x %02x was found by ISAPnP\n",
+ phys_addr[0] & 0xff, phys_addr[0] >> 8,
+ phys_addr[1] & 0xff, phys_addr[1] >> 8,
+ phys_addr[2] & 0xff, phys_addr[2] >> 8);
+ /* Set the adaptor tag so that the next card can be found. */
+ outb(0xd0 + ++current_tag, id_port);
+ return 2;
+ }
+ }
+ }
+#endif /* CONFIG_PNP */
+ return 0;
+
+}
+
+static void __devinit el3_dev_fill(struct net_device *dev, __be16 *phys_addr,
+ int ioaddr, int irq, int if_port,
+ enum el3_cardtype type)
+{
+ struct el3_private *lp = netdev_priv(dev);
+
+ memcpy(dev->dev_addr, phys_addr, ETH_ALEN);
+ dev->base_addr = ioaddr;
+ dev->irq = irq;
+ dev->if_port = if_port;
+ lp->type = type;
+}
+
+static int __devinit el3_isa_match(struct device *pdev,
+ unsigned int ndev)
+{
+ struct net_device *dev;
+ int ioaddr, isa_irq, if_port, err;
+ unsigned int iobase;
+ __be16 phys_addr[3];
+
+ while ((err = el3_isa_id_sequence(phys_addr)) == 2)
+ ; /* Skip to next card when PnP card found */
+ if (err == 1)
+ return 0;
+
+ iobase = id_read_eeprom(8);
+ if_port = iobase >> 14;
+ ioaddr = 0x200 + ((iobase & 0x1f) << 4);
+ if (irq[el3_cards] > 1 && irq[el3_cards] < 16)
+ isa_irq = irq[el3_cards];
+ else
+ isa_irq = id_read_eeprom(9) >> 12;
+
+ dev = alloc_etherdev(sizeof(struct el3_private));
+ if (!dev)
+ return -ENOMEM;
+
+ netdev_boot_setup_check(dev);
+
+ if (!request_region(ioaddr, EL3_IO_EXTENT, "3c509-isa")) {
+ free_netdev(dev);
+ return 0;
+ }
+
+ /* Set the adaptor tag so that the next card can be found. */
+ outb(0xd0 + ++current_tag, id_port);
+
+ /* Activate the adaptor at the EEPROM location. */
+ outb((ioaddr >> 4) | 0xe0, id_port);
+
+ EL3WINDOW(0);
+ if (inw(ioaddr) != 0x6d50) {
+ free_netdev(dev);
+ return 0;
+ }
+
+ /* Free the interrupt so that some other card can use it. */
+ outw(0x0f00, ioaddr + WN0_IRQ);
+
+ el3_dev_fill(dev, phys_addr, ioaddr, isa_irq, if_port, EL3_ISA);
+ dev_set_drvdata(pdev, dev);
+ if (el3_common_init(dev)) {
+ free_netdev(dev);
+ return 0;
+ }
+
+ el3_devs[el3_cards++] = dev;
+ return 1;
+}
+
+static int __devexit el3_isa_remove(struct device *pdev,
+ unsigned int ndev)
+{
+ el3_device_remove(pdev);
+ dev_set_drvdata(pdev, NULL);
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int el3_isa_suspend(struct device *dev, unsigned int n,
+ pm_message_t state)
+{
+ current_tag = 0;
+ return el3_suspend(dev, state);
+}
+
+static int el3_isa_resume(struct device *dev, unsigned int n)
+{
+ struct net_device *ndev = dev_get_drvdata(dev);
+ int ioaddr = ndev->base_addr, err;
+ __be16 phys_addr[3];
+
+ while ((err = el3_isa_id_sequence(phys_addr)) == 2)
+ ; /* Skip to next card when PnP card found */
+ if (err == 1)
+ return 0;
+ /* Set the adaptor tag so that the next card can be found. */
+ outb(0xd0 + ++current_tag, id_port);
+ /* Enable the card */
+ outb((ioaddr >> 4) | 0xe0, id_port);
+ EL3WINDOW(0);
+ if (inw(ioaddr) != 0x6d50)
+ return 1;
+ /* Free the interrupt so that some other card can use it. */
+ outw(0x0f00, ioaddr + WN0_IRQ);
+ return el3_resume(dev);
+}
+#endif
+
+static struct isa_driver el3_isa_driver = {
+ .match = el3_isa_match,
+ .remove = __devexit_p(el3_isa_remove),
+#ifdef CONFIG_PM
+ .suspend = el3_isa_suspend,
+ .resume = el3_isa_resume,
+#endif
+ .driver = {
+ .name = "3c509"
+ },
+};
+static int isa_registered;
+
+#ifdef CONFIG_PNP
+static struct pnp_device_id el3_pnp_ids[] = {
+ { .id = "TCM5090" }, /* 3Com Etherlink III (TP) */
+ { .id = "TCM5091" }, /* 3Com Etherlink III */
+ { .id = "TCM5094" }, /* 3Com Etherlink III (combo) */
+ { .id = "TCM5095" }, /* 3Com Etherlink III (TPO) */
+ { .id = "TCM5098" }, /* 3Com Etherlink III (TPC) */
+ { .id = "PNP80f7" }, /* 3Com Etherlink III compatible */
+ { .id = "PNP80f8" }, /* 3Com Etherlink III compatible */
+ { .id = "" }
+};
+MODULE_DEVICE_TABLE(pnp, el3_pnp_ids);
+
+static int __devinit el3_pnp_probe(struct pnp_dev *pdev,
+ const struct pnp_device_id *id)
+{
+ short i;
+ int ioaddr, irq, if_port;
+ u16 phys_addr[3];
+ struct net_device *dev = NULL;
+ int err;
+
+ ioaddr = pnp_port_start(pdev, 0);
+ if (!request_region(ioaddr, EL3_IO_EXTENT, "3c509-pnp"))
+ return -EBUSY;
+ irq = pnp_irq(pdev, 0);
+ EL3WINDOW(0);
+ for (i = 0; i < 3; i++)
+ phys_addr[i] = htons(read_eeprom(ioaddr, i));
+ if_port = read_eeprom(ioaddr, 8) >> 14;
+ dev = alloc_etherdev(sizeof(struct el3_private));
+ if (!dev) {
+ release_region(ioaddr, EL3_IO_EXTENT);
+ return -ENOMEM;
+ }
+ SET_NETDEV_DEV(dev, &pdev->dev);
+ netdev_boot_setup_check(dev);
+
+ el3_dev_fill(dev, phys_addr, ioaddr, irq, if_port, EL3_PNP);
+ pnp_set_drvdata(pdev, dev);
+ err = el3_common_init(dev);
+
+ if (err) {
+ pnp_set_drvdata(pdev, NULL);
+ free_netdev(dev);
+ return err;
+ }
+
+ el3_devs[el3_cards++] = dev;
+ return 0;
+}
+
+static void __devexit el3_pnp_remove(struct pnp_dev *pdev)
+{
+ el3_common_remove(pnp_get_drvdata(pdev));
+ pnp_set_drvdata(pdev, NULL);
+}
+
+#ifdef CONFIG_PM
+static int el3_pnp_suspend(struct pnp_dev *pdev, pm_message_t state)
+{
+ return el3_suspend(&pdev->dev, state);
+}
+
+static int el3_pnp_resume(struct pnp_dev *pdev)
+{
+ return el3_resume(&pdev->dev);
+}
+#endif
+
+static struct pnp_driver el3_pnp_driver = {
+ .name = "3c509",
+ .id_table = el3_pnp_ids,
+ .probe = el3_pnp_probe,
+ .remove = __devexit_p(el3_pnp_remove),
+#ifdef CONFIG_PM
+ .suspend = el3_pnp_suspend,
+ .resume = el3_pnp_resume,
+#endif
+};
+static int pnp_registered;
+#endif /* CONFIG_PNP */
+
#ifdef CONFIG_EISA
static struct eisa_device_id el3_eisa_ids[] = {
{ "TCM5092" },
@@ -230,13 +490,14 @@
static struct eisa_driver el3_eisa_driver = {
.id_table = el3_eisa_ids,
.driver = {
- .name = "3c509",
+ .name = "3c579",
.probe = el3_eisa_probe,
.remove = __devexit_p (el3_device_remove),
.suspend = el3_suspend,
.resume = el3_resume,
}
};
+static int eisa_registered;
#endif
#ifdef CONFIG_MCA
@@ -271,45 +532,9 @@
.resume = el3_resume,
},
};
+static int mca_registered;
#endif /* CONFIG_MCA */
-#if defined(__ISAPNP__)
-static struct isapnp_device_id el3_isapnp_adapters[] __initdata = {
- { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
- ISAPNP_VENDOR('T', 'C', 'M'), ISAPNP_FUNCTION(0x5090),
- (long) "3Com Etherlink III (TP)" },
- { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
- ISAPNP_VENDOR('T', 'C', 'M'), ISAPNP_FUNCTION(0x5091),
- (long) "3Com Etherlink III" },
- { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
- ISAPNP_VENDOR('T', 'C', 'M'), ISAPNP_FUNCTION(0x5094),
- (long) "3Com Etherlink III (combo)" },
- { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
- ISAPNP_VENDOR('T', 'C', 'M'), ISAPNP_FUNCTION(0x5095),
- (long) "3Com Etherlink III (TPO)" },
- { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
- ISAPNP_VENDOR('T', 'C', 'M'), ISAPNP_FUNCTION(0x5098),
- (long) "3Com Etherlink III (TPC)" },
- { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
- ISAPNP_VENDOR('P', 'N', 'P'), ISAPNP_FUNCTION(0x80f7),
- (long) "3Com Etherlink III compatible" },
- { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
- ISAPNP_VENDOR('P', 'N', 'P'), ISAPNP_FUNCTION(0x80f8),
- (long) "3Com Etherlink III compatible" },
- { } /* terminate list */
-};
-
-static __be16 el3_isapnp_phys_addr[8][3];
-static int nopnp;
-#endif /* __ISAPNP__ */
-
-/* With the driver model introduction for EISA devices, both init
- * and cleanup have been split :
- * - EISA devices probe/remove starts in el3_eisa_probe/el3_device_remove
- * - MCA/ISA still use el3_probe
- *
- * Both call el3_common_init/el3_common_remove. */
-
static int __init el3_common_init(struct net_device *dev)
{
struct el3_private *lp = netdev_priv(dev);
@@ -360,231 +585,11 @@
static void el3_common_remove (struct net_device *dev)
{
- struct el3_private *lp = netdev_priv(dev);
-
- (void) lp; /* Keep gcc quiet... */
-#if defined(__ISAPNP__)
- if (lp->type == EL3_PNP)
- pnp_device_detach(to_pnp_dev(lp->dev));
-#endif
-
unregister_netdev (dev);
release_region(dev->base_addr, EL3_IO_EXTENT);
free_netdev (dev);
}
-static int __init el3_probe(int card_idx)
-{
- struct net_device *dev;
- struct el3_private *lp;
- short lrs_state = 0xff, i;
- int ioaddr, irq, if_port;
- __be16 phys_addr[3];
- static int current_tag;
- int err = -ENODEV;
-#if defined(__ISAPNP__)
- static int pnp_cards;
- struct pnp_dev *idev = NULL;
- int pnp_found = 0;
-
- if (nopnp == 1)
- goto no_pnp;
-
- for (i=0; el3_isapnp_adapters[i].vendor != 0; i++) {
- int j;
- while ((idev = pnp_find_dev(NULL,
- el3_isapnp_adapters[i].vendor,
- el3_isapnp_adapters[i].function,
- idev))) {
- if (pnp_device_attach(idev) < 0)
- continue;
- if (pnp_activate_dev(idev) < 0) {
-__again:
- pnp_device_detach(idev);
- continue;
- }
- if (!pnp_port_valid(idev, 0) || !pnp_irq_valid(idev, 0))
- goto __again;
- ioaddr = pnp_port_start(idev, 0);
- if (!request_region(ioaddr, EL3_IO_EXTENT, "3c509 PnP")) {
- pnp_device_detach(idev);
- return -EBUSY;
- }
- irq = pnp_irq(idev, 0);
- if (el3_debug > 3)
- printk ("ISAPnP reports %s at i/o 0x%x, irq %d\n",
- (char*) el3_isapnp_adapters[i].driver_data, ioaddr, irq);
- EL3WINDOW(0);
- for (j = 0; j < 3; j++)
- el3_isapnp_phys_addr[pnp_cards][j] =
- phys_addr[j] =
- htons(read_eeprom(ioaddr, j));
- if_port = read_eeprom(ioaddr, 8) >> 14;
- dev = alloc_etherdev(sizeof (struct el3_private));
- if (!dev) {
- release_region(ioaddr, EL3_IO_EXTENT);
- pnp_device_detach(idev);
- return -ENOMEM;
- }
-
- SET_NETDEV_DEV(dev, &idev->dev);
- pnp_cards++;
-
- netdev_boot_setup_check(dev);
- pnp_found = 1;
- goto found;
- }
- }
-no_pnp:
-#endif /* __ISAPNP__ */
-
- /* Select an open I/O location at 0x1*0 to do contention select. */
- for ( ; id_port < 0x200; id_port += 0x10) {
- if (!request_region(id_port, 1, "3c509"))
- continue;
- outb(0x00, id_port);
- outb(0xff, id_port);
- if (inb(id_port) & 0x01){
- release_region(id_port, 1);
- break;
- } else
- release_region(id_port, 1);
- }
- if (id_port >= 0x200) {
- /* Rare -- do we really need a warning? */
- printk(" WARNING: No I/O port available for 3c509 activation.\n");
- return -ENODEV;
- }
-
- /* Next check for all ISA bus boards by sending the ID sequence to the
- ID_PORT. We find cards past the first by setting the 'current_tag'
- on cards as they are found. Cards with their tag set will not
- respond to subsequent ID sequences. */
-
- outb(0x00, id_port);
- outb(0x00, id_port);
- for(i = 0; i < 255; i++) {
- outb(lrs_state, id_port);
- lrs_state <<= 1;
- lrs_state = lrs_state & 0x100 ? lrs_state ^ 0xcf : lrs_state;
- }
-
- /* For the first probe, clear all board's tag registers. */
- if (current_tag == 0)
- outb(0xd0, id_port);
- else /* Otherwise kill off already-found boards. */
- outb(0xd8, id_port);
-
- if (id_read_eeprom(7) != 0x6d50) {
- return -ENODEV;
- }
-
- /* Read in EEPROM data, which does contention-select.
- Only the lowest address board will stay "on-line".
- 3Com got the byte order backwards. */
- for (i = 0; i < 3; i++) {
- phys_addr[i] = htons(id_read_eeprom(i));
- }
-
-#if defined(__ISAPNP__)
- if (nopnp == 0) {
- /* The ISA PnP 3c509 cards respond to the ID sequence.
- This check is needed in order not to register them twice. */
- for (i = 0; i < pnp_cards; i++) {
- if (phys_addr[0] == el3_isapnp_phys_addr[i][0] &&
- phys_addr[1] == el3_isapnp_phys_addr[i][1] &&
- phys_addr[2] == el3_isapnp_phys_addr[i][2])
- {
- if (el3_debug > 3)
- printk("3c509 with address %02x %02x %02x %02x %02x %02x was found by ISAPnP\n",
- phys_addr[0] & 0xff, phys_addr[0] >> 8,
- phys_addr[1] & 0xff, phys_addr[1] >> 8,
- phys_addr[2] & 0xff, phys_addr[2] >> 8);
- /* Set the adaptor tag so that the next card can be found. */
- outb(0xd0 + ++current_tag, id_port);
- goto no_pnp;
- }
- }
- }
-#endif /* __ISAPNP__ */
-
- {
- unsigned int iobase = id_read_eeprom(8);
- if_port = iobase >> 14;
- ioaddr = 0x200 + ((iobase & 0x1f) << 4);
- }
- irq = id_read_eeprom(9) >> 12;
-
- dev = alloc_etherdev(sizeof (struct el3_private));
- if (!dev)
- return -ENOMEM;
-
- netdev_boot_setup_check(dev);
-
- /* Set passed-in IRQ or I/O Addr. */
- if (dev->irq > 1 && dev->irq < 16)
- irq = dev->irq;
-
- if (dev->base_addr) {
- if (dev->mem_end == 0x3c509 /* Magic key */
- && dev->base_addr >= 0x200 && dev->base_addr <= 0x3e0)
- ioaddr = dev->base_addr & 0x3f0;
- else if (dev->base_addr != ioaddr)
- goto out;
- }
-
- if (!request_region(ioaddr, EL3_IO_EXTENT, "3c509")) {
- err = -EBUSY;
- goto out;
- }
-
- /* Set the adaptor tag so that the next card can be found. */
- outb(0xd0 + ++current_tag, id_port);
-
- /* Activate the adaptor at the EEPROM location. */
- outb((ioaddr >> 4) | 0xe0, id_port);
-
- EL3WINDOW(0);
- if (inw(ioaddr) != 0x6d50)
- goto out1;
-
- /* Free the interrupt so that some other card can use it. */
- outw(0x0f00, ioaddr + WN0_IRQ);
-
-#if defined(__ISAPNP__)
- found: /* PNP jumps here... */
-#endif /* __ISAPNP__ */
-
- memcpy(dev->dev_addr, phys_addr, sizeof(phys_addr));
- dev->base_addr = ioaddr;
- dev->irq = irq;
- dev->if_port = if_port;
- lp = netdev_priv(dev);
-#if defined(__ISAPNP__)
- lp->dev = &idev->dev;
- if (pnp_found)
- lp->type = EL3_PNP;
-#endif
- err = el3_common_init(dev);
-
- if (err)
- goto out1;
-
- el3_cards++;
- lp->next_dev = el3_root_dev;
- el3_root_dev = dev;
- return 0;
-
-out1:
-#if defined(__ISAPNP__)
- if (idev)
- pnp_device_detach(idev);
-#endif
-out:
- free_netdev(dev);
- return err;
-}
-
#ifdef CONFIG_MCA
static int __init el3_mca_probe(struct device *device)
{
@@ -596,7 +601,6 @@
* redone for multi-card detection by ZP Gu (zpg@castle.net)
* now works as a module */
- struct el3_private *lp;
short i;
int ioaddr, irq, if_port;
u16 phys_addr[3];
@@ -613,7 +617,7 @@
irq = pos5 & 0x0f;
- printk("3c529: found %s at slot %d\n",
+ printk(KERN_INFO "3c529: found %s at slot %d\n",
el3_mca_adapter_names[mdev->index], slot + 1);
/* claim the slot */
@@ -626,7 +630,7 @@
irq = mca_device_transform_irq(mdev, irq);
ioaddr = mca_device_transform_ioport(mdev, ioaddr);
if (el3_debug > 2) {
- printk("3c529: irq %d ioaddr 0x%x ifport %d\n", irq, ioaddr, if_port);
+ printk(KERN_DEBUG "3c529: irq %d ioaddr 0x%x ifport %d\n", irq, ioaddr, if_port);
}
EL3WINDOW(0);
for (i = 0; i < 3; i++) {
@@ -641,13 +645,7 @@
netdev_boot_setup_check(dev);
- memcpy(dev->dev_addr, phys_addr, sizeof(phys_addr));
- dev->base_addr = ioaddr;
- dev->irq = irq;
- dev->if_port = if_port;
- lp = netdev_priv(dev);
- lp->dev = device;
- lp->type = EL3_MCA;
+ el3_dev_fill(dev, phys_addr, ioaddr, irq, if_port, EL3_MCA);
device->driver_data = dev;
err = el3_common_init(dev);
@@ -657,7 +655,7 @@
return -ENOMEM;
}
- el3_cards++;
+ el3_devs[el3_cards++] = dev;
return 0;
}
@@ -666,7 +664,6 @@
#ifdef CONFIG_EISA
static int __init el3_eisa_probe (struct device *device)
{
- struct el3_private *lp;
short i;
int ioaddr, irq, if_port;
u16 phys_addr[3];
@@ -678,7 +675,7 @@
edev = to_eisa_device (device);
ioaddr = edev->base_addr;
- if (!request_region(ioaddr, EL3_IO_EXTENT, "3c509"))
+ if (!request_region(ioaddr, EL3_IO_EXTENT, "3c579-eisa"))
return -EBUSY;
/* Change the register set to the configuration window 0. */
@@ -700,13 +697,7 @@
netdev_boot_setup_check(dev);
- memcpy(dev->dev_addr, phys_addr, sizeof(phys_addr));
- dev->base_addr = ioaddr;
- dev->irq = irq;
- dev->if_port = if_port;
- lp = netdev_priv(dev);
- lp->dev = device;
- lp->type = EL3_EISA;
+ el3_dev_fill(dev, phys_addr, ioaddr, irq, if_port, EL3_EISA);
eisa_set_drvdata (edev, dev);
err = el3_common_init(dev);
@@ -716,12 +707,11 @@
return err;
}
- el3_cards++;
+ el3_devs[el3_cards++] = dev;
return 0;
}
#endif
-#if defined(CONFIG_EISA) || defined(CONFIG_MCA)
/* This remove works for all device types.
*
* The net dev must be stored in the driver_data field */
@@ -734,7 +724,6 @@
el3_common_remove (dev);
return 0;
}
-#endif
/* Read a word from the EEPROM using the regular EEPROM access register.
Assume that we are in register window zero.
@@ -749,7 +738,7 @@
}
/* Read a word from the EEPROM when in the ISA ID probe state. */
-static ushort __init id_read_eeprom(int index)
+static ushort id_read_eeprom(int index)
{
int bit, word = 0;
@@ -765,7 +754,7 @@
word = (word << 1) + (inb(id_port) & 0x01);
if (el3_debug > 3)
- printk(" 3c509 EEPROM word %d %#4.4x.\n", index, word);
+ printk(KERN_DEBUG " 3c509 EEPROM word %d %#4.4x.\n", index, word);
return word;
}
@@ -787,13 +776,13 @@
EL3WINDOW(0);
if (el3_debug > 3)
- printk("%s: Opening, IRQ %d status@%x %4.4x.\n", dev->name,
+ printk(KERN_DEBUG "%s: Opening, IRQ %d status@%x %4.4x.\n", dev->name,
dev->irq, ioaddr + EL3_STATUS, inw(ioaddr + EL3_STATUS));
el3_up(dev);
if (el3_debug > 3)
- printk("%s: Opened 3c509 IRQ %d status %4.4x.\n",
+ printk(KERN_DEBUG "%s: Opened 3c509 IRQ %d status %4.4x.\n",
dev->name, dev->irq, inw(ioaddr + EL3_STATUS));
return 0;
@@ -806,7 +795,7 @@
int ioaddr = dev->base_addr;
/* Transmitter timeout, serious problems. */
- printk("%s: transmit timed out, Tx_status %2.2x status %4.4x "
+ printk(KERN_WARNING "%s: transmit timed out, Tx_status %2.2x status %4.4x "
"Tx FIFO room %d.\n",
dev->name, inb(ioaddr + TX_STATUS), inw(ioaddr + EL3_STATUS),
inw(ioaddr + TX_FREE));
@@ -831,7 +820,7 @@
lp->stats.tx_bytes += skb->len;
if (el3_debug > 4) {
- printk("%s: el3_start_xmit(length = %u) called, status %4.4x.\n",
+ printk(KERN_DEBUG "%s: el3_start_xmit(length = %u) called, status %4.4x.\n",
dev->name, skb->len, inw(ioaddr + EL3_STATUS));
}
#if 0
@@ -840,7 +829,7 @@
ushort status = inw(ioaddr + EL3_STATUS);
if (status & 0x0001 /* IRQ line active, missed one. */
&& inw(ioaddr + EL3_STATUS) & 1) { /* Make sure. */
- printk("%s: Missed interrupt, status then %04x now %04x"
+ printk(KERN_DEBUG "%s: Missed interrupt, status then %04x now %04x"
" Tx %2.2x Rx %4.4x.\n", dev->name, status,
inw(ioaddr + EL3_STATUS), inb(ioaddr + TX_STATUS),
inw(ioaddr + RX_STATUS));
@@ -914,7 +903,7 @@
if (el3_debug > 4) {
status = inw(ioaddr + EL3_STATUS);
- printk("%s: interrupt, status %4.4x.\n", dev->name, status);
+ printk(KERN_DEBUG "%s: interrupt, status %4.4x.\n", dev->name, status);
}
while ((status = inw(ioaddr + EL3_STATUS)) &
@@ -925,7 +914,7 @@
if (status & TxAvailable) {
if (el3_debug > 5)
- printk(" TX room bit was handled.\n");
+ printk(KERN_DEBUG " TX room bit was handled.\n");
/* There's room in the FIFO for a full-sized packet. */
outw(AckIntr | TxAvailable, ioaddr + EL3_CMD);
netif_wake_queue (dev);
@@ -964,7 +953,7 @@
}
if (--i < 0) {
- printk("%s: Infinite loop in interrupt, status %4.4x.\n",
+ printk(KERN_ERR "%s: Infinite loop in interrupt, status %4.4x.\n",
dev->name, status);
/* Clear all interrupts. */
outw(AckIntr | 0xFF, ioaddr + EL3_CMD);
@@ -975,7 +964,7 @@
}
if (el3_debug > 4) {
- printk("%s: exiting interrupt, status %4.4x.\n", dev->name,
+ printk(KERN_DEBUG "%s: exiting interrupt, status %4.4x.\n", dev->name,
inw(ioaddr + EL3_STATUS));
}
spin_unlock(&lp->lock);
@@ -1450,7 +1439,7 @@
}
/* Power Management support functions */
-#ifdef EL3_SUSPEND
+#ifdef CONFIG_PM
static int
el3_suspend(struct device *pdev, pm_message_t state)
@@ -1500,79 +1489,102 @@
return 0;
}
-#endif /* EL3_SUSPEND */
-
-/* Parameters that may be passed into the module. */
-static int debug = -1;
-static int irq[] = {-1, -1, -1, -1, -1, -1, -1, -1};
-static int xcvr[] = {-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1};
+#endif /* CONFIG_PM */
module_param(debug,int, 0);
module_param_array(irq, int, NULL, 0);
-module_param_array(xcvr, int, NULL, 0);
module_param(max_interrupt_work, int, 0);
MODULE_PARM_DESC(debug, "debug level (0-6)");
MODULE_PARM_DESC(irq, "IRQ number(s) (assigned)");
-MODULE_PARM_DESC(xcvr,"transceiver(s) (0=internal, 1=external)");
MODULE_PARM_DESC(max_interrupt_work, "maximum events handled per interrupt");
-#if defined(__ISAPNP__)
+#ifdef CONFIG_PNP
module_param(nopnp, int, 0);
MODULE_PARM_DESC(nopnp, "disable ISA PnP support (0-1)");
-MODULE_DEVICE_TABLE(isapnp, el3_isapnp_adapters);
-#endif /* __ISAPNP__ */
-MODULE_DESCRIPTION("3Com Etherlink III (3c509, 3c509B) ISA/PnP ethernet driver");
+#endif /* CONFIG_PNP */
+MODULE_DESCRIPTION("3Com Etherlink III (3c509, 3c509B, 3c529, 3c579) ethernet driver");
MODULE_LICENSE("GPL");
static int __init el3_init_module(void)
{
int ret = 0;
- el3_cards = 0;
if (debug >= 0)
el3_debug = debug;
- el3_root_dev = NULL;
- while (el3_probe(el3_cards) == 0) {
- if (irq[el3_cards] > 1)
- el3_root_dev->irq = irq[el3_cards];
- if (xcvr[el3_cards] >= 0)
- el3_root_dev->if_port = xcvr[el3_cards];
- el3_cards++;
+#ifdef CONFIG_PNP
+ if (!nopnp) {
+ ret = pnp_register_driver(&el3_pnp_driver);
+ if (!ret)
+ pnp_registered = 1;
+ }
+#endif
+ /* Select an open I/O location at 0x1*0 to do ISA contention select. */
+ /* Start with 0x110 to avoid some sound cards.*/
+ for (id_port = 0x110 ; id_port < 0x200; id_port += 0x10) {
+ if (!request_region(id_port, 1, "3c509-control"))
+ continue;
+ outb(0x00, id_port);
+ outb(0xff, id_port);
+ if (inb(id_port) & 0x01)
+ break;
+ else
+ release_region(id_port, 1);
+ }
+ if (id_port >= 0x200) {
+ id_port = 0;
+ printk(KERN_ERR "No I/O port available for 3c509 activation.\n");
+ } else {
+ ret = isa_register_driver(&el3_isa_driver, EL3_MAX_CARDS);
+ if (!ret)
+ isa_registered = 1;
}
-
#ifdef CONFIG_EISA
ret = eisa_driver_register(&el3_eisa_driver);
+ if (!ret)
+ eisa_registered = 1;
#endif
#ifdef CONFIG_MCA
- {
- int err = mca_register_driver(&el3_mca_driver);
- if (ret == 0)
- ret = err;
- }
+ ret = mca_register_driver(&el3_mca_driver);
+ if (!ret)
+ mca_registered = 1;
+#endif
+
+#ifdef CONFIG_PNP
+ if (pnp_registered)
+ ret = 0;
+#endif
+ if (isa_registered)
+ ret = 0;
+#ifdef CONFIG_EISA
+ if (eisa_registered)
+ ret = 0;
+#endif
+#ifdef CONFIG_MCA
+ if (mca_registered)
+ ret = 0;
#endif
return ret;
}
static void __exit el3_cleanup_module(void)
{
- struct net_device *next_dev;
-
- while (el3_root_dev) {
- struct el3_private *lp = netdev_priv(el3_root_dev);
-
- next_dev = lp->next_dev;
- el3_common_remove (el3_root_dev);
- el3_root_dev = next_dev;
- }
-
+#ifdef CONFIG_PNP
+ if (pnp_registered)
+ pnp_unregister_driver(&el3_pnp_driver);
+#endif
+ if (isa_registered)
+ isa_unregister_driver(&el3_isa_driver);
+ if (id_port)
+ release_region(id_port, 1);
#ifdef CONFIG_EISA
- eisa_driver_unregister (&el3_eisa_driver);
+ if (eisa_registered)
+ eisa_driver_unregister(&el3_eisa_driver);
#endif
#ifdef CONFIG_MCA
- mca_unregister_driver(&el3_mca_driver);
+ if (mca_registered)
+ mca_unregister_driver(&el3_mca_driver);
#endif
}
module_init (el3_init_module);
module_exit (el3_cleanup_module);
-
--
Ondrej Zary
^ permalink raw reply
* [2.6 patch] unexport __inet_hash_connect
From: Adrian Bunk @ 2008-02-13 21:29 UTC (permalink / raw)
To: Pavel Emelyanov, David S. Miller; +Cc: linux-kernel, netdev
This patch removes the unused EXPORT_SYMBOL_GPL(__inet_hash_connect).
Signed-off-by: Adrian Bunk <bunk@kernel.org>
---
ffa30930d4ba630514fd93ded245456f05358140 diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 9cac6c0..e6a0072 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -494,7 +494,6 @@ out:
return ret;
}
}
-EXPORT_SYMBOL_GPL(__inet_hash_connect);
/*
* Bind a port for a connect operation and hash it.
^ permalink raw reply related
* [2.6 patch] unexport inet_listen_wlock
From: Adrian Bunk @ 2008-02-13 21:29 UTC (permalink / raw)
To: netdev
This patch removes the no linger used EXPORT_SYMBOL(inet_listen_wlock).
Signed-off-by: Adrian Bunk <bunk@kernel.org>
---
08c580a0a3219ce72f5c534c8868e75c4c224e01 diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index e6a0072..1aba606 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -120,8 +120,6 @@ void inet_listen_wlock(struct inet_hashinfo *hashinfo)
}
}
-EXPORT_SYMBOL(inet_listen_wlock);
-
/*
* Don't inline this cruft. Here are some nice properties to exploit here. The
* BSD API does not allow a listening sock to specify the remote port nor the
^ permalink raw reply related
* [2.6 patch] make s2io.c:init_tti() static
From: Adrian Bunk @ 2008-02-13 21:30 UTC (permalink / raw)
To: Surjit Reang, Sreenivasa Honnur, Ramkrishna Vepa,
santosh.rastapur, sivakumar.subrama
Cc: netdev, linux-kernel
This patch makes the needlessly global init_tti() static.
Signed-off-by: Adrian Bunk <bunk@kernel.org>
---
This patch has been sent on:
- 30 Jan 2008
1e8c2b07be955b181814e4c24366517a66557908
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 5fab7d7..bc94e84 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -1088,7 +1088,7 @@ static int s2io_print_pci_mode(struct s2io_nic *nic)
* '-1' on failure
*/
-int init_tti(struct s2io_nic *nic, int link)
+static int init_tti(struct s2io_nic *nic, int link)
{
struct XENA_dev_config __iomem *bar0 = nic->bar0;
register u64 val64 = 0;
^ permalink raw reply related
* RE: [2.6 patch] make s2io.c:init_tti() static
From: Ramkrishna Vepa @ 2008-02-13 21:45 UTC (permalink / raw)
To: Jeff Garzik
Cc: netdev, linux-kernel, Adrian Bunk, Surjit Reang,
Sreenivasa Honnur, Ramkrishna Vepa, Rastapur Santosh,
Sivakumar Subramani
In-Reply-To: <20080213213050.GN3383@cs181133002.pp.htv.fi>
Please apply. Looks good.
Ram
> -----Original Message-----
> From: Adrian Bunk [mailto:bunk@kernel.org]
> Sent: Wednesday, February 13, 2008 1:31 PM
> To: Surjit Reang; Sreenivasa Honnur; Ramkrishna Vepa; Rastapur
Santosh;
> Sivakumar Subramani
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [2.6 patch] make s2io.c:init_tti() static
>
> This patch makes the needlessly global init_tti() static.
>
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
>
> ---
>
> This patch has been sent on:
> - 30 Jan 2008
>
> 1e8c2b07be955b181814e4c24366517a66557908
> diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
> index 5fab7d7..bc94e84 100644
> --- a/drivers/net/s2io.c
> +++ b/drivers/net/s2io.c
> @@ -1088,7 +1088,7 @@ static int s2io_print_pci_mode(struct s2io_nic
*nic)
> * '-1' on failure
> */
>
> -int init_tti(struct s2io_nic *nic, int link)
> +static int init_tti(struct s2io_nic *nic, int link)
> {
> struct XENA_dev_config __iomem *bar0 = nic->bar0;
> register u64 val64 = 0;
^ permalink raw reply
* [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-13 22:00 UTC (permalink / raw)
To: linux-kernel; +Cc: shemminger, davem, netdev, dipankar, ego, herbert, akpm
Hello!
This is an updated version of the patch posted last November:
http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
This new version permits arguments with side effects, for example:
rcu_assign_pointer(global_p, p++);
and also verifies that the arguments are pointers, while still avoiding
the unnecessary memory barrier when assigning NULL to a pointer.
This memory-barrier avoidance means that rcu_assign_pointer() is now only
permitted for pointers (not array indexes), and so this version emits a
compiler warning if the first argument is not a pointer. I built a "make
allyesconfig" version on an x86 system, and received no such warnings.
If RCU is ever applied to array indexes, then the second patch in this
series should be applied, and the resulting rcu_assign_index() be used.
Given the rather surprising history of subtlely broken implementations of
rcu_assign_pointer(), I took the precaution of generating a full set of
test cases and verified that memory barriers and compiler warnings were
emitted when required. I guess it is the simple things that get you...
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
rcupdate.h | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
--- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
+++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
@@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
* structure after the pointer assignment. More importantly, this
* call documents which pointers will be dereferenced by RCU read-side
* code.
+ *
+ * Throws a compiler warning for non-pointer arguments.
+ *
+ * Does not insert a memory barrier for a NULL pointer.
*/
-#define rcu_assign_pointer(p, v) ({ \
- smp_wmb(); \
- (p) = (v); \
- })
+#define rcu_assign_pointer(p, v) \
+ ({ \
+ typeof(*p) *_________p1 = (v); \
+ \
+ if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
+ smp_wmb(); \
+ (p) = _________p1; \
+ })
/**
* synchronize_sched - block until all CPUs have exited any non-preemptive
^ permalink raw reply
* [PATCH 2/2] add rcu_assign_index() if ever needed
From: Paul E. McKenney @ 2008-02-13 22:05 UTC (permalink / raw)
To: linux-kernel; +Cc: shemminger, davem, netdev, dipankar, ego, herbert, akpm
In-Reply-To: <20080213220024.GA30729@linux.vnet.ibm.com>
Hello again!
This is a speculative patch that as far as I can tell is not yet required.
If anyone applies RCU to a data structure allocated out of an array, using
array indexes in place of pointers to link the array elements together,
then the rcu_assign_index() function in this patch will be needed to
assign a given element's array index to the RCU-traversed index. The
implementation is exactly that of the old rcu_assign_pointer(), so is
extremely well tested.
The existing rcu_assign_pointer() will emit a compiler warning in cases
where rcu_assign_index() is required.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
rcupdate.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff -urpNa -X dontdiff linux-2.6.24-rap/include/linux/rcupdate.h linux-2.6.24-rai/include/linux/rcupdate.h
--- linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
+++ linux-2.6.24-rai/include/linux/rcupdate.h 2008-02-13 10:55:40.000000000 -0800
@@ -286,6 +286,24 @@ extern struct lockdep_map rcu_lock_map;
})
/**
+ * rcu_assign_index - assign (publicize) a index of a newly
+ * initialized array elementg that will be dereferenced by RCU
+ * read-side critical sections. Returns the value assigned.
+ *
+ * Inserts memory barriers on architectures that require them
+ * (pretty much all of them other than x86), and also prevents
+ * the compiler from reordering the code that initializes the
+ * structure after the index assignment. More importantly, this
+ * call documents which indexes will be dereferenced by RCU read-side
+ * code.
+ */
+
+#define rcu_assign_index(p, v) ({ \
+ smp_wmb(); \
+ (p) = (v); \
+ })
+
+/**
* synchronize_sched - block until all CPUs have exited any non-preemptive
* kernel code sequences.
*
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Andrew Morton @ 2008-02-13 22:11 UTC (permalink / raw)
To: paulmck; +Cc: linux-kernel, shemminger, davem, netdev, dipankar, ego, herbert
In-Reply-To: <20080213220024.GA30729@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 14:00:24 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> Hello!
>
> This is an updated version of the patch posted last November:
>
> http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
>
> This new version permits arguments with side effects, for example:
>
> rcu_assign_pointer(global_p, p++);
>
> and also verifies that the arguments are pointers, while still avoiding
> the unnecessary memory barrier when assigning NULL to a pointer.
> This memory-barrier avoidance means that rcu_assign_pointer() is now only
> permitted for pointers (not array indexes), and so this version emits a
> compiler warning if the first argument is not a pointer. I built a "make
> allyesconfig" version on an x86 system, and received no such warnings.
> If RCU is ever applied to array indexes, then the second patch in this
> series should be applied, and the resulting rcu_assign_index() be used.
>
> Given the rather surprising history of subtlely broken implementations of
> rcu_assign_pointer(), I took the precaution of generating a full set of
> test cases and verified that memory barriers and compiler warnings were
> emitted when required. I guess it is the simple things that get you...
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>
> rcupdate.h | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
whoop, ancient kernel alert.
> @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> * structure after the pointer assignment. More importantly, this
> * call documents which pointers will be dereferenced by RCU read-side
> * code.
> + *
> + * Throws a compiler warning for non-pointer arguments.
> + *
> + * Does not insert a memory barrier for a NULL pointer.
> */
>
> -#define rcu_assign_pointer(p, v) ({ \
> - smp_wmb(); \
> - (p) = (v); \
> - })
> +#define rcu_assign_pointer(p, v) \
> + ({ \
> + typeof(*p) *_________p1 = (v); \
> + \
> + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> + smp_wmb(); \
> + (p) = _________p1; \
> + })
Someone already merged the dont-do-it-for-NULL patch so I reworked this
appropriately. Was too lazy to update the changelog though.
^ permalink raw reply
* Re: BUG: 2.6.25-rc1: iptables postrouting setup causes oops
From: Ben Nizette @ 2008-02-13 22:41 UTC (permalink / raw)
To: Andrew Morton; +Cc: netfilter-devel, linux-kernel, netdev, Haavard Skinnemoen
In-Reply-To: <20080213004829.fd8afdc7.akpm@linux-foundation.org>
On Wed, 2008-02-13 at 00:48 -0800, Andrew Morton wrote:
> On Tue, 12 Feb 2008 22:46:01 +1100 Ben Nizette <bn@niasdigital.com> wrote:
> > Perfectly repeatable.
>
> If my theory is correct, changing pretty much anything in the kernel config
> might just make it go away. But still, it would be most valuable if you
> could try running a bisection search, as described in
> http://www.kernel.org/doc/local/git-quick.html, thanks.
>
Hm, not as stable a bug as I'd thought. I have performed a number of
bisections with different start points and they have not converged on
the same patch twice. Even bisections with the _same_ start point
didn't always converge on the same patch.
What ever the problem is it isn't immediately apparent in latest git so
I guess we'll just have to keep our eyes peeled.
Thanks,
--Ben.
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-13 22:41 UTC (permalink / raw)
To: Stephen Hemminger
Cc: linux-kernel, shemminger, davem, netdev, dipankar, ego, herbert,
akpm
In-Reply-To: <20080213143537.1b806790@extreme>
On Wed, Feb 13, 2008 at 02:35:37PM -0800, Stephen Hemminger wrote:
> On Wed, 13 Feb 2008 14:00:24 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > Hello!
> >
> > This is an updated version of the patch posted last November:
> >
> > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> >
> > This new version permits arguments with side effects, for example:
> >
> > rcu_assign_pointer(global_p, p++);
> >
> > and also verifies that the arguments are pointers, while still avoiding
> > the unnecessary memory barrier when assigning NULL to a pointer.
> > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > permitted for pointers (not array indexes), and so this version emits a
> > compiler warning if the first argument is not a pointer. I built a "make
> > allyesconfig" version on an x86 system, and received no such warnings.
> > If RCU is ever applied to array indexes, then the second patch in this
> > series should be applied, and the resulting rcu_assign_index() be used.
> >
> > Given the rather surprising history of subtlely broken implementations of
> > rcu_assign_pointer(), I took the precaution of generating a full set of
> > test cases and verified that memory barriers and compiler warnings were
> > emitted when required. I guess it is the simple things that get you...
> >
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >
> > rcupdate.h | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > * structure after the pointer assignment. More importantly, this
> > * call documents which pointers will be dereferenced by RCU read-side
> > * code.
> > + *
> > + * Throws a compiler warning for non-pointer arguments.
> > + *
> > + * Does not insert a memory barrier for a NULL pointer.
> > */
> >
> > -#define rcu_assign_pointer(p, v) ({ \
> > - smp_wmb(); \
> > - (p) = (v); \
> > - })
> > +#define rcu_assign_pointer(p, v) \
> > + ({ \
> > + typeof(*p) *_________p1 = (v); \
> > + \
> > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > + smp_wmb(); \
> > + (p) = _________p1; \
> > + })
> >
> > /**
> > * synchronize_sched - block until all CPUs have exited any non-preemptive
>
> Will this still work if p is unsigned long?
Hello, Steve,
If p is unsigned long, then use rcu_assign_index() from the next patch in
the set. Looks like Andrew has applied it to -mm -- so please make sure
that he is aware if you do use it.
Thanx, Paul
^ permalink raw reply
* Re: [RFC, PATCH]: Pass link level header from/to PPP interface
From: David Miller @ 2008-02-13 22:53 UTC (permalink / raw)
To: urs; +Cc: netdev
In-Reply-To: <ygflk5opzhb.fsf@janus.isnogud.escape.de>
From: Urs Thuermann <urs@isnogud.escape.de>
Date: 13 Feb 2008 17:56:00 +0100
> Do you consider this strong enough to justify this change?
Can tcpdump and libpcap already handle PPP properly?
^ permalink raw reply
* Re: [PATCH] fib_trie: rcu_assign_pointer warning fix
From: Paul E. McKenney @ 2008-02-13 22:55 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, shemminger, netdev, linux-kernel
In-Reply-To: <20080212194630.GB2803@ami.dom.local>
On Tue, Feb 12, 2008 at 08:46:30PM +0100, Jarek Poplawski wrote:
> On Tue, Feb 12, 2008 at 08:32:18PM +0100, Jarek Poplawski wrote:
> ...
> > It seems the above version of this macro uses the barrier for 0, but
> > if I miss something, or for these other: documenting reasons,
>
> ...or __builtin_constants could be used for indexing (?!),
Yep. For example:
elem[0].next = 1;
rcu_assign_index(global_index, 0);
Thanx, Paul
> > then of
> > course you are right.
>
> Jarek P.
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-13 23:37 UTC (permalink / raw)
To: Stephen Hemminger
Cc: linux-kernel, shemminger, davem, netdev, dipankar, ego, herbert,
akpm
In-Reply-To: <20080213144233.05e860cb@extreme>
On Wed, Feb 13, 2008 at 02:42:33PM -0800, Stephen Hemminger wrote:
> On Wed, 13 Feb 2008 14:41:34 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > On Wed, Feb 13, 2008 at 02:35:37PM -0800, Stephen Hemminger wrote:
> > > On Wed, 13 Feb 2008 14:00:24 -0800
> > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > > Hello!
> > > >
> > > > This is an updated version of the patch posted last November:
> > > >
> > > > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> > > >
> > > > This new version permits arguments with side effects, for example:
> > > >
> > > > rcu_assign_pointer(global_p, p++);
> > > >
> > > > and also verifies that the arguments are pointers, while still avoiding
> > > > the unnecessary memory barrier when assigning NULL to a pointer.
> > > > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > > > permitted for pointers (not array indexes), and so this version emits a
> > > > compiler warning if the first argument is not a pointer. I built a "make
> > > > allyesconfig" version on an x86 system, and received no such warnings.
> > > > If RCU is ever applied to array indexes, then the second patch in this
> > > > series should be applied, and the resulting rcu_assign_index() be used.
> > > >
> > > > Given the rather surprising history of subtlely broken implementations of
> > > > rcu_assign_pointer(), I took the precaution of generating a full set of
> > > > test cases and verified that memory barriers and compiler warnings were
> > > > emitted when required. I guess it is the simple things that get you...
> > > >
> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > ---
> > > >
> > > > rcupdate.h | 16 ++++++++++++----
> > > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > > >
> > > > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > > > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > > > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > > > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > > > * structure after the pointer assignment. More importantly, this
> > > > * call documents which pointers will be dereferenced by RCU read-side
> > > > * code.
> > > > + *
> > > > + * Throws a compiler warning for non-pointer arguments.
> > > > + *
> > > > + * Does not insert a memory barrier for a NULL pointer.
> > > > */
> > > >
> > > > -#define rcu_assign_pointer(p, v) ({ \
> > > > - smp_wmb(); \
> > > > - (p) = (v); \
> > > > - })
> > > > +#define rcu_assign_pointer(p, v) \
> > > > + ({ \
> > > > + typeof(*p) *_________p1 = (v); \
> > > > + \
> > > > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > > > + smp_wmb(); \
> > > > + (p) = _________p1; \
> > > > + })
> > > >
> > > > /**
> > > > * synchronize_sched - block until all CPUs have exited any non-preemptive
> > >
> > > Will this still work if p is unsigned long?
> >
> > Hello, Steve,
> >
> > If p is unsigned long, then use rcu_assign_index() from the next patch in
> > the set. Looks like Andrew has applied it to -mm -- so please make sure
> > that he is aware if you do use it.
>
> Make sure fib_trie still works and doesn't get warnings.
Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
doesn't cut it. Please accept my apologies for my confusion!!!
Once fib_trie is configured, I do indeed get:
net/ipv4/fib_trie.c: In function ‘node_set_parent’:
net/ipv4/fib_trie.c:182: warning: comparison between pointer and integer
So, given that node->parent is an unsigned long, I changed node_set_parent()
to the following:
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
}
This removes the warnings. I am a little ambivalent about this, as
this is really a pointer in disguise rather than an array index, but
patch below. I suppose that another option would be to make node->parent
be a void* and provide appropriate accessor functions/macros.
Thoughts?
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
fib_trie.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
--- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
+++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 15:22:02.000000000 -0800
@@ -179,8 +179,7 @@ static inline struct tnode *node_parent_
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
- rcu_assign_pointer(node->parent,
- (unsigned long)ptr | NODE_TYPE(node));
+ rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
}
static inline struct node *tnode_get_child(struct tnode *tn, unsigned int i)
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Stephen Hemminger @ 2008-02-13 23:51 UTC (permalink / raw)
To: paulmck
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080213233744.GO12393@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 15:37:44 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Feb 13, 2008 at 02:42:33PM -0800, Stephen Hemminger wrote:
> > On Wed, 13 Feb 2008 14:41:34 -0800
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >
> > > On Wed, Feb 13, 2008 at 02:35:37PM -0800, Stephen Hemminger wrote:
> > > > On Wed, 13 Feb 2008 14:00:24 -0800
> > > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > >
> > > > > Hello!
> > > > >
> > > > > This is an updated version of the patch posted last November:
> > > > >
> > > > > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> > > > >
> > > > > This new version permits arguments with side effects, for example:
> > > > >
> > > > > rcu_assign_pointer(global_p, p++);
> > > > >
> > > > > and also verifies that the arguments are pointers, while still avoiding
> > > > > the unnecessary memory barrier when assigning NULL to a pointer.
> > > > > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > > > > permitted for pointers (not array indexes), and so this version emits a
> > > > > compiler warning if the first argument is not a pointer. I built a "make
> > > > > allyesconfig" version on an x86 system, and received no such warnings.
> > > > > If RCU is ever applied to array indexes, then the second patch in this
> > > > > series should be applied, and the resulting rcu_assign_index() be used.
> > > > >
> > > > > Given the rather surprising history of subtlely broken implementations of
> > > > > rcu_assign_pointer(), I took the precaution of generating a full set of
> > > > > test cases and verified that memory barriers and compiler warnings were
> > > > > emitted when required. I guess it is the simple things that get you...
> > > > >
> > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > ---
> > > > >
> > > > > rcupdate.h | 16 ++++++++++++----
> > > > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > > > > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > > > > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > > > > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > > > > * structure after the pointer assignment. More importantly, this
> > > > > * call documents which pointers will be dereferenced by RCU read-side
> > > > > * code.
> > > > > + *
> > > > > + * Throws a compiler warning for non-pointer arguments.
> > > > > + *
> > > > > + * Does not insert a memory barrier for a NULL pointer.
> > > > > */
> > > > >
> > > > > -#define rcu_assign_pointer(p, v) ({ \
> > > > > - smp_wmb(); \
> > > > > - (p) = (v); \
> > > > > - })
> > > > > +#define rcu_assign_pointer(p, v) \
> > > > > + ({ \
> > > > > + typeof(*p) *_________p1 = (v); \
> > > > > + \
> > > > > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > > > > + smp_wmb(); \
> > > > > + (p) = _________p1; \
> > > > > + })
> > > > >
> > > > > /**
> > > > > * synchronize_sched - block until all CPUs have exited any non-preemptive
> > > >
> > > > Will this still work if p is unsigned long?
> > >
> > > Hello, Steve,
> > >
> > > If p is unsigned long, then use rcu_assign_index() from the next patch in
> > > the set. Looks like Andrew has applied it to -mm -- so please make sure
> > > that he is aware if you do use it.
> >
> > Make sure fib_trie still works and doesn't get warnings.
>
> Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> doesn't cut it. Please accept my apologies for my confusion!!!
>
> Once fib_trie is configured, I do indeed get:
>
> net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> net/ipv4/fib_trie.c:182: warning: comparison between pointer and integer
>
> So, given that node->parent is an unsigned long, I changed node_set_parent()
> to the following:
>
> static inline void node_set_parent(struct node *node, struct tnode *ptr)
> {
> rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
> }
>
> This removes the warnings. I am a little ambivalent about this, as
> this is really a pointer in disguise rather than an array index, but
> patch below. I suppose that another option would be to make node->parent
> be a void* and provide appropriate accessor functions/macros.
>
> Thoughts?
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Maybe cast both sides to void * in this case:
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
}
--
Stephen Hemminger <stephen.hemminger@vyatta.com>
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Andrew Morton @ 2008-02-13 23:52 UTC (permalink / raw)
To: paulmck
Cc: "stephen.hemminger, linux-kernel, shemminger, davem, netdev,
dipankar, ego, herbert
In-Reply-To: <20080213233744.GO12393@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 15:37:44 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> doesn't cut it.
This is not good. The sole purpose of allmodconfig and allyesconfig is for
compilation and linkage coverage testing. Hence we should aim to get as
much code as possible included in those cases.
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Stephen Hemminger @ 2008-02-13 23:55 UTC (permalink / raw)
To: Andrew Morton
Cc: paulmck, "stephen.hemminger, linux-kernel, davem, netdev,
dipankar, ego, herbert
In-Reply-To: <20080213155245.455944f8.akpm@linux-foundation.org>
On Wed, 13 Feb 2008 15:52:45 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 13 Feb 2008 15:37:44 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > doesn't cut it.
>
> This is not good. The sole purpose of allmodconfig and allyesconfig is for
> compilation and linkage coverage testing. Hence we should aim to get as
> much code as possible included in those cases.
>
The current model is compile time choice. It is on my long term list
to make this a runtime option.
--
Stephen Hemminger <stephen.hemminger@vyatta.com>
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: David Miller @ 2008-02-13 23:57 UTC (permalink / raw)
To: akpm
Cc: paulmck, "stephen.hemminger, linux-kernel, shemminger, netdev,
dipankar, ego, herbert
In-Reply-To: <20080213155245.455944f8.akpm@linux-foundation.org>
From: Andrew Morton <akpm@linux-foundation.org>
Date: Wed, 13 Feb 2008 15:52:45 -0800
> On Wed, 13 Feb 2008 15:37:44 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > doesn't cut it.
>
> This is not good. The sole purpose of allmodconfig and allyesconfig is for
> compilation and linkage coverage testing. Hence we should aim to get as
> much code as possible included in those cases.
Well, in this case there is a choice, either you use one routing
lookup datastructure or the other. It's not purposefully being hidden
from the everything builds :-)
^ permalink raw reply
* Network namespace and tc?
From: Stephen Hemminger @ 2008-02-13 23:59 UTC (permalink / raw)
To: netdev
It looks like tc filter won't work on alternate namespaces:
/* Add/change/delete/get a filter node */
static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, void *arg)
{
...
if (net != &init_net)
return -EINVAL;
Haven't played with namespace virtualization yet, but what else is
not supported? Where is this documented?
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Andrew Morton @ 2008-02-14 0:09 UTC (permalink / raw)
To: David Miller
Cc: paulmck, "stephen.hemminger, linux-kernel, shemminger, netdev,
dipankar, ego, herbert
In-Reply-To: <20080213.155738.71849437.davem@davemloft.net>
On Wed, 13 Feb 2008 15:57:38 -0800 (PST)
David Miller <davem@davemloft.net> wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Wed, 13 Feb 2008 15:52:45 -0800
>
> > On Wed, 13 Feb 2008 15:37:44 -0800
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >
> > > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > > doesn't cut it.
> >
> > This is not good. The sole purpose of allmodconfig and allyesconfig is for
> > compilation and linkage coverage testing. Hence we should aim to get as
> > much code as possible included in those cases.
>
> Well, in this case there is a choice, either you use one routing
> lookup datastructure or the other. It's not purposefully being hidden
> from the everything builds :-)
oic. yes, that is a bit of a problem. Oh well.
`make randconfig' seems to be able to enable CONFIG_IP_FIB_TRIE about one
time in eight ;)
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-14 0:14 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080213155158.1b621359@extreme>
On Wed, Feb 13, 2008 at 03:51:58PM -0800, Stephen Hemminger wrote:
> On Wed, 13 Feb 2008 15:37:44 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Feb 13, 2008 at 02:42:33PM -0800, Stephen Hemminger wrote:
> > > On Wed, 13 Feb 2008 14:41:34 -0800
> > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > > On Wed, Feb 13, 2008 at 02:35:37PM -0800, Stephen Hemminger wrote:
> > > > > On Wed, 13 Feb 2008 14:00:24 -0800
> > > > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > > >
> > > > > > Hello!
> > > > > >
> > > > > > This is an updated version of the patch posted last November:
> > > > > >
> > > > > > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> > > > > >
> > > > > > This new version permits arguments with side effects, for example:
> > > > > >
> > > > > > rcu_assign_pointer(global_p, p++);
> > > > > >
> > > > > > and also verifies that the arguments are pointers, while still avoiding
> > > > > > the unnecessary memory barrier when assigning NULL to a pointer.
> > > > > > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > > > > > permitted for pointers (not array indexes), and so this version emits a
> > > > > > compiler warning if the first argument is not a pointer. I built a "make
> > > > > > allyesconfig" version on an x86 system, and received no such warnings.
> > > > > > If RCU is ever applied to array indexes, then the second patch in this
> > > > > > series should be applied, and the resulting rcu_assign_index() be used.
> > > > > >
> > > > > > Given the rather surprising history of subtlely broken implementations of
> > > > > > rcu_assign_pointer(), I took the precaution of generating a full set of
> > > > > > test cases and verified that memory barriers and compiler warnings were
> > > > > > emitted when required. I guess it is the simple things that get you...
> > > > > >
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >
> > > > > > rcupdate.h | 16 ++++++++++++----
> > > > > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > > > > > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > > > > > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > > > > > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > > > > > * structure after the pointer assignment. More importantly, this
> > > > > > * call documents which pointers will be dereferenced by RCU read-side
> > > > > > * code.
> > > > > > + *
> > > > > > + * Throws a compiler warning for non-pointer arguments.
> > > > > > + *
> > > > > > + * Does not insert a memory barrier for a NULL pointer.
> > > > > > */
> > > > > >
> > > > > > -#define rcu_assign_pointer(p, v) ({ \
> > > > > > - smp_wmb(); \
> > > > > > - (p) = (v); \
> > > > > > - })
> > > > > > +#define rcu_assign_pointer(p, v) \
> > > > > > + ({ \
> > > > > > + typeof(*p) *_________p1 = (v); \
> > > > > > + \
> > > > > > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > > > > > + smp_wmb(); \
> > > > > > + (p) = _________p1; \
> > > > > > + })
> > > > > >
> > > > > > /**
> > > > > > * synchronize_sched - block until all CPUs have exited any non-preemptive
> > > > >
> > > > > Will this still work if p is unsigned long?
> > > >
> > > > Hello, Steve,
> > > >
> > > > If p is unsigned long, then use rcu_assign_index() from the next patch in
> > > > the set. Looks like Andrew has applied it to -mm -- so please make sure
> > > > that he is aware if you do use it.
> > >
> > > Make sure fib_trie still works and doesn't get warnings.
> >
> > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > doesn't cut it. Please accept my apologies for my confusion!!!
> >
> > Once fib_trie is configured, I do indeed get:
> >
> > net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> > net/ipv4/fib_trie.c:182: warning: comparison between pointer and integer
> >
> > So, given that node->parent is an unsigned long, I changed node_set_parent()
> > to the following:
> >
> > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > {
> > rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
> > }
> >
> > This removes the warnings. I am a little ambivalent about this, as
> > this is really a pointer in disguise rather than an array index, but
> > patch below. I suppose that another option would be to make node->parent
> > be a void* and provide appropriate accessor functions/macros.
> >
> > Thoughts?
> >
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> Maybe cast both sides to void * in this case:
>
> static inline void node_set_parent(struct node *node, struct tnode *ptr)
> {
> rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
> }
That gets me the following:
net/ipv4/fib_trie.c: In function ‘node_set_parent’:
net/ipv4/fib_trie.c:182: error: invalid lvalue in assignment
However, as with much in computing, an extra level of indirection fixes
things. Your call as to whether or not the cure is preferable to the
disease. ;-)
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
fib_trie.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
--- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
+++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 16:10:07.000000000 -0800
@@ -179,8 +179,8 @@ static inline struct tnode *node_parent_
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
- rcu_assign_pointer(node->parent,
- (unsigned long)ptr | NODE_TYPE(node));
+ rcu_assign_pointer((*(void **)&node->parent),
+ (void *)((unsigned long)ptr | NODE_TYPE(node)));
}
static inline struct node *tnode_get_child(struct tnode *tn, unsigned int i)
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Stephen Hemminger @ 2008-02-14 0:27 UTC (permalink / raw)
To: paulmck
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080214001404.GQ12393@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 16:14:04 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Feb 13, 2008 at 03:51:58PM -0800, Stephen Hemminger wrote:
> > On Wed, 13 Feb 2008 15:37:44 -0800
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > On Wed, Feb 13, 2008 at 02:42:33PM -0800, Stephen Hemminger wrote:
> > > > On Wed, 13 Feb 2008 14:41:34 -0800
> > > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > >
> > > > > On Wed, Feb 13, 2008 at 02:35:37PM -0800, Stephen Hemminger wrote:
> > > > > > On Wed, 13 Feb 2008 14:00:24 -0800
> > > > > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > > > >
> > > > > > > Hello!
> > > > > > >
> > > > > > > This is an updated version of the patch posted last November:
> > > > > > >
> > > > > > > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> > > > > > >
> > > > > > > This new version permits arguments with side effects, for example:
> > > > > > >
> > > > > > > rcu_assign_pointer(global_p, p++);
> > > > > > >
> > > > > > > and also verifies that the arguments are pointers, while still avoiding
> > > > > > > the unnecessary memory barrier when assigning NULL to a pointer.
> > > > > > > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > > > > > > permitted for pointers (not array indexes), and so this version emits a
> > > > > > > compiler warning if the first argument is not a pointer. I built a "make
> > > > > > > allyesconfig" version on an x86 system, and received no such warnings.
> > > > > > > If RCU is ever applied to array indexes, then the second patch in this
> > > > > > > series should be applied, and the resulting rcu_assign_index() be used.
> > > > > > >
> > > > > > > Given the rather surprising history of subtlely broken implementations of
> > > > > > > rcu_assign_pointer(), I took the precaution of generating a full set of
> > > > > > > test cases and verified that memory barriers and compiler warnings were
> > > > > > > emitted when required. I guess it is the simple things that get you...
> > > > > > >
> > > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > > ---
> > > > > > >
> > > > > > > rcupdate.h | 16 ++++++++++++----
> > > > > > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > > > > > > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > > > > > > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > > > > > > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > > > > > > * structure after the pointer assignment. More importantly, this
> > > > > > > * call documents which pointers will be dereferenced by RCU read-side
> > > > > > > * code.
> > > > > > > + *
> > > > > > > + * Throws a compiler warning for non-pointer arguments.
> > > > > > > + *
> > > > > > > + * Does not insert a memory barrier for a NULL pointer.
> > > > > > > */
> > > > > > >
> > > > > > > -#define rcu_assign_pointer(p, v) ({ \
> > > > > > > - smp_wmb(); \
> > > > > > > - (p) = (v); \
> > > > > > > - })
> > > > > > > +#define rcu_assign_pointer(p, v) \
> > > > > > > + ({ \
> > > > > > > + typeof(*p) *_________p1 = (v); \
> > > > > > > + \
> > > > > > > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > > > > > > + smp_wmb(); \
> > > > > > > + (p) = _________p1; \
> > > > > > > + })
> > > > > > >
> > > > > > > /**
> > > > > > > * synchronize_sched - block until all CPUs have exited any non-preemptive
> > > > > >
> > > > > > Will this still work if p is unsigned long?
> > > > >
> > > > > Hello, Steve,
> > > > >
> > > > > If p is unsigned long, then use rcu_assign_index() from the next patch in
> > > > > the set. Looks like Andrew has applied it to -mm -- so please make sure
> > > > > that he is aware if you do use it.
> > > >
> > > > Make sure fib_trie still works and doesn't get warnings.
> > >
> > > Ah. It does take a bit to get fib_trie into one's build -- allyesconfig
> > > doesn't cut it. Please accept my apologies for my confusion!!!
> > >
> > > Once fib_trie is configured, I do indeed get:
> > >
> > > net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> > > net/ipv4/fib_trie.c:182: warning: comparison between pointer and integer
> > >
> > > So, given that node->parent is an unsigned long, I changed node_set_parent()
> > > to the following:
> > >
> > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > {
> > > rcu_assign_index(node->parent, (unsigned long)ptr | NODE_TYPE(node));
> > > }
> > >
> > > This removes the warnings. I am a little ambivalent about this, as
> > > this is really a pointer in disguise rather than an array index, but
> > > patch below. I suppose that another option would be to make node->parent
> > > be a void* and provide appropriate accessor functions/macros.
> > >
> > > Thoughts?
> > >
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >
> > Maybe cast both sides to void * in this case:
> >
> > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > {
> > rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > }
>
> That gets me the following:
>
> net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> net/ipv4/fib_trie.c:182: error: invalid lvalue in assignment
>
> However, as with much in computing, an extra level of indirection fixes
> things. Your call as to whether or not the cure is preferable to the
> disease. ;-)
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>
> fib_trie.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
> --- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
> +++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 16:10:07.000000000 -0800
> @@ -179,8 +179,8 @@ static inline struct tnode *node_parent_
>
> static inline void node_set_parent(struct node *node, struct tnode *ptr)
> {
> - rcu_assign_pointer(node->parent,
> - (unsigned long)ptr | NODE_TYPE(node));
> + rcu_assign_pointer((*(void **)&node->parent),
> + (void *)((unsigned long)ptr | NODE_TYPE(node)));
> }
That is heading towards ugly... Maybe not using the macro at all (for this case) would be best:
static inline void node_set_parent(struct node *node, struct tnode *ptr)
{
smp_wmb();
node->parent = (unsigned long)ptr | NODE_TYPE(node);
}
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-14 0:42 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080213162700.0a32000d@extreme>
On Wed, Feb 13, 2008 at 04:27:00PM -0800, Stephen Hemminger wrote:
> On Wed, 13 Feb 2008 16:14:04 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Feb 13, 2008 at 03:51:58PM -0800, Stephen Hemminger wrote:
[ . . . ]
> > > Maybe cast both sides to void * in this case:
> > >
> > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > {
> > > rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > > }
> >
> > That gets me the following:
> >
> > net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> > net/ipv4/fib_trie.c:182: error: invalid lvalue in assignment
> >
> > However, as with much in computing, an extra level of indirection fixes
> > things. Your call as to whether or not the cure is preferable to the
> > disease. ;-)
> >
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >
> > fib_trie.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
> > --- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
> > +++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 16:10:07.000000000 -0800
> > @@ -179,8 +179,8 @@ static inline struct tnode *node_parent_
> >
> > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > {
> > - rcu_assign_pointer(node->parent,
> > - (unsigned long)ptr | NODE_TYPE(node));
> > + rcu_assign_pointer((*(void **)&node->parent),
> > + (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > }
>
> That is heading towards ugly... Maybe not using the macro at all (for this case) would be best:
>
> static inline void node_set_parent(struct node *node, struct tnode *ptr)
> {
> smp_wmb();
> node->parent = (unsigned long)ptr | NODE_TYPE(node);
> }
Or, alternatively, the rcu_assign_index() patch sent earlier to avoid
the bare memory barrier?
Thanx, Paul
^ permalink raw reply
* [patch] Fix station address detection in smc
From: Chuck Ebbert @ 2008-02-14 0:47 UTC (permalink / raw)
To: Dominik Brodowski; +Cc: linux-pcmcia, Netdev, Jeff Garzik
Megahertz EM1144 PCMCIA ethernet adapter needs special handling
because it has two VERS_1 tuples and the station address is in
the second one. Conversion to generic handling of these fields
broke it. Reverting that fixes the device.
https://bugzilla.redhat.com/show_bug.cgi?id=233255
Thanks go to Jon Stanley for not giving up on this one until the
problem was found.
Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
---
This was broken for over a year before the problem was found...
--- b/drivers/net/pcmcia/smc91c92_cs.c Tue Dec 05 06:09:21 2006 +0500
+++ a/drivers/net/pcmcia/smc91c92_cs.c Tue Dec 05 06:09:20 2006 +0500
@@ -560,8 +560,16 @@
/* Read the station address from the CIS. It is stored as the last
(fourth) string in the Version 1 Version/ID tuple. */
+ tuple->DesiredTuple = CISTPL_VERS_1;
+ if (first_tuple(link, tuple, parse) != CS_SUCCESS) {
+ rc = -1;
+ goto free_cfg_mem;
+ }
+ /* Ugh -- the EM1144 card has two VERS_1 tuples!?! */
+ if (next_tuple(link, tuple, parse) != CS_SUCCESS)
+ first_tuple(link, tuple, parse);
+ if (parse->version_1.ns > 3) {
+ station_addr = parse->version_1.str + parse->version_1.ofs[3];
- if (link->prod_id[3]) {
- station_addr = link->prod_id[3];
if (cvt_ascii_address(dev, station_addr) == 0) {
rc = 0;
goto free_cfg_mem;
^ permalink raw reply
* Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()
From: Jann Traschewski @ 2008-02-14 0:49 UTC (permalink / raw)
To: 'Jarek Poplawski', 'David Miller'
Cc: 'Bernard Pidoux F6BVP', 'Ralf Baechle', netdev
In-Reply-To: <20080213115607.GB2867@ff.dom.local>
Applied and stable with Kernel 2.6.24.2 since 12 hours.
Regards,
Jann
> -----Ursprüngliche Nachricht-----
> Von: Jarek Poplawski [mailto:jarkao2@gmail.com]
> Gesendet: Mittwoch, 13. Februar 2008 12:56
> An: David Miller
> Cc: Jann Traschewski; Bernard Pidoux F6BVP; Ralf Baechle;
> netdev@vger.kernel.org
> Betreff: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()
>
> Hi,
>
> Here is an "official" version of "testing patch #2" from this thread.
> The only difference: ax25->vs is changed only after checking
> skb is not NULL (plus a comment). IMHO it could be applied.
>
> Thanks,
> Jarek P.
>
> ---------------->
>
> Subject: [AX25] ax25_out: check skb for NULL in ax25_kick()
>
> According to some OOPS reports ax25_kick tries to clone NULL
> skbs sometimes. It looks like a race with
> ax25_clear_queues(). Probably there is no need to add more
> than a simple check for this yet.
> Another report suggested there are probably also cases where ax25
> ->paclen == 0 can happen in ax25_output(); this wasn't confirmed
> during testing but let's leave this debugging check for some time.
>
>
> Reported-and-tested-by: Jann Traschewski <jann@gmx.de>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>
> ---
>
> diff -Nurp 2.6.24-mm1-/net/ax25/ax25_out.c
> 2.6.24-mm1+/net/ax25/ax25_out.c
> --- 2.6.24-mm1-/net/ax25/ax25_out.c 2008-01-24
> 22:58:37.000000000 +0000
> +++ 2.6.24-mm1+/net/ax25/ax25_out.c 2008-02-13
> 10:43:50.000000000 +0000
> @@ -117,6 +117,12 @@ void ax25_output(ax25_cb *ax25, int pacl
> unsigned char *p;
> int frontlen, len, fragno, ka9qfrag, first = 1;
>
> + if (paclen < 16) {
> + WARN_ON_ONCE(1);
> + kfree_skb(skb);
> + return;
> + }
> +
> if ((skb->len - 1) > paclen) {
> if (*skb->data == AX25_P_TEXT) {
> skb_pull(skb, 1); /* skip PID */
> @@ -251,8 +257,6 @@ void ax25_kick(ax25_cb *ax25)
> if (start == end)
> return;
>
> - ax25->vs = start;
> -
> /*
> * Transmit data until either we're out of data to send or
> * the window is full. Send a poll on the final I frame
> if @@ -261,8 +265,13 @@ void ax25_kick(ax25_cb *ax25)
>
> /*
> * Dequeue the frame and copy it.
> + * Check for race with ax25_clear_queues().
> */
> skb = skb_dequeue(&ax25->write_queue);
> + if (!skb)
> + return;
> +
> + ax25->vs = start;
>
> do {
> if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) {
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Stephen Hemminger @ 2008-02-14 0:53 UTC (permalink / raw)
To: paulmck
Cc: Stephen Hemminger, linux-kernel, davem, netdev, dipankar, ego,
herbert, akpm
In-Reply-To: <20080214004253.GR12393@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 16:42:53 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Feb 13, 2008 at 04:27:00PM -0800, Stephen Hemminger wrote:
> > On Wed, 13 Feb 2008 16:14:04 -0800
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > On Wed, Feb 13, 2008 at 03:51:58PM -0800, Stephen Hemminger wrote:
>
> [ . . . ]
>
> > > > Maybe cast both sides to void * in this case:
> > > >
> > > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > > {
> > > > rcu_assign_pointer((void *) node->parent, (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > > > }
> > >
> > > That gets me the following:
> > >
> > > net/ipv4/fib_trie.c: In function ‘node_set_parent’:
> > > net/ipv4/fib_trie.c:182: error: invalid lvalue in assignment
> > >
> > > However, as with much in computing, an extra level of indirection fixes
> > > things. Your call as to whether or not the cure is preferable to the
> > > disease. ;-)
> > >
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > ---
> > >
> > > fib_trie.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff -urpNa -X dontdiff linux-2.6.25-rc1/net/ipv4/fib_trie.c linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c
> > > --- linux-2.6.25-rc1/net/ipv4/fib_trie.c 2008-02-13 14:38:12.000000000 -0800
> > > +++ linux-2.6.25-rc1-fib_trie-warn.compile/net/ipv4/fib_trie.c 2008-02-13 16:10:07.000000000 -0800
> > > @@ -179,8 +179,8 @@ static inline struct tnode *node_parent_
> > >
> > > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > > {
> > > - rcu_assign_pointer(node->parent,
> > > - (unsigned long)ptr | NODE_TYPE(node));
> > > + rcu_assign_pointer((*(void **)&node->parent),
> > > + (void *)((unsigned long)ptr | NODE_TYPE(node)));
> > > }
> >
> > That is heading towards ugly... Maybe not using the macro at all (for this case) would be best:
> >
> > static inline void node_set_parent(struct node *node, struct tnode *ptr)
> > {
> > smp_wmb();
> > node->parent = (unsigned long)ptr | NODE_TYPE(node);
> > }
>
> Or, alternatively, the rcu_assign_index() patch sent earlier to avoid
> the bare memory barrier?
>
> Thanx, Paul
I am fine with rcu_assign_index(), and add a comment in node_set_parent.
^ 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