Date: Mon, 2 Feb 2015 01:01:05 -0800
From: Brian Norris <computersforpeace@gmail.com>
To: Niklas Cassel <nks@flawful.org>
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
Content-Length: 2728
Lines: 62

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.
--
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/