From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964829AbbBBJBN (ORCPT ); Mon, 2 Feb 2015 04:01:13 -0500 Received: from mail-pa0-f49.google.com ([209.85.220.49]:56884 "EHLO mail-pa0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932818AbbBBJBJ (ORCPT ); Mon, 2 Feb 2015 04:01:09 -0500 Date: Mon, 2 Feb 2015 01:01:05 -0800 From: Brian Norris To: Niklas Cassel Cc: dwmw2@infradead.org, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mtd: avoid registering reboot notifier twice Message-ID: <20150202090105.GE3523@norris-Latitude-E6410> References: <1422752930-696-1-git-send-email-nks@flawful.org> <20150201230723.GA3523@norris-Latitude-E6410> <54CF2261.1040707@flawful.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54CF2261.1040707@flawful.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Feb 02, 2015 at 08:08:17AM +0100, Niklas Cassel wrote: > > On 02/02/2015 12:07 AM, Brian Norris wrote: > > On Sun, Feb 01, 2015 at 02:08:50AM +0100, Niklas Cassel wrote: > >> Calling mtd_device_parse_register with the same mtd_info > >> (e.g. registering several partitions on a single device) > >> would add the same reboot notifier twice, causing an > >> infinte loop in notifier_chain_register during boot up. > > No driver should be calling mtd_device_parse_register() multiple times > > on the same mtd_info. Under what context do you see this? What driver? > arch/cris/arch-v32/drivers/axisflashmap.c Yikes, that file is ugly! I am reminded of (one of the many reasons) why ARM ditched board files. > It calls mtd_device_register for different partitions on same device using the same mtd_info. > > What do you suggest this driver should do instead? Well, it would be really great if it could aggregate the partitions it wants all into one list (array) and pass that to the MTD core all at once. It also looks like this should have all been written as an MTD partition parser (struct mtd_part_parser), and some additional platform data to select the default maps if they aren't found on the flash. But there are a lot of odd cases in that code that make it difficult to do now. I'm tempted to suggest just calling add_mtd_partitions() directly, since you couldn't possibly be making good use out of the "parse" part of the mtd_device_parse_register() function. If you were, you'd get a ton of duplicate partitions, since the parser (e.g., cmdlinepart) would get called a bunch of times, and it would add the same partitions for *every* time you're calling mtd_device_register(). [1] But (as noted in mtdcore.h) add_mtd_partitions() is really private to the core MTD files. So... any chance you can rewrite this as an MTD parser + platform data? Also, now that you point this issue out, I see that one other driver might have the same problem as you -- diskonchip.c. It has a dubiously-formed partition registration setup. I'm tempted to remove its first mtd_device_register() and drop its 'no_autopart' parameter and see if anyone complains. If we have enough problems with this code, though, I might rip out some of: 3efe41be224c mtd: implement common reboot notifier boilerplate until I can find a better way. Brian [1] This suggests that your code is quite broken already, but just happens to work for the particular cases that are hard-coded here.