ACPI patches for 2.6.28-rc3
Subsystems (6)
Len Brown
reviewerplease speak up if you see any reason these
pathces should not go into 2.6.28-rc3.
thanks,
-Len
Len Brown
reviewerPATCH 1/21 i7300_idle: Kconfig, show menu only on x86_64
From: Venki Pallipadi <[email protected]>
...since today it contains only a single driver
which is visible to just x86_64
--- drivers/idle/Kconfig | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/idle/Kconfig +2 -1
@@ -1,5 +1,6 @@ menu "Memory power savings" +depends on X86_64 config I7300_IDLE_IOAT_CHANNEL bool
@@ -7,7 +8,7 @@ config I7300_IDLE_IOAT_CHANNEL config I7300_IDLE tristate "Intel chipset idle memory power saving driver" select I7300_IDLE_IOAT_CHANNEL - depends on X86_64 && EXPERIMENTAL + depends on EXPERIMENTAL help Enable memory power savings when idle with certain Intel server chipsets. The chipset must have I/O AT support, such as the
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 2/21 ACPI: EC: revert msleep patch
In reply to Len BrownFrom: Alexey Starikovskiy <[email protected]>
With the better solution for EC interrupt storm issue, there is no need to use msleep over udelay.
References:
http://bugzilla.kernel.org/show_bug.cgi?id=11810 http://bugzilla.kernel.org/show_bug.cgi?id=10724
--- drivers/acpi/ec.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/acpi/ec.c +2 -2
@@ -239,10 +239,10 @@ static int ec_check_sci(struct acpi_ec *ec, u8 state)static int ec_poll (struct acpi_ec *ec) {unsigned long delay = jiffies +msecs_to_jiffies (ACPI_EC_DELAY ); -msleep (1 ); +udelay (ACPI_EC_UDELAY );while (time_before (jiffies, delay)) {gpe_transaction (ec,acpi_ec_read_status (ec)); -msleep (1 ); +udelay (ACPI_EC_UDELAY );if (ec_transaction_done (ec))return 0 ; }
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 3/21 sony-laptop: Ignore missing _DIS method on pic device
In reply to Len BrownFrom: Matthew Garrett <[email protected]>
At least the Vaio VGN-Z540N doesn't have this method, so let's not fail to suspend just because it doesn't exist.
--- drivers/misc/sony-laptop.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
drivers/misc/sony-laptop.c +4 -2
@@ -2315,8 +2315,10 @@ end:*/ static int sony_pic_disable (struct acpi_device *device) { -if (ACPI_FAILURE (acpi_evaluate_object (device->handle, -"_DIS" ,NULL ,NULL ))) +acpi_status ret =acpi_evaluate_object (device->handle,"_DIS" ,NULL , +NULL ); + +if (ACPI_FAILURE (ret) && ret !=AE_NOT_FOUND )return -ENXIO ;dprintk ("Device disabled\n" );
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 4/21 ACPI: fix de-reference bug in power resource driver
In reply to Len BrownFrom: Zhao Yakui <[email protected]>
change state to *state in the function of acpi_power_get_state()
--- drivers/acpi/power.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/acpi/power.c +2 -1
@@ -153,7 +153,8 @@ static int acpi_power_get_state(acpi_handle handle, int *state)ACPI_POWER_RESOURCE_STATE_OFF ;ACPI_DEBUG_PRINT ((ACPI_DB_INFO ,"Resource [%s] is %s\n" , -acpi_ut_get_node_name (handle), state ?"on" :"off" )); +acpi_ut_get_node_name (handle), + *state ?"on" :"off" ));return 0 ; }
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 5/21 ACPI: fan: Delete the strict check in power transition
In reply to Len BrownFrom: Zhao Yakui <[email protected]>
On some laptops the Fan device is turned on/off by controlling the corresponding power resource. For example: If the power resource defined in _PR0 object is turned off, it indicates that the FAN device is in off state(the ACPI state is in D3 state).
Maybe the device is already in D3 state and expected to be transited to D3 state. As there is no _PR3 object, the power transition can't be finished and it will be switched to the Unknown state.
Maybe it is more reasonable that the strick check in power transistion is deleted.
http://bugzilla.kernel.org/show_bug.cgi?id=9485
--- drivers/acpi/power.c | 5 ----- 1 files changed, 0 insertions(+), 5 deletions(-)
drivers/acpi/power.c +0 -5
@@ -517,11 +517,6 @@ int acpi_power_transition(struct acpi_device *device, int state) cl = &device->power.states[device->power.state].resources; tl = &device->power.states[state].resources; -if (!cl->count && !tl->count) { - result = -ENODEV ; - goto end; - } -/* TBD: Resources must be ordered. */ /*
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 6/21 ACPI: bugfix reporting of event handler status
In reply to Len BrownFrom: Zhang Rui <[email protected]>
Introduce a new flag showing whether the event has an event handler/method.
For all the GPEs and Fixed Events,
1. ACPI_EVENT_FLAG_HANDLE is cleared, it's an "invalid" ACPI event.
2. Both ACPI_EVENT_FLAG_HANDLE and ACPI_EVENT_FLAG_DISABLE are set, it's "disabled".
3. Both ACPI_EVENT_FLAG_HANDLE and ACPI_EVENT_FLAG_ENABLE are set,
it's "enabled".
4. Both ACPI_EVENT_FLAG_HANDLE and ACPI_EVENT_FLAG_WAKE_ENABLE are set, it's "wake_enabled".
Among other things, this prevents incorrect reporting of ACPI events as being "invalid" when it's really just (temporarily) "disabled".
--- Documentation/ABI/testing/sysfs-firmware-acpi | 16 ++++++++-------- drivers/acpi/events/evxfevnt.c | 6 ++++++ drivers/acpi/system.c | 19 +++++++------------ include/acpi/actypes.h | 1 + 4 files changed, 22 insertions(+), 20 deletions(-)
Documentation/ABI/testing/sysfs-firmware-acpi +8 -8
@@ -89,7 +89,7 @@ Description: error - an interrupt that can't be accounted for above. - invalid: it's either a wakeup GPE or a GPE/Fixed Event that + invalid: it's either a GPE or a Fixed Event that doesn't have an event handler. disable: the GPE/Fixed Event is valid but disabled.
@@ -117,30 +117,30 @@ Description: and other user space applications so that the machine won't shutdown when pressing the power button. # cat ff_pwr_btn - 0 + 0 enabled # press the power button for 3 times; # cat ff_pwr_btn - 3 + 3 enabled # echo disable > ff_pwr_btn # cat ff_pwr_btn - disable + 3 disabled # press the power button for 3 times; # cat ff_pwr_btn - disable + 3 disabled # echo enable > ff_pwr_btn # cat ff_pwr_btn - 4 + 4 enabled /* * this is because the status bit is set even if the enable bit is cleared, * and it triggers an ACPI fixed event when the enable bit is set again */ # press the power button for 3 times; # cat ff_pwr_btn - 7 + 7 enabled # echo disable > ff_pwr_btn # press the power button for 3 times; # echo clear > ff_pwr_btn /* clear the status bit */ # echo disable > ff_pwr_btn # cat ff_pwr_btn - 7 + 7 enabled
drivers/acpi/events/evxfevnt.c +6 -0
@@ -521,6 +521,9 @@ acpi_status acpi_get_event_status(u32 event, acpi_event_status * event_status)if (value) *event_status |=ACPI_EVENT_FLAG_SET ; +if (acpi_gbl_fixed_event_handlers[event].handler) + *event_status |=ACPI_EVENT_FLAG_HANDLE ; +return_ACPI_STATUS (status); }
@@ -571,6 +574,9 @@ acpi_get_gpe_status(acpi_handle gpe_device, status =acpi_hw_get_gpe_status (gpe_event_info, event_status); +if (gpe_event_info->flags &ACPI_GPE_DISPATCH_MASK ) + *event_status |=ACPI_EVENT_FLAG_HANDLE ; + unlock_and_exit:if (flags &ACPI_NOT_ISR ) { (void )acpi_ut_release_mutex (ACPI_MTX_EVENTS );
drivers/acpi/system.c +7 -12
@@ -167,7 +167,6 @@ static int acpi_system_sysfs_init(void)#define COUNT_ERROR 2/* other */ #define NUM_COUNTERS_EXTRA 3 -#define ACPI_EVENT_VALID 0x01struct event_counter {u32 count;u32 flags;
@@ -312,12 +311,6 @@ static int get_status(u32 index, acpi_event_status *status, acpi_handle *handle) }else if (index < (num_gpes +ACPI_NUM_FIXED_EVENTS )) result =acpi_get_event_status (index - num_gpes, status); -/* -* sleep/power button GPE/Fixed Event is enabled after acpi_system_init, -* check the status at runtime and mark it as valid once it's enabled -*/ -if (!result && (*status &ACPI_EVENT_FLAG_ENABLED )) - all_counters[index].flags |=ACPI_EVENT_VALID ; end:return result; }
@@ -346,12 +339,14 @@ static ssize_t counter_show(struct kobject *kobj,if (result) goto end; -if (!(all_counters[index].flags &ACPI_EVENT_VALID )) - size +=sprintf (buf + size," invalid" ); +if (!(status &ACPI_EVENT_FLAG_HANDLE )) + size +=sprintf (buf + size," invalid" );else if (status &ACPI_EVENT_FLAG_ENABLED ) - size +=sprintf (buf + size," enable" ); + size +=sprintf (buf + size," enabled" ); +else if (status &ACPI_EVENT_FLAG_WAKE_ENABLED ) + size +=sprintf (buf + size," wake_enabled" );else - size +=sprintf (buf + size," disable" ); + size +=sprintf (buf + size," disabled" ); end: size +=sprintf (buf + size,"\n" );
@@ -385,7 +380,7 @@ static ssize_t counter_set(struct kobject *kobj,if (result) goto end; -if (!(all_counters[index].flags &ACPI_EVENT_VALID )) { +if (!(status &ACPI_EVENT_FLAG_HANDLE )) { printk(KERN_WARNING PREFIX "Can not change Invalid GPE/Fixed Event status\n");return -EINVAL ;
include/acpi/actypes.h +1 -0
@@ -525,6 +525,7 @@ typedef u32 acpi_event_status;#define ACPI_EVENT_FLAG_ENABLED (acpi_event_status) 0x01#define ACPI_EVENT_FLAG_WAKE_ENABLED (acpi_event_status) 0x02#define ACPI_EVENT_FLAG_SET (acpi_event_status) 0x04 +#define ACPI_EVENT_FLAG_HANDLE (acpi_event_status) 0x08/* * General Purpose Events (GPE)
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 7/21 ACPI EC: Fix regression due to use of uninitialized variable
In reply to Len BrownFrom: Alexey Starikovskiy <[email protected]>
breakage introduced by following patch
commit 27663c5855b10af9ec67bc7dfba001426ba21222
Author: Matthew Wilcox <[email protected]> Date: Fri Oct 10 02:22:59 2008 -0400
acpi_evaluate_integer() does not clear passed variable if there is an error at evaluation.
So if we ignore error, we must supply initialized variable.
References: http://bugzilla.kernel.org/show_bug.cgi?id=11917
http://bugzilla.kernel.org/show_bug.cgi?id=11896
--- drivers/acpi/ec.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/acpi/ec.c +2 -1
@@ -736,7 +736,7 @@ static acpi_status ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval) { acpi_status status; -unsigned long long tmp; +unsigned long long tmp =0 ;struct acpi_ec *ec = context; status = acpi_walk_resources(handle, METHOD_NAME__CRS,
@@ -751,6 +751,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)return status; ec->gpe = tmp;/* Use the global lock for all EC transactions? */ + tmp =0 ;acpi_evaluate_integer (handle,"_GLK" ,NULL , &tmp); ec->global_lock = tmp; ec->handle = handle;
Signature · 1 line
1.5.6.5
Re: [PATCH 07/21] ACPI EC: Fix regression due to use of uninitialized variable
Quoted 7 lines · Len Brown
From: Alexey Starikovskiy <[email protected]>
breakage introduced by following patch
commit27663c5855b10af9ec67bc7dfba001426ba21222Author: Matthew Wilcox <[email protected]> Date: Fri Oct 10 02:22:59 2008 -0400
Quoted 6 lines
acpi_evaluate_integer()does not clear passed variable if there is an error at evaluation.
So if we ignore error, we must supply initialized variable.References: http://bugzilla.kernel.org/show_bug.cgi?id=11917
http://bugzilla.kernel.org/show_bug.cgi?id=11896
The bug references for 11896 should be deleted.
The bug for 11896 can't be resolved by this patch.
Thanks.
Quoted 29 lines
Signed-off-by Alexey Starikovskiy <[email protected]>Tested-by Alan Jenkins <[email protected]>Signed-off-by Len Brown <[email protected]>--- drivers/acpi/ec.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/acpi/ec.c +2
-1@@ -736,7 +736,7 @@ static acpi_status ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval) { acpi_status status; -unsigned long long tmp; +unsigned long long tmp =0 ;struct acpi_ec *ec = context; status = acpi_walk_resources(handle, METHOD_NAME__CRS,@@ -751,6 +751,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)return status; ec->gpe = tmp;/* Use the global lock for all EC transactions? */ + tmp =0 ;acpi_evaluate_integer (handle,"_GLK" ,NULL , &tmp); ec->global_lock = tmp; ec->handle = handle;
Len Brown
reviewerRe: [PATCH 07/21] ACPI EC: Fix regression due to use of uninitialized variable
Quoted 13 lines
Quoted 13 lines
From: Alexey Starikovskiy <[email protected]>
breakage introduced by following patch
commit27663c5855b10af9ec67bc7dfba001426ba21222Author: Matthew Wilcox <[email protected]> Date: Fri Oct 10 02:22:59 2008 -0400
acpi_evaluate_integer()does not clear passed variable if there is an error at evaluation.
So if we ignore error, we must supply initialized variable.
Quoted 5 lines
Quoted 2 lines
References: http://bugzilla.kernel.org/show_bug.cgi?id=11917
http://bugzilla.kernel.org/show_bug.cgi?id=11896The bug references for 11896 should be deleted.
The bug for 11896 can't be resolved by this patch.
Thanks.
amended.
thanks,
-Len
The fatal logic error in EC write transaction(EC transaction is done in interrupt context)
Hi, Alexey
After checking the EC working flowchart I find a fatal logic error that exists in EC write transaction. The issue happens on most laptops. The following is the detailed explanation about this issue:
According to the ACPI the EC write transaction is divided into the following three phases: (Seen in the section 12.6.2 of ACPI 3.0b)
a. Byte #1: Host Writes the EC command to EC CMD I/O PORT. Interrupt is triggered after EC reads the command byte
b. Byte #2: Host writes the address index to EC data I/O port. Interrupt is triggered after EC reads the address index byte
c. Byte #3: Host writes the Data to EC data I/O port. Interrupt is triggered after EC reads the data byte
When the function of acpi_ec_transaction_unlocked is called for EC write transaction, the wlen variable is initialized as 2 and the rlen variable is initialized as zero. Then EC transaction follows the below three steps.
1. OS writes the EC command to EC CMD I/O port
2. EC GPE interrupt is triggered and then the address index is written into EC data I/O port. (The wlen is decreased to 1.This is executed in EC GPE handler)
3. EC GPE interrupt is triggered again and then the data is written into the EC data I/O port. The wlen is decreased to 0.(This is also executed in EC GPE handler). As the wlen is decreased to zero, it means that the EC transaction is finished(True is returned by the function of ec_transaction_done).
>status = acpi_ec_read_status(ec);
>gpe_transaction(ec, status);
> if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
wake_up(&ec->wait);
In this step the status variable only indicates that EC controller already reads the address index written by OS(It can’t indicate that EC controller already reads the data byte written by Host). As the IBF flag is zero, the waiting process is waked up and EC mutex lock is released. In such case it means that OS can begin another EC transaction. But in fact in such case EC transaction is not really finished. After the EC data is written into EC Data I/O port, no flag indicates whether EC transaction is finished. Only when the IBF bit becomes zero again, we can say that EC transaction is really finished. If OS begins another EC transaction before previous EC transaction is really finished, we can't imagine what will happen. IMO Based on the above analysis there exists the fatal logic error in the EC write transaction after the patch from Alexey is shipped. Although now no one reports this issue, it is still an unstable factor. After all it is the fatal logic error.
At the same time there exists the similar logic error when OS begins an EC transaction to disable the EC burst mode.
Welcome the comments.
Best regards
Yakui
Re: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)
Hi Yakui,
Just don't know how I would have lived without your valuable analisys...
Do you remember that ec_transaction starts by waiting for IBF to become 0?
Thus not any new transaction should begin before previous write or burst-disable command completes. So, fatal logic error exists only in your head.
Now, let me explain why it is better to wait for last IBF=0 event at beginning of transaction, rather than at the end of previous one.
Look at the flow chart from different angle -- as fast transaction patch does it:
EC sends interrupt to OS then it is ready for us to proceed. It will send us interrupt marking IBF=0 transition if it expects us to write byte and it will send us interrupt marking OBF=1 transition if it expects us to read byte. It does not ever send us IBF=1 or OBF=0 interrupts just because there is nothing we could of should do in such a case. After we completed write sequence, we don't need to wait for last IBF=0 transition, as we don't know if we will have anything else to write -- it might be last transaction for quite a while. But next transaction knows that it should not start before IBF is cleared, thus it waits for it (and expects possible interrupt).
Regards,
Alex.
Quoted 19 lines · Yakui
Hi, Alexey
After checking the EC working flowchart I find a fatal logic error that exists in EC write transaction. The issue happens on most laptops. The following is the detailed explanation about this issue:
According to the ACPI the EC write transaction is divided into the following three phases: (Seen in the section 12.6.2 of ACPI 3.0b)
a. Byte #1: Host Writes the EC command to EC CMD I/O PORT. Interrupt is triggered after EC reads the command byte
b. Byte #2: Host writes the address index to EC data I/O port. Interrupt is triggered after EC reads the address index byte
c. Byte #3: Host writes the Data to EC data I/O port. Interrupt is triggered after EC reads the data byteWhen the function of
acpi_ec_transaction_unlockedis called for EC write transaction, the wlen variable is initialized as 2 and the rlen variable is initialized as zero. Then EC transaction follows the below three steps.
1. OS writes the EC command to EC CMD I/O port
2. EC GPE interrupt is triggered and then the address index is written into EC data I/O port. (The wlen is decreased to 1.This is executed in EC GPE handler)
3. EC GPE interrupt is triggered again and then the data is written into the EC data I/O port. The wlen is decreased to 0.(This is also executed in EC GPE handler). As the wlen is decreased to zero, it means that the EC transaction isfinished(True is returned by the function of ec_transaction_done).
>status =acpi_ec_read_status(ec);
>gpe_transaction(ec, status);
> if (ec_transaction_done(ec)&& (status &ACPI_EC_FLAG_IBF) == 0)
wake_up(&ec->wait);
In this step the status variable only indicates that EC controller already reads the address index written by OS(It can’t indicate that EC controller already reads the data byte written by Host). As the IBF flag is zero, the waiting process is waked up and EC mutex lock is released. In such case it means that OS can begin another EC transaction. But in fact in such case EC transaction is not really finished. After the EC data is written into EC Data I/O port, no flag indicates whether EC transaction is finished. Only when the IBF bit becomes zero again, we can say that EC transaction is really finished. If OS begins another EC transaction before previous EC transaction is really finished, we can't imagine what will happen.
IMO Based on the above analysis there exists the fatal logic error in the EC write transaction after the patch from Alexey is shipped. Although now no one reports this issue, it is still an unstable factor. After all it is the fatal logic error. At the same time there exists the similar logic error when OS begins an EC transaction to disable the EC burst mode.
Quoted 7 lines
Welcome the comments.
Best regards
Yakui
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)
a. When the host writes data to the command or data register of the embedded controller, the input buffer flag (IBF) in the status register is set within 1 microsecond.( Please see it in the section 12.7 of ACPI 3.0b. This should be paid attention to). So If another EC transaction is started in 1 microsecond, maybe the EC status is not updated. In such case the problem will appear.
At the same time we can't expect that all the EC follows the spec strictly. Maybe on some EC controllers more time is needed.
b. Why not wait before the EC transaction is finished? If it fails, it can't be caught in time. From the programming viewpoint the logic is incorrect. If you insist that viewpoint is correct, IMO this issue can be discussed in the wider range.
In my email I raise the issue about your patch. If your patch can work well, no one is willing to raise any issue about your patch.
-----Original Message-----
From: Alexey Starikovskiy [mailto:[email protected]]
Sent: 2008年11月8日 23:34
To: Zhao, Yakui
Cc: LenBrown; [email protected]; Brown, Len; Li, Shaohua; Zhang, Rui; Lin, Ming M
Subject: Re: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)
Hi Yakui,
Just don't know how I would have lived without your valuable analisys...
Do you remember that ec_transaction starts by waiting for IBF to become 0?
Thus not any new transaction should begin before previous write or burst-disable command completes. So, fatal logic error exists only in your head.
Now, let me explain why it is better to wait for last IBF=0 event at beginning of transaction, rather than at the end of previous one.
Look at the flow chart from different angle -- as fast transaction patch does it:
EC sends interrupt to OS then it is ready for us to proceed. It will send us interrupt marking IBF=0 transition if it expects us to write byte and it will send us interrupt marking OBF=1 transition if it expects us to read byte. It does not ever send us IBF=1 or OBF=0 interrupts just because there is nothing we could of should do in such a case. After we completed write sequence, we don't need to wait for last IBF=0 transition, as we don't know if we will have anything else to write -- it might be last transaction for quite a while. But next transaction knows that it should not start before IBF is cleared, thus it waits
for it (and expects possible interrupt).
Regards,
Alex.
Quoted 28 lines · Yakui
Hi, Alexey
After checking the EC working flowchart I find a fatal logic error that exists in EC write transaction. The issue happens on most laptops. The following is the detailed explanation about this issue:
According to the ACPI the EC write transaction is divided into the following three phases: (Seen in the section 12.6.2 of ACPI 3.0b)
a. Byte #1: Host Writes the EC command to EC CMD I/O PORT. Interrupt is triggered after EC reads the command byte
b. Byte #2: Host writes the address index to EC data I/O port. Interrupt is triggered after EC reads the address index byte
c. Byte #3: Host writes the Data to EC data I/O port. Interrupt is triggered after EC reads the data byteWhen the function of
acpi_ec_transaction_unlockedis called for EC write transaction, the wlen variable is initialized as 2 and the rlen variable is initialized as zero. Then EC transaction follows the below three steps.
1. OS writes the EC command to EC CMD I/O port
2. EC GPE interrupt is triggered and then the address index is written into EC data I/O port. (The wlen is decreased to 1.This is executed in EC GPE handler)
3. EC GPE interrupt is triggered again and then the data is written into the EC data I/O port. The wlen is decreased to 0.(This is also executed in EC GPE handler). As the wlen is decreased to zero, it means that the EC transaction isfinished(True is returned by the function of ec_transaction_done).
>status =acpi_ec_read_status(ec);
>gpe_transaction(ec, status);
> if (ec_transaction_done(ec)&& (status &ACPI_EC_FLAG_IBF) == 0)
wake_up(&ec->wait);
In this step the status variable only indicates that EC controller already reads the address index written by OS(It can’t indicate that EC controller already reads the data byte written by Host). As the IBF flag is zero, the waiting process is waked up and EC mutex lock is released. In such case it means that OS can begin another EC transaction. But in fact in such case EC transaction is not really finished. After the EC data is written into EC Data I/O port, no flag indicates whether EC transaction is finished. Only when the IBF bit becomes zero again, we can say that EC transaction is really finished. If OS begins another EC transaction before previous EC transaction is really finished, we can't imagine what will happen.
IMO Based on the above analysis there exists the fatal logic error in the EC write transaction after the patch from Alexey is shipped. Although now no one reports this issue, it is still an unstable factor. After all it is the fatal logic error.
At the same time there exists the similar logic error when OS begins an EC transaction to disable the EC burst mode.Welcome the comments.
Best regards
Yakui
Re: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)
Yakui,
This is not about my patch, this is about your ego...
My patch did not introduce this behaviour, it was there since "burst mode" was introduced, and may be even earlier. You may ask Luming Yu, why he was so optimistic about 1 microsecond (is he still around? you did not include him into CC yet...).
Regards,
Alex.
Quoted 25 lines · Yakui
a. When the host writes data to the command or data register of the embedded controller, the input buffer flag (IBF) in the status register is set within 1 microsecond.( Please see it in the section 12.7 of ACPI 3.0b. This should be paid attention to). So If another EC transaction is started in 1 microsecond, maybe the EC status is not updated. In such case the problem will appear.
At the same time we can't expect that all the EC follows the spec strictly. Maybe on some EC controllers more time is needed.
b. Why not wait before the EC transaction is finished? If it fails, it can't be caught in time. From the programming viewpoint the logic is incorrect. If you insist that viewpoint is correct, IMO this issue can be discussed in the wider range.In my email I raise the issue about your patch. If your patch can work well, no one is willing to raise any issue about your patch.
-----Original Message-----
From: Alexey Starikovskiy [mailto:[email protected]] Sent: 2008年11月8日 23:34
To: Zhao, Yakui
Cc: LenBrown; [email protected]; Brown, Len; Li, Shaohua; Zhang, Rui; Lin, Ming M
Subject: Re: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)Hi Yakui,
Just don't know how I would have lived without your valuable analisys...
Do you remember thatec_transactionstarts by waiting for IBF to become 0?
Thus not any new transaction should begin before previous write or burst-disable command completes. So, fatal logic error exists only in your head.Now, let me explain why it is better to wait for last IBF=0 event at beginning of transaction, rather than at the end of previous one.
Look at the flow chart from different angle -- as fast transaction patch does it:
EC sends interrupt to OS then it is ready for us to proceed. It will send us interrupt marking IBF=0 transition if it expects us to write byte and it will send us interrupt marking OBF=1 transition if it expects us to read byte. It does not ever send us IBF=1 or OBF=0 interrupts just because there is nothing we could of should do in such a case. After we completed write sequence, we don't need to wait for last IBF=0 transition, as we don't know if we will have anything else to write -- it might be last transaction for quite a while. But next transaction knows that it should not start before IBF is cleared, thus it waits for it (and expects possible interrupt).
Quoted 31 lines
Regards,
Alex.Quoted 26 lines · Yakui
Hi, Alexey
After checking the EC working flowchart I find a fatal logic error that exists in EC write transaction. The issue happens on most laptops. The following is the detailed explanation about this issue:
According to the ACPI the EC write transaction is divided into the following three phases: (Seen in the section 12.6.2 of ACPI 3.0b)
a. Byte #1: Host Writes the EC command to EC CMD I/O PORT. Interrupt is triggered after EC reads the command byte
b. Byte #2: Host writes the address index to EC data I/O port. Interrupt is triggered after EC reads the address index byte
c. Byte #3: Host writes the Data to EC data I/O port. Interrupt is triggered after EC reads the data byteWhen the function of
acpi_ec_transaction_unlockedis called for EC write transaction, the wlen variable is initialized as 2 and the rlen variable is initialized as zero. Then EC transaction follows the below three steps.
1. OS writes the EC command to EC CMD I/O port
2. EC GPE interrupt is triggered and then the address index is written into EC data I/O port. (The wlen is decreased to 1.This is executed in EC GPE handler)
3. EC GPE interrupt is triggered again and then the data is written into the EC data I/O port. The wlen is decreased to 0.(This is also executed in EC GPE handler). As the wlen is decreased to zero, it means that the EC transaction isfinished(True is returned by the function of ec_transaction_done).
>status =acpi_ec_read_status(ec);
>gpe_transaction(ec, status);
> if (ec_transaction_done(ec)&& (status &ACPI_EC_FLAG_IBF) == 0)
wake_up(&ec->wait);
In this step the status variable only indicates that EC controller already reads the address index written by OS(It can’t indicate that EC controller already reads the data byte written by Host). As the IBF flag is zero, the waiting process is waked up and EC mutex lock is released. In such case it means that OS can begin another EC transaction. But in fact in such case EC transaction is not really finished. After the EC data is written into EC Data I/O port, no flag indicates whether EC transaction is finished. Only when the IBF bit becomes zero again, we can say that EC transaction is really finished. If OS begins another EC transaction before previous EC transaction is really finished, we can't imagine what will happen.
IMO Based on the above analysis there exists the fatal logic error in the EC write transaction after the patch from Alexey is shipped. Although now no one reports this issue, it is still an unstable factor. After all it is the fatal logic error. At the same time there exists the similar logic error when OS begins an EC transaction to disable the EC burst mode.Welcome the comments.
Best regards
Yakui
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/1 ACPI: EC: wait for last write gpe
There is a possibility that EC might break if next command is issued within 1 us after write or burst-disable command. This "possibility" was in EC driver for 3.5 years, after 451566f45a2e6cd10ba56e7220a9dd84ba3ef550.
References: http://marc.info/?l=linux-acpi&m=122616284402886&w=4
---
drivers/acpi/ec.c | 21 +++++++++++++-------- 1 files changed, 13 insertions(+), 8 deletions(-)
drivers/acpi/ec.c +13 -8
@@ -102,6 +102,7 @@ struct transaction {u8 command;u8 wlen;u8 rlen; +bool done; }; static struct acpi_ec {
@@ -178,7 +179,7 @@ static int ec_transaction_done(struct acpi_ec *ec)unsigned long flags;int ret =0 ;spin_lock_irqsave (&ec->curr_lock, flags); -if (!ec->curr || (!ec->curr->wlen && !ec->curr->rlen)) +if (!ec->curr || ec->curr->done) ret =1 ;spin_unlock_irqrestore (&ec->curr_lock, flags);return ret;
@@ -195,17 +196,20 @@ static void gpe_transaction(struct acpi_ec *ec, u8 status)acpi_ec_write_data (ec, *(ec->curr->wdata++)); --ec->curr->wlen; }else -/* false interrupt, state didn't change */ - ++ec->curr->irq_count; - + goto err; }else if (ec->curr->rlen >0 ) {if ((status &ACPI_EC_FLAG_OBF ) ==1 ) { *(ec->curr->rdata++) =acpi_ec_read_data (ec); - --ec->curr->rlen; +if (--ec->curr->rlen ==0 ) + ec->curr->done = true; }else -/* false interrupt, state didn't change */ - ++ec->curr->irq_count; - } + goto err; + }else if (ec->curr->wlen ==0 && (status &ACPI_EC_FLAG_IBF ) ==0 ) + ec->curr->done = true; + goto unlock; +err: +/* false interrupt, state didn't change */ + ++ec->curr->irq_count; unlock:spin_unlock_irqrestore (&ec->curr_lock, flags); }
@@ -265,6 +269,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,spin_lock_irqsave (&ec->curr_lock, tmp);/* following two actions should be kept atomic */ t->irq_count =0 ; + t->done = false; ec->curr = t;acpi_ec_write_cmd (ec, ec->curr->command);if (ec->curr->command ==ACPI_EC_COMMAND_QUERY )
Re: [PATCH] ACPI: EC: wait for last write gpe
Quoted 4 lines · Alexey Starikovskiy
There is a possibility that EC might break if next command is issued within 1 us after write or burst-disable command. This "possibility" was in EC driver for 3.5 years, after
451566f45a2e6cd10ba56e7220a9dd84ba3ef550.
It seems OK. Now OS will wait until the EC transaction is really finished. The logic error what I mentioned is eliminated. Will you please attach the patch in the bug 11892/11896 and let the bug reporter try it?
Thanks.
Quoted 72 lines
References: http://marc.info/?l=linux-acpi&m=122616284402886&w=4
Cc Zhao Yakui <[email protected]>Signed-off-by Alexey Starikovskiy <[email protected]>---
drivers/acpi/ec.c | 21 +++++++++++++-------- 1 files changed, 13 insertions(+), 8 deletions(-)drivers/acpi/ec.c +13
-9@@ -102,6 +102,7 @@ struct transaction {u8 command;u8 wlen;u8 rlen; +bool done; }; static struct acpi_ec {@@ -178,7 +179,7 @@ static int ec_transaction_done(struct acpi_ec *ec)unsigned long flags;int ret =0 ;spin_lock_irqsave (&ec->curr_lock, flags); -if (!ec->curr || (!ec->curr->wlen && !ec->curr->rlen)) +if (!ec->curr || ec->curr->done) ret =1 ;spin_unlock_irqrestore (&ec->curr_lock, flags);return ret;@@ -195,17 +196,20 @@ static void gpe_transaction(struct acpi_ec *ec, u8 status)acpi_ec_write_data (ec, *(ec->curr->wdata++)); --ec->curr->wlen; }else -/* false interrupt, state didn't change */ - ++ec->curr->irq_count; - + goto err; }else if (ec->curr->rlen >0 ) {if ((status &ACPI_EC_FLAG_OBF ) ==1 ) { *(ec->curr->rdata++) =acpi_ec_read_data (ec); - --ec->curr->rlen; +if (--ec->curr->rlen ==0 ) + ec->curr->done = true; }else -/* false interrupt, state didn't change */ - ++ec->curr->irq_count; - } + goto err; + }else if (ec->curr->wlen ==0 && (status &ACPI_EC_FLAG_IBF ) ==0 ) + ec->curr->done = true; + goto unlock; +err: +/* false interrupt, state didn't change */ + ++ec->curr->irq_count; unlock:spin_unlock_irqrestore (&ec->curr_lock, flags); }@@ -265,6 +269,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,spin_lock_irqsave (&ec->curr_lock, tmp);/* following two actions should be kept atomic */ t->irq_count =0 ; + t->done = false; ec->curr = t;acpi_ec_write_cmd (ec, ec->curr->command);if (ec->curr->command ==ACPI_EC_COMMAND_QUERY ) --To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)
In reply to Alexey StarikovskiyQuoted 2 lines · Alexey Starikovskiy
Yakui,
This is not about my patch, this is about your ego...
Thanks for the reply. I know that this is not introduced by your patch. In the previous kernel the EC transaction is explicitly divided into several phases. Only when EC transaction is really finished, the EC mutex lock is released. In such case the previous EC transaction is already finished when OS begins another EC transaction. So the issue about "1 microsecond" won't appear. But in your patch(EC transaction is done in interrupt context) when EC mutex is released , maybe the previous EC transaction is not really finished. In such case the issue of "1 microsecond" will appear.
Is what I said right?
Quoted 1 line
My patch did not introduce this behaviour, it was there since "burst mode" was introduced, and may be even earlier. You may ask Luming Yu, why he was so optimistic about 1 microsecond (is he still around? you did not include him into CC yet...).
In my email what I said is only to point out the logic error. Maybe there is no problem that the EC mutex is released before EC transaction is really finished. IMO this is still a logic error. The program logic states that EC transaction is already finished but the EC transaction is not really finished. They are inconsistent..
The more important is that the failure in EC transaction can't be
detected in time.
Do you agree with my analysis?
Best regards.
Yakui
Quoted 65 lines
R egards,
Alex.Quoted 27 lines · Yakui
a. When the host writes data to the command or data register of the embedded controller, the input buffer flag (IBF) in the status register is set within 1 microsecond.( Please see it in the section 12.7 of ACPI 3.0b. This should be paid attention to). So If another EC transaction is started in 1 microsecond, maybe the EC status is not updated. In such case the problem will appear.
At the same time we can't expect that all the EC follows the spec strictly. Maybe on some EC controllers more time is needed.
b. Why not wait before the EC transaction is finished? If it fails, it can't be caught in time. From the programming viewpoint the logic is incorrect. If you insist that viewpoint is correct, IMO this issue can be discussed in the wider range.In my email I raise the issue about your patch. If your patch can work well, no one is willing to raise any issue about your patch.
-----Original Message-----
From: Alexey Starikovskiy [mailto:[email protected]]
Sent: 2008年11月8日 23:34
To: Zhao, Yakui
Cc: LenBrown; [email protected]; Brown, Len; Li, Shaohua; Zhang, Rui; Lin, Ming M
Subject: Re: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)Hi Yakui,
Just don't know how I would have lived without your valuable analisys...
Do you remember thatec_transactionstarts by waiting for IBF to become 0?
Thus not any new transaction should begin before previous write or burst-disable command completes. So, fatal logic error exists only in your head.Now, let me explain why it is better to wait for last IBF=0 event at beginning of transaction, rather than at the end of previous one.
Look at the flow chart from different angle -- as fast transaction patch does it:
EC sends interrupt to OS then it is ready for us to proceed. It will send us interrupt marking IBF=0 transition if it expects us to write byte and it will send us interrupt marking OBF=1 transition if it expects us to read byte. It does not ever send us IBF=1 or OBF=0 interrupts just because there is nothing we could of should do in such a case. After we completed write sequence, we don't need to wait for last IBF=0 transition, as we don't know if we will have anything else to write -- it might be last transaction for quite a while. But next transaction knows that it should not start before IBF is cleared, thus it waits
for it (and expects possible interrupt).Quoted 33 lines
Regards,
Alex.Quoted 28 lines · Yakui
Hi, Alexey
After checking the EC working flowchart I find a fatal logic error that exists in EC write transaction. The issue happens on most laptops. The following is the detailed explanation about this issue:
According to the ACPI the EC write transaction is divided into the following three phases: (Seen in the section 12.6.2 of ACPI 3.0b)
a. Byte #1: Host Writes the EC command to EC CMD I/O PORT. Interrupt is triggered after EC reads the command byte
b. Byte #2: Host writes the address index to EC data I/O port. Interrupt is triggered after EC reads the address index byte
c. Byte #3: Host writes the Data to EC data I/O port. Interrupt is triggered after EC reads the data byteWhen the function of
acpi_ec_transaction_unlockedis called for EC write transaction, the wlen variable is initialized as 2 and the rlen variable is initialized as zero. Then EC transaction follows the below three steps.
1. OS writes the EC command to EC CMD I/O port
2. EC GPE interrupt is triggered and then the address index is written into EC data I/O port. (The wlen is decreased to 1.This is executed in EC GPE handler)
3. EC GPE interrupt is triggered again and then the data is written into the EC data I/O port. The wlen is decreased to 0.(This is also executed in EC GPE handler). As the wlen is decreased to zero, it means that the EC transaction isfinished(True is returned by the function of ec_transaction_done).
>status =acpi_ec_read_status(ec);
>gpe_transaction(ec, status);
> if (ec_transaction_done(ec)&& (status &ACPI_EC_FLAG_IBF) == 0)
wake_up(&ec->wait);
In this step the status variable only indicates that EC controller already reads the address index written by OS(It can’t indicate that EC controller already reads the data byte written by Host). As the IBF flag is zero, the waiting process is waked up and EC mutex lock is released. In such case it means that OS can begin another EC transaction. But in fact in such case EC transaction is not really finished. After the EC data is written into EC Data I/O port, no flag indicates whether EC transaction is finished. Only when the IBF bit becomes zero again, we can say that EC transaction is really finished. If OS begins another EC transaction before previous EC transaction is really finished, we can't imagine what will happen.
IMO Based on the above analysis there exists the fatal logic error in the EC write transaction after the patch from Alexey is shipped. Although now no one reports this issue, it is still an unstable factor. After all it is the fatal logic error.
At the same time there exists the similar logic error when OS begins an EC transaction to disable the EC burst mode.Welcome the comments.
Best regards
Yakui
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Spreading FUD [was: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)]
Yakui,
Please take a look at the subject of the thread -- notice words "fatal" and "(EC transaction is done in interrupt context)".
Looks horrible, isn't it? Now we begin to get into details, and it appears that it is not fatal, introduced 3.5 years ago, and has nothing to do with interrupt context transaction patch.
So, the whole point of your message is to spread FUD about "fast transaction" patch, and show how great you are. I would guess that you've failed in achieving both goals.
If you cry "wolf!" too often, you'll get ignored, when the real wolf appears.
Now, for a positive example, the same message without exclamation marks...
---------------------------------------------------------------------------------------------------------------------------------
Subject: difference in EC transaction protocol
Hi Alex,
I've noticed difference in EC transaction protocol for write and burst-disable commands, namely all that end with writing byte to EC -- they don't wait for last confirmation interrupt and next transaction might start before current one completes.
May be this is the "root cause" we are looking for.
Regards,
Yakui
---------------------------------------------------------------------------------------------------------------------------------
Note that this message is shorter, and is not offensive.
Smile, and people may start to like you...
Regards,
Alex.
P.S. And I don't agree with your analysis as you still believe that before the fast transaction mutex was not released at the same time as it is now.
Quoted 11 lines · Zhao Yakui
Quoted 3 lines · Alexey Starikovskiy
Yakui,
This is not about my patch, this is about your ego...Thanks for the reply. I know that this is not introduced by your patch. In the previous kernel the EC transaction is explicitly divided into several phases. Only when EC transaction is really finished, the EC mutex lock is released. In such case the previous EC transaction is already finished when OS begins another EC transaction. So the issue about "1 microsecond" won't appear.
No. No matter how many phases, transaction is done and EC mutex is released
as soon as last write happens. EC driver never waited for confirmation for last written byte.
Quoted 5 lines
But in your patch(EC transaction is
done in interrupt context) when EC mutex is released , maybe the previous EC transaction is not really finished. In such case the issue of "1 microsecond" will appear.
Is what I said right?
No. my patch has same "optimization" as appeared in EC driver since introduction of interrupt mode. May be it is more visible now, and you managed to notice it, while still failing to see it in earlier EC driver versions? I agree that there are some benefits in not doing this optimization, thus I made the patch to drop it.
Quoted 4 lines
Quoted 4 lines
My patch did not introduce this behaviour, it was there since "burst mode" was introduced, and may be even earlier. You may ask Luming Yu, why he was so optimistic about 1 microsecond (is he still around? you did not include him into CC yet...).
Actually, I gave too much credit to Shanghai office... Interrupt mode patch was written by Dmitry Torokhov, not Luming Yu.
Quoted 2 lines
In my email what I said is only to point out the logic error. Maybe
No, see above.
Quoted 10 lines
there is no problem that the EC mutex is released before EC transaction is really finished. IMO this is still a logic error. The program logic states that EC transaction is already finished but the EC transaction is not really finished. They are inconsistent..
The more important is that the failure in EC transaction can't be
detected in time.Do you agree with my analysis?
No, see above.
Re: Spreading FUD [was: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)]
Quoted 11 lines · Alexey Starikovskiy
Yakui,
Please take a look at the subject of the thread -- notice words "fatal" and "(EC transaction is done in interrupt context)".
Looks horrible, isn't it? Now we begin to get into details, and it appears that it is not fatal, introduced 3.5 years ago, and has nothing to do with interrupt context transaction patch.
So, the whole point of your message is to spread FUD about "fast transaction" patch,
and show how great you are. I would guess that you've failed in achieving both goals.
I won't argue with you about this issue.
In your patch the program logic states that the EC transaction is
already finished. But in fact the EC transaction is not really finished. They are inconsistent. Why can't call this a fatal logic error?
The more important is that the failure in EC transaction can't be
detected in time.
Why can't call this a fatal logic error?
If this is not a fatal logic error, what can be called fatal? Like that Several commits are reverted each other as the following three commits.
a.7c010de7506954e973abfab5c5999c5a97f7a73e
b.4c611060660f0de3e9b8f02df207312bc6f5c331
c.f9319f903f898dd4b15dbc386499725ce6c59776If you still reply the subject so impolitely, Don't blame that I will forward the similar error to wider range.
Quoted 78 lines
If you cry "wolf!" too often, you'll get ignored, when the real wolf appears.
Now, for a positive example, the same message without exclamation marks...
---------------------------------------------------------------------------------------------------------------------------------
Subject: difference in EC transaction protocol
Hi Alex,
I've noticed difference in EC transaction protocol for write and burst-disable commands,
namely all that end with writing byte to EC -- they don't wait for last confirmation interrupt and
next transaction might start before current one completes.
May be this is the "root cause" we are looking for.
Regards,
Yakui
---------------------------------------------------------------------------------------------------------------------------------
Note that this message is shorter, and is not offensive. Smile, and people may start to like you...Regards,
Alex.
P.S. And I don't agree with your analysis as you still believe that before the fast transaction mutex was
not released at the same time as it is now.Quoted 11 lines · Zhao Yakui
Quoted 3 lines · Alexey Starikovskiy
Yakui,
This is not about my patch, this is about your ego...Thanks for the reply. I know that this is not introduced by your patch. In the previous kernel the EC transaction is explicitly divided into several phases. Only when EC transaction is really finished, the EC mutex lock is released. In such case the previous EC transaction is already finished when OS begins another EC transaction. So the issue about "1 microsecond" won't appear.
No. No matter how many phases, transaction is done and EC mutex is released as soon as last write happens. EC driver never waited for confirmation for last written byte.
Quoted 5 lines
But in your patch(EC transaction is
done in interrupt context) when EC mutex is released , maybe the previous EC transaction is not really finished. In such case the issue of "1 microsecond" will appear.
Is what I said right?No. my patch has same "optimization" as appeared in EC driver since introduction of interrupt mode.
May be it is more visible now, and you managed to notice it, while still failing to see it in earlier EC driver versions?
I agree that there are some benefits in not doing this optimization, thus I made the patch to drop it.Quoted 4 lines
Quoted 4 lines
My patch did not introduce this behaviour, it was there since "burst mode" was introduced, and may be even earlier. You may ask Luming Yu, why he was so optimistic about 1 microsecond (is he still around? you did not include him into CC yet...).
Actually, I gave too much credit to Shanghai office... Interrupt mode patch was written by Dmitry Torokhov, not Luming Yu.
Quoted 2 lines
In my email what I said is only to point out the logic error. Maybe
No, see above.
Quoted 10 lines
there is no problem that the EC mutex is released before EC transaction is really finished. IMO this is still a logic error. The program logic states that EC transaction is already finished but the EC transaction is not really finished. They are inconsistent..
The more important is that the failure in EC transaction can't be
detected in time.Do you agree with my analysis?
No, see above.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Spreading FUD [was: The fatal logic error in EC write transaction(EC transaction is done in interrupt context)]
Quoted 12 lines · Zhao Yakui
Quoted 10 lines · Alexey Starikovskiy
Yakui,
Please take a look at the subject of the thread -- notice words "fatal" and "(EC transaction is done in interrupt context)".
Looks horrible, isn't it? Now we begin to get into details, and it appears that it is not fatal, introduced 3.5 years ago, and has nothing to do with interrupt context transaction patch.
So, the whole point of your message is to spread FUD about "fast transaction" patch, and show how great you are. I would guess that you've failed in achieving both goals.
Quoted 2 lines
I won't argue with you about this issue.
Good.
Quoted 7 lines
In your patch the program logic states that the EC transaction is
already finished. But in fact the EC transaction is not really finished. They are inconsistent. Why can't call this a fatal logic error?
The more important is that the failure in EC transaction can't be
detected in time.
Why can't call this a fatal logic error?
Here is Merriam-Webster' definition of "fatal", look at use case number 4, which is closest to your use of it:
<a href="http://www.merriam-webster.com/dictionary/fatal">fatal</a>
As we lived with the problem you decribe all these years, it can not be described as "causing failure/death", thus it can not be called "fatal".
Quoted 8 lines
If this is not a fatal logic error, what can be called fatal? Like that Several commits are reverted each other as the following three commits.
a.7c010de7506954e973abfab5c5999c5a97f7a73e b.4c611060660f0de3e9b8f02df207312bc6f5c331 c.f9319f903f898dd4b15dbc386499725ce6c59776
If I had all these machines sitting on my desk side-by-side, I may have fixed them with one patch, otherwise one need to make rounds. Do you know any other solution?
Quoted 4 lines
If you still reply the subject so impolitely, Don't blame that I will forward the similar error to wider range.
You send false or bended statements not for the first time, do you expect any other treatment? You certainly could do whatever you want, but prepare for others to include you in ignore/spam list.
Regards,
Alex.
Len Brown
reviewerPATCH 8/21 intel_menlow: don't set max_state a negative value
In reply to Len BrownFrom: Zhang Rui <[email protected]>
max_state is unsigned long.
don't set max_state a negative value
--- drivers/misc/intel_menlow.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/misc/intel_menlow.c +4 -1
@@ -71,6 +71,9 @@ static int memory_get_int_max_bandwidth(struct thermal_cooling_device *cdev,if (ACPI_FAILURE (status))return -EFAULT ; +if (!value) +return -EINVAL ; + *max_state = value -1 ;return 0 ; }
@@ -121,7 +124,7 @@ static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev,if (memory_get_int_max_bandwidth (cdev, &max_state))return -EFAULT ; -if (max_state <0 || state > max_state) +if (state > max_state)return -EINVAL ; arg_list.count =1 ;
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 9/21 ACPI: remove comments about debug layer/level to use
In reply to Len BrownFrom: Bjorn Helgaas <[email protected]>
I don't think there's any point in cluttering the code with these. Better to improve the documentation so *anybody* can figure out what layer & level to use.
--- drivers/acpi/pci_root.c | 6 ------ drivers/acpi/video.c | 6 ------ 2 files changed, 0 insertions(+), 12 deletions(-)
drivers/acpi/pci_root.c +0 -6
@@ -376,15 +376,9 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)static int __init acpi_pci_root_init (void ) { -if (acpi_pci_disabled)return 0 ; -/* DEBUG: -acpi_dbg_layer = ACPI_PCI_COMPONENT; -acpi_dbg_level = 0xFFFFFFFF; -*/ -if (acpi_bus_register_driver (&acpi_pci_root_driver) <0 )return -ENODEV ;
drivers/acpi/video.c +0 -6
@@ -2094,12 +2094,6 @@ static int __init acpi_video_init(void) {int result =0 ; - -/* -acpi_dbg_level = 0xFFFFFFFF; -acpi_dbg_layer = 0x08000000; -*/ - acpi_video_dir =proc_mkdir (ACPI_VIDEO_CLASS , acpi_root_dir);if (!acpi_video_dir)return -ENODEV ;
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 10/21 ACPI: SBS: remove useless acpi_cm_sbs_init() initcall
In reply to Len BrownFrom: Bjorn Helgaas <[email protected]>
acpi_cm_sbs_init() doesn't do anything, so we can just remove it.
--- drivers/acpi/cm_sbs.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
drivers/acpi/cm_sbs.c +0 -6
@@ -105,9 +105,3 @@ void acpi_unlock_battery_dir(struct proc_dir_entry *acpi_battery_dir_param)return ; }EXPORT_SYMBOL (acpi_unlock_battery_dir); - -static int __init acpi_cm_sbs_init (void ) -{ -return 0 ; -} -subsys_initcall (acpi_cm_sbs_init);
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 11/21 ACPI: remove CONFIG_ACPI_POWER
In reply to Len BrownFrom: Bjorn Helgaas <[email protected]>
Remove CONFIG_ACPI_POWER. It was always set the same as CONFIG_ACPI, and it had no menu label, so there was no way to set it to anything other than "y".
The interfaces under CONFIG_ACPI_POWER (acpi_device_sleep_wake(), acpi_power_transition(), etc) are called unconditionally from the ACPI core, so we already depend on it always being present.
--- drivers/acpi/Kconfig | 4 ---- drivers/acpi/Makefile | 2 +- include/acpi/acpi_drivers.h | 2 -- 3 files changed, 1 insertions(+), 7 deletions(-)
drivers/acpi/Kconfig +0 -4
@@ -341,10 +341,6 @@ config ACPI_PCI_SLOT help you correlate PCI bus addresses with the physical geography of your slots. If you are unsure, say N. -config ACPI_POWER - bool - default y - config ACPI_SYSTEM bool default y
drivers/acpi/Makefile +1 -1
@@ -51,7 +51,7 @@ obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o obj-$(CONFIG_ACPI_PROCESSOR) += processor.o obj-$(CONFIG_ACPI_CONTAINER) += container.o obj-$(CONFIG_ACPI_THERMAL) += thermal.o -obj-$(CONFIG_ACPI_POWER) += power.o +obj-y += power.o obj-$(CONFIG_ACPI_SYSTEM) += system.o event.o obj-$(CONFIG_ACPI_DEBUG) += debug.o obj-$(CONFIG_ACPI_NUMA) += numa.o
include/acpi/acpi_drivers.h +0 -2
@@ -86,7 +86,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_device *device, int domain,Power Resource -------------------------------------------------------------------------- */ -#ifdef CONFIG_ACPI_POWER int acpi_device_sleep_wake (struct acpi_device *dev,int enable,int sleep_state,int dev_state);int acpi_enable_wakeup_device_power (struct acpi_device *dev,int sleep_state);
@@ -94,7 +93,6 @@ int acpi_disable_wakeup_device_power(struct acpi_device *dev);int acpi_power_get_inferred_state (struct acpi_device *device);int acpi_power_transition (struct acpi_device *device,int state);extern int acpi_power_nocheck; -#endif /* -------------------------------------------------------------------------- Embedded Controller
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 12/21 ACPI: remove CONFIG_ACPI_EC
In reply to Len BrownFrom: Bjorn Helgaas <[email protected]>
Remove CONFIG_ACPI_EC. It was always set the same as CONFIG_ACPI, and it had no menu label, so there was no way to set it to anything other than "y".
Per section 6.5.4 of the ACPI 3.0b specification,
OSPM must make Embedded Controller operation regions, accessed via the Embedded Controllers described in ECDT, available before executing any control method.
The ECDT table is optional, but if it is present, the above text means that the EC it describes is a required part of the ACPI subsystem, so CONFIG_ACPI_EC=n wouldn't make sense.
--- drivers/acpi/Kconfig | 8 -------- drivers/acpi/Makefile | 2 +- drivers/acpi/bus.c | 3 +-- drivers/char/sonypi.c | 4 ++-- drivers/misc/Kconfig | 4 ++-- include/acpi/acpi_drivers.h | 2 -- include/linux/acpi.h | 4 ---- 7 files changed, 6 insertions(+), 21 deletions(-)
drivers/acpi/Kconfig +0 -8
@@ -324,14 +324,6 @@ config ACPI_DEBUG_FUNC_TRACE ACPI Debug Statements slow down ACPI processing. Function trace is about half of the penalty and is rarely useful. -config ACPI_EC - bool - default y - help - This driver is required on some systems for the proper operation of - the battery and thermal drivers. If you are compiling for a - mobile system, say Y. - config ACPI_PCI_SLOT tristate "PCI slot detection driver" default n
drivers/acpi/Makefile +1 -1
@@ -39,7 +39,7 @@ obj-y += sleep/ obj-y += bus.o glue.o obj-y += scan.o # Keep EC driver first. Initialization of others depend on it. -obj-$(CONFIG_ACPI_EC) += ec.o +obj-y += ec.o obj-$(CONFIG_ACPI_AC) += ac.o obj-$(CONFIG_ACPI_BATTERY) += battery.o obj-$(CONFIG_ACPI_BUTTON) += button.o
drivers/acpi/bus.c +1 -2
@@ -774,7 +774,7 @@ static int __init acpi_bus_init(void) "Unable to initialize ACPI OS objects\n"); goto error1; } -#ifdef CONFIG_ACPI_EC +/* * ACPI 2.0 requires the EC driver to be loaded and work before * the EC device is found in the namespace (i.e. before acpi_initialize_objects()
@@ -785,7 +785,6 @@ static int __init acpi_bus_init(void)*/ status =acpi_ec_ecdt_probe ();/* Ignore result. Not having an ECDT is not fatal. */ -#endif status =acpi_initialize_objects (ACPI_FULL_INITIALIZATION );if (ACPI_FAILURE (status)) {
drivers/char/sonypi.c +2 -2
@@ -523,7 +523,7 @@ static int acpi_driver_registered;static int sonypi_ec_write (u8 addr,u8 value) { -#ifdef CONFIG_ACPI_EC +#ifdef CONFIG_ACPI if (SONYPI_ACPI_ACTIVE )return ec_write (addr, value);#endif
@@ -539,7 +539,7 @@ static int sonypi_ec_write(u8 addr, u8 value)static int sonypi_ec_read (u8 addr,u8 *value) { -#ifdef CONFIG_ACPI_EC +#ifdef CONFIG_ACPI if (SONYPI_ACPI_ACTIVE )return ec_read (addr, value);#endif
drivers/misc/Kconfig +2 -2
@@ -230,7 +230,7 @@ config HP_WMI config MSI_LAPTOP tristate "MSI Laptop Extras" depends on X86 - depends on ACPI_EC + depends on ACPI depends on BACKLIGHT_CLASS_DEVICE ---help--- This is a driver for laptops built by MSI (MICRO-STAR
@@ -260,7 +260,7 @@ config PANASONIC_LAPTOP config COMPAL_LAPTOP tristate "Compal Laptop Extras" depends on X86 - depends on ACPI_EC + depends on ACPI depends on BACKLIGHT_CLASS_DEVICE ---help--- This is a driver for laptops built by Compal:
include/acpi/acpi_drivers.h +0 -2
@@ -97,10 +97,8 @@ extern int acpi_power_nocheck;/* -------------------------------------------------------------------------- Embedded Controller -------------------------------------------------------------------------- */ -#ifdef CONFIG_ACPI_EC int acpi_ec_ecdt_probe (void );int acpi_boot_ec_enable (void ); -#endif /* -------------------------------------------------------------------------- Processor
include/linux/acpi.h +0 -4
@@ -163,8 +163,6 @@ struct acpi_pci_driver {int acpi_pci_register_driver (struct acpi_pci_driver *driver);void acpi_pci_unregister_driver (struct acpi_pci_driver *driver); -#ifdef CONFIG_ACPI_EC -extern int ec_read (u8 addr,u8 *val);extern int ec_write (u8 addr,u8 val); extern int ec_transaction(u8 command,
@@ -172,8 +170,6 @@ extern int ec_transaction(u8 command, u8 *rdata, unsigned rdata_len, int force_poll); -#endif /*CONFIG_ACPI_EC*/ -#if defined(CONFIG_ACPI_WMI ) || defined(CONFIG_ACPI_WMI_MODULE )typedef void (*wmi_notify_handler ) (u32 value,void *context);
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 13/21 intel_menlow: Add comment documenting legal GTHS values
In reply to Len BrownFrom: Thomas, Sujith <[email protected]>
--- drivers/misc/intel_menlow.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/misc/intel_menlow.c +5 -0
@@ -52,6 +52,11 @@ MODULE_LICENSE("GPL");#define MEMORY_ARG_CUR_BANDWIDTH 1#define MEMORY_ARG_MAX_BANDWIDTH 0 +/* +* GTHS returning 'n' would mean that [0,n-1] states are supported +* In that case max_cstate would be n-1 +* GTHS returning '0' would mean that no bandwidth control states are supported +*/ static int memory_get_int_max_bandwidth (struct thermal_cooling_device *cdev,unsigned long *max_state) {
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 14/21 intel_menlow: MAINTAINERS
In reply to Len BrownFrom: Thomas, Sujith <[email protected]>
--- MAINTAINERS | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
MAINTAINERS +7 -0
@@ -2178,6 +2178,13 @@ M: [email protected] L: [email protected] (moderated for non-subscribers) S: Maintained +INTEL MENLOW THERMAL DRIVER +P: Sujith Thomas +M: [email protected] +L: [email protected] +W: http://www.lesswatts.org/projects/acpi/ +S: Supported + INTEL IA32 MICROCODE UPDATE SUPPORT P: Tigran Aivazian M: [email protected]
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 15/21 Revert "ACPI: Ingore the RESET_REG_SUP bit when using ACPI reset mechanism"
In reply to Len BrownFrom: Len Brown <[email protected]>
This reverts commit 8fd145917fb62368a9b80db59562c20576238f5a.
http://bugzilla.kernel.org/show_bug.cgi?id=11942
--- drivers/acpi/reboot.c | 25 +++---------------------- 1 files changed, 3 insertions(+), 22 deletions(-)
drivers/acpi/reboot.c +3 -22
@@ -15,28 +15,9 @@ void acpi_reboot(void) rr = &acpi_gbl_FADT.reset_register; -/* -* Is the ACPI reset register supported? -* -* According to ACPI 3.0, FADT.flags.RESET_REG_SUP indicates -* whether the ACPI reset mechanism is supported. -* -* However, some boxes have this bit clear, yet a valid -* ACPI_RESET_REG & RESET_VALUE, and ACPI reboot is the only -* mechanism that works for them after S3. -* -* This suggests that other operating systems may not be checking -* the RESET_REG_SUP bit, and are using other means to decide -* whether to use the ACPI reboot mechanism or not. -* -* So when acpi reboot is requested, -* only the reset_register is checked. If the following -* conditions are met, it indicates that the reset register is supported. -* a. reset_register is not zero -* b. the access width is eight -* c. the bit_offset is zero -*/ -if (!(rr->address) || rr->bit_width !=8 || rr->bit_offset !=0 ) +/* Is the reset register supported? */ +if (!(acpi_gbl_FADT.flags &ACPI_FADT_RESET_REGISTER ) || + rr->bit_width !=8 || rr->bit_offset !=0 )return ; reset_value = acpi_gbl_FADT.reset_value;
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 16/21 PNP: add Bjorn Helgaas as PNP co-maintainer
In reply to Len BrownFrom: Bjorn Helgaas <[email protected]>
Update Adam's email address and add myself as PNP co-maintainer.
--- MAINTAINERS | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
MAINTAINERS +3 -1
@@ -3336,7 +3336,9 @@ S: Maintained PNP SUPPORT P: Adam Belay -M: [email protected] +M: [email protected] +P: Bjorn Helgaas +M: [email protected] S: Maintained PNXxxxx I2C DRIVER
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 17/21 ACPI: struct device - replace bus_id with dev_name(), dev_set_name()
In reply to Len BrownFrom: Kay Sievers <[email protected]>
This patch is part of a larger patch series which will remove the "char bus_id[20]" name string from struct device. The device name is managed in the kobject anyway, and without any size limitation, and just needlessly copied into "struct device".
To set and read the device name dev_name(dev) and dev_set_name(dev) must be used. If your code uses static kobjects, which it shouldn't do, "const char *init_name" can be used to statically provide the name the registered device should have. At registration time, the init_name field is cleared, to enforce the use of dev_name(dev) to access the device name at a later time.
We need to get rid of all occurrences of bus_id in the entire tree to be able to enable the new interface. Please apply this patch, and possibly convert any remaining remaining occurrences of bus_id.
We want to submit a patch to -next, which will remove bus_id from "struct device", to find the remaining pieces to convert, and finally switch over to the new api, which will remove the 20 bytes array and does no longer have a size limitation.
--- drivers/acpi/ac.c | 2 +- drivers/acpi/battery.c | 2 +- drivers/acpi/processor_core.c | 6 +++--- drivers/acpi/scan.c | 8 ++++---- drivers/acpi/sleep/proc.c | 4 ++-- drivers/acpi/thermal.c | 8 ++++---- drivers/acpi/wmi.c | 2 +- 7 files changed, 16 insertions(+), 16 deletions(-)
drivers/acpi/ac.c +1 -1
@@ -242,7 +242,7 @@ static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)acpi_ac_get_state (ac);acpi_bus_generate_proc_event (device, event, (u32 ) ac->state);acpi_bus_generate_netlink_event (device->pnp.device_class, - device->dev.bus_id, event, +dev_name (&device->dev), event, (u32 ) ac->state);#ifdef CONFIG_ACPI_SYSFS_POWER kobject_uevent (&ac->charger.dev->kobj,KOBJ_CHANGE );
drivers/acpi/battery.c +1 -1
@@ -782,7 +782,7 @@ static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)acpi_bus_generate_proc_event (device, event,acpi_battery_present (battery));acpi_bus_generate_netlink_event (device->pnp.device_class, - device->dev.bus_id, event, +dev_name (&device->dev), event,acpi_battery_present (battery));#ifdef CONFIG_ACPI_SYSFS_POWER /* acpi_batter_update could remove power_supply object */
drivers/acpi/processor_core.c +3 -3
@@ -761,20 +761,20 @@ static void acpi_processor_notify(acpi_handle handle, u32 event, void *data)acpi_bus_generate_proc_event (device, event, pr->performance_platform_limit);acpi_bus_generate_netlink_event (device->pnp.device_class, - device->dev.bus_id, event, +dev_name (&device->dev), event, pr->performance_platform_limit);break ;case ACPI_PROCESSOR_NOTIFY_POWER :acpi_processor_cst_has_changed (pr);acpi_bus_generate_proc_event (device, event,0 );acpi_bus_generate_netlink_event (device->pnp.device_class, - device->dev.bus_id, event,0 ); +dev_name (&device->dev), event,0 );break ;case ACPI_PROCESSOR_NOTIFY_THROTTLING :acpi_processor_tstate_has_changed (pr);acpi_bus_generate_proc_event (device, event,0 );acpi_bus_generate_netlink_event (device->pnp.device_class, - device->dev.bus_id, event,0 ); +dev_name (&device->dev), event,0 );default :ACPI_DEBUG_PRINT ((ACPI_DB_INFO ,"Unsupported event [0x%x]\n" , event));
drivers/acpi/scan.c +4 -4
@@ -109,8 +109,7 @@ static int acpi_bus_hot_remove_device(void *context)return 0 ;ACPI_DEBUG_PRINT ((ACPI_DB_INFO , -"Hot-removing device %s...\n" , device->dev.bus_id)); - +"Hot-removing device %s...\n" ,dev_name (&device->dev)));if (acpi_bus_trim (device,1 )) { printk(KERN_ERR PREFIX
@@ -460,7 +459,7 @@ static int acpi_device_register(struct acpi_device *device, acpi_device_bus_id->instance_no =0 ;list_add_tail (&acpi_device_bus_id->node, &acpi_bus_id_list); } -sprintf (device->dev.bus_id,"%s:%02x" , acpi_device_bus_id->bus_id, acpi_device_bus_id->instance_no); +dev_set_name (&device->dev,"%s:%02x" , acpi_device_bus_id->bus_id, acpi_device_bus_id->instance_no);if (device->parent) {list_add_tail (&device->node, &device->parent->children);
@@ -484,7 +483,8 @@ static int acpi_device_register(struct acpi_device *device, result =acpi_device_setup_files (device);if (result) - printk(KERN_ERR PREFIX "Error creating sysfs interface for device %s\n", device->dev.bus_id); + printk(KERN_ERR PREFIX "Error creating sysfs interface for device %s\n", +dev_name (&device->dev)); device->removal_type =ACPI_BUS_REMOVAL_NORMAL ;return 0 ;
drivers/acpi/sleep/proc.c +2 -2
@@ -366,8 +366,8 @@ acpi_system_wakeup_device_seq_show(struct seq_file *seq, void *offset) dev->wakeup.state.enabled ? "enabled" : "disabled");if (ldev)seq_printf (seq,"%s:%s" , - ldev->bus ? ldev->bus->name :"no-bus" , - ldev->bus_id); +dev_name (ldev) ? ldev->bus->name :"no-bus" , +dev_name (ldev));seq_printf (seq,"\n" );put_device (ldev);
drivers/acpi/thermal.c +4 -4
@@ -576,7 +576,7 @@ static int acpi_thermal_critical(struct acpi_thermal *tz)acpi_bus_generate_proc_event (tz->device,ACPI_THERMAL_NOTIFY_CRITICAL , tz->trips.critical.flags.enabled);acpi_bus_generate_netlink_event (tz->device->pnp.device_class, - tz->device->dev.bus_id, +dev_name (&tz->device->dev),ACPI_THERMAL_NOTIFY_CRITICAL , tz->trips.critical.flags.enabled);
@@ -605,7 +605,7 @@ static int acpi_thermal_hot(struct acpi_thermal *tz)acpi_bus_generate_proc_event (tz->device,ACPI_THERMAL_NOTIFY_HOT , tz->trips.hot.flags.enabled);acpi_bus_generate_netlink_event (tz->device->pnp.device_class, - tz->device->dev.bus_id, +dev_name (&tz->device->dev),ACPI_THERMAL_NOTIFY_HOT , tz->trips.hot.flags.enabled);
@@ -1592,14 +1592,14 @@ static void acpi_thermal_notify(acpi_handle handle, u32 event, void *data)acpi_thermal_check (tz);acpi_bus_generate_proc_event (device, event,0 );acpi_bus_generate_netlink_event (device->pnp.device_class, - device->dev.bus_id, event,0 ); +dev_name (&device->dev), event,0 );break ;case ACPI_THERMAL_NOTIFY_DEVICES :acpi_thermal_trips_update (tz,ACPI_TRIPS_REFRESH_DEVICES );acpi_thermal_check (tz);acpi_bus_generate_proc_event (device, event,0 );acpi_bus_generate_netlink_event (device->pnp.device_class, - device->dev.bus_id, event,0 ); +dev_name (&device->dev), event,0 );break ;default : ACPI_DEBUG_PRINT((ACPI_DB_INFO,
drivers/acpi/wmi.c +1 -1
@@ -660,7 +660,7 @@ static void acpi_wmi_notify(acpi_handle handle, u32 event, void *data) wblock->handler (event, wblock->handler_data);acpi_bus_generate_netlink_event ( - device->pnp.device_class, device->dev.bus_id, + device->pnp.device_class,dev_name (&device->dev), event,0 );break ; }
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 18/21 fujitsu-laptop: fix section mismatch warning
In reply to Len BrownFrom: Randy Dunlap <[email protected]>
Could fix a bug in a hotplug add scenario.
WARNING: drivers/misc/fujitsu-laptop.o(.text+0xbde): Section mismatch in reference from the function acpi_fujitsu_add() to the variable .init.data:fujitsu_dmi_table The function acpi_fujitsu_add() references
the variable __initdata fujitsu_dmi_table.
This is often because acpi_fujitsu_add lacks a __initdata
annotation or the annotation of fujitsu_dmi_table is wrong.
--- drivers/misc/fujitsu-laptop.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/misc/fujitsu-laptop.c +1 -1
@@ -473,7 +473,7 @@ static int dmi_check_cb_p8010(const struct dmi_system_id *id)return 0 ; } -static struct dmi_system_id __initdata fujitsu_dmi_table[] = { +static struct dmi_system_id fujitsu_dmi_table[] = { { .ident ="Fujitsu Siemens S6410" , .matches = {
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 19/21 ACPI: use macro to replace hard number
In reply to Len BrownFrom: Yinghai Lu <[email protected]>
Impact: cleanup
Use MACRO for rev 3 fadt id instead of 3 directly.
--- drivers/acpi/tables/tbfadt.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/acpi/tables/tbfadt.c +1 -1
@@ -304,7 +304,7 @@ static void acpi_tb_convert_fadt(void)* The ACPI 1.0 reserved fields that will be zeroed are the bytes located at * offset 45, 55, 95, and the word located at offset 109, 110. */ -if (acpi_gbl_FADT.header.revision <3 ) { +if (acpi_gbl_FADT.header.revision <FADT2_REVISION_ID ) { acpi_gbl_FADT.preferred_profile =0 ; acpi_gbl_FADT.pstate_control =0 ; acpi_gbl_FADT.cst_control =0 ;
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 20/21 ACPI: avoid empty file name in sysfs
In reply to Len BrownFrom: Peter Gruber <[email protected]>
Since commit bc45b1d39a925b56796bebf8a397a0491489d85c acpi tables are allowed to have an empty signature and /sys/firmware/acpi/tables uses the signature as filename. Applications using naive recursion through /sys loop forever. A possible solution would be: (replacing the zero length filename with the string "NULL")
http://bugzilla.kernel.org/show_bug.cgi?id=11539
--- drivers/acpi/system.c | 25 +++++++++++++++++-------- 1 files changed, 17 insertions(+), 8 deletions(-)
drivers/acpi/system.c +17 -8
@@ -78,9 +78,15 @@ static ssize_t acpi_table_show(struct kobject *kobj, container_of(bin_attr, struct acpi_table_attr, attr);struct acpi_table_header *table_header =NULL ;acpi_status status; +char name[ACPI_NAME_SIZE ]; + +if (strncmp (table_attr->name,"NULL" ,4 )) +memcpy (name, table_attr->name,ACPI_NAME_SIZE ); +else +memcpy (name,"\0\0\0\0" ,4 ); status = -acpi_get_table (table_attr->name, table_attr->instance, +acpi_get_table (name, table_attr->instance, &table_header);if (ACPI_FAILURE (status))return -ENODEV ;
@@ -95,21 +101,24 @@ static void acpi_table_attr_init(struct acpi_table_attr *table_attr,struct acpi_table_header *header =NULL ;struct acpi_table_attr *attr =NULL ; -memcpy (table_attr->name, table_header->signature,ACPI_NAME_SIZE ); +if (table_header->signature[0 ] !='\0' ) +memcpy (table_attr->name, table_header->signature, +ACPI_NAME_SIZE ); +else +memcpy (table_attr->name,"NULL" ,4 );list_for_each_entry (attr, &acpi_table_attr_list, node) { -if (!memcmp (table_header->signature, attr->name, -ACPI_NAME_SIZE )) +if (!memcmp (table_attr->name, attr->name,ACPI_NAME_SIZE ))if (table_attr->instance < attr->instance) table_attr->instance = attr->instance; } table_attr->instance++;if (table_attr->instance >1 || (table_attr->instance ==1 && - !acpi_get_table (table_header-> - signature,2 , - &header))) -sprintf (table_attr->name +4 ,"%d" , table_attr->instance); + !acpi_get_table + (table_header->signature,2 , &header))) +sprintf (table_attr->name +ACPI_NAME_SIZE ,"%d" , + table_attr->instance); table_attr->attr.size =0 ; table_attr->attr.read = acpi_table_show;
Signature · 1 line
1.5.6.5
Len Brown
reviewerPATCH 21/21 ACPI: EC: make kernel messages more useful when GPE storm is detected
In reply to Len BrownFrom: Alan Jenkins <[email protected]>
Make sure we can tell if the GPE storm workaround gets activated, and avoid flooding the logs afterwards.
http://bugzilla.kernel.org/show_bug.cgi?id=11841
"plenty of line "ACPI: EC: non-query interrupt received,
switching to interrupt mode" in dmesg"
--- drivers/acpi/ec.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)
drivers/acpi/ec.c +10 -3
@@ -286,7 +286,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,acpi_enable_gpe (NULL , ec->gpe,ACPI_NOT_ISR ); }else if (test_bit (EC_FLAGS_GPE_MODE , &ec->flags) && t->irq_count >ACPI_EC_STORM_THRESHOLD ) { -pr_debug (PREFIX "GPE storm detected\n" ); +pr_info (PREFIX "GPE storm detected, " +"transactions will use polling mode\n" );set_bit (EC_FLAGS_GPE_STORM , &ec->flags); }return ret;
@@ -566,9 +567,15 @@ static u32 acpi_ec_gpe_handler(void *data)if (!test_bit (EC_FLAGS_GPE_MODE , &ec->flags) && !test_bit (EC_FLAGS_NO_GPE , &ec->flags)) {/* this is non-query, must be confirmation */ -if (printk_ratelimit ()) -pr_info (PREFIX "non-query interrupt received," +if (!test_bit (EC_FLAGS_GPE_STORM , &ec->flags)) { +if (printk_ratelimit ()) +pr_info (PREFIX "non-query interrupt received," +" switching to interrupt mode\n" ); + }else { +/* hush, STORM switches the mode every transaction */ +pr_debug (PREFIX "non-query interrupt received," " switching to interrupt mode\n" ); + }set_bit (EC_FLAGS_GPE_MODE , &ec->flags); }return ACPI_INTERRUPT_HANDLED ;
Signature · 1 line
1.5.6.5
Re: [PATCH 21/21] ACPI: EC: make kernel messages more useful when GPE storm is detected
Quoted 5 lines · Len Brown
From: Alan Jenkins <[email protected]>
Make sure we can tell if the GPE storm workaround gets activated, and avoid flooding the logs afterwards.
Quoted 8 lines
http://bugzilla.kernel.org/show_bug.cgi?id=11841
"plenty of line "ACPI: EC: non-query interrupt received,
switching to interrupt mode" in dmesg"Signed-off-by Alan Jenkins <[email protected]>Signed-off-by Len Brown <[email protected]>
I see the EC patches that cause this issue are part of 2.6.27-stable. So I think this patch should also be applied to the -stable tree.
Thanks
Alan
Re: [PATCH 21/21] ACPI: EC: make kernel messages more useful when GPE storm is detected
Quoted 10 lines · Alan Jenkins
Quoted 9 lines · Len Brown
From: Alan Jenkins <[email protected]>
Make sure we can tell if the GPE storm workaround gets activated, and avoid flooding the logs afterwards.
http://bugzilla.kernel.org/show_bug.cgi?id=11841
"plenty of line "ACPI: EC: non-query interrupt received,
switching to interrupt mode" in dmesg"
Quoted 6 lines
Quoted 4 lines
Signed-off-by Alan Jenkins <[email protected]>Signed-off-by Len Brown <[email protected]>I see the EC patches that cause this issue are part of 2.6.27-stable. So I think this patch should also be applied to the -stable tree.
Good point, it should.
Thanks,
Rafael
Re: [stable] [PATCH 21/21] ACPI: EC: make kernel messages more useful when GPE storm is detected
Quoted 11 lines · Rafael J. Wysocki
Quoted 10 lines · Alan Jenkins
Quoted 9 lines · Len Brown
From: Alan Jenkins <[email protected]>
Make sure we can tell if the GPE storm workaround gets activated, and avoid flooding the logs afterwards.
http://bugzilla.kernel.org/show_bug.cgi?id=11841
"plenty of line "ACPI: EC: non-query interrupt received,
switching to interrupt mode" in dmesg"
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Signed-off-by Alan Jenkins <[email protected]>Signed-off-by Len Brown <[email protected]>I see the EC patches that cause this issue are part of 2.6.27-stable. So I think this patch should also be applied to the -stable tree.
Good point, it should.
Ok, what is the git commit id that should be added to the .27-stable tree?
thanks,
greg k-h
Re: [stable] [PATCH 21/21] ACPI: EC: make kernel messages more useful when GPE storm is detected
Quoted 16 lines · Greg KH
Quoted 15 lines · Rafael J. Wysocki
Quoted 14 lines · Alan Jenkins
Quoted 13 lines · Len Brown
From: Alan Jenkins <[email protected]>
Make sure we can tell if the GPE storm workaround gets activated, and avoid flooding the logs afterwards.
http://bugzilla.kernel.org/show_bug.cgi?id=11841
"plenty of line "ACPI: EC: non-query interrupt received,
switching to interrupt mode" in dmesg"Signed-off-by Alan Jenkins <[email protected]>Signed-off-by Len Brown <[email protected]>
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I see the EC patches that cause this issue are part of 2.6.27-stable. So I think this patch should also be applied to the -stable tree.
Good point, it should.
Ok, what is the git commit id that should be added to the .27-stable tree?
Not yet in the Linus' tree, I'll let you know when it's merged.
Thanks,
Rafael
Re: [stable] [PATCH 21/21] ACPI: EC: make kernel messages more useful when GPE storm is detected
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Greg KH
Quoted 18 lines · Rafael J. Wysocki
Quoted 16 lines · Alan Jenkins
Quoted 5 lines · Len Brown
From: Alan Jenkins <[email protected]>
Make sure we can tell if the GPE storm workaround gets activated, and avoid flooding the logs afterwards.
Quoted 8 lines
http://bugzilla.kernel.org/show_bug.cgi?id=11841
"plenty of line "ACPI: EC: non-query interrupt received,
switching to interrupt mode" in dmesg"Signed-off-by Alan Jenkins <[email protected]>Signed-off-by Len Brown <[email protected]>I see the EC patches that cause this issue are part of 2.6.27-stable. So I think this patch should also be applied to the -stable tree.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Good point, it should.
Ok, what is the git commit id that should be added to the .27-stable tree?
Not yet in the Linus' tree, I'll let you know when it's merged.
Thank you for doing this.
greg k-h
Re: [stable] [PATCH 21/21] ACPI: EC: make kernel messages more useful when GPE storm is detected
Quoted 21 lines · Greg KH
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Greg KH
Quoted 18 lines · Rafael J. Wysocki
Quoted 16 lines · Alan Jenkins
Quoted 5 lines · Len Brown
From: Alan Jenkins <[email protected]>
Make sure we can tell if the GPE storm workaround gets activated, and avoid flooding the logs afterwards.
Quoted 8 lines
http://bugzilla.kernel.org/show_bug.cgi?id=11841
"plenty of line "ACPI: EC: non-query interrupt received,
switching to interrupt mode" in dmesg"Signed-off-by Alan Jenkins <[email protected]>Signed-off-by Len Brown <[email protected]>I see the EC patches that cause this issue are part of 2.6.27-stable. So I think this patch should also be applied to the -stable tree.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Good point, it should.
Ok, what is the git commit id that should be added to the .27-stable tree?
Not yet in the Linus' tree, I'll let you know when it's merged.
Thank you for doing this.
Here:
The other EC patches in this merge are also regarded as -stable material, but let's wait a couple of days to avoid possible regressions (although Alex thinks there sould be none ;-)).
Thanks,
Rafael
Re: [stable] [PATCH 21/21] ACPI: EC: make kernel messages more useful when GPE storm is detected
Quoted 31 lines · Rafael J. Wysocki
Quoted 21 lines · Greg KH
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Greg KH
Quoted 18 lines · Rafael J. Wysocki
Quoted 16 lines · Alan Jenkins
Quoted 5 lines · Len Brown
From: Alan Jenkins <[email protected]>
Make sure we can tell if the GPE storm workaround gets activated, and avoid flooding the logs afterwards.
Quoted 8 lines
http://bugzilla.kernel.org/show_bug.cgi?id=11841
"plenty of line "ACPI: EC: non-query interrupt received,
switching to interrupt mode" in dmesg"Signed-off-by Alan Jenkins <[email protected]>Signed-off-by Len Brown <[email protected]>I see the EC patches that cause this issue are part of 2.6.27-stable. So I think this patch should also be applied to the -stable tree.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Good point, it should.
Ok, what is the git commit id that should be added to the .27-stable tree?
Not yet in the Linus' tree, I'll let you know when it's merged.
Thank you for doing this.
Quoted 7 lines
Here:
The other EC patches in this merge are also regarded as -stable material, but let's wait a couple of days to avoid possible regressions (although Alex thinks there sould be none ;-)).
Now applied.
If there's other git ids that should also be added, please let me know what they are.
thanks,
greg k-h
Quoted 3 lines · Len Brown
please speak up if you see any reason these
pathces should not go into 2.6.28-rc3.
You still did not include video brightness patches from Thomas; nor are they in acpi-test. Are they delayed for -29 or rejected? If they are rejected it would be nice to know why so we could fix them ...
Len Brown
reviewerQuoted 5 lines
You still did not include video brightness patches from Thomas; nor are they in acpi-test. Are they delayed for -29 or rejected? If they are rejected it would be nice to know why so we could fix them ...
hmmm, yeah, several folks have pointed this out,
not sure what happened there, but will follow up.
thanks,
-Len