If a device/bay is inside a docking station, we need to register for dock
events additionally to bay events. If a dock event occurs, the dock driver
will call the appropriate handler (ata_acpi_ap_notify() or
ata_acpi_dev_notify()) for us.
Signed-off-by: Holger Macht <[email protected]>
---
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 9e8ec19..fbe7b34 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -191,20 +191,29 @@ void ata_acpi_associate(struct ata_host *host)
else
ata_acpi_associate_ide_port(ap);
- if (ap->acpi_handle)
+ if (ap->acpi_handle) {
acpi_install_notify_handler (ap->acpi_handle,
ACPI_SYSTEM_NOTIFY,
ata_acpi_ap_notify,
ap);
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_ap_notify, ap);
+ }
for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
struct ata_device *dev = &ap->link.device[j];
- if (dev->acpi_handle)
+ if (dev->acpi_handle) {
acpi_install_notify_handler (dev->acpi_handle,
ACPI_SYSTEM_NOTIFY,
ata_acpi_dev_notify,
dev);
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_dev_notify,
+ ap);
+ }
}
}
}
On Thu 14. Feb - 13:40:48, Holger Macht wrote:
> If a device/bay is inside a docking station, we need to register for dock
> events additionally to bay events. If a dock event occurs, the dock driver
> will call the appropriate handler (ata_acpi_ap_notify() or
> ata_acpi_dev_notify()) for us.
>
> Signed-off-by: Holger Macht <[email protected]>
Updated patch which only includes the dock specific function if the dock
driver is actually compiled.
Signed-off-by: Holger Macht <[email protected]>
---
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 9e8ec19..5f16055 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -191,20 +191,33 @@ void ata_acpi_associate(struct ata_host *host)
else
ata_acpi_associate_ide_port(ap);
- if (ap->acpi_handle)
+ if (ap->acpi_handle) {
acpi_install_notify_handler (ap->acpi_handle,
ACPI_SYSTEM_NOTIFY,
ata_acpi_ap_notify,
ap);
+#ifdef CONFIG_ACPI_DOCK
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_ap_notify, ap);
+#endif
+ }
for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
struct ata_device *dev = &ap->link.device[j];
- if (dev->acpi_handle)
+ if (dev->acpi_handle) {
acpi_install_notify_handler (dev->acpi_handle,
ACPI_SYSTEM_NOTIFY,
ata_acpi_dev_notify,
dev);
+#ifdef CONFIG_ACPI_DOCK
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_dev_notify,
+ ap);
+#endif
+ }
}
}
}
Holger Macht wrote:
> On Thu 14. Feb - 13:40:48, Holger Macht wrote:
>> If a device/bay is inside a docking station, we need to register for dock
>> events additionally to bay events. If a dock event occurs, the dock driver
>> will call the appropriate handler (ata_acpi_ap_notify() or
>> ata_acpi_dev_notify()) for us.
>>
>> Signed-off-by: Holger Macht <[email protected]>
>
> Updated patch which only includes the dock specific function if the dock
> driver is actually compiled.
>
> Signed-off-by: Holger Macht <[email protected]>
> ---
>
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 9e8ec19..5f16055 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -191,20 +191,33 @@ void ata_acpi_associate(struct ata_host *host)
> else
> ata_acpi_associate_ide_port(ap);
>
> - if (ap->acpi_handle)
> + if (ap->acpi_handle) {
> acpi_install_notify_handler (ap->acpi_handle,
> ACPI_SYSTEM_NOTIFY,
> ata_acpi_ap_notify,
> ap);
> +#ifdef CONFIG_ACPI_DOCK
> + /* we might be on a docking station */
> + register_hotplug_dock_device(ap->acpi_handle,
> + ata_acpi_ap_notify, ap);
> +#endif
> + }
>
> for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
> struct ata_device *dev = &ap->link.device[j];
>
> - if (dev->acpi_handle)
> + if (dev->acpi_handle) {
> acpi_install_notify_handler (dev->acpi_handle,
> ACPI_SYSTEM_NOTIFY,
> ata_acpi_dev_notify,
> dev);
> +#ifdef CONFIG_ACPI_DOCK
> + /* we might be on a docking station */
> + register_hotplug_dock_device(ap->acpi_handle,
> + ata_acpi_dev_notify,
> + ap);
> +#endif
CONFIG_ACPI_DOCK is a tristate, so you might have CONFIG_ACPI_DOCK_MODULE...
On Wed 20. Feb - 12:11:32, Jeff Garzik wrote:
> Holger Macht wrote:
>> On Thu 14. Feb - 13:40:48, Holger Macht wrote:
>>> If a device/bay is inside a docking station, we need to register for dock
>>> events additionally to bay events. If a dock event occurs, the dock driver
>>> will call the appropriate handler (ata_acpi_ap_notify() or
>>> ata_acpi_dev_notify()) for us.
>>>
>>> Signed-off-by: Holger Macht <[email protected]>
>>
>> Updated patch which only includes the dock specific function if the dock
>> driver is actually compiled.
>>
>> Signed-off-by: Holger Macht <[email protected]>
>> ---
>>
>> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
>> index 9e8ec19..5f16055 100644
>> --- a/drivers/ata/libata-acpi.c
>> +++ b/drivers/ata/libata-acpi.c
>> @@ -191,20 +191,33 @@ void ata_acpi_associate(struct ata_host *host)
>> else
>> ata_acpi_associate_ide_port(ap);
>> - if (ap->acpi_handle)
>> + if (ap->acpi_handle) {
>> acpi_install_notify_handler (ap->acpi_handle,
>> ACPI_SYSTEM_NOTIFY,
>> ata_acpi_ap_notify,
>> ap);
>> +#ifdef CONFIG_ACPI_DOCK
>> + /* we might be on a docking station */
>> + register_hotplug_dock_device(ap->acpi_handle,
>> + ata_acpi_ap_notify, ap);
>> +#endif
>> + }
>> for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
>> struct ata_device *dev = &ap->link.device[j];
>> - if (dev->acpi_handle)
>> + if (dev->acpi_handle) {
>> acpi_install_notify_handler (dev->acpi_handle,
>> ACPI_SYSTEM_NOTIFY,
>> ata_acpi_dev_notify,
>> dev);
>> +#ifdef CONFIG_ACPI_DOCK
>> + /* we might be on a docking station */
>> + register_hotplug_dock_device(ap->acpi_handle,
>> + ata_acpi_dev_notify,
>> + ap);
>> +#endif
>
> CONFIG_ACPI_DOCK is a tristate, so you might have CONFIG_ACPI_DOCK_MODULE...
Thanks for the hint, so once again:
If a device/bay is inside a docking station, we need to register for dock
events additionally to bay events. If a dock event occurs, the dock driver
will call the appropriate handler (ata_acpi_ap_notify() or
ata_acpi_dev_notify()) for us.
Signed-off-by: Holger Macht <[email protected]>
---
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 9e8ec19..563ad72 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -191,20 +191,33 @@ void ata_acpi_associate(struct ata_host *host)
else
ata_acpi_associate_ide_port(ap);
- if (ap->acpi_handle)
+ if (ap->acpi_handle) {
acpi_install_notify_handler (ap->acpi_handle,
ACPI_SYSTEM_NOTIFY,
ata_acpi_ap_notify,
ap);
+#ifdef CONFIG_ACPI_DOCK_MODULE
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_ap_notify, ap);
+#endif
+ }
for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
struct ata_device *dev = &ap->link.device[j];
- if (dev->acpi_handle)
+ if (dev->acpi_handle) {
acpi_install_notify_handler (dev->acpi_handle,
ACPI_SYSTEM_NOTIFY,
ata_acpi_dev_notify,
dev);
+#ifdef CONFIG_ACPI_DOCK_MODULE
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_dev_notify,
+ ap);
+#endif
+ }
}
}
}
> If a device/bay is inside a docking station, we need to register for dock
> events additionally to bay events. If a dock event occurs, the dock driver
> will call the appropriate handler (ata_acpi_ap_notify() or
> ata_acpi_dev_notify()) for us.
>
> Signed-off-by: Holger Macht <[email protected]>
> ---
>
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 9e8ec19..563ad72 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -191,20 +191,33 @@ void ata_acpi_associate(struct ata_host *host)
> else
> ata_acpi_associate_ide_port(ap);
>
> - if (ap->acpi_handle)
> + if (ap->acpi_handle) {
> acpi_install_notify_handler (ap->acpi_handle,
> ACPI_SYSTEM_NOTIFY,
> ata_acpi_ap_notify,
> ap);
> +#ifdef CONFIG_ACPI_DOCK_MODULE
Heh, you need
#if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
Also, another question. Is there a way to tell whether the device or
port is connected behind a dock or not? Just notifying hotplug signal
is fine for hotplugging but to make hot unplug safe for PATA, libata
should be able to tell whether the device is actually gonna go away and
kill it explicitly.
Thanks.
--
tejun
On Fri 22. Feb - 10:34:24, Tejun Heo wrote:
> > If a device/bay is inside a docking station, we need to register for dock
> > events additionally to bay events. If a dock event occurs, the dock driver
> > will call the appropriate handler (ata_acpi_ap_notify() or
> > ata_acpi_dev_notify()) for us.
> >
> > Signed-off-by: Holger Macht <[email protected]>
> > ---
> >
> > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> > index 9e8ec19..563ad72 100644
> > --- a/drivers/ata/libata-acpi.c
> > +++ b/drivers/ata/libata-acpi.c
> > @@ -191,20 +191,33 @@ void ata_acpi_associate(struct ata_host *host)
> > else
> > ata_acpi_associate_ide_port(ap);
> >
> > - if (ap->acpi_handle)
> > + if (ap->acpi_handle) {
> > acpi_install_notify_handler (ap->acpi_handle,
> > ACPI_SYSTEM_NOTIFY,
> > ata_acpi_ap_notify,
> > ap);
> > +#ifdef CONFIG_ACPI_DOCK_MODULE
>
> Heh, you need
>
> #if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
>
> Also, another question. Is there a way to tell whether the device or
> port is connected behind a dock or not? Just notifying hotplug signal
> is fine for hotplugging but to make hot unplug safe for PATA, libata
> should be able to tell whether the device is actually gonna go away and
> kill it explicitly.
The hotplug handler is only called if the device is actually inside the
dock station. If it is not, nothing will happen. I hope that I got your
question right?
However, if this would be helpful, it would be easy to add something like
a am_I_on_dock_station?(...) function to the dock driver.
Regards,
Holger
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 9e8ec19..e1bdca1 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -118,45 +118,76 @@ 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 kobject *kobj,
+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 };
struct ata_eh_info *ehi = &ap->link.eh_info;
+ struct kobject *kobj = NULL;
+ int wait = 0;
+ unsigned long flags;
+
+ if (!ap)
+ ap = dev->link->ap;
+
+ 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;
+
+ 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 (event == 0 || event == 1) {
- unsigned long flags;
- spin_lock_irqsave(ap->lock, flags);
- ata_ehi_clear_desc(ehi);
- ata_ehi_push_desc(ehi, "ACPI event");
- ata_ehi_hotplugged(ehi);
- ata_port_freeze(ap);
- spin_unlock_irqrestore(ap->lock, flags);
+ ata_port_schedule_eh(ap);
+ 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(acpi_handle handle, u32 event, void *data)
{
struct ata_device *dev = data;
- struct kobject *kobj = NULL;
-
- if (dev->sdev)
- kobj = &dev->sdev->sdev_gendev.kobj;
- ata_acpi_handle_hotplug(dev->link->ap, kobj, event);
+ ata_acpi_handle_hotplug(NULL, dev, event);
}
static void ata_acpi_ap_notify(acpi_handle handle, u32 event, void *data)
{
struct ata_port *ap = data;
- ata_acpi_handle_hotplug(ap, &ap->dev->kobj, event);
+ ata_acpi_handle_hotplug(ap, NULL, event);
}
/**
@@ -191,20 +222,33 @@ void ata_acpi_associate(struct ata_host *host)
else
ata_acpi_associate_ide_port(ap);
- if (ap->acpi_handle)
+ if (ap->acpi_handle) {
acpi_install_notify_handler (ap->acpi_handle,
ACPI_SYSTEM_NOTIFY,
ata_acpi_ap_notify,
ap);
+#if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_ap_notify, ap);
+#endif
+ }
for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
struct ata_device *dev = &ap->link.device[j];
- if (dev->acpi_handle)
+ if (dev->acpi_handle) {
acpi_install_notify_handler (dev->acpi_handle,
ACPI_SYSTEM_NOTIFY,
ata_acpi_dev_notify,
dev);
+#if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_dev_notify,
+ ap);
+#endif
+ }
}
}
}
On Thu 28. Feb - 18:35:06, Tejun Heo wrote:
> Holger Macht wrote:
> > The hotplug handler is only called if the device is actually inside the
> > dock station. If it is not, nothing will happen. I hope that I got your
> > question right?
>
> Yes, right.
>
> > However, if this would be helpful, it would be easy to add something like
> > a am_I_on_dock_station?(...) function to the dock driver.
>
> Hmm.. as long as the event is only delivered when the device is actually
> connected behind dock, I think it's okay.
The dock driver also export a is_dock_device(acpi_handle) function, which
could be used to make more fine-grained decisions, but it shouldn't be
needed here.
> Does the attached patch fix the previous undock problem? It now
> explicitly tells libata EH to detach the notified devices on
> EJECT_REQUEST and wait for EH to complete such that control is returned
> to ACPI after all notified devices are actually detached.
No it does not. Apparently, it freezes faster (from 1 second down to
immediately). Before, it just froze when someone (in this case HAL) tried
to access the device. The "echo 1 > undock" call does not even return, so
it might have introduced another problem.
Regards,
Holger
Holger Macht wrote:
> On Thu 28. Feb - 18:35:06, Tejun Heo wrote:
>> Holger Macht wrote:
>>> The hotplug handler is only called if the device is actually inside the
>>> dock station. If it is not, nothing will happen. I hope that I got your
>>> question right?
>> Yes, right.
>>
>>> However, if this would be helpful, it would be easy to add something like
>>> a am_I_on_dock_station?(...) function to the dock driver.
>> Hmm.. as long as the event is only delivered when the device is actually
>> connected behind dock, I think it's okay.
>
> The dock driver also export a is_dock_device(acpi_handle) function, which
> could be used to make more fine-grained decisions, but it shouldn't be
> needed here.
>
>> Does the attached patch fix the previous undock problem? It now
>> explicitly tells libata EH to detach the notified devices on
>> EJECT_REQUEST and wait for EH to complete such that control is returned
>> to ACPI after all notified devices are actually detached.
>
> No it does not. Apparently, it freezes faster (from 1 second down to
> immediately). Before, it just froze when someone (in this case HAL) tried
> to access the device. The "echo 1 > undock" call does not even return, so
> it might have introduced another problem.
The code should be in generally right direction. Can you be persuaded
into tracking down what's going on?
Thanks.
--
tejun
On Thu 28. Feb - 22:05:53, Tejun Heo wrote:
> Holger Macht wrote:
> > On Thu 28. Feb - 18:35:06, Tejun Heo wrote:
> >> Holger Macht wrote:
> >>> The hotplug handler is only called if the device is actually inside the
> >>> dock station. If it is not, nothing will happen. I hope that I got your
> >>> question right?
> >> Yes, right.
> >>
> >>> However, if this would be helpful, it would be easy to add something like
> >>> a am_I_on_dock_station?(...) function to the dock driver.
> >> Hmm.. as long as the event is only delivered when the device is actually
> >> connected behind dock, I think it's okay.
> >
> > The dock driver also export a is_dock_device(acpi_handle) function, which
> > could be used to make more fine-grained decisions, but it shouldn't be
> > needed here.
> >
> >> Does the attached patch fix the previous undock problem? It now
> >> explicitly tells libata EH to detach the notified devices on
> >> EJECT_REQUEST and wait for EH to complete such that control is returned
> >> to ACPI after all notified devices are actually detached.
> >
> > No it does not. Apparently, it freezes faster (from 1 second down to
> > immediately). Before, it just froze when someone (in this case HAL) tried
> > to access the device. The "echo 1 > undock" call does not even return, so
> > it might have introduced another problem.
>
> The code should be in generally right direction. Can you be persuaded
> into tracking down what's going on?
I had a quick glance with adding some printk's. Now I got a different
behaviour once. System did not freeze, but were certainly confused. The
last thing which got printed to messages was exactly before
spin_lock_irqsave(ap->lock, flags); at the beginning of ata_acpi_handle_hotplug(...)
The printk immediately after this call didn't come through anymore (with
being able to use the system for a short time afterwards).
Maybe this helps.
For further debugging, I would have to setup remote debugger, but I doubt
I get around to do this for the next couple of days or even 1-2 weeks.
I could test new patches, of course.
Regards,
Holger
P.S.: You'll need [1] patch for testing, otherwise the hotplug handler is
never called.
[1] http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commit;h=3b5fee5952ff7eb6ff7a64247a01040b8b331b74
On Thu 28. Feb - 16:58:17, Holger Macht wrote:
> On Thu 28. Feb - 22:05:53, Tejun Heo wrote:
> > Holger Macht wrote:
> > > On Thu 28. Feb - 18:35:06, Tejun Heo wrote:
> > >> Holger Macht wrote:
> > >>> The hotplug handler is only called if the device is actually inside the
> > >>> dock station. If it is not, nothing will happen. I hope that I got your
> > >>> question right?
> > >> Yes, right.
> > >>
> > >>> However, if this would be helpful, it would be easy to add something like
> > >>> a am_I_on_dock_station?(...) function to the dock driver.
> > >> Hmm.. as long as the event is only delivered when the device is actually
> > >> connected behind dock, I think it's okay.
> > >
> > > The dock driver also export a is_dock_device(acpi_handle) function, which
> > > could be used to make more fine-grained decisions, but it shouldn't be
> > > needed here.
> > >
> > >> Does the attached patch fix the previous undock problem? It now
> > >> explicitly tells libata EH to detach the notified devices on
> > >> EJECT_REQUEST and wait for EH to complete such that control is returned
> > >> to ACPI after all notified devices are actually detached.
> > >
> > > No it does not. Apparently, it freezes faster (from 1 second down to
> > > immediately). Before, it just froze when someone (in this case HAL) tried
> > > to access the device. The "echo 1 > undock" call does not even return, so
> > > it might have introduced another problem.
> >
> > The code should be in generally right direction. Can you be persuaded
> > into tracking down what's going on?
>
> I had a quick glance with adding some printk's. Now I got a different
> behaviour once. System did not freeze, but were certainly confused. The
> last thing which got printed to messages was exactly before
> spin_lock_irqsave(ap->lock, flags); at the beginning of ata_acpi_handle_hotplug(...)
>
> The printk immediately after this call didn't come through anymore (with
> being able to use the system for a short time afterwards).
Ok, it seems that there is something broken somewhere else in
2.6.25.rc3. Not sure at all if it's your patch freezing the machine. I'll
give 2.6.24.3 a try...
Regards,
Holger
On Thu 28. Feb - 19:32:43, Holger Macht wrote:
> On Thu 28. Feb - 16:58:17, Holger Macht wrote:
> > On Thu 28. Feb - 22:05:53, Tejun Heo wrote:
> > > Holger Macht wrote:
> > > > On Thu 28. Feb - 18:35:06, Tejun Heo wrote:
> > > >> Holger Macht wrote:
> > > >>> The hotplug handler is only called if the device is actually inside the
> > > >>> dock station. If it is not, nothing will happen. I hope that I got your
> > > >>> question right?
> > > >> Yes, right.
> > > >>
> > > >>> However, if this would be helpful, it would be easy to add something like
> > > >>> a am_I_on_dock_station?(...) function to the dock driver.
> > > >> Hmm.. as long as the event is only delivered when the device is actually
> > > >> connected behind dock, I think it's okay.
> > > >
> > > > The dock driver also export a is_dock_device(acpi_handle) function, which
> > > > could be used to make more fine-grained decisions, but it shouldn't be
> > > > needed here.
> > > >
> > > >> Does the attached patch fix the previous undock problem? It now
> > > >> explicitly tells libata EH to detach the notified devices on
> > > >> EJECT_REQUEST and wait for EH to complete such that control is returned
> > > >> to ACPI after all notified devices are actually detached.
> > > >
> > > > No it does not. Apparently, it freezes faster (from 1 second down to
> > > > immediately). Before, it just froze when someone (in this case HAL) tried
> > > > to access the device. The "echo 1 > undock" call does not even return, so
> > > > it might have introduced another problem.
> > >
> > > The code should be in generally right direction. Can you be persuaded
> > > into tracking down what's going on?
> >
> > I had a quick glance with adding some printk's. Now I got a different
> > behaviour once. System did not freeze, but were certainly confused. The
> > last thing which got printed to messages was exactly before
> > spin_lock_irqsave(ap->lock, flags); at the beginning of ata_acpi_handle_hotplug(...)
> >
> > The printk immediately after this call didn't come through anymore (with
> > being able to use the system for a short time afterwards).
>
> Ok, it seems that there is something broken somewhere else in
> 2.6.25.rc3. Not sure at all if it's your patch freezing the machine. I'll
> give 2.6.24.3 a try...
So once again...
After applying your patch, I got the OOPS seen in attachment
'oops-undock-1'. After changing the following, which is hopefully
correct...
--- ../orig/linux-2.6.24.3/drivers/ata/libata-acpi.c 2008-02-29 00:31:44.000000000 +0100
+++ drivers/ata/libata-acpi.c 2008-02-29 00:32:26.000000000 +0100
@@ -123,7 +123,7 @@
{
char event_string[12];
char *envp[] = { event_string, NULL };
- struct ata_eh_info *ehi = &ap->link.eh_info;
+ struct ata_eh_info *ehi;
struct kobject *kobj = NULL;
int wait = 0;
unsigned long flags;
@@ -131,6 +131,8 @@
if (!ap)
ap = dev->link->ap;
+ ehi = &ap->link.eh_info;
+
spin_lock_irqsave(ap->lock, flags);
switch (event) {
...I got both an oops when docking (attachments oops-dock) and when undocking
(attachment oops-undock2).
Regards,
Holger
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 9e8ec19..ea01875 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -118,45 +118,77 @@ 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 kobject *kobj,
+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 };
- struct ata_eh_info *ehi = &ap->link.eh_info;
-
- if (event == 0 || event == 1) {
- unsigned long flags;
- spin_lock_irqsave(ap->lock, flags);
- ata_ehi_clear_desc(ehi);
- ata_ehi_push_desc(ehi, "ACPI event");
- ata_ehi_hotplugged(ehi);
- ata_port_freeze(ap);
- spin_unlock_irqrestore(ap->lock, flags);
+ struct ata_eh_info *ehi;
+ struct kobject *kobj = NULL;
+ int wait = 0;
+ unsigned long flags;
+
+ if (!ap)
+ ap = dev->link->ap;
+ ehi = &ap->link.eh_info;
+
+ 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;
+
+ 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;
+ }
+
+ ata_port_schedule_eh(ap);
+ 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(acpi_handle handle, u32 event, void *data)
{
struct ata_device *dev = data;
- struct kobject *kobj = NULL;
- if (dev->sdev)
- kobj = &dev->sdev->sdev_gendev.kobj;
-
- ata_acpi_handle_hotplug(dev->link->ap, kobj, event);
+ ata_acpi_handle_hotplug(NULL, dev, event);
}
static void ata_acpi_ap_notify(acpi_handle handle, u32 event, void *data)
{
struct ata_port *ap = data;
- ata_acpi_handle_hotplug(ap, &ap->dev->kobj, event);
+ ata_acpi_handle_hotplug(ap, NULL, event);
}
/**
@@ -191,20 +223,30 @@ void ata_acpi_associate(struct ata_host *host)
else
ata_acpi_associate_ide_port(ap);
- if (ap->acpi_handle)
- acpi_install_notify_handler (ap->acpi_handle,
- ACPI_SYSTEM_NOTIFY,
- ata_acpi_ap_notify,
- ap);
+ if (ap->acpi_handle) {
+ acpi_install_notify_handler(ap->acpi_handle,
+ ACPI_SYSTEM_NOTIFY,
+ ata_acpi_ap_notify, ap);
+#if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_ap_notify, ap);
+#endif
+ }
for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
struct ata_device *dev = &ap->link.device[j];
- if (dev->acpi_handle)
- acpi_install_notify_handler (dev->acpi_handle,
- ACPI_SYSTEM_NOTIFY,
- ata_acpi_dev_notify,
- dev);
+ if (dev->acpi_handle) {
+ acpi_install_notify_handler(dev->acpi_handle,
+ ACPI_SYSTEM_NOTIFY,
+ ata_acpi_dev_notify, dev);
+#if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
+ /* we might be on a docking station */
+ register_hotplug_dock_device(ap->acpi_handle,
+ ata_acpi_dev_notify, dev);
+#endif
+ }
}
}
}
On Tue 04. Mar - 13:12:54, Tejun Heo wrote:
> Holger Macht wrote:
> > On Thu 28. Feb - 19:32:43, Holger Macht wrote:
> >> On Thu 28. Feb - 16:58:17, Holger Macht wrote:
> >>> On Thu 28. Feb - 22:05:53, Tejun Heo wrote:
> >>>> Holger Macht wrote:
> >>>>> On Thu 28. Feb - 18:35:06, Tejun Heo wrote:
> >>>>>> Holger Macht wrote:
> >>>>>>> The hotplug handler is only called if the device is actually inside the
> >>>>>>> dock station. If it is not, nothing will happen. I hope that I got your
> >>>>>>> question right?
> >>>>>> Yes, right.
> >>>>>>
> >>>>>>> However, if this would be helpful, it would be easy to add something like
> >>>>>>> a am_I_on_dock_station?(...) function to the dock driver.
> >>>>>> Hmm.. as long as the event is only delivered when the device is actually
> >>>>>> connected behind dock, I think it's okay.
> >>>>> The dock driver also export a is_dock_device(acpi_handle) function, which
> >>>>> could be used to make more fine-grained decisions, but it shouldn't be
> >>>>> needed here.
> >>>>>
> >>>>>> Does the attached patch fix the previous undock problem? It now
> >>>>>> explicitly tells libata EH to detach the notified devices on
> >>>>>> EJECT_REQUEST and wait for EH to complete such that control is returned
> >>>>>> to ACPI after all notified devices are actually detached.
> >>>>> No it does not. Apparently, it freezes faster (from 1 second down to
> >>>>> immediately). Before, it just froze when someone (in this case HAL) tried
> >>>>> to access the device. The "echo 1 > undock" call does not even return, so
> >>>>> it might have introduced another problem.
> >>>> The code should be in generally right direction. Can you be persuaded
> >>>> into tracking down what's going on?
> >>> I had a quick glance with adding some printk's. Now I got a different
> >>> behaviour once. System did not freeze, but were certainly confused. The
> >>> last thing which got printed to messages was exactly before
> >>> spin_lock_irqsave(ap->lock, flags); at the beginning of ata_acpi_handle_hotplug(...)
> >>>
> >>> The printk immediately after this call didn't come through anymore (with
> >>> being able to use the system for a short time afterwards).
> >> Ok, it seems that there is something broken somewhere else in
> >> 2.6.25.rc3. Not sure at all if it's your patch freezing the machine. I'll
> >> give 2.6.24.3 a try...
> >
> > So once again...
> >
> > After applying your patch, I got the OOPS seen in attachment
> > 'oops-undock-1'. After changing the following, which is hopefully
> > correct...
> >
> > --- ../orig/linux-2.6.24.3/drivers/ata/libata-acpi.c 2008-02-29 00:31:44.000000000 +0100
> > +++ drivers/ata/libata-acpi.c 2008-02-29 00:32:26.000000000 +0100
> > @@ -123,7 +123,7 @@
> > {
> > char event_string[12];
> > char *envp[] = { event_string, NULL };
> > - struct ata_eh_info *ehi = &ap->link.eh_info;
> > + struct ata_eh_info *ehi;
> > struct kobject *kobj = NULL;
> > int wait = 0;
> > unsigned long flags;
> > @@ -131,6 +131,8 @@
> > if (!ap)
> > ap = dev->link->ap;
> >
> > + ehi = &ap->link.eh_info;
> > +
> > spin_lock_irqsave(ap->lock, flags);
> >
> > switch (event) {
> >
> >
> > ...I got both an oops when docking (attachments oops-dock) and when undocking
> > (attachment oops-undock2).
>
> Yeah, that was one mistake. There's another.
>
> +#if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
> + /* we might be on a docking station */
> + register_hotplug_dock_device(ap->acpi_handle,
> + ata_acpi_dev_notify,
> + ap);
> +#endif
>
> dev_notify is being registered with a pointer to ap. No wonder it causes
It seems this change is missing from your patch. I've attached a fixed
version...
> strange dereferences later on. Attached is the fixed patch. Can you
> please give it a shot?
...and now the good news...the new patch works flawlessly with
2.6.25-rc5...
undocking...
ata5.00: disabled
ata5.00: detaching (SCSI 4:0:0:0)
ACPI: \_SB_.GDCK - undocking
usb 1-6: USB disconnect, address 5
docking...
ACPI: \_SB_.GDCK - docking
ata5: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xa frozen
ata5: ACPI event
ata5: soft resetting link
ata5.00: ATAPI: HL-DT-ST DVDRAM GSA-4083N, 1.08, max UDMA/33
ata5.00: configured for UDMA/33
ata5: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0x1 t4
ata5: ACPI event
ata5: soft resetting link
ata5.00: configured for UDMA/33
ata5: EH complete
scsi 4:0:0:0: CD-ROM HL-DT-ST DVDRAM GSA-4083N 1.08 PQ: 0 ANSI: 5
sr0: scsi3-mmc drive: 24x/24x writer dvd-ram cd/rw xa/form2 cdda tray
sr 4:0:0:0: Attached scsi CD-ROM sr0
sr 4:0:0:0: Attached scsi generic sg1 type 5
Thanks,
Holger
Hello, Holger.
Holger Macht wrote:
>> dev_notify is being registered with a pointer to ap. No wonder it causes
>
> It seems this change is missing from your patch. I've attached a fixed
> version...
Ah... I must have sent the wrong version of patch. Thanks for the fix.
>> strange dereferences later on. Attached is the fixed patch. Can you
>> please give it a shot?
>
> ...and now the good news...the new patch works flawlessly with
> 2.6.25-rc5...
>
> undocking...
> ata5.00: disabled
> ata5.00: detaching (SCSI 4:0:0:0)
> ACPI: \_SB_.GDCK - undocking
> usb 1-6: USB disconnect, address 5
>
> docking...
> ACPI: \_SB_.GDCK - docking
> ata5: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xa frozen
> ata5: ACPI event
> ata5: soft resetting link
> ata5.00: ATAPI: HL-DT-ST DVDRAM GSA-4083N, 1.08, max UDMA/33
> ata5.00: configured for UDMA/33
> ata5: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0x1 t4
> ata5: ACPI event
> ata5: soft resetting link
> ata5.00: configured for UDMA/33
> ata5: EH complete
> scsi 4:0:0:0: CD-ROM HL-DT-ST DVDRAM GSA-4083N 1.08 PQ: 0 ANSI: 5
> sr0: scsi3-mmc drive: 24x/24x writer dvd-ram cd/rw xa/form2 cdda tray
> sr 4:0:0:0: Attached scsi CD-ROM sr0
> sr 4:0:0:0: Attached scsi generic sg1 type 5
Super, I'll forward the patch upstream.
Thanks.
--
tejun