* [PATCH] set_rte() should get iosapic_lock
@ 2004-04-12 12:36 Kenji Kaneshige
2004-04-13 9:07 ` Liu, Benjamin
2004-04-13 10:30 ` Kenji Kaneshige
0 siblings, 2 replies; 3+ messages in thread
From: Kenji Kaneshige @ 2004-04-12 12:36 UTC (permalink / raw)
To: linux-ia64
[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]
Hi,
Currently set_rte() changes RTE without iosapic_lock held. I guess it
assumes to be called only at the boot time. But set_rte() can be
called by PCI driver not only at the boot time. So I think set_rte()
should get iosapic_lock.
pci_enable_device(drivers/pci/pci.c)
|
+-> pci_enable_device_bars(drivers/pci/pci.c)
|
+-> pcibios_enable_device(arch/ia64/pci/pci.c)
|
+-> acpi_pci_irq_enable (drivers/acpi/pci_irq.c)
|
+-> iosapic_enable_intr (arch/ia64/kernel/iosapic.c)
|
+-> set_rte (arch_ia64/kernel/iosapic.c)
A following patch fixes this issue. I'm also attaching this patch
because my mailer replace all tabs with blanks.
Thanks,
Kenji Kaneshige
diff -Naur linux-2.6.5/arch/ia64/kernel/iosapic.c
linux-2.6.5-changed/arch/ia64/kernel/iosapic.c
--- linux-2.6.5/arch/ia64/kernel/iosapic.c 2004-04-04 12:37:06.000000000
+0900
+++ linux-2.6.5-changed/arch/ia64/kernel/iosapic.c 2004-04-12
21:08:48.491220447 +0900
@@ -172,7 +172,7 @@
static void
set_rte (unsigned int vector, unsigned int dest, int mask)
{
- unsigned long pol, trigger, dmode;
+ unsigned long pol, trigger, dmode, flags;
u32 low32, high32;
char *addr;
int rte_index;
@@ -211,11 +211,15 @@
/* dest contains both id and eid */
high32 = (dest << IOSAPIC_DEST_SHIFT);
- writel(IOSAPIC_RTE_HIGH(rte_index), addr + IOSAPIC_REG_SELECT);
- writel(high32, addr + IOSAPIC_WINDOW);
- writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
- writel(low32, addr + IOSAPIC_WINDOW);
- iosapic_intr_info[vector].low32 = low32;
+ spin_lock_irqsave(&iosapic_lock, flags);
+ {
+ writel(IOSAPIC_RTE_HIGH(rte_index), addr + IOSAPIC_REG_SELECT);
+ writel(high32, addr + IOSAPIC_WINDOW);
+ writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
+ writel(low32, addr + IOSAPIC_WINDOW);
+ iosapic_intr_info[vector].low32 = low32;
+ }
+ spin_unlock_irqrestore(&iosapic_lock, flags);
}
static void
[-- Attachment #2: fix_set_rte.patch --]
[-- Type: application/octet-stream, Size: 1260 bytes --]
diff -Naur linux-2.6.5/arch/ia64/kernel/iosapic.c linux-2.6.5-changed/arch/ia64/kernel/iosapic.c
--- linux-2.6.5/arch/ia64/kernel/iosapic.c 2004-04-04 12:37:06.000000000 +0900
+++ linux-2.6.5-changed/arch/ia64/kernel/iosapic.c 2004-04-12 21:08:48.491220447 +0900
@@ -172,7 +172,7 @@
static void
set_rte (unsigned int vector, unsigned int dest, int mask)
{
- unsigned long pol, trigger, dmode;
+ unsigned long pol, trigger, dmode, flags;
u32 low32, high32;
char *addr;
int rte_index;
@@ -211,11 +211,15 @@
/* dest contains both id and eid */
high32 = (dest << IOSAPIC_DEST_SHIFT);
- writel(IOSAPIC_RTE_HIGH(rte_index), addr + IOSAPIC_REG_SELECT);
- writel(high32, addr + IOSAPIC_WINDOW);
- writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
- writel(low32, addr + IOSAPIC_WINDOW);
- iosapic_intr_info[vector].low32 = low32;
+ spin_lock_irqsave(&iosapic_lock, flags);
+ {
+ writel(IOSAPIC_RTE_HIGH(rte_index), addr + IOSAPIC_REG_SELECT);
+ writel(high32, addr + IOSAPIC_WINDOW);
+ writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
+ writel(low32, addr + IOSAPIC_WINDOW);
+ iosapic_intr_info[vector].low32 = low32;
+ }
+ spin_unlock_irqrestore(&iosapic_lock, flags);
}
static void
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [PATCH] set_rte() should get iosapic_lock
2004-04-12 12:36 [PATCH] set_rte() should get iosapic_lock Kenji Kaneshige
@ 2004-04-13 9:07 ` Liu, Benjamin
2004-04-13 10:30 ` Kenji Kaneshige
1 sibling, 0 replies; 3+ messages in thread
From: Liu, Benjamin @ 2004-04-13 9:07 UTC (permalink / raw)
To: linux-ia64
Hi, Kenji.
Lock is used to avoid racing during resource contention. It seems that your patch is just for 2.6.5 which incorporates Bjorn's patch for ACPI IRQ cleanup.
Here iosapic_lock is used in
1. mask_irq() to protect I/O SAPIC config register write(REG SELECT, and RTE's lower word), iosapic_intr_info read(iosapic_intr_info[vec].low32).
2. unmask_irq() to protect I/O SAPIC config register write(REG SELECT, and RTE's lower word), iosapic_intr_info read(iosapic_intr_info[vec].low32).
3. iosapic_set_affinity() to protect I/O SAPIC config register write(REG SELECT, and RTE's lower/higher words), iosapic_intr_info write(iosapic_intr_info[vec].low32).
And in set_rte(), your patch intends to protect I/O SAPIC config register write(REG SELECT, and RTE's lower/higher words), iosapic_intr_info write(iosapic_intr_info[vec].low32). Bjorn's patch has defered RTE setting until device driver initialization. So device module loading and "echo 0xff >/proc/irq/xxx/smp_affinity" will cause racing, if without the lock in set_rte().
My question is whether it need to be applied to other kernel versions(before ACPI IRQ cleanup patch)?
Thanks,
Pingping (Benjamin) Liu
>-----Original Message-----
>From: linux-ia64-owner@vger.kernel.org
>[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Kenji Kaneshige
>Sent: 2004Äê4ÔÂ12ÈÕ 20:37
>To: linux-ia64@vger.kernel.org
>Subject: [PATCH] set_rte() should get iosapic_lock
>
>Hi,
>
>Currently set_rte() changes RTE without iosapic_lock held. I guess it
>assumes to be called only at the boot time. But set_rte() can be
>called by PCI driver not only at the boot time. So I think set_rte()
>should get iosapic_lock.
>
>pci_enable_device(drivers/pci/pci.c)
>|
>+-> pci_enable_device_bars(drivers/pci/pci.c)
> |
> +-> pcibios_enable_device(arch/ia64/pci/pci.c)
> |
> +-> acpi_pci_irq_enable (drivers/acpi/pci_irq.c)
> |
> +-> iosapic_enable_intr (arch/ia64/kernel/iosapic.c)
> |
> +-> set_rte (arch_ia64/kernel/iosapic.c)
>
>A following patch fixes this issue. I'm also attaching this patch
>because my mailer replace all tabs with blanks.
>
>Thanks,
>Kenji Kaneshige
>
>diff -Naur linux-2.6.5/arch/ia64/kernel/iosapic.c
>linux-2.6.5-changed/arch/ia64/kernel/iosapic.c
>--- linux-2.6.5/arch/ia64/kernel/iosapic.c 2004-04-04
>12:37:06.000000000
>+0900
>+++ linux-2.6.5-changed/arch/ia64/kernel/iosapic.c 2004-04-12
>21:08:48.491220447 +0900
>@@ -172,7 +172,7 @@
> static void
> set_rte (unsigned int vector, unsigned int dest, int mask)
> {
>- unsigned long pol, trigger, dmode;
>+ unsigned long pol, trigger, dmode, flags;
> u32 low32, high32;
> char *addr;
> int rte_index;
>@@ -211,11 +211,15 @@
> /* dest contains both id and eid */
> high32 = (dest << IOSAPIC_DEST_SHIFT);
>
>- writel(IOSAPIC_RTE_HIGH(rte_index), addr + IOSAPIC_REG_SELECT);
>- writel(high32, addr + IOSAPIC_WINDOW);
>- writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
>- writel(low32, addr + IOSAPIC_WINDOW);
>- iosapic_intr_info[vector].low32 = low32;
>+ spin_lock_irqsave(&iosapic_lock, flags);
>+ {
>+ writel(IOSAPIC_RTE_HIGH(rte_index), addr +
>IOSAPIC_REG_SELECT);
>+ writel(high32, addr + IOSAPIC_WINDOW);
>+ writel(IOSAPIC_RTE_LOW(rte_index), addr +
>IOSAPIC_REG_SELECT);
>+ writel(low32, addr + IOSAPIC_WINDOW);
>+ iosapic_intr_info[vector].low32 = low32;
>+ }
>+ spin_unlock_irqrestore(&iosapic_lock, flags);
> }
>
> static void
>
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [PATCH] set_rte() should get iosapic_lock
2004-04-12 12:36 [PATCH] set_rte() should get iosapic_lock Kenji Kaneshige
2004-04-13 9:07 ` Liu, Benjamin
@ 2004-04-13 10:30 ` Kenji Kaneshige
1 sibling, 0 replies; 3+ messages in thread
From: Kenji Kaneshige @ 2004-04-13 10:30 UTC (permalink / raw)
To: linux-ia64
Hi, Benjamin
Thank you for your comments.
> My question is whether it need to be applied to other kernel
> versions(before ACPI IRQ cleanup patch)?
I think it need to be applied to other kernel version because many
PCI device drivers call pci_enable_device() and it is also called by
the PCI hotplug drivers directly or indirectly.
And of course, I believe my patch works with Bjorn's patch.
Thanks,
Kenji Kaneshige
> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org
> [mailto:linux-ia64-owner@vger.kernel.org]On Behalf Of Liu, Benjamin
> Sent: Tuesday, April 13, 2004 6:07 PM
> To: Kenji Kaneshige; linux-ia64@vger.kernel.org
> Subject: RE: [PATCH] set_rte() should get iosapic_lock
>
>
> Hi, Kenji.
>
> Lock is used to avoid racing during resource contention. It seems
> that your patch is just for 2.6.5 which incorporates Bjorn's
> patch for ACPI IRQ cleanup.
>
> Here iosapic_lock is used in
> 1. mask_irq() to protect I/O SAPIC config register write(REG
> SELECT, and RTE's lower word), iosapic_intr_info
> read(iosapic_intr_info[vec].low32).
> 2. unmask_irq() to protect I/O SAPIC config register write(REG
> SELECT, and RTE's lower word), iosapic_intr_info
> read(iosapic_intr_info[vec].low32).
> 3. iosapic_set_affinity() to protect I/O SAPIC config register
> write(REG SELECT, and RTE's lower/higher words),
> iosapic_intr_info write(iosapic_intr_info[vec].low32).
>
> And in set_rte(), your patch intends to protect I/O SAPIC config
> register write(REG SELECT, and RTE's lower/higher words),
> iosapic_intr_info write(iosapic_intr_info[vec].low32). Bjorn's
> patch has defered RTE setting until device driver initialization.
> So device module loading and "echo 0xff
> >/proc/irq/xxx/smp_affinity" will cause racing, if without the
> lock in set_rte().
>
> My question is whether it need to be applied to other kernel
> versions(before ACPI IRQ cleanup patch)?
>
> Thanks,
> Pingping (Benjamin) Liu
>
>
> >-----Original Message-----
> >From: linux-ia64-owner@vger.kernel.org
> >[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Kenji Kaneshige
> >Sent: 2004Äê4ÔÂ12ÈÕ 20:37
> >To: linux-ia64@vger.kernel.org
> >Subject: [PATCH] set_rte() should get iosapic_lock
> >
> >Hi,
> >
> >Currently set_rte() changes RTE without iosapic_lock held. I guess it
> >assumes to be called only at the boot time. But set_rte() can be
> >called by PCI driver not only at the boot time. So I think set_rte()
> >should get iosapic_lock.
> >
> >pci_enable_device(drivers/pci/pci.c)
> >|
> >+-> pci_enable_device_bars(drivers/pci/pci.c)
> > |
> > +-> pcibios_enable_device(arch/ia64/pci/pci.c)
> > |
> > +-> acpi_pci_irq_enable (drivers/acpi/pci_irq.c)
> > |
> > +-> iosapic_enable_intr (arch/ia64/kernel/iosapic.c)
> > |
> > +-> set_rte (arch_ia64/kernel/iosapic.c)
> >
> >A following patch fixes this issue. I'm also attaching this patch
> >because my mailer replace all tabs with blanks.
> >
> >Thanks,
> >Kenji Kaneshige
> >
> >diff -Naur linux-2.6.5/arch/ia64/kernel/iosapic.c
> >linux-2.6.5-changed/arch/ia64/kernel/iosapic.c
> >--- linux-2.6.5/arch/ia64/kernel/iosapic.c 2004-04-04
> >12:37:06.000000000
> >+0900
> >+++ linux-2.6.5-changed/arch/ia64/kernel/iosapic.c 2004-04-12
> >21:08:48.491220447 +0900
> >@@ -172,7 +172,7 @@
> > static void
> > set_rte (unsigned int vector, unsigned int dest, int mask)
> > {
> >- unsigned long pol, trigger, dmode;
> >+ unsigned long pol, trigger, dmode, flags;
> > u32 low32, high32;
> > char *addr;
> > int rte_index;
> >@@ -211,11 +211,15 @@
> > /* dest contains both id and eid */
> > high32 = (dest << IOSAPIC_DEST_SHIFT);
> >
> >- writel(IOSAPIC_RTE_HIGH(rte_index), addr + IOSAPIC_REG_SELECT);
> >- writel(high32, addr + IOSAPIC_WINDOW);
> >- writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
> >- writel(low32, addr + IOSAPIC_WINDOW);
> >- iosapic_intr_info[vector].low32 = low32;
> >+ spin_lock_irqsave(&iosapic_lock, flags);
> >+ {
> >+ writel(IOSAPIC_RTE_HIGH(rte_index), addr +
> >IOSAPIC_REG_SELECT);
> >+ writel(high32, addr + IOSAPIC_WINDOW);
> >+ writel(IOSAPIC_RTE_LOW(rte_index), addr +
> >IOSAPIC_REG_SELECT);
> >+ writel(low32, addr + IOSAPIC_WINDOW);
> >+ iosapic_intr_info[vector].low32 = low32;
> >+ }
> >+ spin_unlock_irqrestore(&iosapic_lock, flags);
> > }
> >
> > static void
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2004-04-13 10:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-04-12 12:36 [PATCH] set_rte() should get iosapic_lock Kenji Kaneshige
2004-04-13 9:07 ` Liu, Benjamin
2004-04-13 10:30 ` Kenji Kaneshige
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox