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?
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/mtd/mtdcore.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index de79576..98efc1e 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -556,7 +556,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
> err = -ENODEV;
> }
>
> - if (mtd->_reboot) {
> + if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) {
> mtd->reboot_notifier.notifier_call = mtd_reboot_notifier;
> register_reboot_notifier(&mtd->reboot_notifier);
> }
Brian
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
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?
>
>> Signed-off-by: Niklas Cassel <[email protected]>
>> ---
>> drivers/mtd/mtdcore.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index de79576..98efc1e 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -556,7 +556,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>> err = -ENODEV;
>> }
>>
>> - if (mtd->_reboot) {
>> + if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) {
>> mtd->reboot_notifier.notifier_call = mtd_reboot_notifier;
>> register_reboot_notifier(&mtd->reboot_notifier);
>> }
> Brian
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.
> On 02/02/2015 12:07 AM, Brian Norris wrote:
> >
> > 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
Looking at it another way, why should it not be allowed, if the only
thing stopping it from working is Niklas' fix below? If there are multiple
ways a driver can acquire partitioning information, it is handy to be able
to do mtd_device_parse_register() several times, rather than collecting
all the partitioning information in one place and doing the call there,
which I suppose would be the alternative.
I've noted that mtd/nand/diskonchip.c does it too:
mtd_device_register(mtd, NULL, 0);
if (!no_autopart)
mtd_device_register(mtd, parts, numparts);
but this driver might be essentially deprecated, I don't know.
> >> Signed-off-by: Niklas Cassel <[email protected]>
> >> ---
> >> drivers/mtd/mtdcore.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> >> index de79576..98efc1e 100644
> >> --- a/drivers/mtd/mtdcore.c
> >> +++ b/drivers/mtd/mtdcore.c
> >> @@ -556,7 +556,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
> >> err = -ENODEV;
> >> }
> >>
> >> - if (mtd->_reboot) {
> >> + if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) {
> >> mtd->reboot_notifier.notifier_call = mtd_reboot_notifier;
> >> register_reboot_notifier(&mtd->reboot_notifier);
> >> }
/Ricard
--
Ricard Wolf Wanderl?f ricardw(at)axis.com
Axis Communications AB, Lund, Sweden http://www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30
On Mon, Feb 02, 2015 at 10:01:51AM +0100, Ricard Wanderlof wrote:
>
> > On 02/02/2015 12:07 AM, Brian Norris wrote:
> > >
> > > 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
>
> Looking at it another way, why should it not be allowed, if the only
> thing stopping it from working is Niklas' fix below?
It severely breaks the way that normal MTD partition parsers work, for
one. cmdlineparts could never be sanely applied here, for instance.
> If there are multiple
> ways a driver can acquire partitioning information, it is handy to be able
> to do mtd_device_parse_register() several times, rather than collecting
> all the partitioning information in one place and doing the call there,
> which I suppose would be the alternative.
It may be handy, but it breaks the abstraction that is established in
most every other case, that partitions are determined by exactly one
source: a single list from platform data, device tree, the kernel
command line, or an on-flash parser (e.g., RedBoot, bcm47xxpart, etc.).
> I've noted that mtd/nand/diskonchip.c does it too:
>
> mtd_device_register(mtd, NULL, 0);
> if (!no_autopart)
> mtd_device_register(mtd, parts, numparts);
>
> but this driver might be essentially deprecated, I don't know.
Yeah, I just noticed that one too.
TBH, I'm not sure the reboot notifier patch places that code in the best
place anyway. There really is no central initialization code for the
'master' mtd_info struct, as this initialization is handled ad-hoc in
every single MTD driver. It just happens that the partition
parsing/registration function fit nicely as the one place that (almost)
all MTD master devices got registered. But this just exposed yet another
instance where my assumptions are challenged.
There's so much legacy crap in MTD that sometimes I wonder why I even
bother...
Brian