2.6.25 semantic change in bay handling?
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.
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 6 lines · Matthew Garrett
48feb3c419508487becfb9ea3afcc54c3eac6d80appears 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?
Quoted 2 lines
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.
Quoted 3 lines
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
Quoted 9 lines · Holger Macht
Quoted 6 lines · Matthew Garrett
48feb3c419508487becfb9ea3afcc54c3eac6d80appears 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).
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Signature · 1 line
Matthew Garrett | [email protected]
(cc'ing Holger Macht, copying whole body for him)
Hello, guys.
Quoted 3 lines · Matthew Garrett
Quoted 2 lines · Holger Macht
Quoted 1 line · Matthew Garrett
48feb3c419508487becfb9ea3afcc54c3eac6d80appears to flag a device as
That should be 233f112042d0b50170212dbff99c3b34b8773cd3, right?
Quoted 26 lines
Quoted 2 lines
Quoted 1 line
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).
Quoted 6 lines
Quoted 1 line
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.
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 9 lines
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.
Quoted 4 lines
Quoted 1 line
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.
Signature · 1 line
tejun
Quoted 4 lines · Tejun Heo
Quoted 2 lines
Quoted 2 lines · Matthew Garrett
Quoted 1 line
48feb3c419508487becfb9ea3afcc54c3eac6d80appears to flag a device asThat should be
233f112042d0b50170212dbff99c3b34b8773cd3, right?
Yeah, my mistake.
Quoted 8 lines
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.
Quoted 2 lines
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.
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 7 lines · Matthew Garrett
Quoted 3 lines · Tejun Heo
Quoted 2 lines
Quoted 2 lines · Matthew Garrett
Quoted 1 line
48feb3c419508487becfb9ea3afcc54c3eac6d80appears to flag a device asThat should be
233f112042d0b50170212dbff99c3b34b8773cd3, right?Yeah, my mistake.
Quoted 7 lines
Quoted 1 line
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.
Quoted 1 line
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.
Signature · 1 line
tejun
Quoted 10 lines · Tejun Heo
Quoted 4 lines · Matthew Garrett
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...
drivers/ata/libata-acpi.c +30 -18
@@ -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.
Signature · 1 line
Matthew Garrett | [email protected]
Hi,
(Can you please keep me CC'ed, I'm not on linux-ide)
Quoting your mail from the archives:
Quoted 8 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 6 lines
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.
Quoted 5 lines
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.
Quoted 2 lines
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
Quoted 5 lines · Holger Macht
Not by default. The dock driver immediately undocks unless
immediate_undockparameter 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.
Quoted 4 lines
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
_DCKmethod). 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.
Signature · 1 line
Matthew Garrett | [email protected]
( I still had to poll linux-ide archives... )
Quoted 10 lines
Quoted 5 lines
Not by default. The dock driver immediately undocks unless
immediate_undockparameter 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.
Quoted 8 lines
Quoted 4 lines
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
_DCKmethod). 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
Quoted 6 lines · Holger Macht
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_dockuserspace 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)
Quoted 2 lines
2. Bay event: libata signals a
BAY_EVENTthrough uevent, userspace writes1 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.
Signature · 1 line
Matthew Garrett | [email protected]
Hi Matthew,
( Please TO or CC [email protected] ! )
Quoted 7 lines
Quoted 6 lines
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_dockuserspace can query)
Quoted 7 lines
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
_STAmethod 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.
Quoted 6 lines
Quoted 2 lines
2. Bay event: libata signals a
BAY_EVENTthrough uevent, userspace writes1 to /sys/.../device/deleteIn the case of an eject request, yes. In the case of a bus or device check, we should call
_STAand then delete/hotplug the device as appropriate.
Yes.
Regards,
Holger
Quoted 3 lines · Holger Macht
Hi Matthew,
( Please TO or CC [email protected] ! )
Then stop setting Mail-Followup-To: :)
Quoted 11 lines
Quoted 7 lines
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
_STAmethod 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 :)
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
2. Bay event: libata signals a
BAY_EVENTthrough uevent, userspace writes1 to /sys/.../device/deleteIn the case of an eject request, yes. In the case of a bus or device check, we should call
_STAand 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.
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 15 lines · Matthew Garrett
Quoted 3 lines · Holger Macht
Hi Matthew,
( Please TO or CC [email protected] ! )
Then stop setting Mail-Followup-To: :)
Quoted 8 lines
Quoted 7 lines
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
_STAmethod on the bay and destroying the device if it's present)
Quoted 5 lines
Quoted 3 lines
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?
Quoted 2 lines
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
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
2. Bay event: libata signals a
BAY_EVENTthrough uevent, userspace writes1 to /sys/.../device/deleteIn the case of an eject request, yes. In the case of a bus or device check, we should call
_STAand then delete/hotplug the device as appropriate.
Quoted 5 lines
Quoted 1 line
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
Quoted 15 lines · Matthew Garrett
Quoted 4 lines · Tejun Heo
Quoted 2 lines
Quoted 2 lines · Matthew Garrett
Quoted 1 line
48feb3c419508487becfb9ea3afcc54c3eac6d80appears to flag a device asThat should be
233f112042d0b50170212dbff99c3b34b8773cd3, right?Yeah, my mistake.
Quoted 6 lines
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.
Quoted 5 lines
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 2 lines · Holger Macht
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.
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 33 lines · Tejun Heo
(cc'ing Holger Macht, copying whole body for him)
Hello, guys.
Quoted 3 lines · Matthew Garrett
Quoted 2 lines · Holger Macht
Quoted 1 line · Matthew Garrett
48feb3c419508487becfb9ea3afcc54c3eac6d80appears to flag a device asThat should be
233f112042d0b50170212dbff99c3b34b8773cd3, right?Quoted 22 lines
Quoted 6 lines
Quoted 5 lines
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).
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
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.
Quoted 10 lines
Quoted 10 lines
Quoted 6 lines
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.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
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.
Quoted 6 lines
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
Quoted 5 lines
Hmmm... It would be perfect if we can tell whether DEVICE/BUS CHECK is in which direction (device coming or going away).
Thanks.
Quoted 6 lines · Holger Macht
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?
Signature · 1 line
Matthew Garrett | [email protected]
PATCH 1/1 Fixups to ATA ACPI hotplug
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.
---
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.
drivers/ata/libata-acpi.c +50 -27
@@ -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)
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] Fixups to ATA ACPI hotplug
Quoted 11 lines · Matthew Garrett
The
libata-acpi.ccode 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_STAmethod 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 <[email protected]>)
---
drivers/ata/libata-acpi.c +72 -31
@@ -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
Quoted 4 lines · Holger Macht
* In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate
_STAto 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 <[email protected]>)
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,
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] Fixups to ATA ACPI hotplug
Quoted 7 lines · Matthew Garrett
Quoted 4 lines · Holger Macht
* In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate
_STAto 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 <[email protected]>)
Quoted 6 lines
The only issue I can see is that this one doesn't check
_EJ0. Unless that's done, you'll (on some hardware) evaluate_STAon 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 <[email protected]>)
---
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c.orig +73 -47
@@ -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
drivers/ata/libata-acpi.c static void ata_acpi_handle_hotplug(stru · line 166
Quoted 3 lines · Holger Macht
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c.orig +3
-0+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.
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] Fixups to ATA ACPI hotplug
Quoted 9 lines · Matthew Garrett
Review
Quoted 3 lines · Holger Macht
(quoted diff) +3
-0+ 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
_EJ0checking 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 <[email protected]>)
---
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c.orig +69 -43
@@ -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); } } }
Matthew Garrett
Acked-byRe: [PATCH] Fixups to ATA ACPI hotplug
Quoted 15 lines · Holger Macht
Handle bay devices in dock stations
* Differentiate between bay devices in dock stations and others:
- When an
ACPI_NOTIFY_EJECT_REQUESTappears, 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 anEJECT_REQUESTwithout 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
Quoted 6 lines
* In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate
_STAto 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 <[email protected]>)Signed-off-by Holger Macht <[email protected]>
As long as this fixes the hang on dock removal, this looks good to me.
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] Fixups to ATA ACPI hotplug
In reply to Holger MachtQuoted 43 lines · On Tue, 20 May 2008 15:58:30 +0200 Holger Macht <[email protected]> wrote:
Quoted 9 lines · Matthew Garrett
Review
Quoted 3 lines · Holger Macht
(quoted diff) +3
-0+ 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
_EJ0checking 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_REQUESTappears, 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 anEJECT_REQUESTwithout 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
_STAto 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 <[email protected]>)Signed-off-by Holger Macht <[email protected]>---
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c.orig +1
-0@@ -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)
drivers/ata/libata-acpi.c static void ata_acpi_associate_ide_port( · line 131
Quoted 10 lines
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c.orig +10
-0+{ +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:
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c~ata-acpi-hotplug-handle-bay-devices-in-dock-stations-cleanup +1 -1
@@ -128,7 +128,7 @@ static void ata_acpi_detach_device(strucata_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.
Quoted 38 lines
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c.orig +14
-8+ } + +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);
drivers/ata/libata-acpi.c static void ata_acpi_handle_hotplug(stru · line 175
Quoted 10 lines
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c.orig +4
-4-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.
Quoted 8 lines
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c.orig +0
-4switch (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 ;
drivers/ata/libata-acpi.c static void ata_acpi_handle_hotplug(stru · line 176
Quoted 10 lines
drivers/ata/libata-acpi.c renamed from drivers/ata/libata-acpi.c.orig +0
-10-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 ;)
Re: [PATCH] Fixups to ATA ACPI hotplug
In reply to Matthew GarrettQuoted 12 lines · Matthew Garrett
The
libata-acpi.ccode 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_STAmethod 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.
Quoted 6 lines
Signed-off-by Matthew Garrett <[email protected]>---
Holger, I'm pretty sure that this deals with the docking station removal case, but don't have the hardware to test. If
EJECT_REQUESTis genuinely
I tried it, it doesn't.
Undock --> some userspace app accesses the device --> hard lockup
Quoted 2 lines
the only notification we receive (ie, no
BUS_CHECKorDEVICE_CHECK) then we'll need to add a separate callback for docking.
Yes, see my patch.
Regards,
Holger
Quoted 29 lines · Holger Macht
Quoted 6 lines · Matthew Garrett
48feb3c419508487becfb9ea3afcc54c3eac6d80appears 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?
Quoted 2 lines
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.
Quoted 7 lines
Quoted 3 lines
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