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

[Jean Delvare <jdelvare@suse.de>]

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:
> > 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