Re: [PATCH 1/2 v2] Driver for Freescale 8610 and 5121 DIU

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

On Thu, 2008-03-20 at 15:27 -0700, Andrew Morton wrote:

> >
> > +static struct diu_hw dr = {
> > +	.mode = MFB_MODE1,
> > +	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),
> > +};
> 
> I'm not clear on what's supposed to happen with __SPIN_LOCK_UNLOCKED().  I
> do know that its documentation is crap.
> 
> I guess you should have used SPIN_LOCK_UNLOCKED there rather than
> open-coding its equivalent.  And SPIN_LOCK_UNLOCKED _is_ documented.  It
> says "don't use this".
> 
> Now, s/documentation/guesswork-and-grep/ says that you're supposed to pick
> a tree-wide-unique string here as your lockdep key.
> 
> So I did this:
> 
> --- a/drivers/video/fsl-diu-fb.c~fbdev-driver-for-freescale-8610-and-5121-diu-fix
> +++ a/drivers/video/fsl-diu-fb.c
> @@ -274,7 +274,7 @@ static struct mfb_info mfb_template[] = 
>  
>  static struct diu_hw dr = {
>  	.mode = MFB_MODE1,
> -	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),
> +	.reg_lock = __SPIN_LOCK_UNLOCKED(diu_hw.reg_lock),
>  };

#define __SPIN_LOCK_UNLOCKED(lockname)

seems pretty suggestive to me, also DEFINE_SPINLOCK(x) shows the proper
usage; the right thing would have been:

static struct diu_hw dr = {
 	.mode = MFB_MODE1,
-	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),
+	.reg_lock = __SPIN_LOCK_UNLOCKED(dr.reg_lock),
};

Where 'dr.reg_lock' is the expression to locate the actual object.

Does something like this make sense to you:

---
Subject: lockdep: document __SPIN_LOCK_UNLOCKED() usage

Small comment clarifying the intended usage of __SPIN_LOCK_UNLOCKED()

[ for the observant readers; yes this prescribes a usage that is more
  than strictly needed, it does however enable interesting future uses ]

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
diff --git a/include/linux/spinlock_types.h b/include/linux/spinlock_types.h
index 68d88f7..4278558 100644
--- a/include/linux/spinlock_types.h
+++ b/include/linux/spinlock_types.h
@@ -63,6 +63,10 @@ typedef struct {
 # define RW_DEP_MAP_INIT(lockname)
 #endif
 
+/*
+ * __*_LOCK_UNLOCKED(lockname), where 'lockname' is a valid C expression
+ * that evaluates to the actual object being initialized.
+ */
 #ifdef CONFIG_DEBUG_SPINLOCK
 # define __SPIN_LOCK_UNLOCKED(lockname)					\
 	(spinlock_t)	{	.raw_lock = __RAW_SPIN_LOCK_UNLOCKED,	\
---

Perhaps I should make the macros do something like:

  typecheck(spinlock_t, lockname)

And sweep the tree to make it compile again.

>  static struct diu_pool pool;
> 
> > +static struct diu_pool pool;
> > +
> > +/*	To allocate memory for framebuffer. First try __get_free_pages(). If it
> > + *	fails, try rh_alloc. The reason is __get_free_pages() cannot allocate
> > + *	very large memory (more than 4MB). We don't want to allocate all memory
> > + *	in rheap since small memory allocation/deallocation will fragment the
> > + *	rheap and make the furture large allocation fail.
> > + */
> > +
> > +void *fsl_diu_alloc(unsigned long size, phys_addr_t *phys)
> > +{
> > +	void *virt;
> > +
> > +	pr_debug("size=%lu\n", size);
> > +
> > +	virt = (void *)__get_free_pages(GFP_DMA | GFP_ATOMIC, get_order(size));
> 
> GFP_DMA implies GFP_ATOMIC, but it's appropriate for documentation purposes.

FWIW, I prefer the form: GFP_type | __GFP_modifiers

For instance: 'GFP_KERNEL | __GFP_DMA | __GFP_ZERO'