From mboxrd@z Thu Jan 1 00:00:00 1970 From: Geert Uytterhoeven Subject: Re: [RFC PATCH] falconide: remove needless ST-DMA locking Date: Sun, 16 Nov 2008 12:07:56 +0100 (CET) Message-ID: References: <200810172041.17191.bzolnier@gmail.com> <200810181225.48329.bzolnier@gmail.com> <200811092001.05628.bzolnier@gmail.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Return-path: Received: from harold.telenet-ops.be ([195.130.133.65]:40301 "EHLO harold.telenet-ops.be" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751722AbYKPLH7 (ORCPT ); Sun, 16 Nov 2008 06:07:59 -0500 In-Reply-To: <200811092001.05628.bzolnier@gmail.com> Sender: linux-m68k-owner@vger.kernel.org List-Id: linux-m68k@vger.kernel.org To: Bartlomiej Zolnierkiewicz Cc: Michael Schmitz , linux-ide@vger.kernel.org, linux-m68k@vger.kernel.org, Michael Schmitz , Stephen R Marenka On Sun, 9 Nov 2008, Bartlomiej Zolnierkiewicz wrote: > On Saturday 18 October 2008, Bartlomiej Zolnierkiewicz wrote: > > On Saturday 18 October 2008, Geert Uytterhoeven wrote: > > > On Sat, 18 Oct 2008, Michael Schmitz wrote: > > > > > While working on ide_do_request() improvements I stumbled upon > > > > > mismatched ide_get_lock() / ide_release_lock() calls. > > > > > > > > > > [ It seems to be known issue: > > > > > http://marc.info/?l=linux-m68k&m=121423752829622&w=2 ] > > > > > > > > It is a known issue, and I submitted a simple fix to Geert a month or so ago. > > > > It involves moving the ide_get_lock call inside the request loop instead of > > > > doing it once at the start of the function. > > > > > > See http://marc.info/?l=linux-ide&m=121473433631934&w=2 > > > > > > In response to this patch, I wondered: > > > > > > > > If hwgroup->busy serves a similar purpose to falconide_intr_lock, what > > > > > about > > > > > moving the setting/clearing of hwgroup->busy into > > > > > ide_{get,release}_lock() > > > > > (and possibly renaming ide_{get,release}_lock() to e.g. > > > > > ide_hwgroup_{set,clear}_busy())? > > > > > > > > > > What about the other places where hwgroup->busy is set/cleared? > > > > > > And Michael responsed: > > > > > > > Uh - that's where it gets a bit sticky again. hwgroup->busy is set and > > > > cleared quite a lot 'preemptively' all over ide-io.c, f.e. in timeout > > > > handling. I'm not sure > > > > whether this would just reintroduce the bug message. > > > > > > > > The lock must be held as long as there are any interrupts to be expected > > > > from IDE. If the hwgroup->busy semantics reflects just that, it's worth > > > > a try. > > > > > > Bart, can you shed a light on the hwgroup->busy semantics? > > > > hwgroup->busy means that hwgroup is busy ;-) > > > > It protects against any access to the underlying hardware so it is > > also used when device is accessed in polling mode (without waiting > > for IRQ). However your proposal still sounds fine. We can treat > > hwgroup->busy as "waiting for IRQ" flag on Falcon and it would be > > correct (we will just hold the lock needlessly sometimes but we're > > already doing exactly that). > > > > [ The one thing that we have to watch out is not to leak IDE core > > specific things to and host/platform specific ones to > > IDE core so some new abstraction level may be needed for handling > > ST-DMA lock itself (->[un]lock_host methods in struct ide_port_ops > > or something along the lines)... ] > > Since there was no follow-ups on this I've just applied Michael's > patch for now (& will push it to Linus for .28)... Thank you very much! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds