Re: [PATCH 2.4] the perils of kunmap_atomic

[Nigel Cunningham <ncunningham@linuxmail.org>]

Hi.

On Wed, 2004-10-27 at 09:31, Jeff Garzik wrote:
> kunmap_atomic() violates the Principle of Least Surprise in a nasty way. 
>     kmap(), kunmap(), and kmap_atomic() all take struct page* to 
> reference the memory location.  kunmap_atomic() is the oddball of the 
> three, and takes a kernel address.
> 
> Ignoring the driver-related bugs that are present due to 
> kunmap_atomic()'s weirdness, there also appears to be a big in the 
> !CONFIG_HIGHMEM implementation in 2.4.x.
> 
> (Bart is poking through some of the 2.6.x-related kunmap_atomic slip-ups)
> 
> Anyway, what do people think about the attached patch to 2.4.x?  I'm 
> surprised it has gone unnoticed until now.
> 
> 	Jeff

Ouch! It got me! That explains why suspend blows up with
CONFIG_DEBUG_HIGHMEM, but doesn't without it (2.6 - haven't tried
DEBUG_HIGHMEM under 2.4). It would be good if any patch produced a
warning if you call kunmap_atomic with the wrong kind of parameter.

Regards,

Nigel
> 
> ______________________________________________________________________
> ===== include/linux/highmem.h 1.12 vs edited =====
> --- 1.12/include/linux/highmem.h	2003-06-30 20:18:42 -04:00
> +++ edited/include/linux/highmem.h	2004-10-26 19:26:14 -04:00
> @@ -70,7 +70,7 @@
>  #define kunmap(page) do { } while (0)
>  
>  #define kmap_atomic(page,idx)		kmap(page)
> -#define kunmap_atomic(page,idx)		kunmap(page)
> +#define kunmap_atomic(addr,idx)		kunmap(virt_to_page(addr))
>  
>  #define bh_kmap(bh)			((bh)->b_data)
>  #define bh_kunmap(bh)			do { } while (0)
-- 
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Everyone lives by faith. Some people just don't believe it.
Want proof? Try to prove that the theory of evolution is true.