On Friday, 26 of September 2008, Alexey Starikovskiy wrote: > Rafael J. Wysocki wrote: > > On Friday, 26 of September 2008, Zhao, Yakui wrote: > > > >> Alan gives a suggestion that after processing the EC notification event, EC driver > >> should continue to issue the query command and process the EC notification > >> event until the query command returns zero. > >> But this suggestion will break my laptop. On my laptop when issuing the query > >> command, a non-zero query event is returned but it can't be processed.(There > >> is no corresponding ACPI _Qxx object). At the same time the SCI_EVT bit won't > >> be cleared. In such case OS can't exit the function of acpi_ec_query_handler, > >> which causes that the acpid kernel thread can't work well. > >> > > > > Well, perhaps we should exit acpi_ec_query_handler() if SCI_EVT is clear _or_ > > the processing of an event fails due to the lack of a _Qxx object? Alex? > > > > > 1. There is no such function acpi_ec_query_handler(). > 2. acpi_ec_gpe_query() does two things: > a) checks if EC returns non-zero query index. If EC has nothing to > query -- it return 0 and we stop. > b) we check index with stored available handlers, and if there is > such handler, we execute it. > This function is scheduled to run in response to raised SCI_EVT, and > as I know no EC which would clear SCI_EVT by itself (and it's against > spec), > there is no reason to check if it's raised inside the function. > SCI_EVT is cleared by EC in response to QUERY command issued by driver, > and this is always done in acpi_ec_gpe_query(). OK > We (Alan and me) already found that machines in 9998 would break if you try > to do several QUERY commands for single raise of SCI_EVT in a hope to "clean > the EC query buffer" -- EC would just return _same_ event over and over, > thus > driver does not try to do this. Please not mix patch from Alan (with > above query loop) > with my patch(it never had above query loop). > >> In fact according to my analysis IMO Alexey's patch will break my laptop. > >> > > > > That obviously would be a regression. > > > I highly suspect quality of such "analysis" based on 2 weeks of > conversation with > Yakui. Feel free to make your own judgment though... Well, I think your patch is correct. However, I'd like it to be tested on the Zhao Yakui's box anyway. > >> But now the laptop is not in my hand and I can't do the test. After I own the > >> laptop again, I will do the test and attach the corresponding info. > >> > > > > OK, how much time is it going to take, approximately? > > > > > >> At the same time there are two generic issues in this patch: > >> a. spin_lock is overkill. When 2000 EC I/O read/write are executed, the > >> CPU interrupt will be disabled for about 1.5ms. This will affect the normal > >> laptops. > >> > > > > OTOH, I think we need _some_ locking in there in order to prevent races from > > happening. > > > > > He used 1000 EC I/O per second before, and now it's raised to 2000 > just to make a good number. One full read of battery with SBS (most > irq-intensive) > is in order of 100 GPEs. You need to update your battery state 20 times > per second > to get this number. > I will not try to doubt 0.7usec of inb/outb operation to EC, but my > understanding > is that you don't wait while LPC transaction is over, so it is inb/outb > to south bridge > which needs to be counted. > Irq handler itself will disable interrupts as well, it will read several > ACPI GPE registers > in order to find GPE bit, which caused interrupt (first delay). > second, even the most dumb GPE EC handler needs to read status register > of EC > (second delay). > Now, my patch adds either read or write of EC data register to this > (third delay). > > So, in the very worst case of 0.7usec access to EC register (and free > GPE register access), > we will disable interrupt for 1.5usec instead of 0.7usec. > Notice, we enable interrupt between each of these 1.5usec intervals. > Notice also, spinlock does not add anything to these timings. OK, so the 2000 EC I/O operations per second are unrealistic and there's really nothing to worry about (in theory). > >> b. the local variable that resides in process stack space is used in > >> interrupt-context. > >> > > > > Which one? > > > > > He refers to the ec->curr (former ec->t). Hm, I'm not sure what the failing scenario in this case would be. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
