Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Zwane Mwaikambo]

On Sat, 25 Sep 2004, Zwane Mwaikambo wrote:

> Kenji Kaneshige wrote:
> 
> >     - Changed acpi_pci_irq_disable() to leave 'dev->irq' as it
> >       is. Clearing 'dev->irq' by some magic constant
> >       (e.g. PCI_UNDEFINED_IRQ) is TBD.
> 
> This may not be safe with CONFIG_PCI_MSI, you may want to verify against 
> that if you already haven't.
> 
> > +acpi_unregister_gsi (unsigned int irq)
> > +{
> > +}
> > +EXPORT_SYMBOL(acpi_unregister_gsi);
> 
> Why not just make these static inlines in header files? Since you're on 
> this, how about making irq_desc and friends dynamic too?

Sorry, i broke Cc.

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Kenji Kaneshige]

Zwane Mwaikambo wrote:
> On Sat, 25 Sep 2004, Zwane Mwaikambo wrote:
> 
>> Kenji Kaneshige wrote:
>> 
>> >     - Changed acpi_pci_irq_disable() to leave 'dev->irq' as it
>> >       is. Clearing 'dev->irq' by some magic constant
>> >       (e.g. PCI_UNDEFINED_IRQ) is TBD.
>> 
>> This may not be safe with CONFIG_PCI_MSI, you may want to verify against 
>> that if you already haven't.
>> 

Thank you for commemts.
You are right. If the linux IRQ number is allocated by MSI code,
clearing 'dev->irq' would cause a problem. So we need to consider
clearing 'dev->irq' carefully.

The latest patch doesn't clear 'dev->irq' so far.

>> > +acpi_unregister_gsi (unsigned int irq)
>> > +{
>> > +}
>> > +EXPORT_SYMBOL(acpi_unregister_gsi);
>> 
>> Why not just make these static inlines in header files? Since you're on 
>> this, how about making irq_desc and friends dynamic too?
> 
> Sorry, i broke Cc.
> 

I'm not quite sure what you are saying, but my idea is defining
acpi_unregister_gsi() as a opposite part of acpi_register_gsi().
Acpi_register_gsi() is defined for each arch (i386, ia64), so
acpi_unregister_gsi() is defined for each i386 and ia64 too. 

Thanks,
Kenji Kaneshige

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Zwane Mwaikambo]

On Mon, 27 Sep 2004, Kenji Kaneshige wrote:

> > > Why not just make these static inlines in header files? Since you're on
> > > this, how about making irq_desc and friends dynamic too?
> 
> I'm not quite sure what you are saying, but my idea is defining
> acpi_unregister_gsi() as a opposite part of acpi_register_gsi().
> Acpi_register_gsi() is defined for each arch (i386, ia64), so
> acpi_unregister_gsi() is defined for each i386 and ia64 too. 

Well i meant can't you define them in a header file as follows;

static void inline acpi_unregister_gsi (unsigned int irq)
{
}

An advantage is that it saves memory since you don't also have to create 
the extra data objects for the exported symbol. But really you don't have 
to export something which does nothing.

Thanks,
	Zwane

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Kenji Kaneshige]

Zwane Mwaikambo wrote:
> On Mon, 27 Sep 2004, Kenji Kaneshige wrote:
> 
>> > > Why not just make these static inlines in header files? Since you're on
>> > > this, how about making irq_desc and friends dynamic too?
>> 
>> I'm not quite sure what you are saying, but my idea is defining
>> acpi_unregister_gsi() as a opposite part of acpi_register_gsi().
>> Acpi_register_gsi() is defined for each arch (i386, ia64), so
>> acpi_unregister_gsi() is defined for each i386 and ia64 too. 
> 
> Well i meant can't you define them in a header file as follows;
> 
> static void inline acpi_unregister_gsi (unsigned int irq)
> {
> }
> 
> An advantage is that it saves memory since you don't also have to create 
> the extra data objects for the exported symbol. But really you don't have 
> to export something which does nothing.
> 
Hi Zwane,

I understand what you mean. It looks good to me.
I'll update my patch.

Thanks,
Kenji Kaneshige

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Kenji Kaneshige]

Hi Zwane,

I'm trying to update my patch based on the feedback from you. Updated
patch defines acpi_unregister_gsi() in both include/asm-i386/acpi.h
and include/asm-ia64/acpi.h. But now I'm having one concern about it.

Some arch specific functions would be called from acpi_unregister_gsi()
when it is implemented. But include/asm-xxx/acpi.h is included before
many other header files, so many 'implicit declaration of function xxx'
warning message would be appeared. These warning messages are disappeared
if we declare all functions called by acpi_unregister_gsi() also in
include/asm-xxx/acpi.h. But I don't like this approach very much.

After all, now I think it is better not to define acpi_unregister_gsi()
in header files.

How do you think?

Thanks,
Kenji Kaneshige

Zwane Mwaikambo wrote:

> On Mon, 27 Sep 2004, Kenji Kaneshige wrote:
> 
>> > > Why not just make these static inlines in header files? Since you're on
>> > > this, how about making irq_desc and friends dynamic too?
>> 
>> I'm not quite sure what you are saying, but my idea is defining
>> acpi_unregister_gsi() as a opposite part of acpi_register_gsi().
>> Acpi_register_gsi() is defined for each arch (i386, ia64), so
>> acpi_unregister_gsi() is defined for each i386 and ia64 too. 
> 
> Well i meant can't you define them in a header file as follows;
> 
> static void inline acpi_unregister_gsi (unsigned int irq)
> {
> }
> 
> An advantage is that it saves memory since you don't also have to create 
> the extra data objects for the exported symbol. But really you don't have 
> to export something which does nothing.
> 
> Thanks,
> 	Zwane
> 

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Zwane Mwaikambo]

On Wed, 29 Sep 2004, Kenji Kaneshige wrote:

> I'm trying to update my patch based on the feedback from you. Updated
> patch defines acpi_unregister_gsi() in both include/asm-i386/acpi.h
> and include/asm-ia64/acpi.h. But now I'm having one concern about it.
> 
> Some arch specific functions would be called from acpi_unregister_gsi()
> when it is implemented. But include/asm-xxx/acpi.h is included before
> many other header files, so many 'implicit declaration of function xxx'
> warning message would be appeared. These warning messages are disappeared
> if we declare all functions called by acpi_unregister_gsi() also in
> include/asm-xxx/acpi.h. But I don't like this approach very much.
> 
> After all, now I think it is better not to define acpi_unregister_gsi()
> in header files.

Ok i think i may have not conveyed my meaning properly, my mistake. What i 
think would be better is if the architectures which have no-op 
acpi_unregister_gsi to declare them as static inline in header files. For 
architectures (such as ia64) which have a functional acpi_unregister_gsi, 
we can declare them in a .c file with the proper exports etc.

Thanks Kenji and sorry for the confusion.
	Zwane

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Kenji Kaneshige]

Zwane Mwaikambo wrote:
> 
> Ok i think i may have not conveyed my meaning properly, my mistake. What i 
> think would be better is if the architectures which have no-op 
> acpi_unregister_gsi to declare them as static inline in header files. For 
> architectures (such as ia64) which have a functional acpi_unregister_gsi, 
> we can declare them in a .c file with the proper exports etc.
> 

Now I (maybe) properly understand what you mean :-). But I still have one
concern about your idea.

For architectures which have a functional acpi_unregister_gsi, we need to
declare "extern void acpi_unregister_gsi(int gsi);" in include/linux/acpi.h
that is common to all architectures. I think include/linux/acpi.h is the
best place to declare it because acpi_register_gsi(), opposite portion of
acpi_unregister_gsi(), is declared in it. On the other hand, for archtectures
that have no-op acpi_unregister_gsi(), acpi_unregister_gsi() is defined as
static inline function in arch specific header files. This looks not natural
to me.

How do you think?

Thanks,
Kenji Kaneshige

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Zwane Mwaikambo]

On Thu, 30 Sep 2004, Kenji Kaneshige wrote:

> Zwane Mwaikambo wrote:
> > 
> > Ok i think i may have not conveyed my meaning properly, my mistake. What i
> > think would be better is if the architectures which have no-op
> > acpi_unregister_gsi to declare them as static inline in header files. For
> > architectures (such as ia64) which have a functional acpi_unregister_gsi, we
> > can declare them in a .c file with the proper exports etc.
> > 
> 
> Now I (maybe) properly understand what you mean :-). But I still have one
> concern about your idea.
> 
> For architectures which have a functional acpi_unregister_gsi, we need to
> declare "extern void acpi_unregister_gsi(int gsi);" in include/linux/acpi.h
> that is common to all architectures. I think include/linux/acpi.h is the
> best place to declare it because acpi_register_gsi(), opposite portion of
> acpi_unregister_gsi(), is declared in it. On the other hand, for archtectures
> that have no-op acpi_unregister_gsi(), acpi_unregister_gsi() is defined as
> static inline function in arch specific header files. This looks not natural
> to me.

Can't you declare "extern void acpi_unregister_gsi(int gsi)" in 
include/asm/acpi.h? That way it stays arch specific and you don't have the 
conflicting declarations. You can also move acpi_unregister_gsi into arch 
specific headers too.

Thanks,
	Zwane

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Kenji Kaneshige]

Zwane Mwaikambo wrote:

> 
> Can't you declare "extern void acpi_unregister_gsi(int gsi)" in 
> include/asm/acpi.h? That way it stays arch specific and you don't have the 
> conflicting declarations. You can also move acpi_unregister_gsi into arch 
> specific headers too.
> 

OK. I'll update my patch based on your feedback.

Thanks,
Kenji Kaneshige

[PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Kenji Kaneshige]

This is a patch for ACPI code that has no dependencies.
Len, please apply.

----
Change Log:

    - Fixed some typos in comments.

    - Changed 'unsigned char irq_disabled' to 'unsigned int
      irq_disabled' because pci_dev.irq is unsigned int.

----
Name:		IRQ_deallocation_acpi.patch
Kernel Version:	2.6.9-rc2-mm1
Depends:	none
Description:

This patch is ACPI portion of IRQ deallocation. This patch defines the
following new interface. The implementation of this interface depends
on each platform.

    o void acpi_unregister_gsi(int irq)

        This is a opposite portion of acpi_register_gsi(). This has a
        responsibility for deallocating IRQ resources associated with
        the specified linux IRQ number.

        We need to consider the case of shared interrupt. In the case
        of shared interrupt, acpi_register_gsi() is called multiple
        times for one gsi. That is, registrations and unregistrations
        can be nested.

        This function undoes the effect of one call to
        acpi_register_gsi(). If this matches the last registration,
        IRQ resources associated with the specified linux IRQ number
        are freed.

This patch also adds the following new function.

    o void acpi_pci_irq_disable (struct pci_dev *dev)

        This function is a opposite portion of
        acpi_pci_enable_irq(). It clears the device's linux IRQ number
        and calls acpi_unregister_gsi() to deallocate IRQ resources.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji-+CUm20s59erQFUHtdCDX3A@public.gmane.org>


---

 linux-2.6.9-rc2-mm1-kanesige/arch/i386/kernel/acpi/boot.c |   11 +++++
 linux-2.6.9-rc2-mm1-kanesige/arch/ia64/kernel/acpi.c      |   11 +++++
 linux-2.6.9-rc2-mm1-kanesige/drivers/acpi/pci_irq.c       |   27 ++++++++++++++
 linux-2.6.9-rc2-mm1-kanesige/include/linux/acpi.h         |    2 +
 4 files changed, 51 insertions(+)

diff -puN arch/i386/kernel/acpi/boot.c~IRQ_deallocation_acpi arch/i386/kernel/acpi/boot.c
--- linux-2.6.9-rc2-mm1/arch/i386/kernel/acpi/boot.c~IRQ_deallocation_acpi	2004-09-22 20:29:23.000000000 +0900
+++ linux-2.6.9-rc2-mm1-kanesige/arch/i386/kernel/acpi/boot.c	2004-09-22 20:30:41.000000000 +0900
@@ -480,6 +480,17 @@ unsigned int acpi_register_gsi(u32 gsi, 
 }
 EXPORT_SYMBOL(acpi_register_gsi);
 
+/*
+ * This function undoes the effect of one call to acpi_register_gsi().
+ * If this matches the last registration, any IRQ resources for gsi
+ * associated with the irq are freed.
+ */
+void
+acpi_unregister_gsi (unsigned int irq)
+{
+}
+EXPORT_SYMBOL(acpi_unregister_gsi);
+
 static unsigned long __init
 acpi_scan_rsdp (
 	unsigned long		start,
diff -puN arch/ia64/kernel/acpi.c~IRQ_deallocation_acpi arch/ia64/kernel/acpi.c
--- linux-2.6.9-rc2-mm1/arch/ia64/kernel/acpi.c~IRQ_deallocation_acpi	2004-09-22 20:29:23.000000000 +0900
+++ linux-2.6.9-rc2-mm1-kanesige/arch/ia64/kernel/acpi.c	2004-09-24 13:57:16.780496484 +0900
@@ -516,6 +516,17 @@ acpi_register_gsi (u32 gsi, int edge_lev
 }
 EXPORT_SYMBOL(acpi_register_gsi);
 
+/*
+ * This function undoes the effect of one call to acpi_register_gsi().
+ * If this matches the last registration, any IRQ resources for gsi
+ * associated with the irq are freed.
+ */
+void
+acpi_unregister_gsi (unsigned int irq)
+{
+}
+EXPORT_SYMBOL(acpi_unregister_gsi);
+
 static int __init
 acpi_parse_fadt (unsigned long phys_addr, unsigned long size)
 {
diff -puN drivers/acpi/pci_irq.c~IRQ_deallocation_acpi drivers/acpi/pci_irq.c
--- linux-2.6.9-rc2-mm1/drivers/acpi/pci_irq.c~IRQ_deallocation_acpi	2004-09-22 20:29:23.000000000 +0900
+++ linux-2.6.9-rc2-mm1-kanesige/drivers/acpi/pci_irq.c	2004-09-22 20:29:23.000000000 +0900
@@ -390,3 +390,30 @@ acpi_pci_irq_enable (
 
 	return_VALUE(dev->irq);
 }
+
+void
+acpi_pci_irq_disable (
+	struct pci_dev		*dev)
+{
+	unsigned int		irq_disabled;
+	unsigned char		irq;
+
+	ACPI_FUNCTION_TRACE("acpi_pci_irq_disable");
+
+	irq_disabled = dev->irq;
+
+	/*
+	 * dev->irq is cleared by BIOS-assigned IRQ set during boot.
+	 */
+	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
+	if (irq)
+		pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
+	dev->irq = irq;
+
+	printk(KERN_INFO PREFIX "PCI interrupt for device %s disabled\n",
+	       pci_name(dev));
+
+	acpi_unregister_gsi(irq_disabled);
+
+	return_VOID;
+}
diff -puN include/linux/acpi.h~IRQ_deallocation_acpi include/linux/acpi.h
--- linux-2.6.9-rc2-mm1/include/linux/acpi.h~IRQ_deallocation_acpi	2004-09-22 20:29:23.000000000 +0900
+++ linux-2.6.9-rc2-mm1-kanesige/include/linux/acpi.h	2004-09-22 20:29:23.000000000 +0900
@@ -414,6 +414,7 @@ static inline int acpi_boot_init(void)
 #endif 	/*!CONFIG_ACPI_BOOT*/
 
 unsigned int acpi_register_gsi (u32 gsi, int edge_level, int active_high_low);
+void acpi_unregister_gsi (unsigned int);
 int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);
 
 #ifdef CONFIG_ACPI_PCI
@@ -439,6 +440,7 @@ extern struct acpi_prt_list	acpi_prt;
 struct pci_dev;
 
 int acpi_pci_irq_enable (struct pci_dev *dev);
+void acpi_pci_irq_disable (struct pci_dev *dev);
 
 struct acpi_pci_driver {
 	struct acpi_pci_driver *next;

_



-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php

Re: [ACPI] [PATCH] Updated patches for PCI IRQ resource deallocation support [2/3]

[Kenji Kaneshige]

Len,

I've updated ACPI portion of PCI IRQ resource deallocation patch
again based on the feedback from Takayoshi Kochi. Please apply
the following patch instead of the patch I sent before.

Thanks,
Kenji Kaneshige

----
Change Log:

    - Changed acpi_pci_irq_disable() to leave 'dev->irq' as it
      is. Clearing 'dev->irq' by some magic constant
      (e.g. PCI_UNDEFINED_IRQ) is TBD.

Change Log:

    - Fixed some typos in comments.

    - Changed 'unsigned char irq_disabled' to 'unsigned int
      irq_disabled' because pci_dev.irq is unsigned int.

----
Name:		IRQ_deallocation_acpi.patch
Kernel Version:	2.6.9-rc2-mm1
Depends:	none
Description:

This patch is ACPI portion of IRQ deallocation. This patch defines the
following new interface. The implementation of this interface depends
on each platform.

    o void acpi_unregister_gsi(int irq)

        This is a opposite portion of acpi_register_gsi(). This has a
        responsibility for deallocating IRQ resources associated with
        the specified linux IRQ number.

        We need to consider the case of shared interrupt. In the case
        of shared interrupt, acpi_register_gsi() is called multiple
        times for one gsi. That is, registrations and unregistrations
        can be nested.

        This function undoes the effect of one call to
        acpi_register_gsi(). If this matches the last registration,
        IRQ resources associated with the specified linux IRQ number
        are freed.

This patch also adds the following new function.

    o void acpi_pci_irq_disable (struct pci_dev *dev)

        This function is a opposite portion of
        acpi_pci_enable_irq(). It clears the device's linux IRQ number
        and calls acpi_unregister_gsi() to deallocate IRQ resources.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>


---

 linux-2.6.9-rc2-mm1-kanesige/arch/i386/kernel/acpi/boot.c |   11 ++++++++
 linux-2.6.9-rc2-mm1-kanesige/arch/ia64/kernel/acpi.c      |   11 ++++++++
 linux-2.6.9-rc2-mm1-kanesige/drivers/acpi/pci_irq.c       |   19 ++++++++++++++
 linux-2.6.9-rc2-mm1-kanesige/include/linux/acpi.h         |    2 +
 4 files changed, 43 insertions(+)

diff -puN arch/i386/kernel/acpi/boot.c~IRQ_deallocation_acpi arch/i386/kernel/acpi/boot.c
--- linux-2.6.9-rc2-mm1/arch/i386/kernel/acpi/boot.c~IRQ_deallocation_acpi	2004-09-22 20:29:23.000000000 +0900
+++ linux-2.6.9-rc2-mm1-kanesige/arch/i386/kernel/acpi/boot.c	2004-09-22 20:30:41.000000000 +0900
@@ -480,6 +480,17 @@ unsigned int acpi_register_gsi(u32 gsi, 
 }
 EXPORT_SYMBOL(acpi_register_gsi);
 
+/*
+ * This function undoes the effect of one call to acpi_register_gsi().
+ * If this matches the last registration, any IRQ resources for gsi
+ * associated with the irq are freed.
+ */
+void
+acpi_unregister_gsi (unsigned int irq)
+{
+}
+EXPORT_SYMBOL(acpi_unregister_gsi);
+
 static unsigned long __init
 acpi_scan_rsdp (
 	unsigned long		start,
diff -puN arch/ia64/kernel/acpi.c~IRQ_deallocation_acpi arch/ia64/kernel/acpi.c
--- linux-2.6.9-rc2-mm1/arch/ia64/kernel/acpi.c~IRQ_deallocation_acpi	2004-09-22 20:29:23.000000000 +0900
+++ linux-2.6.9-rc2-mm1-kanesige/arch/ia64/kernel/acpi.c	2004-09-24 15:33:01.254399900 +0900
@@ -516,6 +516,17 @@ acpi_register_gsi (u32 gsi, int edge_lev
 }
 EXPORT_SYMBOL(acpi_register_gsi);
 
+/*
+ * This function undoes the effect of one call to acpi_register_gsi().
+ * If this matches the last registration, any IRQ resources for gsi
+ * associated with the irq are freed.
+ */
+void
+acpi_unregister_gsi (unsigned int irq)
+{
+}
+EXPORT_SYMBOL(acpi_unregister_gsi);
+
 static int __init
 acpi_parse_fadt (unsigned long phys_addr, unsigned long size)
 {
diff -puN drivers/acpi/pci_irq.c~IRQ_deallocation_acpi drivers/acpi/pci_irq.c
--- linux-2.6.9-rc2-mm1/drivers/acpi/pci_irq.c~IRQ_deallocation_acpi	2004-09-22 20:29:23.000000000 +0900
+++ linux-2.6.9-rc2-mm1-kanesige/drivers/acpi/pci_irq.c	2004-09-24 15:46:01.666952803 +0900
@@ -390,3 +390,22 @@ acpi_pci_irq_enable (
 
 	return_VALUE(dev->irq);
 }
+
+void
+acpi_pci_irq_disable (
+	struct pci_dev		*dev)
+{
+	ACPI_FUNCTION_TRACE("acpi_pci_irq_disable");
+
+	/*
+	 * TBD: It might be worth clearing dev->irq by magic constant
+	 * (e.g. PCI_UNDEFINED_IRQ).
+	 */
+
+	printk(KERN_INFO PREFIX "PCI interrupt for device %s disabled\n",
+	       pci_name(dev));
+
+	acpi_unregister_gsi(dev->irq);
+
+	return_VOID;
+}
diff -puN include/linux/acpi.h~IRQ_deallocation_acpi include/linux/acpi.h
--- linux-2.6.9-rc2-mm1/include/linux/acpi.h~IRQ_deallocation_acpi	2004-09-22 20:29:23.000000000 +0900
+++ linux-2.6.9-rc2-mm1-kanesige/include/linux/acpi.h	2004-09-22 20:29:23.000000000 +0900
@@ -414,6 +414,7 @@ static inline int acpi_boot_init(void)
 #endif 	/*!CONFIG_ACPI_BOOT*/
 
 unsigned int acpi_register_gsi (u32 gsi, int edge_level, int active_high_low);
+void acpi_unregister_gsi (unsigned int);
 int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);
 
 #ifdef CONFIG_ACPI_PCI
@@ -439,6 +440,7 @@ extern struct acpi_prt_list	acpi_prt;
 struct pci_dev;
 
 int acpi_pci_irq_enable (struct pci_dev *dev);
+void acpi_pci_irq_disable (struct pci_dev *dev);
 
 struct acpi_pci_driver {
 	struct acpi_pci_driver *next;

_