Message-ID: <20080106131343.GA3120@lazybastard.org>
Date:	Sun, 6 Jan 2008 14:13:43 +0100
From:	Jörn Engel <joern@...fs.org>
To:	Erez Zadok <ezk@...sunysb.edu>
Cc:	dwmw2@...radead.org, linux-mtd@...ts.infradead.org,
	peterz@...radead.org, mingo@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] block2mtd lockdep_init_map warning

Maybe it is not obvious that I maintain this driver and would like to be
kept on Cc:.  Will send a patch to fix that shortly.

On Sun, 6 January 2008 02:17:32 -0500, Erez Zadok wrote:
> 
> Hi David,
> 
> I've reported before a lockdep warning when block2mtd is modloaded, and a
> device is initialized, as in
> 
> 	modprobe block2mtd block2mtd=/dev/loop0
> 
> A typical warning looks like this:
> 
> BUG: key f88565c0 not in .data!
> WARNING: at kernel/lockdep.c:2331 lockdep_init_map()
> Pid: 1823, comm: modprobe Not tainted 2.6.24-rc6-unionfs2 #135
>  [<c02038c0>] show_trace_log_lvl+0x1a/0x2f
>  [<c02042bb>] show_trace+0x12/0x14
>  [<c0204a01>] dump_stack+0x6c/0x72
>  [<c022edf0>] lockdep_init_map+0x94/0x374
>  [<c022e79d>] debug_mutex_init+0x2c/0x3c
>  [<c0229bb0>] __mutex_init+0x38/0x40
>  [<f885520d>] 0xf885520d
>  [<c0226816>] parse_args+0x121/0x1fb
>  [<c0237aaf>] sys_init_module+0x10e8/0x1576
>  [<c0202836>] sysenter_past_esp+0x5f/0xa5
>  =======================
> 
> This is a long-standing problem I've seen in several of the latest stable
> kernels.  Once lockdep turns itself off, there's no easy way to turn it back
> on short of a reboot.
> 
> I looked more closely at the mtd code.  I believe the problem is that
> lockdep doesn't like a mutex_init to be called from a module_init code path,
> possibly because the module's symbols aren't all initialized yet.  (This
> could arguably be considered a limitation of lockdep.)
> 
> So I tried to defer the call to module_init until it's absolutely needed.  I
> couldn't find a clean way to do that via the struct mtd_info ops (there's no
> suitable ->init op), so instead I used an int to mark whether the mutex is
> initialized or not.  Below is a patch.  It works, but it's not as clean as
> it should be: a better way would be to probably add an mtd_info ->init op or
> so.
> 
> At least with this patch, lockdep doesn't complain any longer, so I can run
> a clean set of regression tests w/ unionfs on top of jffs2 and other file
> systems.
> 
> In lieu of a better fix, is this patch acceptable?

Not for me.  I don't mind if you keep such a hack until a proper
solution if found in your private tree.  But it is a horrible solution
to a problem introduced elsewhere.

Ingo, Peter, does either of you actually care about this problem?  In
the last round when I debugged this problem there was a notable lack of
reaction from either of you.

> block2mtd: defer mutex initialization to avoid a lockdep warning
> 
> Signed-off-by: Erez Zadok <ezk@...sunysb.edu>
> 
> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> index be4b994..2c6d3e7 100644
> --- a/drivers/mtd/devices/block2mtd.c
> +++ b/drivers/mtd/devices/block2mtd.c
> @@ -32,6 +32,7 @@ struct block2mtd_dev {
>  	struct list_head list;
>  	struct block_device *blkdev;
>  	struct mtd_info mtd;
> +	int mutex_initialized;
>  	struct mutex write_mutex;
>  };
>  
> @@ -85,6 +86,11 @@ static int block2mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
>  	size_t len = instr->len;
>  	int err;
>  
> +	if (!dev->mutex_initialized) {
> +		mutex_init(&dev->write_mutex);
> +		dev->mutex_initialized = 1;
> +	}
> +
>  	instr->state = MTD_ERASING;
>  	mutex_lock(&dev->write_mutex);
>  	err = _block2mtd_erase(dev, from, len);
> @@ -194,6 +200,11 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	struct block2mtd_dev *dev = mtd->priv;
>  	int err;
>  
> +	if (!dev->mutex_initialized) {
> +		mutex_init(&dev->write_mutex);
> +		dev->mutex_initialized = 1;
> +	}
> +
>  	if (!len)
>  		return 0;
>  	if (to >= mtd->size)
> @@ -275,8 +286,6 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size)
>  		goto devinit_err;
>  	}
>  
> -	mutex_init(&dev->write_mutex);
> -
>  	/* Setup the MTD structure */
>  	/* make the name contain the block device in */
>  	dev->mtd.name = kmalloc(sizeof("block2mtd: ") + strlen(devname),
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Jörn

-- 
Joern's library part 13:
http://www.chip-architect.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists