Re: ANNOUNCE: CE Linux Forum - Specification V1.0 draft

From: Bartlomiej Zolnierkiewicz (B.Zolnierkiewicz_at_elka.pw.edu.pl)
Date: 05/18/04

  • Next message: Matt Mackall: "Re: [patch] kill off PC9800" 
    To: Russell King <rmk+lkml@arm.linux.org.uk>, Mark Gross <mgross@linux.jf.intel.com>
Date:	Tue, 18 May 2004 22:45:23 +0200

    On Tuesday 18 of May 2004 21:56, Russell King wrote:
    > On Tue, May 18, 2004 at 12:32:48PM -0700, Mark Gross wrote:
    > > > --- linux-2.4.20.orig/drivers/ide/ide.c Thu Nov 28 23:53:13 2002
    > > > +++ celinux-040213/drivers/ide/ide.c Thu Feb 12 10:25:12 2004
    > > > @@ -2739,12 +2776,17 @@
    > > > */
    > > > void ide_delay_50ms (void)
    > > > {
    > > > +#ifdef CONFIG_IDE_PREEMPT
    > > > + __set_current_state(TASK_UNINTERRUPTIBLE);
    > > > + schedule_timeout(1+HZ/20); /* from 2.5 */
    > > > +#else /* CONFIG_IDE_PREEMPT */
    > > > #ifndef CONFIG_BLK_DEV_IDECS
    > > > mdelay(50);
    > > > #else
    > > > __set_current_state(TASK_UNINTERRUPTIBLE);
    > > > schedule_timeout(HZ/20);
    > > > #endif /* CONFIG_BLK_DEV_IDECS */
    > > > +#endif /* CONFIG_IDE_PREEMPT */
    > > > }
    > > >
    > > > This great piece 'called IDE-preempt' to be buzzword-compliant is (and
    > > > that's noticeable just from looking at the diff!) so braindead that
    > > > it's not explainable by incompetence alone. You'd get your same result
    > > > by just _disabling_ CONFIG_BLK_DEV_IDECS instead of adding another
    > > > broken config option (modulo 2.6 adjustments to the sleep time).
    > > >
    > > > Every engineer with the slightest clue would first disable that option,
    > > > or if ide-cs support is actually needed think _why_ it's different
    > > > instead of just adding a config option to disable it. Either it's safe
    > > > to always use the sleeping variant in which case the original ifdef
    > > > should go away, or it's not in which case your patch is completely
    > > > broken.
    > >
    > > OK I'll bite, but just because in your blind hostility and haste you've
    > > made a mistake ;)
    >
    > Christoph is actually making a valid point here and I suspect is trying
    > to point out the lack of thought put into this change. The things that
    > _should_ have been considered before making the change are:
    >
    > 1. Why do we use mdelay() here if CONFIG_BLK_DEV_IDECS is defined?

    why we don't, it is ifndef not ifdef

    > 2. Is the reason for this still valid?
    >
    > 3. If it is safe to sleep here even if CONFIG_CLK_DEV_IDECS is set,
    > why bother with mdelay() in the first place?

    even ifndef

    > The _correct_ patch is actually:
    >
    > void ide_delay_50ms (void)
    > {
    > -#ifndef CONFIG_BLK_DEV_IDECS
    > - mdelay(50);
    > -#else
    > __set_current_state(TASK_UNINTERRUPTIBLE);
    > schedule_timeout(HZ/20);
    > -#endif /* CONFIG_BLK_DEV_IDECS */
    > }
    >
    > since PCMCIA always calls drivers from process context now.

    Probably somebody got the logic wrong while adding this ifndef.

    I've checked (quickly) all users of ide_delay_50ms() + their callers
    and it seems that this change is safe.

    Cheers.

    >
    > (Unfortunately I can't write upside down, but I'll give the answers to
    > those three items above. Look away now if you don't want to read the
    > answers! 8) )
    >
    >
    > 1. PCMCIA used to call drivers in IRQ context, which made it impossible
    > to sleep.
    >
    > 2. No, because PCMCIA always calls drivers in process context now, so
    > sleeping is possible.
    >
    > 3. Left as an exercise to the reader. 8)

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  • Next message: Matt Mackall: "Re: [patch] kill off PC9800"

    Relevant Pages
    Loading