On Thu, 2008-03-20 at 15:27 -0700, Andrew Morton wrote:
quoted hunk ↗ jump to hunk
quoted
+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 <redacted>
---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;
quoted
+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'