ACPI: Set flag DOCK_UNDOCKING when triggered via sysfs

PATCH In review Holger Macht 3 participants 5 messages Mar 12, 2008 – Mar 13, 2008 ACPI

Share & tools

Holger Macht

author

2008-03-12 00:05 UTC #0 lore raw

PATCH 1/1 ACPI: Set flag DOCK_UNDOCKING when triggered via sysfs

begin_undock() is only called when triggered via a acpi notify handler (pressing the undock button on the dock station), but complete_undock() is always called after the eject. So if a undock is triggered through a sysfs write, the flag DOCK_UNDOCKING has to be set for the dock station, too. Otherwise this will freeze the system hard.

Signed-off-by Holger Macht <[email protected]>

---

drivers/acpi/dock.c +1 -0

index 307cef6..fa44fb9 100644
@@ -710,6 +710,7 @@ static ssize_t write_undock(struct device *dev, struct device_attribute *attr,
 	if (!count)
 		return -EINVAL;
 
+	begin_undock(dock_station);
 	ret = handle_eject_request(dock_station, ACPI_NOTIFY_EJECT_REQUEST);
 	return ret ? ret: count;
 }

Andrew Morton

2008-03-12 05:36 UTC #1 lore raw

Quoted 6 lines · On Wed, 12 Mar 2008 01:07:27 +0100 Holger Macht <[email protected]> wrote:

begin_undock() is only called when triggered via a acpi notify handler (pressing the undock button on the dock station), but complete_undock() is always called after the eject. So if a undock is triggered through a sysfs write, the flag DOCK_UNDOCKING has to be set for the dock station, too. Otherwise this will freeze the system hard.

We prefer not to make systems freeze hard.

Quoted 7 lines

---
drivers/acpi/dock.c +0 -0
index 307cef6..fa44fb9 100644

drivers/acpi/dock.c static ssize_t write_undock(struct device *dev, struct device_attribute *attr, · line 716

Quoted 7 lines

drivers/acpi/dock.c +1 -0

@@ -710,6 +710,7 @@ static ssize_t write_undock(struct device *dev, struct device_attribute *attr,
 	if (!count)
 		return -EINVAL;
 
+	begin_undock(dock_station);
 	ret = handle_eject_request(dock_station, ACPI_NOTIFY_EJECT_REQUEST);
 	return ret ? ret: count;
 }

I assume that this is needed in 2.6.24.x as well?

Holger Macht

author

2008-03-12 10:39 UTC #2 lore raw

Quoted 21 lines · Andrew Morton

Quoted 6 lines · On Wed, 12 Mar 2008 01:07:27 +0100 Holger Macht <[email protected]> wrote:
begin_undock() is only called when triggered via a acpi notify handler (pressing the undock button on the dock station), but complete_undock() is always called after the eject. So if a undock is triggered through a sysfs write, the flag DOCK_UNDOCKING has to be set for the dock station, too. Otherwise this will freeze the system hard.
We prefer not to make systems freeze hard.
Quoted 10 lines
---
drivers/acpi/dock.c +0 -0
index 307cef6..fa44fb9 100644
@@ -710,6 +710,7 @@ static ssize_t write_undock(struct device *dev, struct device_attribute *attr,
 	if (!count)
 		return -EINVAL;
 

Quoted 6 lines

drivers/acpi/dock.c static ssize_t write_undock(struct device *dev, struct device_attribute *attr, · line 716
Quoted 4 lines
drivers/acpi/dock.c +1 -0
+	begin_undock(dock_station);
 	ret = handle_eject_request(dock_station, ACPI_NOTIFY_EJECT_REQUEST);
 	return ret ? ret: count;
 }
I assume that this is needed in 2.6.24.x as well?

I tested this, and no, I weren't able to reproduce on 2.6.24.

Moreover, it seems that the freeze is related to some libata issue because it only freezes when a drive is in the dock station.

Nevertheless, I think the patch is correct. Maybe there's a race involved somewhere I'm not aware of. I really like to wait for Kristen to comment before pushing it into more trees.

Thanks,
Holger
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kristen Carlson Accardi

Acked-by

2008-03-13 17:27 UTC #3 lore raw

Quoted 36 lines · On Wed, 12 Mar 2008 11:40:35 +0100 Holger Macht <[email protected]> wrote:

Quoted 23 lines · Andrew Morton
Quoted 7 lines · On Wed, 12 Mar 2008 01:07:27 +0100 Holger Macht <[email protected]> wrote:
begin_undock() is only called when triggered via a acpi notify handler (pressing the undock button on the dock station), but complete_undock() is always called after the eject. So if a undock is triggered through a sysfs write, the flag DOCK_UNDOCKING has to be set for the dock station, too. Otherwise this will freeze the system hard.
We prefer not to make systems freeze hard.
Quoted 10 lines
---
drivers/acpi/dock.c +0 -0
index 307cef6..fa44fb9 100644
@@ -710,6 +710,7 @@ static ssize_t write_undock(struct device
 *dev, struct device_attribute *attr, if (!count)
 		return -EINVAL;
 
Quoted 6 lines
drivers/acpi/dock.c static ssize_t write_undock(struct device *dev, struct device_attribute *attr, · line 716
Quoted 4 lines
drivers/acpi/dock.c +1 -0
+	begin_undock(dock_station);
 	ret = handle_eject_request(dock_station,
 ACPI_NOTIFY_EJECT_REQUEST); return ret ? ret: count;
 }
I assume that this is needed in 2.6.24.x as well?
I tested this, and no, I weren't able to reproduce on 2.6.24.
Moreover, it seems that the freeze is related to some libata issue because it only freezes when a drive is in the dock station.

Quoted 7 lines

Nevertheless, I think the patch is correct. Maybe there's a race involved somewhere I'm not aware of. I really like to wait for Kristen to comment before pushing it into more trees.
Thanks,
Holger

Hum, I'm surprised this caused a freeze - you are sure this fixes your problem? This flag is never checked anywhere in the dock code, and if it's not set, it will not matter, it will still do the undock. The flags file is exported to user space via sysfs though, so maybe because the flag isn't being set, some user space script is doing something it shouldn't? The patch does seem correct though, so might as well add it.

Acked-by Kristen Carlson Accardi <[email protected]>

Holger Macht

author

2008-03-13 18:13 UTC #4 lore raw

Quoted 47 lines · Kristen Carlson Accardi

Quoted 36 lines · On Wed, 12 Mar 2008 11:40:35 +0100 Holger Macht <[email protected]> wrote:
Quoted 23 lines · Andrew Morton
Quoted 7 lines · On Wed, 12 Mar 2008 01:07:27 +0100 Holger Macht <[email protected]> wrote:
begin_undock() is only called when triggered via a acpi notify handler (pressing the undock button on the dock station), but complete_undock() is always called after the eject. So if a undock is triggered through a sysfs write, the flag DOCK_UNDOCKING has to be set for the dock station, too. Otherwise this will freeze the system hard.
We prefer not to make systems freeze hard.
Quoted 10 lines
---
drivers/acpi/dock.c +0 -0
index 307cef6..fa44fb9 100644
@@ -710,6 +710,7 @@ static ssize_t write_undock(struct device
 *dev, struct device_attribute *attr, if (!count)
 		return -EINVAL;
 
Quoted 6 lines
drivers/acpi/dock.c static ssize_t write_undock(struct device *dev, struct device_attribute *attr, · line 716
Quoted 4 lines
drivers/acpi/dock.c +1 -0
+	begin_undock(dock_station);
 	ret = handle_eject_request(dock_station,
 ACPI_NOTIFY_EJECT_REQUEST); return ret ? ret: count;
 }
I assume that this is needed in 2.6.24.x as well?
I tested this, and no, I weren't able to reproduce on 2.6.24.
Moreover, it seems that the freeze is related to some libata issue because it only freezes when a drive is in the dock station.
Quoted 7 lines
Nevertheless, I think the patch is correct. Maybe there's a race involved somewhere I'm not aware of. I really like to wait for Kristen to comment before pushing it into more trees.
Thanks,
Holger

Quoted 6 lines

Hum, I'm surprised this caused a freeze - you are sure this fixes your problem? This flag is never checked anywhere in the dock code, and if it's not set, it will not matter, it will still do the undock. The flags file is exported to user space via sysfs though, so maybe because the flag isn't being set, some user space script is doing something it shouldn't? The patch does seem correct though, so might as well add it.

No, now I know that it doesn't seem to be the root cause. It definitely helps here, so it seems to be a race.

My userspace does not read the flags file and is doing something bad in turn of it. The freeze is caused by HAL, which reads /dev/sr0 after docking, but just before libata detaches the device. We're currently about to fix this:

http://lkml.org/lkml/2008/3/11/418

However, it seems that the patch is not perfect yet :-(

Quoted 1 line

Acked-by Kristen Carlson Accardi <[email protected]>

Thanks,
Holger
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html