LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Holger Macht <hmacht@suse.de>
To: Tejun Heo <htejun@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org,
	alan@redhat.com, jeff@garzik.org,
	Kristen Carlson Accardi <kristen.c.accardi@intel.com>
Subject: Re: [PATCH] libata: Forcing PIO0 mode on reset must not freeze system
Date: Mon, 11 Feb 2008 11:04:46 +0100	[thread overview]
Message-ID: <20080211100446.GA4646@homac.suse.de> (raw)
In-Reply-To: <47AFB4DB.1000204@gmail.com>

On Mon 11. Feb - 11:37:15, Tejun Heo wrote:
> Hello,
> 
> Holger Macht wrote:
> > Calling ap->ops->set_piomode(ap, dev) on a device/controller which got
> > already removed, locks the system hard. Reproducibly on an X60 attached to
> > a dock station containing a cdrom device with doing
> > 
> >   $ echo 1 > /sys/devices/platform/dock.0/undock && echo 123 > /dev/sr0
> > 
> > This calls ata_eh_reset(...) which in turn tries to force PIO mode 0. But
> > the device is already gone.
> > 
> > Bisecting revealed the following commit as culprit:
> > 
> >   commit cdeab1140799f09c5f728a5ff85e0bdfa5679cd2
> >   Author: Tejun Heo <htejun@gmail.com>
> >   Date:   Mon Oct 29 16:41:09 2007 +0900
> >   
> >       libata: relocate forcing PIO0 on reset
> >       
> >       Forcing PIO0 on reset was done inside ata_bus_softreset(), which is a
> >       bit out of place as it should be applied to all resets - hard, soft
> >       and implementation which don't use ata_bus_softreset().  Relocate it
> >       such that...
> >       
> >       * For new EH, it's done in ata_eh_reset() before calling prereset.
> >       
> >       * For old EH, it's done before calling ap->ops->phy_reset() in
> >         ata_bus_probe().
> >       
> >       This makes PIO0 forced after all resets.  Another difference is that
> >       reset itself is done after PIO0 is forced.
> >       
> >       Signed-off-by: Tejun Heo <htejun@gmail.com>
> >       Acked-by: Alan Cox <alan@redhat.com>
> >       Signed-off-by: Jeff Garzik <jeff@garzik.org>
> > 
> > 
> > ATTENTION! The following patch solves the problem on my system, but please
> > be aware that I don't really know what I'm doing because I don't have the
> > big picture. There's surely a better way to check if the device/controller
> > is still functional than calling ata_link_{online,offline}.
> 
> 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)

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?

It's completely ok for me to handle this from userspace, if that's the
position of the libata developers.

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.

Regards,
	Holger

  reply	other threads:[~2008-02-11 10:04 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-10 19:55 Holger Macht
2008-02-11  2:37 ` Tejun Heo
2008-02-11 10:04   ` Holger Macht [this message]
2008-02-11 11:16     ` Tejun Heo
2008-02-11 11:28       ` Holger Macht
2008-02-11 13:11         ` Tejun Heo
2008-02-11 14:06           ` Holger Macht
2008-02-11 23:31             ` Tejun Heo
2008-02-14 12:42           ` Holger Macht
2008-02-11 11:25     ` Alan Cox
2008-02-11 10:29   ` Holger Macht
2008-02-11 10:49     ` Tejun Heo
2008-02-11 11:32     ` Alan Cox
2008-02-11 11:50   ` Alan Cox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080211100446.GA4646@homac.suse.de \
    --to=hmacht@suse.de \
    --cc=alan@redhat.com \
    --cc=htejun@gmail.com \
    --cc=jeff@garzik.org \
    --cc=kristen.c.accardi@intel.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH] libata: Forcing PIO0 mode on reset must not freeze system' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).