Re: [PATCH] lockdep: annotate i386 apm

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Wed, 2006-10-11 at 23:39 -0700, Andrew Morton wrote:
> On Thu, 12 Oct 2006 08:06:20 +0200
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > #define local_irq_restore(flags)                                \
> >         do {                                                    \
> >                 if (raw_irqs_disabled_flags(flags)) {           \
> >                         raw_local_irq_restore(flags);           \
> >                         trace_hardirqs_off();                   \
> >                 } else {                                        \
> >                         trace_hardirqs_on();                    \
> >                         raw_local_irq_restore(flags);           \
> >                 }                                               \
> >         } while (0)
> > 
> > So, say interrupts were enabled when entering apm_bios_call*(); you now
> > save that in flags, disable interrupts, and enable them again.
> > Upon reaching local_irq_restore(), we'll hit the else branch with irq's
> > enabled and call trace_hardirqs_on(), which goes EEEK!
> 
> I'd assumed lockdep was less stupid than that ;) This?  Seems a bit
> overdone..

Seems simple enough, bit on the verbose side, _however_ it depends on
the BIOS call not changing the irq state. I don't know about BIOSs but I
thought the general consensus was to distrust them.

Was my original _that_ hard to read?

> --- a/arch/i386/kernel/apm.c~lockdep-annotate-i386-apm
> +++ a/arch/i386/kernel/apm.c
> @@ -540,12 +540,6 @@ static inline void apm_restore_cpus(cpum
>   * Also, we KNOW that for the non error case of apm_bios_call, there
>   * is no useful data returned in the low order 8 bits of eax.
>   */
> -#define APM_DO_CLI	\
> -	if (apm_info.allow_ints) \
> -		local_irq_enable(); \
> -	else \
> -		local_irq_disable();
> -
>  #ifdef APM_ZERO_SEGS
>  #	define APM_DECL_SEGS \
>  		unsigned int saved_fs; unsigned int saved_gs;
> @@ -583,11 +577,12 @@ static u8 apm_bios_call(u32 func, u32 eb
>  	u32 *eax, u32 *ebx, u32 *ecx, u32 *edx, u32 *esi)
>  {
>  	APM_DECL_SEGS
> -	unsigned long		flags;
>  	cpumask_t		cpus;
>  	int			cpu;
>  	struct desc_struct	save_desc_40;
>  	struct desc_struct	*gdt;
> +	int			enable_irqs = 0;
> +	int			disable_irqs = 0;
>  
>  	cpus = apm_save_cpus();
>  	
> @@ -596,12 +591,26 @@ static u8 apm_bios_call(u32 func, u32 eb
>  	save_desc_40 = gdt[0x40 / 8];
>  	gdt[0x40 / 8] = bad_bios_desc;
>  
> -	local_save_flags(flags);
> -	APM_DO_CLI;
> +	if (apm_info.allow_ints) {
> +		if (irqs_disabled()) {
> +			local_irq_enable();
> +			disable_irqs = 1;
> +		}
> +	} else {
> +		if (!irqs_disabled()) {
> +			local_irq_disable();
> +			enable_irqs = 1;
> +		}
> +	}
> +
> +
>  	APM_DO_SAVE_SEGS;
>  	apm_bios_call_asm(func, ebx_in, ecx_in, eax, ebx, ecx, edx, esi);
>  	APM_DO_RESTORE_SEGS;
> -	local_irq_restore(flags);
> +	if (disable_irqs)
> +		local_irq_disable();
> +	if (enable_irqs)
> +		local_irq_enable();
>  	gdt[0x40 / 8] = save_desc_40;
>  	put_cpu();
>  	apm_restore_cpus(cpus);
> @@ -627,11 +636,12 @@ static u8 apm_bios_call_simple(u32 func,
>  {
>  	u8			error;
>  	APM_DECL_SEGS
> -	unsigned long		flags;
>  	cpumask_t		cpus;
>  	int			cpu;
>  	struct desc_struct	save_desc_40;
>  	struct desc_struct	*gdt;
> +	int			enable_irqs = 0;
> +	int			disable_irqs = 0;
>  
>  	cpus = apm_save_cpus();
>  	
> @@ -640,12 +650,25 @@ static u8 apm_bios_call_simple(u32 func,
>  	save_desc_40 = gdt[0x40 / 8];
>  	gdt[0x40 / 8] = bad_bios_desc;
>  
> -	local_save_flags(flags);
> -	APM_DO_CLI;
> +	if (apm_info.allow_ints) {
> +		if (irqs_disabled()) {
> +			local_irq_enable();
> +			disable_irqs = 1;
> +		}
> +	} else {
> +		if (!irqs_disabled()) {
> +			local_irq_disable();
> +			enable_irqs = 1;
> +		}
> +	}
> +
>  	APM_DO_SAVE_SEGS;
>  	error = apm_bios_call_simple_asm(func, ebx_in, ecx_in, eax);
>  	APM_DO_RESTORE_SEGS;
> -	local_irq_restore(flags);
> +	if (disable_irqs)
> +		local_irq_disable();
> +	if (enable_irqs)
> +		local_irq_enable();
>  	gdt[0x40 / 8] = save_desc_40;
>  	put_cpu();
>  	apm_restore_cpus(cpus);
> _
>