Re: [patch] s390: do not use _local_bh_enable()

[Heiko Carstens]

On Fri, Feb 23, 2007 at 07:14:59AM +0100, Ingo Molnar wrote:
> 
> Heiko, what do you think about the patch below - is there perhaps some 
> deeper reason to s390's _local_bh_enable() use that i missed?

Yes, both of these usages are quite subtle.

> Index: linux/drivers/s390/char/sclp.c
> ===================================================================
> --- linux.orig/drivers/s390/char/sclp.c
> +++ linux/drivers/s390/char/sclp.c
> @@ -407,8 +407,8 @@ sclp_sync_wait(void)
>  	}
>  	local_irq_disable();
>  	__ctl_load(cr0, 0, 0);
> -	_local_bh_enable();
>  	local_irq_restore(flags);
> +	local_bh_enable();
>  }
> 
>  EXPORT_SYMBOL(sclp_sync_wait);

The code here looks a bit different in the meantime.
See c59d744bd8a0e283daf6726881e4c9aa4bd25261. What we want to do here: the
console driver has requests pending and needs to wait _synchronously_ that
an interrupt arrives. It does that regardless of whatever context we are in:
process/softirq/hardirq/irqs disabled/irqs enabled. That's why we have the
local_irq_save() at the very beginning. Also when we leave this function we
do not want to execute bottom halves, since it could be that interrupts
where disabled before this function was called.
That's why we call _local_bh_enable() instead of local_bh_enable().

> Index: linux/drivers/s390/cio/cio.c
> ===================================================================
> --- linux.orig/drivers/s390/cio/cio.c
> +++ linux/drivers/s390/cio/cio.c
> @@ -140,7 +140,6 @@ cio_tpi(void)
>  	sch = (struct subchannel *)(unsigned long)tpi_info->intparm;
>  	if (!sch)
>  		return 1;
> -	local_bh_disable();
>  	irq_enter ();
>  	spin_lock(sch->lock);
>  	memcpy (&sch->schib.scsw, &irb->scsw, sizeof (struct scsw));
> @@ -148,7 +147,7 @@ cio_tpi(void)
>  		sch->driver->irq(&sch->dev);
>  	spin_unlock(sch->lock);
>  	irq_exit ();
> -	_local_bh_enable();
> +
>  	return 1;
>  }

Same here: this is not really an irq handler but a function that gets called
from different contexts and pretends to be an irq handler. The
local_bh_disable()/_local_bh_enable() pair is just a trick to prevent bottom
halve execution. I think you can blame Martin for this ;)

Probably both of these functions needs some comment I guess... and this one
as well: bf6f6aa46feada857a52cb67d99a7c2fe4a70e87 (our new __udelay
implementation).

-- 
Heiko Carstens
Linux on System z Development

IBM Deutschland Entwicklung GmbH
Vorsitzender des Aufsichtsrats: Johann Weihen
Geschaeftsfuehrung : Herbert Kircher
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/