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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Mon, Jul 04, 2016 at 10:22:12AM +0200, Jean Delvare wrote:
> 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?

The whole point of this patch is that we expect that nobody never uses
that OpRegion. I'm 99% sure you don't find a single machine where it is
actually in use.

The fallback code is there just to be sure we do not blow things up if
it turns out some machine is using that.

I think the best we can do is to lock down the module (prevent it from
unloading) if we notice that the OpRegion is used.