RE: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

RE: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Yu, Luming]

My comments:

1. In acpi_gpe_block_add, 

+	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+			acpi_gpe_block_crs_add, &block);
+	if (block.register_count == 0)
+		return 0;

1.1  Shouldn't it return AE_ERROR.
1.2  You assume there always have IRQ resource for the gpe blocks.

2. In acpi_gpe_block_remove,
+	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+			acpi_gpe_block_crs_remove, &fadt);
+	if (fadt)
+		return AE_OK;

2.1 fadt seems always to be 0, So why you need it?
2.2 How to free IRQ resource? You just free them at acpi_os_terminate.

3. In acpi_gpe_block_crs_addr
+	data->address.register_bit_width = ACPI_GPE_REGISTER_WIDTH;
+	data->address.register_bit_offset = ACPI_GPE_REGISTER_WIDTH;
+	data->address.address = addr->min_address_range;
+	data->register_count = size / 2;

3.1 you should specify address.address_space_id
3.2  address.reserved must be 0. (You allocate it at stack)
3.3 Why you think register_bit_offset is ACPI_GPE_REGISTER_WIDTH.
3.3 Why you think register_count should be size/2 ? 


Thanks
Luming 

>-----Original Message-----
>From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org 
>[mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of 
>Matthew Wilcox
>Sent: 2004年11月11日 23:08
>To: Brown, Len
>Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Subject: [ACPI] Re: [PATCH] Teach OSL to handle multiple 
>interrupts [1/2]
>
>
>Patches sent after this one have been applied.  Is this patch 
>still in review?
>
>On Sun, Nov 07, 2004 at 02:47:54PM +0000, Matthew Wilcox wrote:
>> 
>> OSL assumed that there would only be one ACPI IRQ.  In the 
>presence of
>> GPE blocks, there are multiple ACPI IRQs.
>> 
>> Index: linux-2.6/drivers/acpi/osl.c
>> ===================================================================
>> RCS file: /var/cvs/linux-2.6/drivers/acpi/osl.c,v
>> retrieving revision 1.13
>> diff -u -p -r1.13 osl.c
>> --- linux-2.6/drivers/acpi/osl.c	11 Oct 2004 21:41:01 
>-0000	1.13
>> +++ linux-2.6/drivers/acpi/osl.c	7 Nov 2004 14:34:33 -0000
>> @@ -66,9 +66,6 @@ int acpi_in_debugger;
>>  extern char line_buf[80];
>>  #endif /*ENABLE_DEBUGGER*/
>>  
>> -static unsigned int acpi_irq_irq;
>> -static acpi_osd_handler acpi_irq_handler;
>> -static void *acpi_irq_context;
>>  static struct workqueue_struct *kacpid_wq;
>>  
>>  acpi_status
>> @@ -96,13 +93,12 @@ acpi_os_initialize1(void)
>>  	return AE_OK;
>>  }
>>  
>> +static void acpi_os_remove_interrupt_handlers(void);
>> +
>>  acpi_status
>>  acpi_os_terminate(void)
>>  {
>> -	if (acpi_irq_handler) {
>> -		acpi_os_remove_interrupt_handler(acpi_irq_irq,
>> -						 acpi_irq_handler);
>> -	}
>> +	acpi_os_remove_interrupt_handlers();
>>  
>>  	destroy_workqueue(kacpid_wq);
>>  
>> @@ -255,52 +251,129 @@ acpi_os_table_override (struct acpi_tabl
>>  	return AE_OK;
>>  }
>>  
>> +struct acpi_dev_id {
>> +	acpi_osd_handler handler;
>> +	void *context;
>> +	int irq;
>> +	struct acpi_dev_id *next;
>> +};
>> +
>>  static irqreturn_t
>>  acpi_irq(int irq, void *dev_id, struct pt_regs *regs)
>>  {
>> -	return (*acpi_irq_handler)(acpi_irq_context) ? 
>IRQ_HANDLED : IRQ_NONE;
>> +	struct acpi_dev_id *irq_ctx = dev_id;
>> +	acpi_osd_handler handler = irq_ctx->handler;
>> +	void *context = irq_ctx->context;
>> +	return (*handler)(context) ? IRQ_HANDLED : IRQ_NONE;
>> +}
>> +
>> +struct acpi_dev_id *dev_id_list;
>> +
>> +static void *alloc_dev_id(acpi_osd_handler handler, void 
>*context, int irq)
>> +{
>> +	struct acpi_dev_id *irq_ctx = kmalloc(sizeof(*irq_ctx), 
>GFP_KERNEL);
>> +	irq_ctx->handler = handler;
>> +	irq_ctx->context = context;
>> +	irq_ctx->irq = irq;
>> +	irq_ctx->next = dev_id_list;
>> +	dev_id_list = irq_ctx;
>> +	return irq_ctx;
>> +}
>> +
>> +/*
>> + * XXX: Ideally, we would match the context too, but that 
>information
>> + * isn't passed to the acpi_os_remove_interrupt_handler() function.
>> + */
>> +static void *find_dev_id(int irq, acpi_osd_handler handler)
>> +{
>> +	struct acpi_dev_id *irq_ctx = dev_id_list;
>> +	for (irq_ctx = dev_id_list; irq_ctx; irq_ctx = irq_ctx->next) {
>> +		if ((irq_ctx->handler == handler) && 
>(irq_ctx->irq == irq))
>> +			return irq_ctx;
>> +	}
>> +	printk("find_dev_id: handler %p for irq %d not 
>found\n", handler, irq);
>> +	return NULL;
>> +}
>> +
>> +static void free_dev_id(struct acpi_dev_id *dev_id)
>> +{
>> +	struct acpi_dev_id **pprev = &dev_id_list;
>> +	for (;;) {
>> +		struct acpi_dev_id *irq_ctx = *pprev;
>> +		if (!irq_ctx)
>> +			break;
>> +		if (irq_ctx != dev_id) {
>> +			pprev = &irq_ctx->next;
>> +			continue;
>> +		}
>> +
>> +		*pprev = irq_ctx->next;
>> +		kfree(irq_ctx);
>> +		return;
>> +	}
>> +	printk("free_dev_id: dev_id %p not found\n", dev_id);
>>  }
>>  
>>  acpi_status
>>  acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler 
>handler, void *context)
>>  {
>>  	unsigned int irq;
>> +	void *dev_id;
>>  
>>  	/*
>> -	 * Ignore the GSI from the core, and use the value in 
>our copy of the
>> -	 * FADT. It may not be the same if an interrupt source 
>override exists
>> -	 * for the SCI.
>> +	 * If this is the FADT interrupt, ignore the GSI from 
>the core and
>> +	 * use the value in our copy of the FADT instead.  It 
>may not be the
>> +	 * same if an interrupt source override exists for the SCI.
>>  	 */
>> -	gsi = acpi_fadt.sci_int;
>> +	if (gsi == acpi_gbl_FADT->sci_int)
>> +		gsi = acpi_fadt.sci_int;
>> +
>>  	if (acpi_gsi_to_irq(gsi, &irq) < 0) {
>>  		printk(KERN_ERR PREFIX "SCI (ACPI GSI %d) not 
>registered\n",
>>  		       gsi);
>>  		return AE_OK;
>>  	}
>>  
>> -	acpi_irq_handler = handler;
>> -	acpi_irq_context = context;
>> -	if (request_irq(irq, acpi_irq, SA_SHIRQ, "acpi", acpi_irq)) {
>> +	dev_id = alloc_dev_id(handler, context, irq);
>> +	if (request_irq(irq, acpi_irq, SA_SHIRQ, "acpi", dev_id)) {
>>  		printk(KERN_ERR PREFIX "SCI (IRQ%d) allocation 
>failed\n", irq);
>> +		free_dev_id(dev_id);
>>  		return AE_NOT_ACQUIRED;
>>  	}
>> -	acpi_irq_irq = irq;
>>  
>>  	return AE_OK;
>>  }
>>  
>>  acpi_status
>> -acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
>> +acpi_os_remove_interrupt_handler(u32 gsi, acpi_osd_handler handler)
>>  {
>> -	if (irq) {
>> -		free_irq(irq, acpi_irq);
>> -		acpi_irq_handler = NULL;
>> -		acpi_irq_irq = 0;
>> +	int irq;
>> +	void *dev_id;
>> +
>> +	if (gsi == acpi_gbl_FADT->sci_int)
>> +		gsi = acpi_fadt.sci_int;
>> +
>> +	if (acpi_gsi_to_irq(gsi, &irq) < 0) {
>> +		printk(KERN_ERR PREFIX "SCI (ACPI GSI %d) not 
>registered\n",
>> +		       gsi);
>> +		return AE_OK;
>>  	}
>>  
>> +	dev_id = find_dev_id(irq, handler);
>> +	free_irq(irq, dev_id);
>> +	free_dev_id(dev_id);
>> +
>>  	return AE_OK;
>>  }
>>  
>> +static void acpi_os_remove_interrupt_handlers(void)
>> +{
>> +	while (dev_id_list) {
>> +		acpi_os_remove_interrupt_handler(dev_id_list->irq,
>> +				dev_id_list->handler);
>> +	}
>> +}
>> +
>>  /*
>>   * Running in interpreter thread context, safe to sleep
>>   */
>> 
>> -- 
>> "Next the statesmen will invent cheap lies, putting the blame upon 
>> the nation that is attacked, and every man will be glad of those
>> conscience-soothing falsities, and will diligently study 
>them, and refuse
>> to examine any refutations of them; and thus he will by and 
>by convince 
>> himself that the war is just, and will thank God for the 
>better sleep 
>> he enjoys after this process of grotesque self-deception." 
>-- Mark Twain
>
>-- 
>"Next the statesmen will invent cheap lies, putting the blame upon 
>the nation that is attacked, and every man will be glad of those
>conscience-soothing falsities, and will diligently study them, 
>and refuse
>to examine any refutations of them; and thus he will by and by 
>convince 
>himself that the war is just, and will thank God for the better sleep 
>he enjoys after this process of grotesque self-deception." -- 
>Mark Twain
>
>
>-------------------------------------------------------
>This SF.Net email is sponsored by:
>Sybase ASE Linux Express Edition - download now for FREE
>LinuxWorld Reader's Choice Award Winner for best database on Linux.
>http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click
>_______________________________________________
>Acpi-devel mailing list
>Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>https://lists.sourceforge.net/lists/listinfo/acpi-devel
>


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_idU88&alloc_id\x12065&op=click

Re: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Matthew Wilcox]

On Sat, Nov 13, 2004 at 01:28:54AM +0800, Yu, Luming wrote:
> My comments:

These all seem to relate to the gpe-block device patch, right?  You see no
problems with the OSL patch you actually quote, correct?

> 1. In acpi_gpe_block_add, 
> 
> +	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> +			acpi_gpe_block_crs_add, &block);
> +	if (block.register_count == 0)
> +		return 0;
> 
> 1.1  Shouldn't it return AE_ERROR.

I don't think it really matters.  The driver will attach to these devices,
but that doesn't do any harm.

> 1.2  You assume there always have IRQ resource for the gpe blocks.

No, I assume there are io or memory resources for these gpe blocks.  If
there aren't, it's pretty useless!

> 2. In acpi_gpe_block_remove,
> +	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> +			acpi_gpe_block_crs_remove, &fadt);
> +	if (fadt)
> +		return AE_OK;
> 
> 2.1 fadt seems always to be 0, So why you need it?

fadt is only set to 0 when _CRS resources exist.  ie for GPE blocks that
aren't described in the FADT.  We shouldn't unregister GPE blocks that're
described in the FADT because we didn't register them in the first place.

> 2.2 How to free IRQ resource? You just free them at acpi_os_terminate.

Huh?  acpi_remove_gpe_block() releases the IRQ resource.  I have actually
loaded and unloaded this driver ;-)

> 3. In acpi_gpe_block_crs_addr
> +	data->address.register_bit_width = ACPI_GPE_REGISTER_WIDTH;
> +	data->address.register_bit_offset = ACPI_GPE_REGISTER_WIDTH;
> +	data->address.address = addr->min_address_range;
> +	data->register_count = size / 2;
> 
> 3.1 you should specify address.address_space_id

I do that right above the section you quoted.

> 3.2  address.reserved must be 0. (You allocate it at stack)

Doesn't really matter.  It isn't used.  But I'll fix that.

> 3.3 Why you think register_bit_offset is ACPI_GPE_REGISTER_WIDTH.

Why do you think it isn't?  BTW, the current ACPICA ignores what values
get passed in for register_bit_width and register_bit_offset and hardcodes
ACPI_GPE_REGISTER_WIDTH instead.

> 3.3 Why you think register_count should be size/2 ? 

Because there's an enable and status byte for each event.  I basically
got the idea from:
                register_count0 = (u16) (acpi_gbl_FADT->gpe0_blk_len / 2);


BTW, I think your comments basically show why the callback interface
that ACPICA is based on sucks.  It forces too much information to be 
remembered between two different functions.  Much better to do it all
in one function.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Yu, Luming]

>On Sat, Nov 13, 2004 at 01:28:54AM +0800, Yu, Luming wrote:
>> My comments:
>
>These all seem to relate to the gpe-block device patch, right? 
> You see no
>problems with the OSL patch you actually quote, correct?
>

I'm sorry for not mention that, because the I looked over it , and
didn't find problem. I just re-checked it. Here are some comments:

In acpi_os_install_interrupt_handler:

+	 * If this is the FADT interrupt, ignore the GSI from the core
and
+	 * use the value in our copy of the FADT instead.  It may not be
the
+	 * same if an interrupt source override exists for the SCI.
 	 */
-	gsi = acpi_fadt.sci_int;
+	if (gsi == acpi_gbl_FADT->sci_int)
+		gsi = acpi_fadt.sci_int;

4.1: 

>> 1. In acpi_gpe_block_add, 
>> 
>> +	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
>> +			acpi_gpe_block_crs_add, &block);
>> +	if (block.register_count == 0)
>> +		return 0;
>> 
>> 1.1  Shouldn't it return AE_ERROR.
>
>I don't think it really matters.  The driver will attach to 
>these devices,
>but that doesn't do any harm.
>
>> 1.2  You assume there always have IRQ resource for the gpe blocks.
>
>No, I assume there are io or memory resources for these gpe blocks.  If
>there aren't, it's pretty useless!
>
>> 2. In acpi_gpe_block_remove,
>> +	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
>> +			acpi_gpe_block_crs_remove, &fadt);
>> +	if (fadt)
>> +		return AE_OK;
>> 
>> 2.1 fadt seems always to be 0, So why you need it?
>
>fadt is only set to 0 when _CRS resources exist.  ie for GPE 
>blocks that
>aren't described in the FADT.  We shouldn't unregister GPE 
>blocks that're
>described in the FADT because we didn't register them in the 
>first place.
>
>> 2.2 How to free IRQ resource? You just free them at 
>acpi_os_terminate.
>
>Huh?  acpi_remove_gpe_block() releases the IRQ resource.  I 
>have actually
>loaded and unloaded this driver ;-)
>
>> 3. In acpi_gpe_block_crs_addr
>> +	data->address.register_bit_width = ACPI_GPE_REGISTER_WIDTH;
>> +	data->address.register_bit_offset = ACPI_GPE_REGISTER_WIDTH;
>> +	data->address.address = addr->min_address_range;
>> +	data->register_count = size / 2;
>> 
>> 3.1 you should specify address.address_space_id
>
>I do that right above the section you quoted.
>
>> 3.2  address.reserved must be 0. (You allocate it at stack)
>
>Doesn't really matter.  It isn't used.  But I'll fix that.
>
>> 3.3 Why you think register_bit_offset is ACPI_GPE_REGISTER_WIDTH.
>
>Why do you think it isn't?  BTW, the current ACPICA ignores what values
>get passed in for register_bit_width and register_bit_offset 
>and hardcodes
>ACPI_GPE_REGISTER_WIDTH instead.
>
>> 3.3 Why you think register_count should be size/2 ? 
>
>Because there's an enable and status byte for each event.  I basically
>got the idea from:
>                register_count0 = (u16) 
>(acpi_gbl_FADT->gpe0_blk_len / 2);
>
>
>BTW, I think your comments basically show why the callback interface
>that ACPICA is based on sucks.  It forces too much information to be 
>remembered between two different functions.  Much better to do it all
>in one function.
>
>-- 
>"Next the statesmen will invent cheap lies, putting the blame upon 
>the nation that is attacked, and every man will be glad of those
>conscience-soothing falsities, and will diligently study them, 
>and refuse
>to examine any refutations of them; and thus he will by and by 
>convince 
>himself that the war is just, and will thank God for the better sleep 
>he enjoys after this process of grotesque self-deception." -- 
>Mark Twain
>


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Yu, Luming]

Sorry, Please revert this one. My mailer  sent out my draft mail instead of
completed one!  (because PPP connection broken).

I will rewrite it later. Sorry for confusing.


Thanks
Luming 

>-----Original Message-----
>From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org 
>[mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Yu, Luming
>Sent: 2004年11月17日 1:49
>To: Matthew Wilcox
>Cc: Brown, Len; Moore, Robert; Fu, Michael; 
>acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Subject: RE: [ACPI] Re: [PATCH] Teach OSL to handle multiple 
>interrupts [1/2]
>
>>On Sat, Nov 13, 2004 at 01:28:54AM +0800, Yu, Luming wrote:
>>> My comments:
>>
>>These all seem to relate to the gpe-block device patch, right? 
>> You see no
>>problems with the OSL patch you actually quote, correct?
>>
>
>I'm sorry for not mention that, because the I looked over it , and
>didn't find problem. I just re-checked it. Here are some comments:
>
>In acpi_os_install_interrupt_handler:
>
>+	 * If this is the FADT interrupt, ignore the GSI from the core
>and
>+	 * use the value in our copy of the FADT instead.  It may not be
>the
>+	 * same if an interrupt source override exists for the SCI.
> 	 */
>-	gsi = acpi_fadt.sci_int;
>+	if (gsi == acpi_gbl_FADT->sci_int)
>+		gsi = acpi_fadt.sci_int;
>
>4.1: 
>
>>> 1. In acpi_gpe_block_add, 
>>> 
>>> +	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
>>> +			acpi_gpe_block_crs_add, &block);
>>> +	if (block.register_count == 0)
>>> +		return 0;
>>> 
>>> 1.1  Shouldn't it return AE_ERROR.
>>
>>I don't think it really matters.  The driver will attach to 
>>these devices,
>>but that doesn't do any harm.
>>
>>> 1.2  You assume there always have IRQ resource for the gpe blocks.
>>
>>No, I assume there are io or memory resources for these gpe 
>blocks.  If
>>there aren't, it's pretty useless!
>>
>>> 2. In acpi_gpe_block_remove,
>>> +	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
>>> +			acpi_gpe_block_crs_remove, &fadt);
>>> +	if (fadt)
>>> +		return AE_OK;
>>> 
>>> 2.1 fadt seems always to be 0, So why you need it?
>>
>>fadt is only set to 0 when _CRS resources exist.  ie for GPE 
>>blocks that
>>aren't described in the FADT.  We shouldn't unregister GPE 
>>blocks that're
>>described in the FADT because we didn't register them in the 
>>first place.
>>
>>> 2.2 How to free IRQ resource? You just free them at 
>>acpi_os_terminate.
>>
>>Huh?  acpi_remove_gpe_block() releases the IRQ resource.  I 
>>have actually
>>loaded and unloaded this driver ;-)
>>
>>> 3. In acpi_gpe_block_crs_addr
>>> +	data->address.register_bit_width = ACPI_GPE_REGISTER_WIDTH;
>>> +	data->address.register_bit_offset = ACPI_GPE_REGISTER_WIDTH;
>>> +	data->address.address = addr->min_address_range;
>>> +	data->register_count = size / 2;
>>> 
>>> 3.1 you should specify address.address_space_id
>>
>>I do that right above the section you quoted.
>>
>>> 3.2  address.reserved must be 0. (You allocate it at stack)
>>
>>Doesn't really matter.  It isn't used.  But I'll fix that.
>>
>>> 3.3 Why you think register_bit_offset is ACPI_GPE_REGISTER_WIDTH.
>>
>>Why do you think it isn't?  BTW, the current ACPICA ignores 
>what values
>>get passed in for register_bit_width and register_bit_offset 
>>and hardcodes
>>ACPI_GPE_REGISTER_WIDTH instead.
>>
>>> 3.3 Why you think register_count should be size/2 ? 
>>
>>Because there's an enable and status byte for each event.  I basically
>>got the idea from:
>>                register_count0 = (u16) 
>>(acpi_gbl_FADT->gpe0_blk_len / 2);
>>
>>
>>BTW, I think your comments basically show why the callback interface
>>that ACPICA is based on sucks.  It forces too much information to be 
>>remembered between two different functions.  Much better to do it all
>>in one function.
>>
>>-- 
>>"Next the statesmen will invent cheap lies, putting the blame upon 
>>the nation that is attacked, and every man will be glad of those
>>conscience-soothing falsities, and will diligently study them, 
>>and refuse
>>to examine any refutations of them; and thus he will by and by 
>>convince 
>>himself that the war is just, and will thank God for the better sleep 
>>he enjoys after this process of grotesque self-deception." -- 
>>Mark Twain
>>
>
>
>-------------------------------------------------------
>This SF.Net email is sponsored by: InterSystems CACHE
>FREE OODBMS DOWNLOAD - A multidimensional database that combines
>robust object and relational technologies, making it a perfect match
>for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8
>_______________________________________________
>Acpi-devel mailing list
>Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>https://lists.sourceforge.net/lists/listinfo/acpi-devel
>


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Matthew Wilcox]

On Tue, Nov 16, 2004 at 03:22:18AM +0000, Matthew Wilcox wrote:
> > 1.1  Shouldn't it return AE_ERROR.
> 
> I don't think it really matters.  The driver will attach to these devices,
> but that doesn't do any harm.
> 
> > 2.1 fadt seems always to be 0, So why you need it?
> 
> fadt is only set to 0 when _CRS resources exist.  ie for GPE blocks that
> aren't described in the FADT.  We shouldn't unregister GPE blocks that're
> described in the FADT because we didn't register them in the first place.
> 
> > 3.2  address.reserved must be 0. (You allocate it at stack)
> 
> Doesn't really matter.  It isn't used.  But I'll fix that.

OK, here's an interdiff from the 0.2 to the 0.3 driver which fixes these
issues:

diff -u linux-2.6/drivers/acpi/gpe-block.c linux-2.6/drivers/acpi/gpe-block.c
--- linux-2.6/drivers/acpi/gpe-block.c  7 Nov 2004 14:34:33 -0000
+++ linux-2.6/drivers/acpi/gpe-block.c  16 Nov 2004 18:12:52 -0000
@@ -18,7 +18,7 @@
 
 MODULE_AUTHOR("Matthew Wilcox <willy-VXdhtT5mjnY@public.gmane.org>");
 MODULE_LICENSE("GPL");
-MODULE_VERSION("0.2");
+MODULE_VERSION("0.3");
 
 #define NAME "gpe-block"
 
@@ -83,9 +83,6 @@
 {
        struct acpi_resource_address64 addr;
        int size;
-       int *fadt = context;
-
-       *fadt = 0;
 
        if (res->id != ACPI_RSTYPE_ADDRESS16 &&
            res->id != ACPI_RSTYPE_ADDRESS32 &&
@@ -107,26 +104,24 @@
 {
        struct gpe_block block;
 
-       block.register_count = 0;
+       memset(&block, 0, sizeof(block));
        acpi_walk_resources(device->handle, METHOD_NAME__CRS,
                        acpi_gpe_block_crs_add, &block);
+
+       /* Decline to handle GPE blocks with no _CRS. */
        if (block.register_count == 0)
-               return 0;
+               return -ENODEV;
 
        acpi_install_gpe_block(device->handle, &block.address,
                        block.register_count, block.interrupt_level);
 
-       return AE_OK;
+       return 0;
 }
 
 static int acpi_gpe_block_remove(struct acpi_device *device, int type)
 {
-       int fadt = 1;
-
        acpi_walk_resources(device->handle, METHOD_NAME__CRS,
-                       acpi_gpe_block_crs_remove, &fadt);
-       if (fadt)
-               return AE_OK;
+                       acpi_gpe_block_crs_remove, NULL);
 
        return acpi_remove_gpe_block(device->handle);
 }

This driver still works, so if I don't get any more comments, I'll submit
the 0.3 driver for inclusion ...

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Yu, Luming]

My more comments :

1.	-static unsigned int acpi_irq_irq;
	-static acpi_osd_handler acpi_irq_handler;
	-static void *acpi_irq_context;

1.1  I don't think it is good idea to remove acpi_irq_irq, which just
for FADT sci. If you haven't add FADT stuff in the dev_id_list.
So , please consider add FADT GPE onto dev_id_list.

2.  	 +struct acpi_dev_id {
	+	acpi_osd_handler handler;
	+	void *context;
	+	int irq;
	+	struct acpi_dev_id *next;
	+};

2.1  Do you think a spin lock or semaphore is unnecessary here?

3.  acpi_os_install_interrupt_handler

+	 * If this is the FADT interrupt, ignore the GSI from the core and
+	 * use the value in our copy of the FADT instead.  It may not be the
+	 * same if an interrupt source override exists for the SCI.
 	 */
-	gsi = acpi_fadt.sci_int;
+	if (gsi == acpi_gbl_FADT->sci_int)
+		gsi = acpi_fadt.sci_int;

3.1  This part should be revised to handle gsi != acpi_gbl_FADT->sci_int case.
Because ACPI spec said:" You must have an interrupt source override entry for 
the IRQ mapped to the SCI interrupt if this IRQ is not identity mapped."
GPE device need to look for their own override recorde.

4. acpi_os_remove_interrupt_handler

+	if (gsi == acpi_gbl_FADT->sci_int)
+		gsi = acpi_fadt.sci_int;
+
+	if (acpi_gsi_to_irq(gsi, &irq) < 0) {
+		printk(KERN_ERR PREFIX "SCI (ACPI GSI %d) not registered\n",
+		       gsi);
+		return AE_OK;
 	}

4.1 same reason as 3.1


5. acpi_gpe_block_remove

5.1 If you check code acpi_ev_delete_gpe_xrupt, you will find that
if a GPE device's interrupt_level happen to equal to acpi_gbl_FADT->sci_int,
the cleanup will abort.



Thanks
Luming 

>-----Original Message-----
>From: willy-1walMZg8u8rXmaaqVzeoHQ@public.gmane.org [mailto:willy-1walMZg8u8rXmaaqVzeoHQ@public.gmane.org] 
>On Behalf Of Matthew Wilcox
>Sent: 2004年11月17日 2:47
>To: Matthew Wilcox
>Cc: Yu, Luming; Brown, Len; acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; 
>Fu, Michael
>Subject: Re: [ACPI] Re: [PATCH] Teach OSL to handle multiple 
>interrupts [1/2]
>
>On Tue, Nov 16, 2004 at 03:22:18AM +0000, Matthew Wilcox wrote:
>> > 1.1  Shouldn't it return AE_ERROR.
>> 
>> I don't think it really matters.  The driver will attach to 
>these devices,
>> but that doesn't do any harm.
>> 
>> > 2.1 fadt seems always to be 0, So why you need it?
>> 
>> fadt is only set to 0 when _CRS resources exist.  ie for GPE 
>blocks that
>> aren't described in the FADT.  We shouldn't unregister GPE 
>blocks that're
>> described in the FADT because we didn't register them in the 
>first place.
>> 
>> > 3.2  address.reserved must be 0. (You allocate it at stack)
>> 
>> Doesn't really matter.  It isn't used.  But I'll fix that.
>
>OK, here's an interdiff from the 0.2 to the 0.3 driver which 
>fixes these
>issues:
>
>diff -u linux-2.6/drivers/acpi/gpe-block.c 
>linux-2.6/drivers/acpi/gpe-block.c
>--- linux-2.6/drivers/acpi/gpe-block.c  7 Nov 2004 14:34:33 -0000
>+++ linux-2.6/drivers/acpi/gpe-block.c  16 Nov 2004 18:12:52 -0000
>@@ -18,7 +18,7 @@
> 
> MODULE_AUTHOR("Matthew Wilcox <willy-VXdhtT5mjnY@public.gmane.org>");
> MODULE_LICENSE("GPL");
>-MODULE_VERSION("0.2");
>+MODULE_VERSION("0.3");
> 
> #define NAME "gpe-block"
> 
>@@ -83,9 +83,6 @@
> {
>        struct acpi_resource_address64 addr;
>        int size;
>-       int *fadt = context;
>-
>-       *fadt = 0;
> 
>        if (res->id != ACPI_RSTYPE_ADDRESS16 &&
>            res->id != ACPI_RSTYPE_ADDRESS32 &&
>@@ -107,26 +104,24 @@
> {
>        struct gpe_block block;
> 
>-       block.register_count = 0;
>+       memset(&block, 0, sizeof(block));
>        acpi_walk_resources(device->handle, METHOD_NAME__CRS,
>                        acpi_gpe_block_crs_add, &block);
>+
>+       /* Decline to handle GPE blocks with no _CRS. */
>        if (block.register_count == 0)
>-               return 0;
>+               return -ENODEV;
> 
>        acpi_install_gpe_block(device->handle, &block.address,
>                        block.register_count, block.interrupt_level);
> 
>-       return AE_OK;
>+       return 0;
> }
> 
> static int acpi_gpe_block_remove(struct acpi_device *device, int type)
> {
>-       int fadt = 1;
>-
>        acpi_walk_resources(device->handle, METHOD_NAME__CRS,
>-                       acpi_gpe_block_crs_remove, &fadt);
>-       if (fadt)
>-               return AE_OK;
>+                       acpi_gpe_block_crs_remove, NULL);
> 
>        return acpi_remove_gpe_block(device->handle);
> }
>
>This driver still works, so if I don't get any more comments, 
>I'll submit
>the 0.3 driver for inclusion ...
>
>-- 
>"Next the statesmen will invent cheap lies, putting the blame upon 
>the nation that is attacked, and every man will be glad of those
>conscience-soothing falsities, and will diligently study them, 
>and refuse
>to examine any refutations of them; and thus he will by and by 
>convince 
>himself that the war is just, and will thank God for the better sleep 
>he enjoys after this process of grotesque self-deception." -- 
>Mark Twain
>


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Matthew Wilcox]

On Wed, Nov 17, 2004 at 10:27:58AM +0800, Yu, Luming wrote:
> 1.	-static unsigned int acpi_irq_irq;
> 	-static acpi_osd_handler acpi_irq_handler;
> 	-static void *acpi_irq_context;
> 
> 1.1  I don't think it is good idea to remove acpi_irq_irq, which just
> for FADT sci. If you haven't add FADT stuff in the dev_id_list.
> So , please consider add FADT GPE onto dev_id_list.

The FADT GPE blocks are handled separately, see
drivers/acpi/events/evsci.c and drivers/acpi/events/evgpeblk.c
However, they still call acpi_os_install_interrupt_handler() and
acpi_os_remove_interrupt_handler().

> 2.  	 +struct acpi_dev_id {
> 	+	acpi_osd_handler handler;
> 	+	void *context;
> 	+	int irq;
> 	+	struct acpi_dev_id *next;
> 	+};
> 
> 2.1  Do you think a spin lock or semaphore is unnecessary here?

I was wondering about that.  The question is whether two
processors can access dev_id_list at the same time.  Unfortunately, I
think it _is_ possible, so I'll send an updated patch to fix this issue.

> 3.  acpi_os_install_interrupt_handler
> 
> +	 * If this is the FADT interrupt, ignore the GSI from the core and
> +	 * use the value in our copy of the FADT instead.  It may not be the
> +	 * same if an interrupt source override exists for the SCI.
>  	 */
> -	gsi = acpi_fadt.sci_int;
> +	if (gsi == acpi_gbl_FADT->sci_int)
> +		gsi = acpi_fadt.sci_int;
> 
> 3.1  This part should be revised to handle gsi != acpi_gbl_FADT->sci_int case.
> Because ACPI spec said:" You must have an interrupt source override entry for 
> the IRQ mapped to the SCI interrupt if this IRQ is not identity mapped."
> GPE device need to look for their own override recorde.

This is something I don't understand very well.  Why is there an override?
Is this just for buggy BIOSes?  Why don't we override the acpi_gbl_FADT
sci_int?  Why do we have two copies of the FADT anyway?  Why does the core
use the pristine copy of the FADT when it might be wrong?  Why would we
need an override for GPE block interrupts?

> 4. acpi_os_remove_interrupt_handler
> 
> +	if (gsi == acpi_gbl_FADT->sci_int)
> +		gsi = acpi_fadt.sci_int;
> +
> +	if (acpi_gsi_to_irq(gsi, &irq) < 0) {
> +		printk(KERN_ERR PREFIX "SCI (ACPI GSI %d) not registered\n",
> +		       gsi);
> +		return AE_OK;
>  	}
> 
> 4.1 same reason as 3.1

Same questions as 3.1 ;-)

> 5. acpi_gpe_block_remove
> 
> 5.1 If you check code acpi_ev_delete_gpe_xrupt, you will find that
> if a GPE device's interrupt_level happen to equal to acpi_gbl_FADT->sci_int,
> the cleanup will abort.

But I don't call acpi_gpe_block_remove for FADT-mentioned GPE blocks.
Only new ones.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Yu, Luming]

>On Wed, Nov 17, 2004 at 10:27:58AM +0800, Yu, Luming wrote:
>> 1.	-static unsigned int acpi_irq_irq;
>> 	-static acpi_osd_handler acpi_irq_handler;
>> 	-static void *acpi_irq_context;
>> 
>> 1.1  I don't think it is good idea to remove acpi_irq_irq, which just
>> for FADT sci. If you haven't add FADT stuff in the dev_id_list.
>> So , please consider add FADT GPE onto dev_id_list.
>
>The FADT GPE blocks are handled separately, see
>drivers/acpi/events/evsci.c and drivers/acpi/events/evgpeblk.c
>However, they still call acpi_os_install_interrupt_handler() and
>acpi_os_remove_interrupt_handler().

Actually, FAD gpes has been added onto dev_id_list.
I'm sorry for confusion. :-). 

But it leads to other problem that is all acpi_gpe_xrupt_info
are hooked onto a global list of acpi_gbl_gpe_xrupt_list_head
you patch install GPE device like:

acpi_install_gpe_block ->acpi_ev_create_gpe_block
	->acpi_ev_install_gpe_block
	->acpi_ev_get_gpe_xrupt_block
So, all acpi_gpe_xrupt_info for new installed GPE device
are hooked onto acpi_gbl_gpe_xrupt_list_head.

And, only install acpi_ev_gpe_xrupt_handler for GPEs , which
has different interrupt_level with acpi_gbl_FADT->sci_int.
That means, if one GPE device has interrupt_level equal to
acpi_gbl_FADT->sci_int, then the interrupt handler for this
GPE device will be acpi_ev_sci_xrupt_handler, rather than
acpi_ev_gpe_xrupt_handler. Maybe this is expected behaviour
that the GPE device is sharing same irq with acpi_gbl_FADT->sci_in.

But,  



>
>> 2.  	 +struct acpi_dev_id {
>> 	+	acpi_osd_handler handler;
>> 	+	void *context;
>> 	+	int irq;
>> 	+	struct acpi_dev_id *next;
>> 	+};
>> 
>> 2.1  Do you think a spin lock or semaphore is unnecessary here?
>
>I was wondering about that.  The question is whether two
>processors can access dev_id_list at the same time.  Unfortunately, I
>think it _is_ possible, so I'll send an updated patch to fix 
>this issue.
>
>> 3.  acpi_os_install_interrupt_handler
>> 
>> +	 * If this is the FADT interrupt, ignore the GSI from 
>the core and
>> +	 * use the value in our copy of the FADT instead.  It 
>may not be the
>> +	 * same if an interrupt source override exists for the SCI.
>>  	 */
>> -	gsi = acpi_fadt.sci_int;
>> +	if (gsi == acpi_gbl_FADT->sci_int)
>> +		gsi = acpi_fadt.sci_int;
>> 
>> 3.1  This part should be revised to handle gsi != 
>acpi_gbl_FADT->sci_int case.
>> Because ACPI spec said:" You must have an interrupt source 
>override entry for 
>> the IRQ mapped to the SCI interrupt if this IRQ is not 
>identity mapped."
>> GPE device need to look for their own override recorde.
>
>This is something I don't understand very well.  Why is there 
>an override?
>Is this just for buggy BIOSes?  Why don't we override the acpi_gbl_FADT
>sci_int?  Why do we have two copies of the FADT anyway?  Why 
>does the core
>use the pristine copy of the FADT when it might be wrong?  Why would we
>need an override for GPE block interrupts?
>
>> 4. acpi_os_remove_interrupt_handler
>> 
>> +	if (gsi == acpi_gbl_FADT->sci_int)
>> +		gsi = acpi_fadt.sci_int;
>> +
>> +	if (acpi_gsi_to_irq(gsi, &irq) < 0) {
>> +		printk(KERN_ERR PREFIX "SCI (ACPI GSI %d) not 
>registered\n",
>> +		       gsi);
>> +		return AE_OK;
>>  	}
>> 
>> 4.1 same reason as 3.1
>
>Same questions as 3.1 ;-)
>
>> 5. acpi_gpe_block_remove
>> 
>> 5.1 If you check code acpi_ev_delete_gpe_xrupt, you will find that
>> if a GPE device's interrupt_level happen to equal to 
>acpi_gbl_FADT->sci_int,
>> the cleanup will abort.
>
>But I don't call acpi_gpe_block_remove for FADT-mentioned GPE blocks.
>Only new ones.
>
>-- 
>"Next the statesmen will invent cheap lies, putting the blame upon 
>the nation that is attacked, and every man will be glad of those
>conscience-soothing falsities, and will diligently study them, 
>and refuse
>to examine any refutations of them; and thus he will by and by 
>convince 
>himself that the war is just, and will thank God for the better sleep 
>he enjoys after this process of grotesque self-deception." -- 
>Mark Twain
>


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Yu, Luming]

Why the mail is broken?   :-(  
(Ugh, another PPP broken?)

Please ignore my previous mail.

I summary what I sent (but get replaced by a unfinished version):

1. Current ACPI CA seems to have defect when handling
case if there have duplicate GPE interrupt level. 
It's due to all acpi_gpe_xrupt_info
are hooked onto a global list of acpi_gbl_gpe_xrupt_list_head. 
And, if found a duplicate interrupt, the old acpi_gpe_xrupt_info
will be override by new one, Please take a look at 
acpi_ev_get_gpe_xrupt_block. So you patch need to avoid
duplication of GPE interrupt level at this moment.
 I'm not sure if this limitation is NOT ACPI CA 's bug. ( I will figure
it out)

2. Due to the global list of acpi_gbl_gpe_xrupt_list_head,
All GPEs will be checked at each GPE interrupt level,
This is not the expected behaviour of just checking GPE at GPE's
interrupt
level.

3. Interrupt Source Overrides are necessary to describe variances 
between the IA-PC standard dual 8259 interrupt definition and the 
platform's implementation.

Please take a look at ACPI SPEC
5.2.11.8   Interrupt Source Overrides

Sorry for my broken mailer and PPP .

Thnaks,
Luming


>>On Wed, Nov 17, 2004 at 10:27:58AM +0800, Yu, Luming wrote:
>>> 1.	-static unsigned int acpi_irq_irq;
>>> 	-static acpi_osd_handler acpi_irq_handler;
>>> 	-static void *acpi_irq_context;
>>> 
>>> 1.1  I don't think it is good idea to remove acpi_irq_irq, 
>which just
>>> for FADT sci. If you haven't add FADT stuff in the dev_id_list.
>>> So , please consider add FADT GPE onto dev_id_list.
>>
>>The FADT GPE blocks are handled separately, see
>>drivers/acpi/events/evsci.c and drivers/acpi/events/evgpeblk.c
>>However, they still call acpi_os_install_interrupt_handler() and
>>acpi_os_remove_interrupt_handler().
>
>Actually, FAD gpes has been added onto dev_id_list.
>I'm sorry for confusion. :-). 
>
>But it leads to other problem that is all acpi_gpe_xrupt_info
>are hooked onto a global list of acpi_gbl_gpe_xrupt_list_head
>you patch install GPE device like:
>
>acpi_install_gpe_block ->acpi_ev_create_gpe_block
>	->acpi_ev_install_gpe_block
>	->acpi_ev_get_gpe_xrupt_block
>So, all acpi_gpe_xrupt_info for new installed GPE device
>are hooked onto acpi_gbl_gpe_xrupt_list_head.
>
>And, only install acpi_ev_gpe_xrupt_handler for GPEs , which
>has different interrupt_level with acpi_gbl_FADT->sci_int.
>That means, if one GPE device has interrupt_level equal to
>acpi_gbl_FADT->sci_int, then the interrupt handler for this
>GPE device will be acpi_ev_sci_xrupt_handler, rather than
>acpi_ev_gpe_xrupt_handler. Maybe this is expected behaviour
>that the GPE device is sharing same irq with acpi_gbl_FADT->sci_in.
>
>But,  
>
>
>
>>
>>> 2.  	 +struct acpi_dev_id {
>>> 	+	acpi_osd_handler handler;
>>> 	+	void *context;
>>> 	+	int irq;
>>> 	+	struct acpi_dev_id *next;
>>> 	+};
>>> 
>>> 2.1  Do you think a spin lock or semaphore is unnecessary here?
>>
>>I was wondering about that.  The question is whether two
>>processors can access dev_id_list at the same time.  Unfortunately, I
>>think it _is_ possible, so I'll send an updated patch to fix 
>>this issue.
>>
>>> 3.  acpi_os_install_interrupt_handler
>>> 
>>> +	 * If this is the FADT interrupt, ignore the GSI from 
>>the core and
>>> +	 * use the value in our copy of the FADT instead.  It 
>>may not be the
>>> +	 * same if an interrupt source override exists for the SCI.
>>>  	 */
>>> -	gsi = acpi_fadt.sci_int;
>>> +	if (gsi == acpi_gbl_FADT->sci_int)
>>> +		gsi = acpi_fadt.sci_int;
>>> 
>>> 3.1  This part should be revised to handle gsi != 
>>acpi_gbl_FADT->sci_int case.
>>> Because ACPI spec said:" You must have an interrupt source 
>>override entry for 
>>> the IRQ mapped to the SCI interrupt if this IRQ is not 
>>identity mapped."
>>> GPE device need to look for their own override recorde.
>>
>>This is something I don't understand very well.  Why is there 
>>an override?
>>Is this just for buggy BIOSes?  Why don't we override the 
>acpi_gbl_FADT
>>sci_int?  Why do we have two copies of the FADT anyway?  Why 
>>does the core
>>use the pristine copy of the FADT when it might be wrong?  
>Why would we
>>need an override for GPE block interrupts?
>>
>>> 4. acpi_os_remove_interrupt_handler
>>> 
>>> +	if (gsi == acpi_gbl_FADT->sci_int)
>>> +		gsi = acpi_fadt.sci_int;
>>> +
>>> +	if (acpi_gsi_to_irq(gsi, &irq) < 0) {
>>> +		printk(KERN_ERR PREFIX "SCI (ACPI GSI %d) not 
>>registered\n",
>>> +		       gsi);
>>> +		return AE_OK;
>>>  	}
>>> 
>>> 4.1 same reason as 3.1
>>
>>Same questions as 3.1 ;-)
>>
>>> 5. acpi_gpe_block_remove
>>> 
>>> 5.1 If you check code acpi_ev_delete_gpe_xrupt, you will find that
>>> if a GPE device's interrupt_level happen to equal to 
>>acpi_gbl_FADT->sci_int,
>>> the cleanup will abort.
>>
>>But I don't call acpi_gpe_block_remove for FADT-mentioned GPE blocks.
>>Only new ones.
>>
>>-- 
>>"Next the statesmen will invent cheap lies, putting the blame upon 
>>the nation that is attacked, and every man will be glad of those
>>conscience-soothing falsities, and will diligently study them, 
>>and refuse
>>to examine any refutations of them; and thus he will by and by 
>>convince 
>>himself that the war is just, and will thank God for the better sleep 
>>he enjoys after this process of grotesque self-deception." -- 
>>Mark Twain
>>


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Matthew Wilcox]

On Fri, Nov 19, 2004 at 12:41:54AM +0800, Yu, Luming wrote:
> 1. Current ACPI CA seems to have defect when handling
> case if there have duplicate GPE interrupt level. 
> It's due to all acpi_gpe_xrupt_info
> are hooked onto a global list of acpi_gbl_gpe_xrupt_list_head. 
> And, if found a duplicate interrupt, the old acpi_gpe_xrupt_info
> will be override by new one, Please take a look at 
> acpi_ev_get_gpe_xrupt_block. So you patch need to avoid
> duplication of GPE interrupt level at this moment.
>  I'm not sure if this limitation is NOT ACPI CA 's bug. ( I will figure
> it out)

I think this is just a misreading.  There's one acpi_gpe_xrupt_info per
interrupt.  There can be multiple gpe blocks per acpi_gpe_xrupt_info.
See acpi_ev_install_gpe_block():

        gpe_xrupt_block = acpi_ev_get_gpe_xrupt_block (interrupt_level);
        acpi_os_acquire_lock (acpi_gbl_gpe_lock, ACPI_NOT_ISR);
        if (gpe_xrupt_block->gpe_block_list_head) {
                next_gpe_block = gpe_xrupt_block->gpe_block_list_head;
                while (next_gpe_block->next) {
                        next_gpe_block = next_gpe_block->next;
                }

                next_gpe_block->next = gpe_block;
                gpe_block->previous = next_gpe_block;
        }
        else {
                gpe_xrupt_block->gpe_block_list_head = gpe_block;
        }

> 2. Due to the global list of acpi_gbl_gpe_xrupt_list_head,
> All GPEs will be checked at each GPE interrupt level,
> This is not the expected behaviour of just checking GPE at GPE's
> interrupt
> level.

I think this misunderstanding follows from the above misunderstanding.

> 3. Interrupt Source Overrides are necessary to describe variances 
> between the IA-PC standard dual 8259 interrupt definition and the 
> platform's implementation.
> 
> Please take a look at ACPI SPEC
> 5.2.11.8   Interrupt Source Overrides

OK, but that doesn't explain why we have two copies of the FADT.  Or,
for that matter, why we only override SCI INT in one copy of the FADT
and not the other.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: Re: [PATCH] Teach OSL to handle multiple interrupts [1/2]

[Yu, Luming]

>> 2. Due to the global list of acpi_gbl_gpe_xrupt_list_head,
>> All GPEs will be checked at each GPE interrupt level,
>> This is not the expected behaviour of just checking GPE at GPE's
>> interrupt
>> level.
>
>I think this misunderstanding follows from the above misunderstanding.
>

Yes, you are right. I got conclusion too early. It's my bad. :-(

But, I think there is a race condition on path of acpi_remove_gpe_block.
Say,when acpi_remove_gpe_block finished,  there could have defered task 
on workqueue of kacpid, which was referencing acpi_gpe_event_info
belongs
to just removed gpe block.

>> 3. Interrupt Source Overrides are necessary to describe variances 
>> between the IA-PC standard dual 8259 interrupt definition and the 
>> platform's implementation.
>> 
>> Please take a look at ACPI SPEC
>> 5.2.11.8   Interrupt Source Overrides
>
>OK, but that doesn't explain why we have two copies of the FADT.  Or,
>for that matter, why we only override SCI INT in one copy of the FADT
>and not the other.

acpi_fadt : a separate copy of the FADT for use by other drivers.
acpi_gbl_FAD:  2.0 FADT from BIOS.

Thanks,
Luming


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8