Re: [v5] i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR

Hi Mika,

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

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.

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

Correct, whatever we do.

-- 
Jean Delvare
SUSE L3 Support