2.6.25 semantic change in bay handling?

2.6.25 semantic change in bay handling?

[Matthew Garrett]

48feb3c419508487becfb9ea3afcc54c3eac6d80 appears to flag a device as 
detached if an acpi eject request is received. In 2.6.24 and earlier, an 
eject request merely sent an event to userland which could then cleanly 
unmount the device and let the user know when it was safe to remove the 
drive. Removing the device would then send another acpi request that 
triggered the actual hotplug and bus rescan.

This seems like a regression - it's no longer possible to ensure that a 
bay device is cleanly unmounted. Was this really the desired behaviour? 
It should be noted that not all hardware sends the eject request at all 
(Thinkpads do, but Dell and HP laptops don't), so we can't depend on 
receiving this when dealing with a bay event.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: 2.6.25 semantic change in bay handling?

[Holger Macht]

On Mo 05. Mai - 23:33:57, Matthew Garrett wrote:
> 48feb3c419508487becfb9ea3afcc54c3eac6d80 appears to flag a device as 
> detached if an acpi eject request is received. In 2.6.24 and earlier, an 
> eject request merely sent an event to userland which could then cleanly 
> unmount the device and let the user know when it was safe to remove the 
> drive. Removing the device would then send another acpi request that 
> triggered the actual hotplug and bus rescan.

What second acpi request are you referring to?

> This seems like a regression - it's no longer possible to ensure that a 
> bay device is cleanly unmounted. Was this really the desired behaviour? 

I'm thinking about his for several days now and looking for a proper
solution how to ensure that userland has the possibility to cleanly
unmount a device. But it's definitely no regression. Before...systems with
a bay in a dock stations simply froze hard in certain circumstances. It
was pure luck that it worked for one major kernel version or so.

The only sane way for me seems that userland has to be involved before
actually triggering any event or removing any device. Something like
"savely remove this piece of hardware".

For this to archive, we would need another sysfs entry flagging a bay
device as "on dock station", so that userland knows what to unmount/eject
before a dock event. Userspace relying on the bay event on the device is
not a proper solution. The device may have been gone before userland
finishes his work, or as you mention, there's no bay event.

> It should be noted that not all hardware sends the eject request at all 
> (Thinkpads do, but Dell and HP laptops don't), so we can't depend on 
> receiving this when dealing with a bay event.

I don't think we depend on the event. If the device gets removed without
an appropriate event, the behaviour should be the same as before. If not,
that wasn't the intentional behaviour.

Regards,
	Holger

Re: 2.6.25 semantic change in bay handling?

[Matthew Garrett]

On Tue, May 06, 2008 at 10:13:47AM +0200, Holger Macht wrote:
> On Mo 05. Mai - 23:33:57, Matthew Garrett wrote:
> > 48feb3c419508487becfb9ea3afcc54c3eac6d80 appears to flag a device as 
> > detached if an acpi eject request is received. In 2.6.24 and earlier, an 
> > eject request merely sent an event to userland which could then cleanly 
> > unmount the device and let the user know when it was safe to remove the 
> > drive. Removing the device would then send another acpi request that 
> > triggered the actual hotplug and bus rescan.
> 
> What second acpi request are you referring to?

For bay devices, the sequence is an optional eject request (before 
removal, and not implemented on all hardware) followed by either a bus 
or device check request (after removal).

> > This seems like a regression - it's no longer possible to ensure that a 
> > bay device is cleanly unmounted. Was this really the desired behaviour? 
> 
> I'm thinking about his for several days now and looking for a proper
> solution how to ensure that userland has the possibility to cleanly
> unmount a device. But it's definitely no regression. Before...systems with
> a bay in a dock stations simply froze hard in certain circumstances. It
> was pure luck that it worked for one major kernel version or so.

Ah. I think the issue here is that you're trying to treat bays in docks 
in the same way as internal bays. These should probably be different 
callbacks.

> The only sane way for me seems that userland has to be involved before
> actually triggering any event or removing any device. Something like
> "savely remove this piece of hardware".

That's what the eject request is for.

> For this to archive, we would need another sysfs entry flagging a bay
> device as "on dock station", so that userland knows what to unmount/eject
> before a dock event. Userspace relying on the bay event on the device is
> not a proper solution. The device may have been gone before userland
> finishes his work, or as you mention, there's no bay event.

Yes. The user can, of course, do bad things. That doesn't mean we should 
avoid the opportunity to cleanly unmount filesystems when the user 
doesn't behave pathologically.

> > It should be noted that not all hardware sends the eject request at all 
> > (Thinkpads do, but Dell and HP laptops don't), so we can't depend on 
> > receiving this when dealing with a bay event.
> 
> I don't think we depend on the event. If the device gets removed without
> an appropriate event, the behaviour should be the same as before. If not,
> that wasn't the intentional behaviour.

What ought to be possible for dock devices is something like "Receive 
eject request, clean up anything that has to be done in kernel, allow 
userspace to intervene and tidy up its side of things, let userspace 
signal completion, complete the undocking sequence". For internal bays, 
we certainly shouldn't be detaching the device when all we've received 
is an eject request.

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

Re: 2.6.25 semantic change in bay handling?

[Holger Macht]

On Tue 06. May - 10:13:47, Holger Macht wrote:
> On Mo 05. Mai - 23:33:57, Matthew Garrett wrote:
> > 48feb3c419508487becfb9ea3afcc54c3eac6d80 appears to flag a device as 
> > detached if an acpi eject request is received. In 2.6.24 and earlier, an 
> > eject request merely sent an event to userland which could then cleanly 
> > unmount the device and let the user know when it was safe to remove the 
> > drive. Removing the device would then send another acpi request that 
> > triggered the actual hotplug and bus rescan.
> 
> What second acpi request are you referring to?
> 
> > This seems like a regression - it's no longer possible to ensure that a 
> > bay device is cleanly unmounted. Was this really the desired behaviour? 
> 
> I'm thinking about his for several days now and looking for a proper
> solution how to ensure that userland has the possibility to cleanly
> unmount a device. But it's definitely no regression. Before...systems with
> a bay in a dock stations simply froze hard in certain circumstances. It
> was pure luck that it worked for one major kernel version or so.
> 
> The only sane way for me seems that userland has to be involved before
> actually triggering any event or removing any device. Something like
> "savely remove this piece of hardware".
> 
> For this to archive, we would need another sysfs entry flagging a bay
> device as "on dock station", so that userland knows what to unmount/eject
> before a dock event. Userspace relying on the bay event on the device is
> not a proper solution. The device may have been gone before userland
> finishes his work, or as you mention, there's no bay event.
> 
> > It should be noted that not all hardware sends the eject request at all 
> > (Thinkpads do, but Dell and HP laptops don't), so we can't depend on 
> > receiving this when dealing with a bay event.
> 
> I don't think we depend on the event. If the device gets removed without
> an appropriate event, the behaviour should be the same as before. If not,
> that wasn't the intentional behaviour.

AFAICT!

I just saw that the initial mail was not explicitly meant for me and the
patches regarding bay devices I've sent a couple of weeks ago.

Regards,
	Holger

Re: 2.6.25 semantic change in bay handling?

[Tejun Heo]

(cc'ing Holger Macht, copying whole body for him)

Hello, guys.

Matthew Garrett wrote:
> On Tue, May 06, 2008 at 10:13:47AM +0200, Holger Macht wrote:
>> On Mo 05. Mai - 23:33:57, Matthew Garrett wrote:
>>> 48feb3c419508487becfb9ea3afcc54c3eac6d80 appears to flag a device as 

That should be 233f112042d0b50170212dbff99c3b34b8773cd3, right?

>>> detached if an acpi eject request is received. In 2.6.24 and earlier, an 
>>> eject request merely sent an event to userland which could then cleanly 
>>> unmount the device and let the user know when it was safe to remove the 
>>> drive. Removing the device would then send another acpi request that 
>>> triggered the actual hotplug and bus rescan.
>> What second acpi request are you referring to?
> 
> For bay devices, the sequence is an optional eject request (before 
> removal, and not implemented on all hardware) followed by either a bus 
> or device check request (after removal).
> 
>>> This seems like a regression - it's no longer possible to ensure that a 
>>> bay device is cleanly unmounted. Was this really the desired behaviour? 
>> I'm thinking about his for several days now and looking for a proper
>> solution how to ensure that userland has the possibility to cleanly
>> unmount a device. But it's definitely no regression. Before...systems with
>> a bay in a dock stations simply froze hard in certain circumstances. It
>> was pure luck that it worked for one major kernel version or so.
> 
> Ah. I think the issue here is that you're trying to treat bays in docks 
> in the same way as internal bays. These should probably be different 
> callbacks.
> 
>> The only sane way for me seems that userland has to be involved before
>> actually triggering any event or removing any device. Something like
>> "savely remove this piece of hardware".
> 
> That's what the eject request is for.
> 
>> For this to archive, we would need another sysfs entry flagging a bay
>> device as "on dock station", so that userland knows what to unmount/eject
>> before a dock event. Userspace relying on the bay event on the device is
>> not a proper solution. The device may have been gone before userland
>> finishes his work, or as you mention, there's no bay event.
> 
> Yes. The user can, of course, do bad things. That doesn't mean we should 
> avoid the opportunity to cleanly unmount filesystems when the user 
> doesn't behave pathologically.
> 
>>> It should be noted that not all hardware sends the eject request at all 
>>> (Thinkpads do, but Dell and HP laptops don't), so we can't depend on 
>>> receiving this when dealing with a bay event.
>> I don't think we depend on the event. If the device gets removed without
>> an appropriate event, the behaviour should be the same as before. If not,
>> that wasn't the intentional behaviour.
> 
> What ought to be possible for dock devices is something like "Receive 
> eject request, clean up anything that has to be done in kernel, allow 
> userspace to intervene and tidy up its side of things, let userspace 
> signal completion, complete the undocking sequence". For internal bays, 
> we certainly shouldn't be detaching the device when all we've received 
> is an eject request.
> 

The original change was from Holder Macht and IIRC it was to avoid 
machine hard lock up on certain laptops which happens when libata EH 
goes out to find out what happened when it receives bus/device check 
after removal.  Maybe what should be done instead is that eject request 
doesn't do anything but tells acpid to unmount and delete the block 
device by echoing 1 to sysfs delete node.

Hmmm... It would be perfect if we can tell whether DEVICE/BUS CHECK is 
in which direction (device coming or going away).

Thanks.

-- 
tejun

Re: 2.6.25 semantic change in bay handling?

[Matthew Garrett]

On Tue, May 06, 2008 at 05:40:39PM +0900, Tejun Heo wrote:
> >>On Mo 05. Mai - 23:33:57, Matthew Garrett wrote:
> >>>48feb3c419508487becfb9ea3afcc54c3eac6d80 appears to flag a device as 
> 
> That should be 233f112042d0b50170212dbff99c3b34b8773cd3, right?

Yeah, my mistake.

> The original change was from Holder Macht and IIRC it was to avoid 
> machine hard lock up on certain laptops which happens when libata EH 
> goes out to find out what happened when it receives bus/device check 
> after removal.  Maybe what should be done instead is that eject request 
> doesn't do anything but tells acpid to unmount and delete the block 
> device by echoing 1 to sysfs delete node.

>From my point of view that's fine, but I'd be more interested to know 
about the case Holger was having trouble with. For internal bays, at 
least, we can't guarantee that we'll get an eject request before the 
device is removed - if that leads to hangs, we probably need to work out 
a way of being more robust here.

> Hmmm... It would be perfect if we can tell whether DEVICE/BUS CHECK is 
> in which direction (device coming or going away).

Yeah, but I can't see an easy way of doing that. It's not enough to keep 
track of the current state and assume that it's either an insertion or 
removal as a result - some machines fire bus checks on resume, even if 
the bay device hasn't been changed.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: 2.6.25 semantic change in bay handling?

[Tejun Heo]

Matthew Garrett wrote:
> On Tue, May 06, 2008 at 05:40:39PM +0900, Tejun Heo wrote:
>>>> On Mo 05. Mai - 23:33:57, Matthew Garrett wrote:
>>>>> 48feb3c419508487becfb9ea3afcc54c3eac6d80 appears to flag a device as 
>> That should be 233f112042d0b50170212dbff99c3b34b8773cd3, right?
> 
> Yeah, my mistake.
> 
>> The original change was from Holder Macht and IIRC it was to avoid 
>> machine hard lock up on certain laptops which happens when libata EH 
>> goes out to find out what happened when it receives bus/device check 
>> after removal.  Maybe what should be done instead is that eject request 
>> doesn't do anything but tells acpid to unmount and delete the block 
>> device by echoing 1 to sysfs delete node.
> 
> From my point of view that's fine, but I'd be more interested to know 
> about the case Holger was having trouble with. For internal bays, at 
> least, we can't guarantee that we'll get an eject request before the 
> device is removed - if that leads to hangs, we probably need to work out 
> a way of being more robust here.
> 
>> Hmmm... It would be perfect if we can tell whether DEVICE/BUS CHECK is 
>> in which direction (device coming or going away).
> 
> Yeah, but I can't see an easy way of doing that. It's not enough to keep 
> track of the current state and assume that it's either an insertion or 
> removal as a result - some machines fire bus checks on resume, even if 
> the bay device hasn't been changed.

All we need is separate out the ejection case out.  For suspend, resume, 
attach or whatever can be handled the same way.  The problem occurs 
because some controllers get very unhappy when certain registers are 
accessed when there's no device attached to it.

Thanks.

-- 
tejun

Re: 2.6.25 semantic change in bay handling?

[Matthew Garrett]

On Tue, May 06, 2008 at 05:53:17PM +0900, Tejun Heo wrote:
> Matthew Garrett wrote:
> >Yeah, but I can't see an easy way of doing that. It's not enough to keep 
> >track of the current state and assume that it's either an insertion or 
> >removal as a result - some machines fire bus checks on resume, even if 
> >the bay device hasn't been changed.
> 
> All we need is separate out the ejection case out.  For suspend, resume, 
> attach or whatever can be handled the same way.  The problem occurs 
> because some controllers get very unhappy when certain registers are 
> accessed when there's no device attached to it.

Ok. What we probably ought to be doing then is checking the _STA method 
on the bay when we receive a bus check. That should be sufficient for 
determining whether the device is actually there (if so, perform a 
hotplug) or not (flag the device as detached, don't probe). I don't have 
the hardware here right now, but something like...

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 8c1cfc6..f1d5c87 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -126,37 +126,49 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
 	struct ata_eh_info *ehi;
 	struct kobject *kobj = NULL;
 	int wait = 0;
-	unsigned long flags;
+	unsigned long flags, sta;
+	acpi_status status;
+	acpi_handle handle;
 
 	if (!ap)
 		ap = dev->link->ap;
 	ehi = &ap->link.eh_info;
 
+	if (dev)
+		handle = dev->acpi_handle;
+	else
+		handle = ap->handle;
+
 	spin_lock_irqsave(ap->lock, flags);
 
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
+		status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+		if (!ACPI_SUCCESS(status)) {
+			printk(KERN_ERR "Unable to evaluate bay status\n");
+			break;
+		}
 		ata_ehi_push_desc(ehi, "ACPI event");
-		ata_ehi_hotplugged(ehi);
-		ata_port_freeze(ap);
-		break;
-
-	case ACPI_NOTIFY_EJECT_REQUEST:
-		ata_ehi_push_desc(ehi, "ACPI event");
-		if (dev)
-			dev->flags |= ATA_DFLAG_DETACH;
-		else {
-			struct ata_link *tlink;
-			struct ata_device *tdev;
-
-			ata_port_for_each_link(tlink, ap)
-				ata_link_for_each_dev(tdev, tlink)
+		if (sta) {
+			ata_ehi_hotplugged(ehi);
+			ata_port_freeze(ap);
+		} else {
+			/* The device has gone - unplug it */
+			if (dev)
+				dev->flags |= ATA_DFLAG_DETACH;
+			else {
+				struct ata_link *tlink;
+				struct ata_device *tdev;
+				
+				ata_port_for_each_link(tlink, ap)
+					ata_link_for_each_dev(tdev, tlink)
 					tdev->flags |= ATA_DFLAG_DETACH;
+			}
+			wait = 1;
+			ata_port_freeze(ap);
+			ata_port_schedule_eh(ap);
 		}
-
-		ata_port_schedule_eh(ap);
-		wait = 1;
 		break;
 	}

Might work. Possibly. I'll be able to test on real hardware sometime 
next week, but I don't have access to an ACPI dock with an internal bay. 
I'm not sure how that case would be handled off-hand.

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

Re: 2.6.25 semantic change in bay handling?

[Holger Macht]

On Di 06. Mai - 17:40:39, Tejun Heo wrote:
> (cc'ing Holger Macht, copying whole body for him)
>
> Hello, guys.
>
> Matthew Garrett wrote:
>> On Tue, May 06, 2008 at 10:13:47AM +0200, Holger Macht wrote:
>>> On Mo 05. Mai - 23:33:57, Matthew Garrett wrote:
>>>> 48feb3c419508487becfb9ea3afcc54c3eac6d80 appears to flag a device as 
>
> That should be 233f112042d0b50170212dbff99c3b34b8773cd3, right?
>
>>>> detached if an acpi eject request is received. In 2.6.24 and earlier, an 
>>>> eject request merely sent an event to userland which could then cleanly 
>>>> unmount the device and let the user know when it was safe to remove the 
>>>> drive. Removing the device would then send another acpi request that 
>>>> triggered the actual hotplug and bus rescan.
>>> What second acpi request are you referring to?
>>
>> For bay devices, the sequence is an optional eject request (before 
>> removal, and not implemented on all hardware) followed by either a bus or 
>> device check request (after removal).
>>
>>>> This seems like a regression - it's no longer possible to ensure that a 
>>>> bay device is cleanly unmounted. Was this really the desired behaviour? 
>>> I'm thinking about his for several days now and looking for a proper
>>> solution how to ensure that userland has the possibility to cleanly
>>> unmount a device. But it's definitely no regression. Before...systems with
>>> a bay in a dock stations simply froze hard in certain circumstances. It
>>> was pure luck that it worked for one major kernel version or so.
>>
>> Ah. I think the issue here is that you're trying to treat bays in docks in 
>> the same way as internal bays. These should probably be different 
>> callbacks.

Right, another solution I'm thinking of is to just handle those bays in
the docks differently. Going back to the old bahaviour, thus letting full
control to userland about deleting a bay device.

For the dockstation case, we have to detach a bay on the dock
immediately. Otherwise userland might call into a dead interface. You can
differentiate already through the dock driver, having different callbacks
for "dock notification" and the acpi notification for the bay device.

>>
>>> The only sane way for me seems that userland has to be involved before
>>> actually triggering any event or removing any device. Something like
>>> "savely remove this piece of hardware".
>>
>> That's what the eject request is for.

No. The moment when the bay event is generated, the moment the user pushes
the lever on the bay, the user might immediately pull out the device,
letting no chance for cleaning up...There must be an event before the user
touches anything, to have the time to unmount etc. and then tell "now it's
save to remove the bay device". This would also solve the "there's no bay
event" case.

>>
>>> For this to archive, we would need another sysfs entry flagging a bay
>>> device as "on dock station", so that userland knows what to unmount/eject
>>> before a dock event. Userspace relying on the bay event on the device is
>>> not a proper solution. The device may have been gone before userland
>>> finishes his work, or as you mention, there's no bay event.
>>
>> Yes. The user can, of course, do bad things. That doesn't mean we should 
>> avoid the opportunity to cleanly unmount filesystems when the user doesn't 
>> behave pathologically.

Agreed.

>>
>>>> It should be noted that not all hardware sends the eject request at all 
>>>> (Thinkpads do, but Dell and HP laptops don't), so we can't depend on 
>>>> receiving this when dealing with a bay event.
>>> I don't think we depend on the event. If the device gets removed without
>>> an appropriate event, the behaviour should be the same as before. If not,
>>> that wasn't the intentional behaviour.
>>
>> What ought to be possible for dock devices is something like "Receive 
>> eject request, clean up anything that has to be done in kernel, allow 
>> userspace to intervene and tidy up its side of things, let userspace 
>> signal completion, complete the undocking sequence". For internal bays, we 
>> certainly shouldn't be detaching the device when all we've received is an 
>> eject request.

When the bay is inside the dock, it should be save to immediately remove
the device. Userspace is in control of sending the dock event whenever it
likes to, being able to unmount filesystems etc. before generating the
event. Userland just needs to know which bays are in the dock.

> The original change was from Holder Macht and IIRC it was to avoid machine 
> hard lock up on certain laptops which happens when libata EH goes out to 
> find out what happened when it receives bus/device check after removal.  
> Maybe what should be done instead is that eject request doesn't do anything 
> but tells acpid to unmount and delete the block device by echoing 1 to 
> sysfs delete node.

If that's acceptable, I'm ok with letting full control to userspace.

Regards,
	Holger

> Hmmm... It would be perfect if we can tell whether DEVICE/BUS CHECK is in 
> which direction (device coming or going away).
>
> Thanks.
>

Re: 2.6.25 semantic change in bay handling?

[Holger Macht]

On Di 06. Mai - 09:46:25, Matthew Garrett wrote:
> On Tue, May 06, 2008 at 05:40:39PM +0900, Tejun Heo wrote:
> > >>On Mo 05. Mai - 23:33:57, Matthew Garrett wrote:
> > >>>48feb3c419508487becfb9ea3afcc54c3eac6d80 appears to flag a device as 
> > 
> > That should be 233f112042d0b50170212dbff99c3b34b8773cd3, right?
> 
> Yeah, my mistake.
> 
> > The original change was from Holder Macht and IIRC it was to avoid 
> > machine hard lock up on certain laptops which happens when libata EH 
> > goes out to find out what happened when it receives bus/device check 
> > after removal.  Maybe what should be done instead is that eject request 
> > doesn't do anything but tells acpid to unmount and delete the block 
> > device by echoing 1 to sysfs delete node.
> 
> From my point of view that's fine, but I'd be more interested to know 
> about the case Holger was having trouble with. For internal bays, at 
> least, we can't guarantee that we'll get an eject request before the 
> device is removed - if that leads to hangs, we probably need to work out 
> a way of being more robust here.

Right, so you never can rely on receiving a BAY_EVENT. Why not just
disregard this case and looking for a common solution?

Regards,
	Holger

> > Hmmm... It would be perfect if we can tell whether DEVICE/BUS CHECK is 
> > in which direction (device coming or going away).
> 
> Yeah, but I can't see an easy way of doing that. It's not enough to keep 
> track of the current state and assume that it's either an insertion or 
> removal as a result - some machines fire bus checks on resume, even if 
> the bay device hasn't been changed.

Re: 2.6.25 semantic change in bay handling?

[Matthew Garrett]

On Tue, May 06, 2008 at 11:26:53AM +0200, Holger Macht wrote:

> No. The moment when the bay event is generated, the moment the user pushes
> the lever on the bay, the user might immediately pull out the device,
> letting no chance for cleaning up...There must be an event before the user
> touches anything, to have the time to unmount etc. and then tell "now it's
> save to remove the bay device". This would also solve the "there's no bay
> event" case.

Hm. The hardware I have here has a separate "eject request" button and 
"physical eject" button. Hitting the former sends the request to the OS, 
and the light then pulses until the OS confirms that the dock can be 
removed. If the user removes the dock before this happens, that's user 
error. Unfortunately, the hardware in question doesn't contain a bay. 
The only dock I have with no request button doesn't present as an ACPI 
dock to begin with. Do Thinkpad docks differ from this?

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

Re: 2.6.25 semantic change in bay handling?

[Matthew Garrett]

On Tue, May 06, 2008 at 11:29:35AM +0200, Holger Macht wrote:

> Right, so you never can rely on receiving a BAY_EVENT. Why not just
> disregard this case and looking for a common solution?

Oh, I agree - we need to solve this in any case. But for hardware where 
there is a separate request event before the device is pulled, users 
already have scripts that prompt them to unmount hardware. These worked 
in <2.6.25, but they're broken now. It'd be nice to get them working 
again.

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

Re: 2.6.25 semantic change in bay handling?

[Holger Macht]

Hi,

(Can you please keep me CC'ed, I'm not on linux-ide)

Quoting your mail from the archives:

>> Right, so you never can rely on receiving a BAY_EVENT. Why not just
>> disregard this case and looking for a common solution?

> Oh, I agree - we need to solve this in any case. But for hardware where 
> there is a separate request event before the device is pulled, users 
> already have scripts that prompt them to unmount hardware. These worked 
> in <2.6.25, but they're broken now. It'd be nice to get them working 
> again.

Yes, the scripts were broken for the advantage to not freeze some devices ;-)

But I agree, we need such a possibility.

>> No. The moment when the bay event is generated, the moment the user pushes
>> the lever on the bay, the user might immediately pull out the device,
>> letting no chance for cleaning up...There must be an event before the user
>> touches anything, to have the time to unmount etc. and then tell "now it's
>> save to remove the bay device". This would also solve the "there's no bay
>> event" case.

> Hm. The hardware I have here has a separate "eject request" button and 
> "physical eject" button. Hitting the former sends the request to the OS, 
> and the light then pulses until the OS confirms that the dock can be 
> removed. If the user removes the dock before this happens, that's user 
> error.

Not by default. The dock driver immediately undocks unless
immediate_undock parameter is set to 0. Any access to the bay inside the
dock afterwards might freeze the system.

Actually I was talking about the "bay not in the dock"-case here.

Unfortunately, the hardware in question doesn't contain a bay.

> The only dock I have with no request button doesn't present as an ACPI 
> dock to begin with. Do Thinkpad docks differ from this?

There are those which do simple PCI hotplugging without the involvement of
ACPI (many/all HPs AFAIK) and those which present themselves as a
dockstation through ACPI (those with a _DCK method). The thinkpad X60 dock
I have here has the request button, too.

Regards,
	Holger

Re: 2.6.25 semantic change in bay handling?

[Matthew Garrett]

On Tue, May 06, 2008 at 01:21:45PM +0200, Holger Macht wrote:

> Not by default. The dock driver immediately undocks unless
> immediate_undock parameter is set to 0. Any access to the bay inside the
> dock afterwards might freeze the system.
> 
> Actually I was talking about the "bay not in the dock"-case here.

In that case, the effect differs depending on the hardware. In the ideal 
universe, Thinkpad users flick out the eject lever and wait for a 
confirmation that they can pull out the drive. Dell and HP users have to 
request a safe remove in advance.

> There are those which do simple PCI hotplugging without the involvement of
> ACPI (many/all HPs AFAIK) and those which present themselves as a
> dockstation through ACPI (those with a _DCK method). The thinkpad X60 dock
> I have here has the request button, too.

Right. In that case, it needs to be possible for userspace to indicate 
that the eject request should not automatically destroy the device. 
Actually removing the dock should do so in all circumstances.

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

Re: 2.6.25 semantic change in bay handling?

[Holger Macht]

( I still had to poll linux-ide archives... )

>> Not by default. The dock driver immediately undocks unless
>> immediate_undock parameter is set to 0. Any access to the bay inside the
>> dock afterwards might freeze the system.
>> 
>> Actually I was talking about the "bay not in the dock"-case here.

> In that case, the effect differs depending on the hardware. In the ideal 
> universe, Thinkpad users flick out the eject lever and wait for a 
> confirmation that they can pull out the drive. Dell and HP users have to 
> request a safe remove in advance.

If you need to make clear that the user has to wait for a notification
before he removes the device, you could just make him clear that he has to
request a safe remove in advance.

But yes, I see your point.

>> There are those which do simple PCI hotplugging without the involvement of
>> ACPI (many/all HPs AFAIK) and those which present themselves as a
>> dockstation through ACPI (those with a _DCK method). The thinkpad X60 dock
>> I have here has the request button, too.

> Right. In that case, it needs to be possible for userspace to indicate 
> that the eject request should not automatically destroy the device. 
> Actually removing the dock should do so in all circumstances.

That's what I'm trying to say.

Just to make clear that we agree on the design, if so, I'll try to provide
a patch:

 1. Dock event: libata immediately detaches the device

    (libata will need another sysfs flag is_on_dock userspace can query)

 2. Bay event: libata signals a BAY_EVENT through uevent, userspace writes
    	       1 to /sys/.../device/delete

Regards,
	Holger

Re: 2.6.25 semantic change in bay handling?

[Matthew Garrett]

On Tue, May 06, 2008 at 07:27:43PM +0200, Holger Macht wrote:

> Just to make clear that we agree on the design, if so, I'll try to provide
> a patch:
> 
>  1. Dock event: libata immediately detaches the device
> 
>     (libata will need another sysfs flag is_on_dock userspace can query)

Hm. I'm not absolutely certain about this. Do we get a bus check 
notification after the dock has been removed? If so, I think it ought to 
be handled the same way as the internal bay (ie, signal userspace and 
let it clean up and destroy the device - if it fails to do so, then 
destroy the device when the dock is actually removed, by catching the 
bus/device check, calling the _STA method on the bay and destroying the 
device if it's present)

>  2. Bay event: libata signals a BAY_EVENT through uevent, userspace writes
>     	       1 to /sys/.../device/delete

In the case of an eject request, yes. In the case of a bus or device 
check, we should call _STA and then delete/hotplug the device as 
appropriate.

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

Re: 2.6.25 semantic change in bay handling?

[Holger Macht]

Hi Matthew,

( Please TO or CC hmacht@suse.de ! )

>> Just to make clear that we agree on the design, if so, I'll try to provide
>> a patch:
>> 
>>  1. Dock event: libata immediately detaches the device
>> 
>>     (libata will need another sysfs flag is_on_dock userspace can query)

> Hm. I'm not absolutely certain about this. Do we get a bus check 
> notification after the dock has been removed? If so, I think it ought to 
> be handled the same way as the internal bay (ie, signal userspace and 
> let it clean up and destroy the device - if it fails to do so, then 
> destroy the device when the dock is actually removed, by catching the 
> bus/device check, calling the _STA method on the bay and destroying the 
> device if it's present)

libata is notified through the dock driver when a dock event occurs, just
before the dock driver undocks, giving no time to userspace to clean
up. libata doesn't receive an additional acpi bay event.

>>  2. Bay event: libata signals a BAY_EVENT through uevent, userspace writes
>>     	       1 to /sys/.../device/delete

> In the case of an eject request, yes. In the case of a bus or device 
> check, we should call _STA and then delete/hotplug the device as 
> appropriate.

Yes.

Regards,
	Holger

Re: 2.6.25 semantic change in bay handling?

[Matthew Garrett]

On Tue, May 06, 2008 at 08:36:12PM +0200, Holger Macht wrote:
> Hi Matthew,
> 
> ( Please TO or CC hmacht@suse.de ! )

Then stop setting Mail-Followup-To: :)

> > Hm. I'm not absolutely certain about this. Do we get a bus check 
> > notification after the dock has been removed? If so, I think it ought to 
> > be handled the same way as the internal bay (ie, signal userspace and 
> > let it clean up and destroy the device - if it fails to do so, then 
> > destroy the device when the dock is actually removed, by catching the 
> > bus/device check, calling the _STA method on the bay and destroying the 
> > device if it's present)
> 
> libata is notified through the dock driver when a dock event occurs, just
> before the dock driver undocks, giving no time to userspace to clean
> up. libata doesn't receive an additional acpi bay event.

That's the current situation, I'm not sure it's ideal. But I'll skip 
worrying about that until I've actually got some hardware to work out 
how to do it properly :)

> >>  2. Bay event: libata signals a BAY_EVENT through uevent, userspace writes
> >>     	       1 to /sys/.../device/delete
> 
> > In the case of an eject request, yes. In the case of a bus or device 
> > check, we should call _STA and then delete/hotplug the device as 
> > appropriate.
> 
> Yes.

Excellent. I /think/ the patch I sent earlier basically does this - 
you'll want to register a separate callback for the dock event in that 
case, though.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: 2.6.25 semantic change in bay handling?

[Holger Macht]

On Di 06. Mai - 19:48:28, Matthew Garrett wrote:
> On Tue, May 06, 2008 at 08:36:12PM +0200, Holger Macht wrote:
> > Hi Matthew,
> > 
> > ( Please TO or CC hmacht@suse.de ! )
> 
> Then stop setting Mail-Followup-To: :)
> 
> > > Hm. I'm not absolutely certain about this. Do we get a bus check 
> > > notification after the dock has been removed? If so, I think it ought to 
> > > be handled the same way as the internal bay (ie, signal userspace and 
> > > let it clean up and destroy the device - if it fails to do so, then 
> > > destroy the device when the dock is actually removed, by catching the 
> > > bus/device check, calling the _STA method on the bay and destroying the 
> > > device if it's present)
> > 
> > libata is notified through the dock driver when a dock event occurs, just
> > before the dock driver undocks, giving no time to userspace to clean
> > up. libata doesn't receive an additional acpi bay event.
> 
> That's the current situation, I'm not sure it's ideal. But I'll skip 

What exactly do you think is not ideal?

> worrying about that until I've actually got some hardware to work out 
> how to do it properly :)

Maybe this is what you're thinking of?:
  http://marc.info/?l=linux-acpi&m=121009411900390&w=2

> 
> > >>  2. Bay event: libata signals a BAY_EVENT through uevent, userspace writes
> > >>     	       1 to /sys/.../device/delete
> > 
> > > In the case of an eject request, yes. In the case of a bus or device 
> > > check, we should call _STA and then delete/hotplug the device as 
> > > appropriate.
> > 
> > Yes.
> 
> Excellent. I /think/ the patch I sent earlier basically does this - 
> you'll want to register a separate callback for the dock event in that 
> case, though.

I'll take a look the following days, thanks.

Regards,
	Holger

[PATCH] Fixups to ATA ACPI hotplug

[Matthew Garrett]

The libata-acpi.c code currently accepts hotplug messages from both the 
port and the device. This does not match the behaviour of the bay 
driver, and may result in confusion when two hotplug requests are 
received for the same device. This patch limits the hotplug notification 
to removable ACPI devices, which in turn allows it to use the _STA 
method to determine whether the device has been removed or inserted. 
On removal, devices are marked as detached. On insertion, a hotplug scan 
is started. This should avoid lockups caused by the ata layer attempting 
to scan devices which have been removed. The uevent sending is moved 
outside the spinlock in order to avoid a warning generated by it firing 
when interrupts are disabled.

Signed-off-by: Matthew Garrett <mjg@redhat.com>

---

Holger, I'm pretty sure that this deals with the docking station removal 
case, but don't have the hardware to test. If EJECT_REQUEST is genuinely 
the only notification we receive (ie, no BUS_CHECK or DEVICE_CHECK) then 
we'll need to add a separate callback for docking.

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 70b77e0..865a552 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -118,8 +118,8 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
-static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
-				    u32 event)
+static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device 
+				    *dev, u32 event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
@@ -127,39 +127,67 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
 	struct kobject *kobj = NULL;
 	int wait = 0;
 	unsigned long flags;
-
+	acpi_handle handle, tmphandle;
+	unsigned long sta;
+	acpi_status status;
+	
 	if (!ap)
 		ap = dev->link->ap;
 	ehi = &ap->link.eh_info;
 
 	spin_lock_irqsave(ap->lock, flags);
 
+	if (dev)
+		handle = dev->acpi_handle;
+	else
+		handle = ap->acpi_handle;
+
+	status = acpi_get_handle(handle, "_EJ0", &tmphandle);
+	if (ACPI_FAILURE(status)) {
+		/* This device is not ejectable */
+		spin_unlock_irqrestore(ap->lock, flags);
+		return;
+	}
+
+	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+	if (ACPI_FAILURE(status)) {
+		printk ("Unable to determine bay status\n");
+		spin_unlock_irqrestore(ap->lock, flags);
+		return;
+	}
+
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		ata_ehi_hotplugged(ehi);
-		ata_port_freeze(ap);
-		break;
-
-	case ACPI_NOTIFY_EJECT_REQUEST:
-		ata_ehi_push_desc(ehi, "ACPI event");
-		if (dev)
-			dev->flags |= ATA_DFLAG_DETACH;
-		else {
-			struct ata_link *tlink;
-			struct ata_device *tdev;
-
-			ata_port_for_each_link(tlink, ap)
-				ata_link_for_each_dev(tdev, tlink)
-					tdev->flags |= ATA_DFLAG_DETACH;
+		if (!sta) {
+                /* Device has been unplugged */
+			if (dev)
+				dev->flags |= ATA_DFLAG_DETACH;
+			else {
+				struct ata_link *tlink;
+				struct ata_device *tdev;
+				
+				ata_port_for_each_link(tlink, ap) {
+					ata_link_for_each_dev(tdev, tlink) {
+						tdev->flags |= 
+							ATA_DFLAG_DETACH;
+					}
+				}
+			}
+			ata_port_schedule_eh(ap);
+			wait = 1;
+		} else {
+			ata_ehi_hotplugged(ehi);
+			ata_port_freeze(ap);
 		}
-
-		ata_port_schedule_eh(ap);
-		wait = 1;
-		break;
 	}
 
+	spin_unlock_irqrestore(ap->lock, flags);
+
+	if (wait)
+		ata_port_wait_eh(ap);
+
 	if (dev) {
 		if (dev->sdev)
 			kobj = &dev->sdev->sdev_gendev.kobj;
@@ -170,11 +198,6 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
 		sprintf(event_string, "BAY_EVENT=%d", event);
 		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
 	}
-
-	spin_unlock_irqrestore(ap->lock, flags);
-
-	if (wait)
-		ata_port_wait_eh(ap);
 }
 
 static void ata_acpi_dev_notify(acpi_handle handle, u32 event, void *data)

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

Re: [PATCH] Fixups to ATA ACPI hotplug

[Holger Macht]

On Mon 19. May - 17:29:34, Matthew Garrett wrote:
> The libata-acpi.c code currently accepts hotplug messages from both the 
> port and the device. This does not match the behaviour of the bay 
> driver, and may result in confusion when two hotplug requests are 
> received for the same device. This patch limits the hotplug notification 
> to removable ACPI devices, which in turn allows it to use the _STA 
> method to determine whether the device has been removed or inserted. 
> On removal, devices are marked as detached. On insertion, a hotplug scan 
> is started. This should avoid lockups caused by the ata layer attempting 
> to scan devices which have been removed. The uevent sending is moved 
> outside the spinlock in order to avoid a warning generated by it firing 
> when interrupts are disabled.

Ok, it seems we're currently doing the same work two times. I've also got
a patch for this. Already tested, so please have a look if this would also
match your requirements:

Handle bay devices in dock stations

* Differentiate between bay devices in dock stations and others:

 - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
   userspace (that is when the optional eject button on a bay device is
   pressed/pulled) giving the possibility to unmount file systems and to
   clean up. Also, only send uevent in case we get an
   ACPI_NOTIFY_EJECT_REQUEST without doing anything else. In other cases,
   you'll get an add/remove event because libata attaches/detaches the
   device.

 - In case of a dock event, which in turn signals an
   ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
   may already have been gone

* In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
  the device has been plugged or unplugged. If plugged, hotplug it, if
  unplugged, just signal event to userspace
  (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)

Signed-off-by: Holger Macht <hmacht@suse.de>
---

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 70b77e0..62d94e4 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -118,77 +118,118 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
+static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
+{
+	if (dev)
+		dev->flags |= ATA_DFLAG_DETACH;
+	else {
+		struct ata_link *tlink;
+		struct ata_device *tdev;
+
+		ata_port_for_each_link(tlink, ap)
+			ata_link_for_each_dev(tdev, tlink)
+			tdev->flags |= ATA_DFLAG_DETACH;
+	}
+
+	ata_port_freeze(ap);
+	ata_port_schedule_eh(ap);
+}
+
 static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
-				    u32 event)
+				    u32 event, int is_dock_event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
 	struct ata_eh_info *ehi;
 	struct kobject *kobj = NULL;
 	int wait = 0;
-	unsigned long flags;
+	unsigned long flags, sta;
+	acpi_status status;
+	acpi_handle handle;
 
 	if (!ap)
 		ap = dev->link->ap;
 	ehi = &ap->link.eh_info;
 
+	if (dev) {
+		if (dev->sdev)
+			kobj = &dev->sdev->sdev_gendev.kobj;
+		handle = dev->acpi_handle;
+	} else {
+		kobj = &ap->dev->kobj;
+		handle = ap->acpi_handle;
+	}
+
+	if (kobj && !is_dock_event) {
+		sprintf(event_string, "BAY_EVENT=%d", event);
+		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
+	}
+
 	spin_lock_irqsave(ap->lock, flags);
 
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		ata_ehi_hotplugged(ehi);
-		ata_port_freeze(ap);
-		break;
+		status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+		if (!ACPI_SUCCESS(status)) {
+			printk(KERN_INFO "Unable to evaluate bay status\n");
+			break;
+		}
+
+		if (sta) {
+			ata_ehi_hotplugged(ehi);
+			ata_port_freeze(ap);
+		} else {
+			/* The device has gone - unplug it */
+			ata_acpi_detach_device(ap, dev);
 
+			wait = 1;
+		}
+		break;
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		if (dev)
-			dev->flags |= ATA_DFLAG_DETACH;
-		else {
-			struct ata_link *tlink;
-			struct ata_device *tdev;
-
-			ata_port_for_each_link(tlink, ap)
-				ata_link_for_each_dev(tdev, tlink)
-					tdev->flags |= ATA_DFLAG_DETACH;
-		}
+		if (!is_dock_event)
+			break;
 
-		ata_port_schedule_eh(ap);
+		/* undock event - immediate unplug */
+		ata_acpi_detach_device(ap, dev);
 		wait = 1;
 		break;
 	}
 
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
-
-	if (kobj) {
-		sprintf(event_string, "BAY_EVENT=%d", event);
-		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
-	}
-
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	if (wait)
 		ata_port_wait_eh(ap);
 }
 
+static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_device *dev = data;
+
+	ata_acpi_handle_hotplug(NULL, dev, event, 1);
+}
+
+static void ata_acpi_ap_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_port *ap = data;
+
+	ata_acpi_handle_hotplug(ap, NULL, event, 1);
+}
+
 static void ata_acpi_dev_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_device *dev = data;
 
-	ata_acpi_handle_hotplug(NULL, dev, event);
+	ata_acpi_handle_hotplug(NULL, dev, event, 0);
 }
 
 static void ata_acpi_ap_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_port *ap = data;
 
-	ata_acpi_handle_hotplug(ap, NULL, event);
+	ata_acpi_handle_hotplug(ap, NULL, event, 0);
 }
 
 /**
@@ -229,7 +270,7 @@ void ata_acpi_associate(struct ata_host *host)
 						    ata_acpi_ap_notify, ap);
 			/* we might be on a docking station */
 			register_hotplug_dock_device(ap->acpi_handle,
-						     ata_acpi_ap_notify, ap);
+					     ata_acpi_ap_notify_dock, ap);
 		}
 
 		for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
@@ -241,7 +282,7 @@ void ata_acpi_associate(struct ata_host *host)
 						ata_acpi_dev_notify, dev);
 				/* we might be on a docking station */
 				register_hotplug_dock_device(dev->acpi_handle,
-						ata_acpi_dev_notify, dev);
+					     ata_acpi_dev_notify_dock, dev);
 			}
 		}
 	}

Re: [PATCH] Fixups to ATA ACPI hotplug

[Holger Macht]

On Mon 19. May - 17:29:34, Matthew Garrett wrote:
> The libata-acpi.c code currently accepts hotplug messages from both the 
> port and the device. This does not match the behaviour of the bay 
> driver, and may result in confusion when two hotplug requests are 
> received for the same device. This patch limits the hotplug notification 
> to removable ACPI devices, which in turn allows it to use the _STA 
> method to determine whether the device has been removed or inserted. 
> On removal, devices are marked as detached. On insertion, a hotplug scan 
> is started. This should avoid lockups caused by the ata layer attempting 
> to scan devices which have been removed. The uevent sending is moved 
> outside the spinlock in order to avoid a warning generated by it firing 
> when interrupts are disabled.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> ---
> 
> Holger, I'm pretty sure that this deals with the docking station removal 
> case, but don't have the hardware to test. If EJECT_REQUEST is genuinely 

I tried it, it doesn't.

 Undock --> some userspace app accesses the device --> hard lockup

> the only notification we receive (ie, no BUS_CHECK or DEVICE_CHECK) then 
> we'll need to add a separate callback for docking.

Yes, see my patch.

Regards,
	Holger

Re: [PATCH] Fixups to ATA ACPI hotplug

[Matthew Garrett]

On Tue, May 20, 2008 at 09:44:42AM +0200, Holger Macht wrote:

> * In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
>   the device has been plugged or unplugged. If plugged, hotplug it, if
>   unplugged, just signal event to userspace
>   (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)

The only issue I can see is that this one doesn't check _EJ0. Unless 
that's done, you'll (on some hardware) evaluate _STA on the bay itself 
rather than the device within the bay, which leads to confusion as to 
whether the device has been inserted. Other than that, looks good. 
Jeff's sent my patch to Linus, so can you redo this on top and I'll sign 
it off?

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

Re: [PATCH] Fixups to ATA ACPI hotplug

[Holger Macht]

On Tue 20. May - 11:20:38, Matthew Garrett wrote:
> On Tue, May 20, 2008 at 09:44:42AM +0200, Holger Macht wrote:
> 
> > * In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
> >   the device has been plugged or unplugged. If plugged, hotplug it, if
> >   unplugged, just signal event to userspace
> >   (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)
> 
> The only issue I can see is that this one doesn't check _EJ0. Unless 
> that's done, you'll (on some hardware) evaluate _STA on the bay itself 
> rather than the device within the bay, which leads to confusion as to 
> whether the device has been inserted. Other than that, looks good. 
> Jeff's sent my patch to Linus, so can you redo this on top and I'll sign 
> it off?

Handle bay devices in dock stations

* Differentiate between bay devices in dock stations and others:

 - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
   userspace (that is when the optional eject button on a bay device is
   pressed/pulled) giving the possibility to unmount file systems and to
   clean up. Also, only send uevent in case we get an EJECT_REQUEST
   without doing anything else. In other cases, you'll get an add/remove
   event because libata attaches/detaches the device.

 - In case of a dock event, which in turn signals an
   ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
   may already have been gone

* In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
  the device has been plugged or unplugged. If plugged, hotplug it, if
  unplugged, just signal event to userspace
  (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)

Signed-off-by: Holger Macht <hmacht@suse.de>
---

--- linux-2.6.25/drivers/ata/libata-acpi.c.orig	2008-05-20 13:25:50.000000000 +0200
+++ linux-2.6.25/drivers/ata/libata-acpi.c	2008-05-20 15:12:03.000000000 +0200
@@ -118,8 +118,25 @@ static void ata_acpi_associate_ide_port(
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
+static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
+{
+	if (dev)
+		dev->flags |= ATA_DFLAG_DETACH;
+	else {
+		struct ata_link *tlink;
+		struct ata_device *tdev;
+
+		ata_port_for_each_link(tlink, ap)
+			ata_link_for_each_dev(tdev, tlink)
+			tdev->flags |= ATA_DFLAG_DETACH;
+	}
+
+	ata_port_freeze(ap);
+	ata_port_schedule_eh(ap);
+}
+
 static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device
-				    *dev, u32 event)
+				    *dev, u32 event, int is_dock_event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
@@ -135,83 +152,92 @@ static void ata_acpi_handle_hotplug(stru
 		ap = dev->link->ap;
 	ehi = &ap->link.eh_info;
 
-	spin_lock_irqsave(ap->lock, flags);
-
-	if (dev)
+	if (dev) {
+		if (dev->sdev)
+			kobj = &dev->sdev->sdev_gendev.kobj;
 		handle = dev->acpi_handle;
-	else
+	} else {
+		kobj = &ap->dev->kobj;
 		handle = ap->acpi_handle;
-
-	status = acpi_get_handle(handle, "_EJ0", &tmphandle);
-	if (ACPI_FAILURE(status)) {
-		/* This device is not ejectable */
-		spin_unlock_irqrestore(ap->lock, flags);
-		return;
 	}
 
-	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
-	if (ACPI_FAILURE(status)) {
-		printk ("Unable to determine bay status\n");
-		spin_unlock_irqrestore(ap->lock, flags);
-		return;
+	if (kobj && !is_dock_event) {
+		sprintf(event_string, "BAY_EVENT=%d", event);
+		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
 	}
 
+	spin_lock_irqsave(ap->lock, flags);
+
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		if (!sta) {
-                /* Device has been unplugged */
-			if (dev)
-				dev->flags |= ATA_DFLAG_DETACH;
-			else {
-				struct ata_link *tlink;
-				struct ata_device *tdev;
-
-				ata_port_for_each_link(tlink, ap) {
-					ata_link_for_each_dev(tdev, tlink) {
-						tdev->flags |=
-							ATA_DFLAG_DETACH;
-					}
-				}
-			}
-			ata_port_schedule_eh(ap);
-			wait = 1;
-		} else {
+
+		status = acpi_get_handle(handle, "_EJ0", &tmphandle);
+		if (ACPI_FAILURE(status)) {
+			/* This device is not ejectable */
+			break;
+		}
+
+		status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+		if (ACPI_FAILURE(status)) {
+			printk(KERN_ERR "Unable to determine bay status\n");
+			break;
+		}
+
+		if (sta) {
 			ata_ehi_hotplugged(ehi);
 			ata_port_freeze(ap);
+		} else {
+			/* The device has gone - unplug it */
+			ata_acpi_detach_device(ap, dev);
+			wait = 1;
 		}
+		break;
+	case ACPI_NOTIFY_EJECT_REQUEST:
+		ata_ehi_push_desc(ehi, "ACPI event");
+
+		if (!is_dock_event)
+			break;
+
+		/* undock event - immediate unplug */
+		ata_acpi_detach_device(ap, dev);
+		wait = 1;
+		break;
 	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	if (wait)
 		ata_port_wait_eh(ap);
+}
 
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
+static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_device *dev = data;
 
-	if (kobj) {
-		sprintf(event_string, "BAY_EVENT=%d", event);
-		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
-	}
+	ata_acpi_handle_hotplug(NULL, dev, event, 1);
+}
+
+static void ata_acpi_ap_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_port *ap = data;
+
+	ata_acpi_handle_hotplug(ap, NULL, event, 1);
 }
 
 static void ata_acpi_dev_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_device *dev = data;
 
-	ata_acpi_handle_hotplug(NULL, dev, event);
+	ata_acpi_handle_hotplug(NULL, dev, event, 0);
 }
 
 static void ata_acpi_ap_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_port *ap = data;
 
-	ata_acpi_handle_hotplug(ap, NULL, event);
+	ata_acpi_handle_hotplug(ap, NULL, event, 0);
 }
 
 /**
@@ -252,7 +278,7 @@ void ata_acpi_associate(struct ata_host
 						    ata_acpi_ap_notify, ap);
 			/* we might be on a docking station */
 			register_hotplug_dock_device(ap->acpi_handle,
-						     ata_acpi_ap_notify, ap);
+					     ata_acpi_ap_notify_dock, ap);
 		}
 
 		for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
@@ -264,7 +290,7 @@ void ata_acpi_associate(struct ata_host
 						ata_acpi_dev_notify, dev);
 				/* we might be on a docking station */
 				register_hotplug_dock_device(dev->acpi_handle,
-						ata_acpi_dev_notify, dev);
+					     ata_acpi_dev_notify_dock, dev);
 			}
 		}
 	}

Re: [PATCH] Fixups to ATA ACPI hotplug

[Matthew Garrett]

On Tue, May 20, 2008 at 03:18:32PM +0200, Holger Macht wrote:

> +	if (kobj && !is_dock_event) {
> +		sprintf(event_string, "BAY_EVENT=%d", event);
> +		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);

I think we want to do the _EJ0 checking before this, otherwise we'll 
generate two uevents for the same removal on some hardware. Otherwise, 
looks good.

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

Re: [PATCH] Fixups to ATA ACPI hotplug

[Holger Macht]

On Tue 20. May - 14:22:17, Matthew Garrett wrote:
> On Tue, May 20, 2008 at 03:18:32PM +0200, Holger Macht wrote:
> 
> > +	if (kobj && !is_dock_event) {
> > +		sprintf(event_string, "BAY_EVENT=%d", event);
> > +		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
> 
> I think we want to do the _EJ0 checking before this, otherwise we'll 
> generate two uevents for the same removal on some hardware. Otherwise, 
> looks good.

So once again:

Handle bay devices in dock stations

* Differentiate between bay devices in dock stations and others:

 - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
   userspace (that is when the optional eject button on a bay device is
   pressed/pulled) giving the possibility to unmount file systems and to
   clean up. Also, only send uevent in case we get an EJECT_REQUEST
   without doing anything else. In other cases, you'll get an add/remove
   event because libata attaches/detaches the device.

 - In case of a dock event, which in turn signals an
   ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
   may already have been gone

* In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
  the device has been plugged or unplugged. If plugged, hotplug it, if
  unplugged, just signal event to userspace
  (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)

Signed-off-by: Holger Macht <hmacht@suse.de>
---

--- linux-2.6.25/drivers/ata/libata-acpi.c.orig	2008-05-20 13:25:50.000000000 +0200
+++ linux-2.6.25/drivers/ata/libata-acpi.c	2008-05-20 15:56:38.000000000 +0200
@@ -118,8 +118,25 @@ static void ata_acpi_associate_ide_port(
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
+static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
+{
+	if (dev)
+		dev->flags |= ATA_DFLAG_DETACH;
+	else {
+		struct ata_link *tlink;
+		struct ata_device *tdev;
+
+		ata_port_for_each_link(tlink, ap)
+			ata_link_for_each_dev(tdev, tlink)
+			tdev->flags |= ATA_DFLAG_DETACH;
+	}
+
+	ata_port_freeze(ap);
+	ata_port_schedule_eh(ap);
+}
+
 static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device
-				    *dev, u32 event)
+				    *dev, u32 event, int is_dock_event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
@@ -135,83 +152,92 @@ static void ata_acpi_handle_hotplug(stru
 		ap = dev->link->ap;
 	ehi = &ap->link.eh_info;
 
-	spin_lock_irqsave(ap->lock, flags);
-
-	if (dev)
+	if (dev) {
+		if (dev->sdev)
+			kobj = &dev->sdev->sdev_gendev.kobj;
 		handle = dev->acpi_handle;
-	else
+	} else {
+		kobj = &ap->dev->kobj;
 		handle = ap->acpi_handle;
+	}
 
 	status = acpi_get_handle(handle, "_EJ0", &tmphandle);
 	if (ACPI_FAILURE(status)) {
-		/* This device is not ejectable */
-		spin_unlock_irqrestore(ap->lock, flags);
+		/* This device does not support hotplug */
 		return;
 	}
 
-	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
-	if (ACPI_FAILURE(status)) {
-		printk ("Unable to determine bay status\n");
-		spin_unlock_irqrestore(ap->lock, flags);
-		return;
+	if (kobj && !is_dock_event) {
+		sprintf(event_string, "BAY_EVENT=%d", event);
+		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
 	}
 
+	spin_lock_irqsave(ap->lock, flags);
+
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		if (!sta) {
-                /* Device has been unplugged */
-			if (dev)
-				dev->flags |= ATA_DFLAG_DETACH;
-			else {
-				struct ata_link *tlink;
-				struct ata_device *tdev;
-
-				ata_port_for_each_link(tlink, ap) {
-					ata_link_for_each_dev(tdev, tlink) {
-						tdev->flags |=
-							ATA_DFLAG_DETACH;
-					}
-				}
-			}
-			ata_port_schedule_eh(ap);
-			wait = 1;
-		} else {
+
+		status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+		if (ACPI_FAILURE(status)) {
+			printk(KERN_ERR "Unable to determine bay status\n");
+			break;
+		}
+
+		if (sta) {
 			ata_ehi_hotplugged(ehi);
 			ata_port_freeze(ap);
+		} else {
+			/* The device has gone - unplug it */
+			ata_acpi_detach_device(ap, dev);
+			wait = 1;
 		}
+		break;
+	case ACPI_NOTIFY_EJECT_REQUEST:
+		ata_ehi_push_desc(ehi, "ACPI event");
+
+		if (!is_dock_event)
+			break;
+
+		/* undock event - immediate unplug */
+		ata_acpi_detach_device(ap, dev);
+		wait = 1;
+		break;
 	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	if (wait)
 		ata_port_wait_eh(ap);
+}
 
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
+static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_device *dev = data;
 
-	if (kobj) {
-		sprintf(event_string, "BAY_EVENT=%d", event);
-		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
-	}
+	ata_acpi_handle_hotplug(NULL, dev, event, 1);
+}
+
+static void ata_acpi_ap_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_port *ap = data;
+
+	ata_acpi_handle_hotplug(ap, NULL, event, 1);
 }
 
 static void ata_acpi_dev_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_device *dev = data;
 
-	ata_acpi_handle_hotplug(NULL, dev, event);
+	ata_acpi_handle_hotplug(NULL, dev, event, 0);
 }
 
 static void ata_acpi_ap_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_port *ap = data;
 
-	ata_acpi_handle_hotplug(ap, NULL, event);
+	ata_acpi_handle_hotplug(ap, NULL, event, 0);
 }
 
 /**
@@ -252,7 +278,7 @@ void ata_acpi_associate(struct ata_host
 						    ata_acpi_ap_notify, ap);
 			/* we might be on a docking station */
 			register_hotplug_dock_device(ap->acpi_handle,
-						     ata_acpi_ap_notify, ap);
+					     ata_acpi_ap_notify_dock, ap);
 		}
 
 		for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
@@ -264,7 +290,7 @@ void ata_acpi_associate(struct ata_host
 						ata_acpi_dev_notify, dev);
 				/* we might be on a docking station */
 				register_hotplug_dock_device(dev->acpi_handle,
-						ata_acpi_dev_notify, dev);
+					     ata_acpi_dev_notify_dock, dev);
 			}
 		}
 	}

Re: [PATCH] Fixups to ATA ACPI hotplug

[Matthew Garrett]

On Tue, May 20, 2008 at 03:58:30PM +0200, Holger Macht wrote:

> Handle bay devices in dock stations
> 
> * Differentiate between bay devices in dock stations and others:
> 
>  - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
>    userspace (that is when the optional eject button on a bay device is
>    pressed/pulled) giving the possibility to unmount file systems and to
>    clean up. Also, only send uevent in case we get an EJECT_REQUEST
>    without doing anything else. In other cases, you'll get an add/remove
>    event because libata attaches/detaches the device.
> 
>  - In case of a dock event, which in turn signals an
>    ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
>    may already have been gone
> 
> * In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
>   the device has been plugged or unplugged. If plugged, hotplug it, if
>   unplugged, just signal event to userspace
>   (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)
> 
> Signed-off-by: Holger Macht <hmacht@suse.de>

As long as this fixes the hang on dock removal, this looks good to me.

Acked-by: Matthew Garrett <mjg@redhat.com>
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Fixups to ATA ACPI hotplug

[Andrew Morton]

On Tue, 20 May 2008 15:58:30 +0200
Holger Macht <hmacht@suse.de> wrote:

> On Tue 20. May - 14:22:17, Matthew Garrett wrote:
> > On Tue, May 20, 2008 at 03:18:32PM +0200, Holger Macht wrote:
> > 
> > > +	if (kobj && !is_dock_event) {
> > > +		sprintf(event_string, "BAY_EVENT=%d", event);
> > > +		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
> > 
> > I think we want to do the _EJ0 checking before this, otherwise we'll 
> > generate two uevents for the same removal on some hardware. Otherwise, 
> > looks good.
> 
> So once again:
> 
> Handle bay devices in dock stations
> 
> * Differentiate between bay devices in dock stations and others:
> 
>  - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
>    userspace (that is when the optional eject button on a bay device is
>    pressed/pulled) giving the possibility to unmount file systems and to
>    clean up. Also, only send uevent in case we get an EJECT_REQUEST
>    without doing anything else. In other cases, you'll get an add/remove
>    event because libata attaches/detaches the device.
> 
>  - In case of a dock event, which in turn signals an
>    ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
>    may already have been gone
> 
> * In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
>   the device has been plugged or unplugged. If plugged, hotplug it, if
>   unplugged, just signal event to userspace
>   (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)
> 
> Signed-off-by: Holger Macht <hmacht@suse.de>
> ---
> 
> --- linux-2.6.25/drivers/ata/libata-acpi.c.orig	2008-05-20 13:25:50.000000000 +0200
> +++ linux-2.6.25/drivers/ata/libata-acpi.c	2008-05-20 15:56:38.000000000 +0200
> @@ -118,8 +118,25 @@ static void ata_acpi_associate_ide_port(
>  		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
>  }
>  
> +static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
> +{
> +	if (dev)
> +		dev->flags |= ATA_DFLAG_DETACH;
> +	else {
> +		struct ata_link *tlink;
> +		struct ata_device *tdev;
> +
> +		ata_port_for_each_link(tlink, ap)
> +			ata_link_for_each_dev(tdev, tlink)
> +			tdev->flags |= ATA_DFLAG_DETACH;

Looks odd.   I guess this:

--- a/drivers/ata/libata-acpi.c~ata-acpi-hotplug-handle-bay-devices-in-dock-stations-cleanup
+++ a/drivers/ata/libata-acpi.c
@@ -128,7 +128,7 @@ static void ata_acpi_detach_device(struc
 
 		ata_port_for_each_link(tlink, ap)
 			ata_link_for_each_dev(tdev, tlink)
-			tdev->flags |= ATA_DFLAG_DETACH;
+				tdev->flags |= ATA_DFLAG_DETACH;
 	}
 
 	ata_port_freeze(ap);
_

was intended.

> +	}
> +
> +	ata_port_freeze(ap);
> +	ata_port_schedule_eh(ap);
> +}
> +
>  static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device
> -				    *dev, u32 event)
> +				    *dev, u32 event, int is_dock_event)
>  {
>  	char event_string[12];
>  	char *envp[] = { event_string, NULL };
> @@ -135,83 +152,92 @@ static void ata_acpi_handle_hotplug(stru
>  		ap = dev->link->ap;
>  	ehi = &ap->link.eh_info;
>  
> -	spin_lock_irqsave(ap->lock, flags);
> -
> -	if (dev)
> +	if (dev) {
> +		if (dev->sdev)
> +			kobj = &dev->sdev->sdev_gendev.kobj;
>  		handle = dev->acpi_handle;
> -	else
> +	} else {
> +		kobj = &ap->dev->kobj;
>  		handle = ap->acpi_handle;
> +	}
>  
>  	status = acpi_get_handle(handle, "_EJ0", &tmphandle);
>  	if (ACPI_FAILURE(status)) {
> -		/* This device is not ejectable */
> -		spin_unlock_irqrestore(ap->lock, flags);
> +		/* This device does not support hotplug */
>  		return;
>  	}
>  
> -	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
> -	if (ACPI_FAILURE(status)) {
> -		printk ("Unable to determine bay status\n");
> -		spin_unlock_irqrestore(ap->lock, flags);
> -		return;
> +	if (kobj && !is_dock_event) {
> +		sprintf(event_string, "BAY_EVENT=%d", event);
> +		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
>  	}
>  
> +	spin_lock_irqsave(ap->lock, flags);

Wow, lots of stuff happening under spinlock here, but it all looks OK to me.

>  	switch (event) {
>  	case ACPI_NOTIFY_BUS_CHECK:
>  	case ACPI_NOTIFY_DEVICE_CHECK:
>  		ata_ehi_push_desc(ehi, "ACPI event");
> -		if (!sta) {
> -                /* Device has been unplugged */
> -			if (dev)
> -				dev->flags |= ATA_DFLAG_DETACH;
> -			else {
> -				struct ata_link *tlink;
> -				struct ata_device *tdev;
> -
> -				ata_port_for_each_link(tlink, ap) {
> -					ata_link_for_each_dev(tdev, tlink) {
> -						tdev->flags |=
> -							ATA_DFLAG_DETACH;
> -					}
> -				}

The old code was less odd ;)