Re: [PATCH] libata: Forcing PIO0 mode on reset must not freeze system

[Holger Macht <hmacht@suse.de>]

On Mon 11. Feb - 20:16:38, Tejun Heo wrote:
> Hello,
> 
> Holger Macht wrote:
> >> In the above example, even the reset sequence itself can cause hang if
> >> the hardware is implemented slightly differently.  The reason why
> >> set_piomode() locks up but reset sequence doesn't is simple dumb luck.
> >> I think the proper fix is to tell libata to detach the cdrom before
> >> undocking.
> > 
> > Wouldn't the proper fix be to call ata_acpi_handle_hotplug _somewhere_?
> > (which is currently called nowhere AFAICS)
> 
> It should be called via ata_acpi_{ap|dev}_notify() callbacks installed
> via acpi_install_notify_handler().  Can you add dump_stack() in the
> function and verify that it actually is being called?  It could be that
> the method is called too late or libata takes too long to actually
> unplug the device.  Hmmm... It seems what ata_acpi_handle_hotplug() does
> isn't enough for undock.  It probably should request detaching the
> device instead of just notifying hotplug event.  Anyways, please lemme
> know whether and when the function is called.

I already checked, it's never called AFAICS. And I couldn't find a place
where it should be installed, otherwise, I would have sent a patch. The
dock driver already calls the notify methods on devices in the dock
station before doing the real undock.

> > Anyway, kernel hackers keep telling me that the kernel should just do the
> > right thing. Shouldn't userspace never be able to freeze the system?
> 
> Yeah, I think most things should be done automatically but it's true
> that somethings are a bit awkward to handle in kernel.  Also, if you're
> root, you can almost always crash the machine from userland.
> 
> > It's completely ok for me to handle this from userspace, if that's the
> > position of the libata developers.
> 
> Let's see whether we can fix the ACPI handler first.

Yes,

> > In this case, we should change the dock driver to default to
> > immediate_undock=false, because otherwise it's far too risky to freeze the
> > system.
> 
> I'm not too familiar with how docks work.  Can you please explain
> briefly what immediate_undock is?

immediate_undock=1:
 User presses undock button on the dock station, dock driver calls ACPI
 undock method immediately.

immediate_undock=0:
 User presses undock button on the dock station, dock driver throws uevent
 and waits for userland to undock the system via sysfs.

immediate_undock is currently set to 1 by default.

Regards,
	Holger