lkml.org 
[lkml]   [2006]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] sparse fix: add many lock annotations
    On Wed, 22 Nov 2006 21:33:07 +0300
    Alexey Dobriyan <adobriyan@gmail.com> wrote:

    > On Wed, Nov 22, 2006 at 12:11:46AM -0800, Ira Snyder wrote:
    > > This patch adds many lock annotations to the kernel source to quiet
    > > warnings from sparse. In almost every case, it quiets the warning caused
    > > by locks that are intentionally grabbed in one function and released in
    > > another.
    > >
    > > In the other cases, __acquire() and __release() are used to make sparse
    > > believe that a lock was grabbed (even though it was not), in order to
    > > make all exit points have equal lock counts. These follow the style in
    > > kernel/sched.c.
    >
    > > --- a/arch/i386/kernel/smp.c
    > > +++ b/arch/i386/kernel/smp.c
    > > @@ -507,11 +507,13 @@ struct call_data_struct {
    > > };
    > >
    > > void lock_ipi_call_lock(void)
    > > +__acquires(call_lock)
    > > {
    > > spin_lock_irq(&call_lock);
    > > }
    > >
    > > void unlock_ipi_call_lock(void)
    > > +__releases(call_lock)
    > > {
    > > spin_unlock_irq(&call_lock);
    > > }
    >
    > Wrong place. Prototypes should be marked instead. How else would you
    > know about:
    >
    > lock_ipi_call_lock();
    > if (foo)
    > return -E;
    > lock_ipi_call_lock();
    >
    > on another compilation unit?
    >

    Are you saying I should use something like this instead?:
    diff --git a/include/asm-i386/smp.h b/include/asm-i386/smp.h
    index bd59c15..9489602 100644
    --- a/include/asm-i386/smp.h
    +++ b/include/asm-i386/smp.h
    @@ -38,8 +38,8 @@ extern cpumask_t cpu_core_map[];

    extern void (*mtrr_hook) (void);
    extern void zap_low_mappings (void);
    -extern void lock_ipi_call_lock(void);
    -extern void unlock_ipi_call_lock(void);
    +extern void lock_ipi_call_lock(void) __acquires(call_lock);
    +extern void unlock_ipi_call_lock(void) __releases(call_lock);

    #define MAX_APICID 256
    extern u8 x86_cpu_to_apicid[];
    If so, it doesn't remove the warning from sparse, it still shows:
    CHECK
    arch/i386/kernel/smp.c arch/i386/kernel/smp.c:509:6: warning: context
    imbalance in 'lock_ipi_call_lock' - wrong count at exit
    arch/i386/kernel/smp.c:514:6: warning: context imbalance in
    'unlock_ipi_call_lock' - unexpected unlock

    Those go away with the original patch.

    I was following some examples currently in the source, such as in
    arch/i386/kernel/efi.c on function efi_call_phys_prelog(). Or
    expand_fdtable() in fs/file.c is another good example.

    > > --- a/drivers/acpi/osl.c
    > > +++ b/drivers/acpi/osl.c
    > > @@ -1004,6 +1004,7 @@ EXPORT_SYMBOL(max_cstate);
    > > */
    > >
    > > acpi_cpu_flags acpi_os_acquire_lock(acpi_spinlock lockp)
    > > +__acquires(lockp)
    > > {
    > > acpi_cpu_flags flags;
    > > spin_lock_irqsave(lockp, flags);
    > > @@ -1015,6 +1016,7 @@ acpi_cpu_flags acpi_os_acquire_lock(acpi
    > > */
    > >
    > > void acpi_os_release_lock(acpi_spinlock lockp, acpi_cpu_flags flags)
    > > +__releases(lockp)
    > > {
    > > spin_unlock_irqrestore(lockp, flags);
    > > }
    >
    > Again, wrong. IMO, sparse should deduce itself that lock is grabbed in such
    > trivial cases.
    >

    I'm not sure that it can. See: http://lwn.net/Articles/109066/ where
    Linus talks about addings __acquires(lockname) and __releases(lockname)
    to functions whose purpose is to grab and hold a lock at exit.

    Anyway, this is my first patch on the LKML, so I'd like to try and get
    it right.

    Thanks,
    Ira
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-11-22 20:37    [W:5.157 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site