All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] skge: request IRQ on activating the interface
@ 2009-09-22 12:01 Michal Schmidt
  2009-09-22 16:28 ` Stephen Hemminger
  0 siblings, 1 reply; 13+ messages in thread
From: Michal Schmidt @ 2009-09-22 12:01 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger

skge requests IRQ in its probe function. This causes a problem in
the following real-life scenario with two different NICs in the machine:

1. modprobe skge
   The card is detected as eth0 and requests IRQ 17. Directory
   /proc/irq/17/eth0 is created.
2. There is an udev rule which says this interface should be called
   eth1, so udev renames eth0 -> eth1.
3. modprobe 8139too
   The Realtek card is detected as eth0. It will be using IRQ 17 too.
4. ip link set eth0 up
   Now 8139too requests IRQ 17.

The result is:
WARNING: at fs/proc/generic.c:590 proc_register ...
proc_dir_entry '17/eth0' already registered
...

And "ls /proc/irq/17" shows two subdirectories, both called eth0.

Fix it by requesting the IRQ in skge when the interface is activated.
This works, because interfaces can be renamed only while they are down.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---

 drivers/net/skge.c |   27 +++++++++++++++------------
 1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/net/skge.c b/drivers/net/skge.c
index 62e852e..7e90f27 100644
--- a/drivers/net/skge.c
+++ b/drivers/net/skge.c
@@ -105,6 +105,7 @@ static void yukon_init(struct skge_hw *hw, int port);
 static void genesis_mac_init(struct skge_hw *hw, int port);
 static void genesis_link_up(struct skge_port *skge);
 static void skge_set_multicast(struct net_device *dev);
+static irqreturn_t skge_intr(int irq, void *dev_id);
 
 /* Avoid conditionals by using array */
 static const int txqaddr[] = { Q_XA1, Q_XA2 };
@@ -2572,18 +2573,26 @@ static int skge_up(struct net_device *dev)
 	if (netif_msg_ifup(skge))
 		printk(KERN_INFO PFX "%s: enabling interface\n", dev->name);
 
+	err = request_irq(dev->irq, skge_intr, IRQF_SHARED, dev->name, hw);
+	if (err) {
+		dev_err(&hw->pdev->dev, "%s: cannot assign irq %d\n",
+			dev->name, dev->irq);
+		return err;
+	}
+
 	if (dev->mtu > RX_BUF_SIZE)
 		skge->rx_buf_size = dev->mtu + ETH_HLEN;
 	else
 		skge->rx_buf_size = RX_BUF_SIZE;
 
-
 	rx_size = skge->rx_ring.count * sizeof(struct skge_rx_desc);
 	tx_size = skge->tx_ring.count * sizeof(struct skge_tx_desc);
 	skge->mem_size = tx_size + rx_size;
 	skge->mem = pci_alloc_consistent(hw->pdev, skge->mem_size, &skge->dma);
-	if (!skge->mem)
-		return -ENOMEM;
+	if (!skge->mem) {
+		err = -ENOMEM;
+		goto free_irq;
+	}
 
 	BUG_ON(skge->dma & 7);
 
@@ -2646,6 +2655,8 @@ static int skge_up(struct net_device *dev)
  free_pci_mem:
 	pci_free_consistent(hw->pdev, skge->mem_size, skge->mem, skge->dma);
 	skge->mem = NULL;
+ free_irq:
+	free_irq(dev->irq, hw);
 
 	return err;
 }
@@ -2733,6 +2744,7 @@ static int skge_down(struct net_device *dev)
 	kfree(skge->tx_ring.start);
 	pci_free_consistent(hw->pdev, skge->mem_size, skge->mem, skge->dma);
 	skge->mem = NULL;
+	free_irq(dev->irq, hw);
 	return 0;
 }
 
@@ -3974,12 +3986,6 @@ static int __devinit skge_probe(struct pci_dev *pdev,
 		goto err_out_free_netdev;
 	}
 
-	err = request_irq(pdev->irq, skge_intr, IRQF_SHARED, dev->name, hw);
-	if (err) {
-		dev_err(&pdev->dev, "%s: cannot assign irq %d\n",
-		       dev->name, pdev->irq);
-		goto err_out_unregister;
-	}
 	skge_show_addr(dev);
 
 	if (hw->ports > 1 && (dev1 = skge_devinit(hw, 1, using_dac))) {
@@ -3996,8 +4002,6 @@ static int __devinit skge_probe(struct pci_dev *pdev,
 
 	return 0;
 
-err_out_unregister:
-	unregister_netdev(dev);
 err_out_free_netdev:
 	free_netdev(dev);
 err_out_led_off:
@@ -4041,7 +4045,6 @@ static void __devexit skge_remove(struct pci_dev *pdev)
 	skge_write16(hw, B0_LED, LED_STAT_OFF);
 	skge_write8(hw, B0_CTST, CS_RST_SET);
 
-	free_irq(pdev->irq, hw);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 	if (dev1)


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] skge: request IRQ on activating the interface
  2009-09-22 12:01 [PATCH] skge: request IRQ on activating the interface Michal Schmidt
@ 2009-09-22 16:28 ` Stephen Hemminger
  2009-10-01 10:27   ` [PATCH] skge: use unique IRQ name Michal Schmidt
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Hemminger @ 2009-09-22 16:28 UTC (permalink / raw)
  To: Michal Schmidt; +Cc: netdev

On Tue, 22 Sep 2009 14:01:31 +0200
Michal Schmidt <mschmidt@redhat.com> wrote:

> skge requests IRQ in its probe function. This causes a problem in
> the following real-life scenario with two different NICs in the machine:
> 
> 1. modprobe skge
>    The card is detected as eth0 and requests IRQ 17. Directory
>    /proc/irq/17/eth0 is created.
> 2. There is an udev rule which says this interface should be called
>    eth1, so udev renames eth0 -> eth1.
> 3. modprobe 8139too
>    The Realtek card is detected as eth0. It will be using IRQ 17 too.
> 4. ip link set eth0 up
>    Now 8139too requests IRQ 17.
> 
> The result is:
> WARNING: at fs/proc/generic.c:590 proc_register ...
> proc_dir_entry '17/eth0' already registered
> ...
> 
> And "ls /proc/irq/17" shows two subdirectories, both called eth0.
> 
> Fix it by requesting the IRQ in skge when the interface is activated.
> This works, because interfaces can be renamed only while they are down.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

No. because two port cards have a single IRQ for both ports.
The choice of ethX in irq name was done because irqbalance looks for this.
Probably better to change skge/sky2 and other devices with same issue
to use skge-N ... for request_irq, and teach irqbalance how to do deal
with it.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] skge: use unique IRQ name
  2009-09-22 16:28 ` Stephen Hemminger
@ 2009-10-01 10:27   ` Michal Schmidt
  2009-10-01 16:06     ` Stephen Hemminger
                       ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Michal Schmidt @ 2009-10-01 10:27 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev

Most network drivers request their IRQ when the interface is activated.
skge does it in ->probe() instead, because it can work with two-port
cards where the two net_devices use the same IRQ. This works fine most
of the time, except in some situations when the interface gets renamed.
Consider this example:

1. modprobe skge
   The card is detected as eth0 and requests IRQ 17. Directory
   /proc/irq/17/eth0 is created.
2. There is an udev rule which says this interface should be called
   eth1, so udev renames eth0 -> eth1.
3. modprobe 8139too
   The Realtek card is detected as eth0. It will be using IRQ 17 too.
4. ip link set eth0 up
   Now 8139too requests IRQ 17.

The result is:
WARNING: at fs/proc/generic.c:590 proc_register ...
proc_dir_entry '17/eth0' already registered
...
And "ls /proc/irq/17" shows two subdirectories, both called eth0.

Fix it by using a unique name for skge's IRQ, based on the PCI address.
The naming from the example then looks like this:
$ grep skge /proc/interrupts
 17:        169   IO-APIC-fasteoi   skge@0000:00:0a.0, eth0

irqbalance daemon will have to be taught to recognize "skge@" as an
Ethernet interrupt. This will be a one-liner addition in classify.c. I
will send a patch to irqbalance if this change is accepted.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Index: kernel/drivers/net/skge.c
===================================================================
--- kernel.orig/drivers/net/skge.c
+++ kernel/drivers/net/skge.c
@@ -3895,6 +3895,7 @@ static int __devinit skge_probe(struct p
 	struct net_device *dev, *dev1;
 	struct skge_hw *hw;
 	int err, using_dac = 0;
+	size_t irq_name_len;
 
 	err = pci_enable_device(pdev);
 	if (err) {
@@ -3935,11 +3936,13 @@ static int __devinit skge_probe(struct p
 #endif
 
 	err = -ENOMEM;
-	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
+	irq_name_len = strlen(DRV_NAME) + strlen(dev_name(&pdev->dev)) + 2;
+	hw = kzalloc(sizeof(*hw) + irq_name_len, GFP_KERNEL);
 	if (!hw) {
 		dev_err(&pdev->dev, "cannot allocate hardware struct\n");
 		goto err_out_free_regions;
 	}
+	sprintf(hw->irq_name, DRV_NAME "@%s", dev_name(&pdev->dev));
 
 	hw->pdev = pdev;
 	spin_lock_init(&hw->hw_lock);
@@ -3974,7 +3977,7 @@ static int __devinit skge_probe(struct p
 		goto err_out_free_netdev;
 	}
 
-	err = request_irq(pdev->irq, skge_intr, IRQF_SHARED, dev->name, hw);
+	err = request_irq(pdev->irq, skge_intr, IRQF_SHARED, hw->irq_name, hw);
 	if (err) {
 		dev_err(&pdev->dev, "%s: cannot assign irq %d\n",
 		       dev->name, pdev->irq);
Index: kernel/drivers/net/skge.h
===================================================================
--- kernel.orig/drivers/net/skge.h
+++ kernel/drivers/net/skge.h
@@ -2423,6 +2423,8 @@ struct skge_hw {
 	u16		     phy_addr;
 	spinlock_t	     phy_lock;
 	struct tasklet_struct phy_task;
+
+	char		     irq_name[0]; /* name for /proc/interrupts */
 };
 
 enum pause_control {

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] skge: use unique IRQ name
  2009-10-01 10:27   ` [PATCH] skge: use unique IRQ name Michal Schmidt
@ 2009-10-01 16:06     ` Stephen Hemminger
  2009-10-01 16:50     ` Stephen Hemminger
  2009-10-01 17:11     ` [PATCH] sky2: irqname based on pci address Stephen Hemminger
  2 siblings, 0 replies; 13+ messages in thread
From: Stephen Hemminger @ 2009-10-01 16:06 UTC (permalink / raw)
  To: Michal Schmidt; +Cc: netdev

On Thu, 1 Oct 2009 12:27:20 +0200
Michal Schmidt <mschmidt@redhat.com> wrote:

> Most network drivers request their IRQ when the interface is activated.
> skge does it in ->probe() instead, because it can work with two-port
> cards where the two net_devices use the same IRQ. This works fine most
> of the time, except in some situations when the interface gets renamed.
> Consider this example:
> 
> 1. modprobe skge
>    The card is detected as eth0 and requests IRQ 17. Directory
>    /proc/irq/17/eth0 is created.
> 2. There is an udev rule which says this interface should be called
>    eth1, so udev renames eth0 -> eth1.
> 3. modprobe 8139too
>    The Realtek card is detected as eth0. It will be using IRQ 17 too.
> 4. ip link set eth0 up
>    Now 8139too requests IRQ 17.
> 
> The result is:
> WARNING: at fs/proc/generic.c:590 proc_register ...
> proc_dir_entry '17/eth0' already registered
> ...
> And "ls /proc/irq/17" shows two subdirectories, both called eth0.
> 
> Fix it by using a unique name for skge's IRQ, based on the PCI address.
> The naming from the example then looks like this:
> $ grep skge /proc/interrupts
>  17:        169   IO-APIC-fasteoi   skge@0000:00:0a.0, eth0
> 
> irqbalance daemon will have to be taught to recognize "skge@" as an
> Ethernet interrupt. This will be a one-liner addition in classify.c. I
> will send a patch to irqbalance if this change is accepted.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> 
> Index: kernel/drivers/net/skge.c
> ===================================================================
> --- kernel.orig/drivers/net/skge.c
> +++ kernel/drivers/net/skge.c
> @@ -3895,6 +3895,7 @@ static int __devinit skge_probe(struct p
>  	struct net_device *dev, *dev1;
>  	struct skge_hw *hw;
>  	int err, using_dac = 0;
> +	size_t irq_name_len;
>  
>  	err = pci_enable_device(pdev);
>  	if (err) {
> @@ -3935,11 +3936,13 @@ static int __devinit skge_probe(struct p
>  #endif
>  
>  	err = -ENOMEM;
> -	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> +	irq_name_len = strlen(DRV_NAME) + strlen(dev_name(&pdev->dev)) + 2;
> +	hw = kzalloc(sizeof(*hw) + irq_name_len, GFP_KERNEL);
>  	if (!hw) {
>  		dev_err(&pdev->dev, "cannot allocate hardware struct\n");
>  		goto err_out_free_regions;
>  	}
> +	sprintf(hw->irq_name, DRV_NAME "@%s", dev_name(&pdev->dev));

I like this with one small change. Please use:
         skge@pci:0000:00:02.0
This makes the driver follow same format as existing DRM graphics drivers.
Michal could you follow up with additional patches for:
   1. sky2 driver has same issue
   2. irqbalance has a list of special drivers that needs to be updated



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] skge: use unique IRQ name
  2009-10-01 10:27   ` [PATCH] skge: use unique IRQ name Michal Schmidt
  2009-10-01 16:06     ` Stephen Hemminger
@ 2009-10-01 16:50     ` Stephen Hemminger
  2009-10-01 18:02       ` Michal Schmidt
  2009-10-01 17:11     ` [PATCH] sky2: irqname based on pci address Stephen Hemminger
  2 siblings, 1 reply; 13+ messages in thread
From: Stephen Hemminger @ 2009-10-01 16:50 UTC (permalink / raw)
  To: Michal Schmidt, David Miller; +Cc: netdev

(revised to use pci:)

From: Michal Schmidt <mschmidt@redhat.com>

Most network drivers request their IRQ when the interface is activated.
skge does it in ->probe() instead, because it can work with two-port
cards where the two net_devices use the same IRQ. This works fine most
of the time, except in some situations when the interface gets renamed.
Consider this example:

1. modprobe skge
   The card is detected as eth0 and requests IRQ 17. Directory
   /proc/irq/17/eth0 is created.
2. There is an udev rule which says this interface should be called
   eth1, so udev renames eth0 -> eth1.
3. modprobe 8139too
   The Realtek card is detected as eth0. It will be using IRQ 17 too.
4. ip link set eth0 up
   Now 8139too requests IRQ 17.

The result is:
WARNING: at fs/proc/generic.c:590 proc_register ...
proc_dir_entry '17/eth0' already registered
...
And "ls /proc/irq/17" shows two subdirectories, both called eth0.

Fix it by using a unique name for skge's IRQ, based on the PCI address.
The naming from the example then looks like this:
$ grep skge /proc/interrupts
 17:        169   IO-APIC-fasteoi   skge@pci:0000:00:0a.0, eth0

irqbalance daemon will have to be taught to recognize "skge" as an
Ethernet interrupt. This will be a one-liner addition in classify.c. I
will send a patch to irqbalance if this change is accepted.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Acked-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/skge.c	2009-10-01 09:36:21.893095582 -0700
+++ b/drivers/net/skge.c	2009-10-01 09:46:22.064555679 -0700
@@ -3895,6 +3895,7 @@ static int __devinit skge_probe(struct p
 	struct net_device *dev, *dev1;
 	struct skge_hw *hw;
 	int err, using_dac = 0;
+	size_t irq_name_len;
 
 	err = pci_enable_device(pdev);
 	if (err) {
@@ -3935,11 +3936,14 @@ static int __devinit skge_probe(struct p
 #endif
 
 	err = -ENOMEM;
-	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
+	/* space for skge@pci:0000:04:00.0 */
+	irq_name_len = strlen(DRV_NAME) + strlen(dev_name(&pdev->dev)) + 6;
+	hw = kzalloc(sizeof(*hw) + irq_name_len, GFP_KERNEL);
 	if (!hw) {
 		dev_err(&pdev->dev, "cannot allocate hardware struct\n");
 		goto err_out_free_regions;
 	}
+	sprintf(hw->irq_name, DRV_NAME "@pci:%s", pci_name(pdev));
 
 	hw->pdev = pdev;
 	spin_lock_init(&hw->hw_lock);
@@ -3974,7 +3978,7 @@ static int __devinit skge_probe(struct p
 		goto err_out_free_netdev;
 	}
 
-	err = request_irq(pdev->irq, skge_intr, IRQF_SHARED, dev->name, hw);
+	err = request_irq(pdev->irq, skge_intr, IRQF_SHARED, hw->irq_name, hw);
 	if (err) {
 		dev_err(&pdev->dev, "%s: cannot assign irq %d\n",
 		       dev->name, pdev->irq);
--- a/drivers/net/skge.h	2009-10-01 09:34:51.036505545 -0700
+++ b/drivers/net/skge.h	2009-10-01 09:47:38.096558002 -0700
@@ -2423,6 +2423,8 @@ struct skge_hw {
 	u16		     phy_addr;
 	spinlock_t	     phy_lock;
 	struct tasklet_struct phy_task;
+
+	char		     irq_name[0]; /* skge@pci:000:04:00.0 */
 };
 
 enum pause_control {

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] sky2: irqname based on pci address
  2009-10-01 10:27   ` [PATCH] skge: use unique IRQ name Michal Schmidt
  2009-10-01 16:06     ` Stephen Hemminger
  2009-10-01 16:50     ` Stephen Hemminger
@ 2009-10-01 17:11     ` Stephen Hemminger
  2009-10-01 17:14       ` Stephen Hemminger
  2009-10-01 18:03       ` Michal Schmidt
  2 siblings, 2 replies; 13+ messages in thread
From: Stephen Hemminger @ 2009-10-01 17:11 UTC (permalink / raw)
  To: Michal Schmidt, David Miller; +Cc: netdev

This is based on Michal Schmidt fix for skge.

Most network drivers request their IRQ when the interface is activated.
sky2 does it in ->probe() instead, because it can work with two-port
cards where the two net_devices use the same IRQ. This works fine most
of the time, except in some situations when the interface gets renamed.
Consider this example:

1. modprobe sky2
   The card is detected as eth0 and requests IRQ 17. Directory
   /proc/irq/17/eth0 is created.
2. There is an udev rule which says this interface should be called
   eth1, so udev renames eth0 -> eth1.
3. modprobe 8139too
   The Realtek card is detected as eth0. It will be using IRQ 17 too.
4. ip link set eth0 up
   Now 8139too requests IRQ 17.

The result is:
WARNING: at fs/proc/generic.c:590 proc_register ...
proc_dir_entry '17/eth0' already registered

The fix is for sky2 to name the irq based on the pci device, as is done
by some other devices DRM, infiniband, ...  ie. sky2@pci:0000:00:00

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


--- a/drivers/net/sky2.c	2009-10-01 09:51:30.604556725 -0700
+++ b/drivers/net/sky2.c	2009-10-01 09:56:38.893342161 -0700
@@ -4487,13 +4487,16 @@ static int __devinit sky2_probe(struct p
 	wol_default = device_may_wakeup(&pdev->dev) ? WAKE_MAGIC : 0;
 
 	err = -ENOMEM;
-	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
+
+	hw = kzalloc(sizeof(*hw) + strlen(DRV_NAME "@pci:")
+		     + strlen(pci_name(pdev)) + 1, GFP_KERNEL);
 	if (!hw) {
 		dev_err(&pdev->dev, "cannot allocate hardware struct\n");
 		goto err_out_free_regions;
 	}
 
 	hw->pdev = pdev;
+	sprintf(hw->irq_name, DRV_NAME "@pci:%s", pci_name(pdev));
 
 	hw->regs = ioremap_nocache(pci_resource_start(pdev, 0), 0x4000);
 	if (!hw->regs) {
@@ -4539,7 +4542,7 @@ static int __devinit sky2_probe(struct p
 
 	err = request_irq(pdev->irq, sky2_intr,
 			  (hw->flags & SKY2_HW_USE_MSI) ? 0 : IRQF_SHARED,
-			  dev->name, hw);
+			  hw->irq_name, hw);
 	if (err) {
 		dev_err(&pdev->dev, "cannot assign irq %d\n", pdev->irq);
 		goto err_out_unregister;
--- a/drivers/net/sky2.h	2009-10-01 09:51:17.553559116 -0700
+++ b/drivers/net/sky2.h	2009-10-01 09:51:42.069510492 -0700
@@ -2085,6 +2085,8 @@ struct sky2_hw {
 	struct timer_list    watchdog_timer;
 	struct work_struct   restart_work;
 	wait_queue_head_t    msi_wait;
+
+	char		     irq_name[0];
 };
 
 static inline int sky2_is_copper(const struct sky2_hw *hw)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] sky2: irqname based on pci address
  2009-10-01 17:11     ` [PATCH] sky2: irqname based on pci address Stephen Hemminger
@ 2009-10-01 17:14       ` Stephen Hemminger
  2009-10-01 18:03       ` Michal Schmidt
  1 sibling, 0 replies; 13+ messages in thread
From: Stephen Hemminger @ 2009-10-01 17:14 UTC (permalink / raw)
  To: Michal Schmidt; +Cc: David Miller, netdev

On Thu, 1 Oct 2009 10:11:46 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> This is based on Michal Schmidt fix for skge.
> 
> Most network drivers request their IRQ when the interface is activated.
> sky2 does it in ->probe() instead, because it can work with two-port
> cards where the two net_devices use the same IRQ. This works fine most
> of the time, except in some situations when the interface gets renamed.
> Consider this example:
> 
> 1. modprobe sky2
>    The card is detected as eth0 and requests IRQ 17. Directory
>    /proc/irq/17/eth0 is created.
> 2. There is an udev rule which says this interface should be called
>    eth1, so udev renames eth0 -> eth1.
> 3. modprobe 8139too
>    The Realtek card is detected as eth0. It will be using IRQ 17 too.
> 4. ip link set eth0 up
>    Now 8139too requests IRQ 17.

One other note, the issue is less of a problem for most usage of sky2
because the drive is used mostly on systems that support MSI interrupts
which can never be shared.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] skge: use unique IRQ name
  2009-10-01 16:50     ` Stephen Hemminger
@ 2009-10-01 18:02       ` Michal Schmidt
  2009-10-01 18:13         ` [PATCH v3] " Stephen Hemminger
  0 siblings, 1 reply; 13+ messages in thread
From: Michal Schmidt @ 2009-10-01 18:02 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Miller, netdev

Dne Thu, 1 Oct 2009 09:50:32 -0700 Stephen Hemminger napsal:
>  	err = -ENOMEM;
> -	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> +	/* space for skge@pci:0000:04:00.0 */
> +	irq_name_len = strlen(DRV_NAME) +
> strlen(dev_name(&pdev->dev)) + 6;

You replaced "dev_name(&pdev->dev)" with "pci_name(pdev)" below.
That's nice, so we should replace it here too for consistency.

> +	hw = kzalloc(sizeof(*hw) + irq_name_len, GFP_KERNEL);
>  	if (!hw) {
>  		dev_err(&pdev->dev, "cannot allocate hardware
> struct\n"); goto err_out_free_regions;
>  	}
> +	sprintf(hw->irq_name, DRV_NAME "@pci:%s", pci_name(pdev));

Michal

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] sky2: irqname based on pci address
  2009-10-01 17:11     ` [PATCH] sky2: irqname based on pci address Stephen Hemminger
  2009-10-01 17:14       ` Stephen Hemminger
@ 2009-10-01 18:03       ` Michal Schmidt
  2009-10-01 22:17         ` David Miller
  1 sibling, 1 reply; 13+ messages in thread
From: Michal Schmidt @ 2009-10-01 18:03 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Miller, netdev

Dne Thu, 1 Oct 2009 10:11:46 -0700 Stephen Hemminger napsal(a):
> This is based on Michal Schmidt fix for skge.
> 
> Most network drivers request their IRQ when the interface is
> activated. sky2 does it in ->probe() instead, because it can work
> with two-port cards where the two net_devices use the same IRQ. This
> works fine most of the time, except in some situations when the
> interface gets renamed. Consider this example:
> 
> 1. modprobe sky2
>    The card is detected as eth0 and requests IRQ 17. Directory
>    /proc/irq/17/eth0 is created.
> 2. There is an udev rule which says this interface should be called
>    eth1, so udev renames eth0 -> eth1.
> 3. modprobe 8139too
>    The Realtek card is detected as eth0. It will be using IRQ 17 too.
> 4. ip link set eth0 up
>    Now 8139too requests IRQ 17.
> 
> The result is:
> WARNING: at fs/proc/generic.c:590 proc_register ...
> proc_dir_entry '17/eth0' already registered
> 
> The fix is for sky2 to name the irq based on the pci device, as is
> done by some other devices DRM, infiniband, ...  ie.
> sky2@pci:0000:00:00
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Reviewed-by: Michal Schmidt <mschmidt@redhat.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3] skge: use unique IRQ name
  2009-10-01 18:02       ` Michal Schmidt
@ 2009-10-01 18:13         ` Stephen Hemminger
  2009-10-01 20:31           ` David Miller
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Hemminger @ 2009-10-01 18:13 UTC (permalink / raw)
  To: Michal Schmidt; +Cc: David Miller, netdev


From: Michal Schmidt <mschmidt@redhat.com>

Most network drivers request their IRQ when the interface is activated.
skge does it in ->probe() instead, because it can work with two-port
cards where the two net_devices use the same IRQ. This works fine most
of the time, except in some situations when the interface gets renamed.
Consider this example:

1. modprobe skge
   The card is detected as eth0 and requests IRQ 17. Directory
   /proc/irq/17/eth0 is created.
2. There is an udev rule which says this interface should be called
   eth1, so udev renames eth0 -> eth1.
3. modprobe 8139too
   The Realtek card is detected as eth0. It will be using IRQ 17 too.
4. ip link set eth0 up
   Now 8139too requests IRQ 17.

The result is:
WARNING: at fs/proc/generic.c:590 proc_register ...
proc_dir_entry '17/eth0' already registered
...
And "ls /proc/irq/17" shows two subdirectories, both called eth0.

Fix it by using a unique name for skge's IRQ, based on the PCI address.
The naming from the example then looks like this:
$ grep skge /proc/interrupts
 17:        169   IO-APIC-fasteoi   skge@0000:00:0a.0, eth0

irqbalance daemon will have to be taught to recognize "skge@" as an
Ethernet interrupt. This will be a one-liner addition in classify.c. I
will send a patch to irqbalance if this change is accepted.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Acked-by: Stephen Hemminger <shemminger@vyatta.com>

---
 Changes:
   v2 use pci: in irq name
   v3 simplify calculation of string length

--- a/drivers/net/skge.c	2009-10-01 11:09:19.057676199 -0700
+++ b/drivers/net/skge.c	2009-10-01 11:10:45.786382284 -0700
@@ -3935,11 +3935,14 @@ static int __devinit skge_probe(struct p
 #endif
 
 	err = -ENOMEM;
-	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
+	/* space for skge@pci:0000:04:00.0 */
+	hw = kzalloc(sizeof(*hw) + strlen(DRV_NAME "@pci:" )
+		     + strlen(pci_name(pdev)) + 1, GFP_KERNEL);
 	if (!hw) {
 		dev_err(&pdev->dev, "cannot allocate hardware struct\n");
 		goto err_out_free_regions;
 	}
+	sprintf(hw->irq_name, DRV_NAME "@pci:%s", pci_name(pdev));
 
 	hw->pdev = pdev;
 	spin_lock_init(&hw->hw_lock);
@@ -3974,7 +3977,7 @@ static int __devinit skge_probe(struct p
 		goto err_out_free_netdev;
 	}
 
-	err = request_irq(pdev->irq, skge_intr, IRQF_SHARED, dev->name, hw);
+	err = request_irq(pdev->irq, skge_intr, IRQF_SHARED, hw->irq_name, hw);
 	if (err) {
 		dev_err(&pdev->dev, "%s: cannot assign irq %d\n",
 		       dev->name, pdev->irq);
--- a/drivers/net/skge.h	2009-10-01 11:09:19.067695070 -0700
+++ b/drivers/net/skge.h	2009-10-01 11:09:34.147674089 -0700
@@ -2423,6 +2423,8 @@ struct skge_hw {
 	u16		     phy_addr;
 	spinlock_t	     phy_lock;
 	struct tasklet_struct phy_task;
+
+	char		     irq_name[0]; /* skge@pci:000:04:00.0 */
 };
 
 enum pause_control {

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] skge: use unique IRQ name
  2009-10-01 18:13         ` [PATCH v3] " Stephen Hemminger
@ 2009-10-01 20:31           ` David Miller
  2009-10-01 22:17             ` David Miller
  0 siblings, 1 reply; 13+ messages in thread
From: David Miller @ 2009-10-01 20:31 UTC (permalink / raw)
  To: shemminger; +Cc: mschmidt, netdev

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 1 Oct 2009 11:13:23 -0700

> $ grep skge /proc/interrupts
>  17:        169   IO-APIC-fasteoi   skge@0000:00:0a.0, eth0

Please fix this example output to match the code :-)

> +	sprintf(hw->irq_name, DRV_NAME "@pci:%s", pci_name(pdev));

Thanks!

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] sky2: irqname based on pci address
  2009-10-01 18:03       ` Michal Schmidt
@ 2009-10-01 22:17         ` David Miller
  0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2009-10-01 22:17 UTC (permalink / raw)
  To: mschmidt; +Cc: shemminger, netdev

From: Michal Schmidt <mschmidt@redhat.com>
Date: Thu, 1 Oct 2009 20:03:55 +0200

> Dne Thu, 1 Oct 2009 10:11:46 -0700 Stephen Hemminger napsal(a):
>> This is based on Michal Schmidt fix for skge.
>> 
>> Most network drivers request their IRQ when the interface is
>> activated. sky2 does it in ->probe() instead, because it can work
>> with two-port cards where the two net_devices use the same IRQ. This
>> works fine most of the time, except in some situations when the
>> interface gets renamed. Consider this example:
 ...
>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> Reviewed-by: Michal Schmidt <mschmidt@redhat.com>

Applied.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] skge: use unique IRQ name
  2009-10-01 20:31           ` David Miller
@ 2009-10-01 22:17             ` David Miller
  0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2009-10-01 22:17 UTC (permalink / raw)
  To: shemminger; +Cc: mschmidt, netdev

From: David Miller <davem@davemloft.net>
Date: Thu, 01 Oct 2009 13:31:55 -0700 (PDT)

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Thu, 1 Oct 2009 11:13:23 -0700
> 
>> $ grep skge /proc/interrupts
>>  17:        169   IO-APIC-fasteoi   skge@0000:00:0a.0, eth0
> 
> Please fix this example output to match the code :-)
> 
>> +	sprintf(hw->irq_name, DRV_NAME "@pci:%s", pci_name(pdev));
> 
> Thanks!

Nevermind, I took care of this when applying your patch.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2009-10-01 22:17 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-22 12:01 [PATCH] skge: request IRQ on activating the interface Michal Schmidt
2009-09-22 16:28 ` Stephen Hemminger
2009-10-01 10:27   ` [PATCH] skge: use unique IRQ name Michal Schmidt
2009-10-01 16:06     ` Stephen Hemminger
2009-10-01 16:50     ` Stephen Hemminger
2009-10-01 18:02       ` Michal Schmidt
2009-10-01 18:13         ` [PATCH v3] " Stephen Hemminger
2009-10-01 20:31           ` David Miller
2009-10-01 22:17             ` David Miller
2009-10-01 17:11     ` [PATCH] sky2: irqname based on pci address Stephen Hemminger
2009-10-01 17:14       ` Stephen Hemminger
2009-10-01 18:03       ` Michal Schmidt
2009-10-01 22:17         ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.