Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Jean Delvare]

ACPI people,

Do you have any objection to the patch below, or can I push it upstream?

On Tue, 9 Mar 2010 13:56:36 +0100, Jean Delvare wrote:
> When the user passes the kernel parameter acpi_enforce_resources=lax,
> the ACPI resources are no longer protected, so a native driver can
> make use of them. In that case, we do not want the asus_atk0110 to be
> loaded. Unfortunately, this driver loads automatically due to its
> MODULE_DEVICE_TABLE, so the user ends up with two drivers loaded for
> the same device - this is bad.
> 
> So I suggest that we prevent the asus_atk0110 driver from loading if
> acpi_enforce_resources=lax.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Luca Tettamanti <kronos.it@gmail.com>
> Cc: Len Brown <lenb@kernel.org>
> ---
> Luca, what do you think? I had the idea to write this patch after
> seeing this bug report:
> https://bugzilla.novell.com/show_bug.cgi?id=580988
> I'm not sure if the driver conflict is the root cause of the problem,
> but it certainly did not help.
> 
> Another approach would be to hide the ATK0110 ACPI device itself, but I
> don't know if it is possible?
> 
>  drivers/acpi/osl.c           |    9 +++++++++
>  drivers/hwmon/asus_atk0110.c |    7 +++++++
>  include/linux/acpi.h         |    2 ++
>  3 files changed, 18 insertions(+)
> 
> --- linux-2.6.34-rc1.orig/drivers/acpi/osl.c	2010-03-09 08:24:52.000000000 +0100
> +++ linux-2.6.34-rc1/drivers/acpi/osl.c	2010-03-09 13:38:49.000000000 +0100
> @@ -1206,6 +1206,15 @@ int acpi_check_mem_region(resource_size_
>  EXPORT_SYMBOL(acpi_check_mem_region);
>  
>  /*
> + * Let drivers know whether the resource checks are effective
> + */
> +int acpi_resources_are_enforced(void)
> +{
> +	return acpi_enforce_resources == ENFORCE_RESOURCES_STRICT;
> +}
> +EXPORT_SYMBOL(acpi_resources_are_enforced);
> +
> +/*
>   * Acquire a spinlock.
>   *
>   * handle is a pointer to the spinlock_t.
> --- linux-2.6.34-rc1.orig/drivers/hwmon/asus_atk0110.c	2010-02-25 09:12:22.000000000 +0100
> +++ linux-2.6.34-rc1/drivers/hwmon/asus_atk0110.c	2010-03-09 13:23:50.000000000 +0100
> @@ -1406,6 +1406,13 @@ static int __init atk0110_init(void)
>  {
>  	int ret;
>  
> +	/* Make sure it's safe to access the device through ACPI */
> +	if (!acpi_resources_are_enforced()) {
> +		pr_err("atk: Resources not safely usable due to "
> +		       "acpi_enforce_resources kernel parameter\n");
> +		return -EBUSY;
> +	}
> +
>  	ret = acpi_bus_register_driver(&atk_driver);
>  	if (ret)
>  		pr_info("atk: acpi_bus_register_driver failed: %d\n", ret);
> --- linux-2.6.34-rc1.orig/include/linux/acpi.h	2010-02-25 09:13:22.000000000 +0100
> +++ linux-2.6.34-rc1/include/linux/acpi.h	2010-03-09 13:46:33.000000000 +0100
> @@ -247,6 +247,8 @@ int acpi_check_region(resource_size_t st
>  int acpi_check_mem_region(resource_size_t start, resource_size_t n,
>  		      const char *name);
>  
> +int acpi_resources_are_enforced(void);
> +
>  #ifdef CONFIG_PM_SLEEP
>  void __init acpi_no_s4_hw_signature(void);
>  void __init acpi_old_suspend_ordering(void);
> 
> 


-- 
Jean Delvare

Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Matthew Garrett]

On Tue, Mar 30, 2010 at 12:03:19PM +0200, Jean Delvare wrote:
> ACPI people,
> 
> Do you have any objection to the patch below, or can I push it upstream?

If you pass that boot option, you're already indicating that you don't 
care about multiple things accessing the same resources simultaneously. 
I think it's reasonable to leave things up to the user to configure at 
that point.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Jean Delvare]

Hi Matthew,

On Tue, 30 Mar 2010 14:21:32 +0100, Matthew Garrett wrote:
> On Tue, Mar 30, 2010 at 12:03:19PM +0200, Jean Delvare wrote:
> > ACPI people,
> > 
> > Do you have any objection to the patch below, or can I push it upstream?
> 
> If you pass that boot option, you're already indicating that you don't 
> care about multiple things accessing the same resources simultaneously. 
> I think it's reasonable to leave things up to the user to configure at 
> that point.

The asus_atk0110 driver loads automatically. The user doesn't configure
anything, and it just loads. That's the problem. If the driver didn't
load automatically, I wouldn't care. If you know of any way to prevent
the asus_atk0110 driver from auto-loading as soon as
acpi_enforce_resouce=lax or =no is passed, I am fine with this as well.
This is what I wanted to do originally, but I couldn't find a way to do
it, which is why I went for the more radical option of preventing the
asus_atk0110 driver from loading.

Thanks,
-- 
Jean Delvare

Re: [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Matthew Garrett]

On Tue, Mar 30, 2010 at 09:47:32PM +0200, Jean Delvare wrote:

> The asus_atk0110 driver loads automatically. The user doesn't configure
> anything, and it just loads. That's the problem. If the driver didn't
> load automatically, I wouldn't care. If you know of any way to prevent
> the asus_atk0110 driver from auto-loading as soon as
> acpi_enforce_resouce=lax or =no is passed, I am fine with this as well.
> This is what I wanted to do originally, but I couldn't find a way to do
> it, which is why I went for the more radical option of preventing the
> asus_atk0110 driver from loading.

echo blacklist asus_atk0110 >/etc/modprobe.d/hwmon_blacklist.conf

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Jean Delvare]

On Tue, 30 Mar 2010 20:48:36 +0100, Matthew Garrett wrote:
> On Tue, Mar 30, 2010 at 09:47:32PM +0200, Jean Delvare wrote:
> 
> > The asus_atk0110 driver loads automatically. The user doesn't configure
> > anything, and it just loads. That's the problem. If the driver didn't
> > load automatically, I wouldn't care. If you know of any way to prevent
> > the asus_atk0110 driver from auto-loading as soon as
> > acpi_enforce_resouce=lax or =no is passed, I am fine with this as well.
> > This is what I wanted to do originally, but I couldn't find a way to do
> > it, which is why I went for the more radical option of preventing the
> > asus_atk0110 driver from loading.
> 
> echo blacklist asus_atk0110 >/etc/modprobe.d/hwmon_blacklist.conf

Sure, but why do you insist on having the user configure this manually
when we can automate this at the kernel level? When
acpi_enforce_resouce=yes, the kernel doesn't let non-ACPI driver be
loaded, so I fail to see why we let ACPI drivers (for which we also
have native drivers) load when acpi_enforce_resouce=no.

Or is there something really wrong with my patch?

-- 
Jean Delvare

Re: [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Matthew Garrett]

On Tue, Mar 30, 2010 at 10:32:51PM +0200, Jean Delvare wrote:

> Sure, but why do you insist on having the user configure this manually
> when we can automate this at the kernel level? When
> acpi_enforce_resouce=yes, the kernel doesn't let non-ACPI driver be
> loaded, so I fail to see why we let ACPI drivers (for which we also
> have native drivers) load when acpi_enforce_resouce=no.

Because the situation with the asus driver loaded isn't obviously any 
worse than not having it loaded. The user is telling us that they're 
happy with racy access to their hwmon hardware. Automatically blocking 
the loading of the ACPI driver does nothing other than imply to the user 
that things are safe, when in reality they're anything but.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Luca Tettamanti]

On Tue, Mar 30, 2010 at 11:10 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Mar 30, 2010 at 10:32:51PM +0200, Jean Delvare wrote:
>
>> Sure, but why do you insist on having the user configure this manually
>> when we can automate this at the kernel level? When
>> acpi_enforce_resouce=yes, the kernel doesn't let non-ACPI driver be
>> loaded, so I fail to see why we let ACPI drivers (for which we also
>> have native drivers) load when acpi_enforce_resouce=no.
>
> Because the situation with the asus driver loaded isn't obviously any
> worse than not having it loaded. The user is telling us that they're
> happy with racy access to their hwmon hardware.

"lax" option is typically used when the user wants to load the native
driver; in this case we know that it's not safe to load asus driver.

Luca

Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Matthew Garrett]

On Tue, Mar 30, 2010 at 11:45:46PM +0200, Luca Tettamanti wrote:
> On Tue, Mar 30, 2010 at 11:10 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Because the situation with the asus driver loaded isn't obviously any
> > worse than not having it loaded. The user is telling us that they're
> > happy with racy access to their hwmon hardware.
> 
> "lax" option is typically used when the user wants to load the native
> driver; in this case we know that it's not safe to load asus driver.

It's not safe to load the native driver, full stop. Not loading the asus 
driver doesn't alter that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Jean Delvare]

On Tue, 30 Mar 2010 22:53:05 +0100, Matthew Garrett wrote:
> On Tue, Mar 30, 2010 at 11:45:46PM +0200, Luca Tettamanti wrote:
> > On Tue, Mar 30, 2010 at 11:10 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > > Because the situation with the asus driver loaded isn't obviously any
> > > worse than not having it loaded. The user is telling us that they're
> > > happy with racy access to their hwmon hardware.
> > 
> > "lax" option is typically used when the user wants to load the native
> > driver; in this case we know that it's not safe to load asus driver.
> 
> It's not safe to load the native driver, full stop. Not loading the asus 
> driver doesn't alter that.

It definitely does. At least on some of the affected boards, no access
to the I/O area in question is done at all as long as the asus_atk0110
driver is not loaded. So it is actually safe to pass
acpi_enforce_resources=lax and not load the asus_atk0110 driver. And
people are actually doing this because native drivers can control the
speed of their fans while the asus_atk0110 driver can't. So they see
the current situation as a regression.

Again, can you please point to problems my patch has?

-- 
Jean Delvare

Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Matthew Garrett]

On Wed, Mar 31, 2010 at 09:30:04AM +0200, Jean Delvare wrote:

> It definitely does. At least on some of the affected boards, no access
> to the I/O area in question is done at all as long as the asus_atk0110
> driver is not loaded.

You've audited the system management code?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Jean Delvare]

On Wed, 31 Mar 2010 13:51:23 +0100, Matthew Garrett wrote:
> On Wed, Mar 31, 2010 at 09:30:04AM +0200, Jean Delvare wrote:
> 
> > It definitely does. At least on some of the affected boards, no access
> > to the I/O area in question is done at all as long as the asus_atk0110
> > driver is not loaded.
> 
> You've audited the system management code?

No, I did not. But you did not review the system management code of all
computers out there either, and you are still writing kernel code that
could be broken by it. So what's the point?

-- 
Jean Delvare

Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Matthew Garrett]

On Wed, Mar 31, 2010 at 03:25:00PM +0200, Jean Delvare wrote:
> On Wed, 31 Mar 2010 13:51:23 +0100, Matthew Garrett wrote:
> > On Wed, Mar 31, 2010 at 09:30:04AM +0200, Jean Delvare wrote:
> > 
> > > It definitely does. At least on some of the affected boards, no access
> > > to the I/O area in question is done at all as long as the asus_atk0110
> > > driver is not loaded.
> > 
> > You've audited the system management code?
> 
> No, I did not. But you did not review the system management code of all
> computers out there either, and you are still writing kernel code that
> could be broken by it. So what's the point?

If the firmware tells us that a resource range using indexed access is 
reserved, it's not safe to assume that the firmware isn't using it.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [lm-sensors] [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Jean Delvare]

On Wed, 31 Mar 2010 14:27:11 +0100, Matthew Garrett wrote:
> If the firmware tells us that a resource range using indexed access is 
> reserved, it's not safe to assume that the firmware isn't using it.

Whether it is safe of not is irrelevant. The user is telling us he/she
_will_ use it nevertheless. So what is wrong with doing our best to
avoid problems?

The user paid for the hardware, and firmwares are often crappy. You
know that better than anyone else, having worked on ACPI for some time
now. 

-- 
Jean Delvare

Re: [PATCH] hwmon: (asus_atk0110) Don't load if ACPI resources aren't enforced

[Jean Delvare]

On Tue, 30 Mar 2010 22:10:49 +0100, Matthew Garrett wrote:
> On Tue, Mar 30, 2010 at 10:32:51PM +0200, Jean Delvare wrote:
> 
> > Sure, but why do you insist on having the user configure this manually
> > when we can automate this at the kernel level? When
> > acpi_enforce_resouce=yes, the kernel doesn't let non-ACPI driver be
> > loaded, so I fail to see why we let ACPI drivers (for which we also
> > have native drivers) load when acpi_enforce_resouce=no.
> 
> Because the situation with the asus driver loaded isn't obviously any 
> worse than not having it loaded. The user is telling us that they're 
> happy with racy access to their hwmon hardware.

No, they are not saying that. They are saying: I want to let native
drivers access ACPI-reserved I/O ports. That's all they are saying.
Attaching a single interpretation to this is incorrect. They might do
this because their BIOS improperly reserves ports it doesn't use. Or
because they prefer using a native driver over an ACPI driver for a
given chip. Or indeed because they want to crash their machine. Just
because it can't be called generally safe (and in all honesty is unsafe
in many cases, quite possibly the majority) doesn't mean there are no
cases where it makes sense. Hell, we lived without ACPI resource
reservation until kernel 2.6.32 and without the asus_atk0110 driver and
most people were totally happy with the situation.

> Automatically blocking 
> the loading of the ACPI driver does nothing other than imply to the user 
> that things are safe,

Again, no, it doesn't imply that. It implies that loading the
asus_atk0110 driver together with a native driver for the same device
is very bad and should never be done, period.

> when in reality they're anything but.

They sometimes are, this is the point.

-- 
Jean Delvare