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

[Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl>]

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)