Re: [PATCH] consolidate all within() implementations

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 20 May 2008 10:08:32 +0200 Peter Oberparleiter <oberparleiter@googlemail.com> wrote:

> Harvey Harrison wrote:
> > On Mon, 2008-05-19 at 10:45 +0200, Peter Oberparleiter wrote:
> >> From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
> >> 
> >> This patch consolidates a number of different implementations of the
> >> within() function which checks whether an address is within a specified
> >> address range.
> > 
> > Would it be that hard to just make them static inlines taking unsigned
> > longs?
> 
> I was hoping to get by without the spray of unsigned long casts that
> entails the enforcement of this specific parameter type, seeing that
> each implementation had it's own combination of longs and void *.
> 
> On the other hand, an inline function would of course be the cleaner
> approach, so if the code owners agree, here goes take #2:
> 
> --
> 
> From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
> 
> This patch consolidates a number of different implementations of the
> within() function which checks whether an address is within a specified
> address range. Apart from parameter typing, existing implementations can
> be classified in two categories which differ in the way the range is
> specified:
> 
>   1) by start and end address
>   2) by start and size
> 
> Case 1) is covered by addr_within() while 2) is covered by
> addr_within_len().
> 
> Signed-off-by: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
> ---
>  arch/x86/mm/pageattr.c |   20 ++++++++------------
>  include/linux/kernel.h |   24 ++++++++++++++++++++++++
>  kernel/lockdep.c       |   12 +++++-------
>  kernel/module.c        |   35 ++++++++++++++++++++---------------
>  4 files changed, 57 insertions(+), 34 deletions(-)
> 
> Index: linux-2.6.26-rc3/include/linux/kernel.h
> ===================================================================
> --- linux-2.6.26-rc3.orig/include/linux/kernel.h
> +++ linux-2.6.26-rc3/include/linux/kernel.h
> @@ -434,6 +434,30 @@ static inline char *pack_hex_byte(char *
>  	__val > __max ? __max: __val; })
>  
>  /**
> + * addr_within - check whether address is in start-and-end address range
> + * @addr: address
> + * @start: start address (included in range)
> + * @end: end address (excluded from range)
> + */
> +static inline int addr_within(unsigned long addr, unsigned long start,
> +			      unsigned long end)
> +{
> +	return (addr >= start) && (addr < end);
> +}
> +
> +/**
> + * addr_within_len - check whether address is in start-and-length address range
> + * @addr: address
> + * @start: start of range
> + * @len: number of bytes in range
> + */
> +static inline int addr_within_len(unsigned long addr, unsigned long start,
> +				  unsigned long len)
> +{
> +	return (addr >= start) && (addr < (start + len));
> +}

The kernel's use of unsigned long to represent pointers sometimes makes
sense, but often gets us into a mess.  It's OK in bootup code which
fiddles with memory map layout because there is no reason why such
code will ever dereference any of the addresses.

But I don't think we can assume this usage pattern when creating a
kernel-wide facility in kernel.h.

So yes, I do think that an address-comparison tool like this should
operate on void*'s.  (They will need to be const void*'s).

> +			if (addr_within_len((unsigned long) class->key,
> +					    (unsigned long) start, size))
> +			else if (addr_within_len((unsigned long) class->name,
> +						 (unsigned long) start, size))
> +	if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		    || addr_within_len(addr, (unsigned long) mod->module_core,
> +		if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		    addr_within_len(addr, (unsigned long) mod->module_core,
> +		if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		    addr_within_len(addr, (unsigned long) mod->module_core,
> +		if (addr_within_len(addr, (unsigned long) mod->module_core,
> +		if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		    || addr_within_len(addr, (unsigned long) mod->module_core,

And you've had to add a great pile of casts anwyay?