On Monday 04 July 2016 10:22:12 Jean Delvare wrote:

Hi Mika,

On Wed, 29 Jun 2016 13:39:51 +0300, Mika Westerberg wrote:

quoted

On Fri, Jun 24, 2016 at 04:12:38PM +0200, Jean Delvare wrote:

quoted

I think Pali is correct. The only purpose of handling the region is to
detect that it is being accessed so we can set priv->acpi_reserved.
Once it is set, i801_acpi_io_handler becomes transparent: it forwards
the requests without doing anything with them. The very same would
happen if we would unregister the handler at that point, but without the
extra overhead.

So while the current code does work fine, unregistering the handler
when we set priv->acpi_reserved would be more optimal.

Unless both Pali and myself are missing something, that is.

I'm not sure unregistering the handler actually resets back to the
default handler.

I'm no ACPI expert. I read the code of
acpi_remove_address_space_handler() and a few other related ACPI
functions and can't claim I understood it all. But indeed it doesn't
look like it restores the original behavior. Probably
acpi_install_address_space_handler(..., ACPI_ADR_SPACE_SYSTEM_IO,
ACPI_DEFAULT_HANDLER, ...) should be used instead.

This raises another question though: if
acpi_remove_address_space_handler() doesn't restore the previous
behavior then we shouldn't be calling it when the driver is being
unloaded either. As I understand it, it breaks the ACPI handling of the
device.

However I can't test it, as the installed handler is never called
on my system. Can anyone test unloading the i2c-i801 driver on a system
where ACPI actually accesses the device?

After looking at the ACPI code, I am no longer convinced that restoring
the default handler would improve performance. The default handler
itself (acpi_ex_system_io_space_handler) has a lot of overhead. OTOH
this makes me wonder if it is really correct to call
acpi_os_read_port() and acpi_os_write_port() directly.
acpi_ex_system_io_space_handler() calls acpi_hw_read_port() and
acpi_hw_write_port() which perform additional checks. Actually it would
seem safer to call acpi_ex_system_io_space_handler() instead... if it
was exported. Oh well.

quoted

Besides, this patch has been already merged for a while
so it requires a followup patch on top.

Correct, whatever we do.

Now Martin Vajnar confirmed that accelerometer on his notebook working
fine with that patch. But it does not mean that we should not fix
address space handler code in i801 correctly...

Can some ACPI expert look at it? Jean already wrote some useful
informations.

-- 
Pali Rohár
pali.rohar@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html