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

["Pali Rohár" <pali.rohar@gmail.com>]

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

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