lkml.org 
[lkml]   [2008]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] consolidate all within() implementations
    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?


    \
     
     \ /
      Last update: 2008-05-20 11:49    [W:0.042 / U:0.220 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site