[BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Foster]

Hello!

Massive apologies if this is not the correct list or
not the correct way of reporting MTD-utils bugs.
My searches failed to find anything ....  ;-\ 

MTD-utils 1.4.5, when `mtdinfo -a' tries to print the
eraseblock regions of NOR-Flash, it open(2)s the nil
pointer (NULL).  The problem seems to be in mtdinfo.c's
print_region_info(), which assumes there is always a MTD
/dev node name (args.node).  However, in the case of `-a',
args.node is always NULL.  Oops!

------------------------------------
# dmesg           #output abbreviated ....
 ...
[   15.890000] physmap platform flash device: 02000000 at 40000000
[   15.910000] physmap-flash.0: Found 1 x16 devices at 0x0 in 16-bit bank. Manufacturer ID 0x000001 Chip ID 0x002201
[   15.930000] Amd/Fujitsu Extended Query Table at 0x0040
[   15.930000]   Amd/Fujitsu Extended Query version 1.3.
[   15.940000] number of CFI chips: 1
 ...

# mtdinfo /dev/mtd0
mtd0
Name:                           physmap-flash.0
Type:                           nor
Eraseblock size:                131072 bytes, 128.0 KiB
Amount of eraseblocks:          256 (33554432 bytes, 32.0 MiB)
Minimum input/output unit size: 1 byte
Sub-page size:                  1 byte
Additional erase regions:       0
Character device major/minor:   90:0
Bad blocks are allowed:         false
Device is writable:             true
Eraseblock region 0:  offset: 0xc10e2e21 size: 0xc10e2fe8 numblocks: 0x1

# mtdinfo -a      #output abbreviated ....
 ...
mtd0
 ...
Bad blocks are allowed:         false
Device is writable:             true
mtdinfo: error!: couldn't open MTD dev: (null)
         error 14 (Bad address)
 ...
------------------------------------

I think the problem was introduced by commit 266061ebd5
(CC-ed to the author of that commit, Mike Frysinger).
It appears to still exist in the the current wavefront.
I do not have a proposed fix, Sorry!

cheers,
	-blf-
-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Norris]

On Mon, Jul 25, 2011 at 2:48 AM, Brian Foster <brian.foster@maxim-ic.com> wrote:
> Massive apologies if this is not the correct list or
> not the correct way of reporting MTD-utils bugs.
> My searches failed to find anything ....  ;-\

This is exactly the right place! Sorry it was difficult to find.

> MTD-utils 1.4.5, when `mtdinfo -a' tries to print the
> eraseblock regions of NOR-Flash, it open(2)s the nil
> pointer (NULL).  The problem seems to be in mtdinfo.c's
> print_region_info(), which assumes there is always a MTD
> /dev node name (args.node).  However, in the case of `-a',
> args.node is always NULL.  Oops!

I don't have a test case that can reproduce the bug, as I don't have
devices with "region info", but I can see the problem fairly clearly.
It seems that the basic issue we need to solve is how to find the
correct file devfs/udev path for a device using only its mtd_info
object (e.g., use its major/minor number?). Then we could fix this
line in print_region_info() for cases where args.node == NULL:
        fd = open(args.node, O_RDONLY | O_CLOEXEC);

Also, is "region_info" a potential candidate for exporting via sysfs?
That would make this support easier to include in libmtd.

Brian

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Foster]

On Monday 25 July 2011 19:10:46 Brian Norris wrote:
> On Mon, Jul 25, 2011 at 2:48 AM, Brian Foster <brian.foster@maxim-ic.com> wrote:
>[...]
> This is exactly the right place! Sorry it was difficult to find.

 The list was easy to find; what I could not find was
 any indication of how/where bugs should be reported.

> > MTD-utils 1.4.5, when `mtdinfo -a' tries to print the
> > eraseblock regions of NOR-Flash, it open(2)s the nil
> > pointer (NULL).  [...]
> 
> I don't have a test case that can reproduce the bug, as I don't have
> devices with "region info", but I can see the problem fairly clearly.
> It seems that the basic issue we need to solve is how to find the
> correct file devfs/udev path  [...]

 As an FYI, in our current embedded systems (plural),
 a static /dev is used (no udev, mdev, &tc).  Hence,
 udev (and I assume also, devfs) path may not work
 in such an environment?

> Also, is "region_info" a potential candidate for exporting via sysfs?
> That would make this support easier to include in libmtd.

 I don't know if it is exportable or not, but I concur
 this seems like a possible/plausible solution.

cheers!
	-blf-
-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Norris]

On Tue, Jul 26, 2011 at 12:21 AM, Brian Foster
<brian.foster@maxim-ic.com> wrote:
>  The list was easy to find; what I could not find was
>  any indication of how/where bugs should be reported.

Hmm, it seems like maybe the closest info is hidden away at:
http://www.linux-mtd.infradead.org/doc/ubifs.html#L_how_send_bugreport
But much of this refers to UBIFS-specific stuff. Perhaps the site
should be updated.

>> It seems that the basic issue we need to solve is how to find the
>> correct file devfs/udev path  [...]
>
>  As an FYI, in our current embedded systems (plural),
>  a static /dev is used (no udev, mdev, &tc).  Hence,
>  udev (and I assume also, devfs) path may not work
>  in such an environment?

Now that I think about it, this doesn't seem like a very reliable
solution. Too much user-space variability.

>> Also, is "region_info" a potential candidate for exporting via sysfs?
>> That would make this support easier to include in libmtd.
>
>  I don't know if it is exportable or not, but I concur
>  this seems like a possible/plausible solution.

Even if we do this (which seems like a lot of info), it would only
apply to the newest kernels, where mtd-utils are *supposed* to be
backwards-compatible if possible, where they can fall back to old
methods if needed...

Anyway, since I don't have these types of flash readily available, I
don't even use these options :) And apparently, one of the biggest
contributors for mtd-utils, Mike Frysinger, doesn't use region_info
either...

Since there's been no response from anyone else for a while, I'm
sending a workaround patch for now that will kill the error and
disable region_info in `mtdinfo -a'.

Brian

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Mike Frysinger]

On Thu, Aug 4, 2011 at 10:46, Brian Norris wrote:
> On Tue, Jul 26, 2011 at 12:21 AM, Brian Foster wrote:
>>> It seems that the basic issue we need to solve is how to find the
>>> correct file devfs/udev path  [...]
>>
>>  As an FYI, in our current embedded systems (plural),
>>  a static /dev is used (no udev, mdev, &tc).  Hence,
>>  udev (and I assume also, devfs) path may not work
>>  in such an environment?
>
> Now that I think about it, this doesn't seem like a very reliable
> solution. Too much user-space variability.

when extending mtdinfo, we decided to only support device paths which
the user gave us.  hence the -m option going away.  libmtd itself has
/dev/mtd# hardcoded (and imo, that's the only path we should support
"automatically"), so if we're going to keep the all option, we'll want
to continue with that (by using the func that libmtd provides rather
than mtdinfo constructing the string itself).

>>> Also, is "region_info" a potential candidate for exporting via sysfs?
>>> That would make this support easier to include in libmtd.
>>
>>  I don't know if it is exportable or not, but I concur
>>  this seems like a possible/plausible solution.
>
> Even if we do this (which seems like a lot of info), it would only
> apply to the newest kernels, where mtd-utils are *supposed* to be
> backwards-compatible if possible, where they can fall back to old
> methods if needed...

i posted this question recently, and iirc the answer was that it isnt
currently exported via sysfs, but shouldnt be hard to extend.  just
have to find someone who wants to do it ;).

> Anyway, since I don't have these types of flash readily available, I
> don't even use these options :) And apparently, one of the biggest
> contributors for mtd-utils, Mike Frysinger, doesn't use region_info
> either...

i dont think any of the flashes (parallel nor, spi nor, nand, etc...)
included region_info.  that's why the func print_region_info() sets up
a dummy region_info struct that describes the entire flash before
calling print_region_map().
-mike

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Norris]

On Thu, Aug 4, 2011 at 3:41 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thu, Aug 4, 2011 at 10:46, Brian Norris wrote:
>> On Tue, Jul 26, 2011 at 12:21 AM, Brian Foster wrote:
>>>> It seems that the basic issue we need to solve is how to find the
>>>> correct file devfs/udev path  [...]
>>>
>>>  As an FYI, in our current embedded systems (plural),
>>>  a static /dev is used (no udev, mdev, &tc).  Hence,
>>>  udev (and I assume also, devfs) path may not work
>>>  in such an environment?
>>
>> Now that I think about it, this doesn't seem like a very reliable
>> solution. Too much user-space variability.
>
> when extending mtdinfo, we decided to only support device paths which
> the user gave us.  hence the -m option going away.  libmtd itself has
> /dev/mtd# hardcoded (and imo, that's the only path we should support
> "automatically"), so if we're going to keep the all option, we'll want
> to continue with that (by using the func that libmtd provides rather
> than mtdinfo constructing the string itself).

Hmm, where exactly in mtd-utils/libmtd is the /dev/mtd# hardcoded?
Sorry, but I'm not finding it...

>>>> Also, is "region_info" a potential candidate for exporting via sysfs?
>>>> That would make this support easier to include in libmtd.
>>>
>>>  I don't know if it is exportable or not, but I concur
>>>  this seems like a possible/plausible solution.
>>
>> Even if we do this (which seems like a lot of info), it would only
>> apply to the newest kernels, where mtd-utils are *supposed* to be
>> backwards-compatible if possible, where they can fall back to old
>> methods if needed...
>
> i posted this question recently, and iirc the answer was that it isnt
> currently exported via sysfs, but shouldnt be hard to extend.  just
> have to find someone who wants to do it ;).

"someone who wants to do it" - that's the key for everything, isn't it?

>> Anyway, since I don't have these types of flash readily available, I
>> don't even use these options :) And apparently, one of the biggest
>> contributors for mtd-utils, Mike Frysinger, doesn't use region_info
>> either...
>
> i dont think any of the flashes (parallel nor, spi nor, nand, etc...)
> included region_info.  that's why the func print_region_info() sets up
> a dummy region_info struct that describes the entire flash before
> calling print_region_map().

I don't know much about this, but what's the feature for if no one uses it?

Brian

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Mike Frysinger]

On Fri, Aug 5, 2011 at 17:09, Brian Norris wrote:
> On Thu, Aug 4, 2011 at 3:41 PM, Mike Frysinger wrote:
>> On Thu, Aug 4, 2011 at 10:46, Brian Norris wrote:
>>> On Tue, Jul 26, 2011 at 12:21 AM, Brian Foster wrote:
>>>>> It seems that the basic issue we need to solve is how to find the
>>>>> correct file devfs/udev path  [...]
>>>>
>>>>  As an FYI, in our current embedded systems (plural),
>>>>  a static /dev is used (no udev, mdev, &tc).  Hence,
>>>>  udev (and I assume also, devfs) path may not work
>>>>  in such an environment?
>>>
>>> Now that I think about it, this doesn't seem like a very reliable
>>> solution. Too much user-space variability.
>>
>> when extending mtdinfo, we decided to only support device paths which
>> the user gave us.  hence the -m option going away.  libmtd itself has
>> /dev/mtd# hardcoded (and imo, that's the only path we should support
>> "automatically"), so if we're going to keep the all option, we'll want
>> to continue with that (by using the func that libmtd provides rather
>> than mtdinfo constructing the string itself).
>
> Hmm, where exactly in mtd-utils/libmtd is the /dev/mtd# hardcoded?

i was thinking of legacy_get_dev_info1()

alternatively, we punt --all and just make users run `mtdinfo
/dev/mtd?`.  i'm fine with that too.

>>>>> Also, is "region_info" a potential candidate for exporting via sysfs?
>>>>> That would make this support easier to include in libmtd.
>>>>
>>>>  I don't know if it is exportable or not, but I concur
>>>>  this seems like a possible/plausible solution.
>>>
>>> Even if we do this (which seems like a lot of info), it would only
>>> apply to the newest kernels, where mtd-utils are *supposed* to be
>>> backwards-compatible if possible, where they can fall back to old
>>> methods if needed...
>>
>> i posted this question recently, and iirc the answer was that it isnt
>> currently exported via sysfs, but shouldnt be hard to extend.  just
>> have to find someone who wants to do it ;).
>
> "someone who wants to do it" - that's the key for everything, isn't it?

indeed.  i have no interest or need, let alone any way of testing it :).

>>> Anyway, since I don't have these types of flash readily available, I
>>> don't even use these options :) And apparently, one of the biggest
>>> contributors for mtd-utils, Mike Frysinger, doesn't use region_info
>>> either...
>>
>> i dont think any of the flashes (parallel nor, spi nor, nand, etc...)
>> included region_info.  that's why the func print_region_info() sets up
>> a dummy region_info struct that describes the entire flash before
>> calling print_region_map().
>
> I don't know much about this, but what's the feature for if no one uses it?

i use the print_region_map() which is why i wrote it.  if the kernel
doesnt define any regions, then i still want to see the whole device
as one region.
-mike

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Foster]

On Saturday 06 August 2011 03:18:53 Mike Frysinger wrote:
>[...]
> alternatively, we punt --all and just make users run
> `mtdinfo /dev/mtd?`.  i'm fine with that too.

 Alternatively, --all only prints what it can get,
 and --help warns there _may_ be additional data not
 printed by --all (and to use ‘mtdinfo /dev/mtd<N>’).

cheers!
	-blf-
-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Norris]

On Fri, Aug 5, 2011 at 6:18 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Fri, Aug 5, 2011 at 17:09, Brian Norris wrote:
>> On Thu, Aug 4, 2011 at 3:41 PM, Mike Frysinger wrote:
>>> when extending mtdinfo, we decided to only support device paths which
>>> the user gave us.  hence the -m option going away.  libmtd itself has
>>> /dev/mtd# hardcoded (and imo, that's the only path we should support
>>> "automatically"), so if we're going to keep the all option, we'll want
>>> to continue with that (by using the func that libmtd provides rather
>>> than mtdinfo constructing the string itself).
>>
>> Hmm, where exactly in mtd-utils/libmtd is the /dev/mtd# hardcoded?
>
> i was thinking of legacy_get_dev_info1()

I'm not sure we should be reaching back to legacy options for current
support. Besides, I see in the feature-removal messages that were
placed for the removal of the `-m' option to mtdinfo (commit
74bef2b8693c7e4f78e1ef742933b7231e49c7ad to mtd-utils):

"We cannot assume that mtd device names follow the "/dev/mtd%d" pattern"

It seems silly to reverse this just for the sake of some region_info stuff.

> alternatively, we punt --all and just make users run `mtdinfo
> /dev/mtd?`.  i'm fine with that too.

I also feel like `--all' is too valuable to be killed just because of
region_info.

Instead, I'm a little more inclined to go for Brian Foster's
suggestion (from another branch of this thread):

"Alternatively, --all only prints what it can get,
and --help warns there _may_ be additional data not
printed by --all (and to use ‘mtdinfo /dev/mtd<N>’)."

Only problem I see is that this provides a little bit of
inconsistency, where `mtdinfo --all' isn't quite all the info. But
with an addition help message as described by Mr. Foster, I don't
think this would be much problem.

I guess I'm leaning toward this suggestion partly because it is
effectively the same as my proposed patch and partly because the other
alternatives involve dropping support for features (either full udev
support or the `--all' option).

Brian

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Foster]

On Tuesday 09 August 2011 01:19:11 Brian Norris wrote:
> On Fri, Aug 5, 2011 at 6:18 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>[ ... ]
> > alternatively, we punt --all and just make users run `mtdinfo
> > /dev/mtd?`.  i'm fine with that too.
> 
> I also feel like `--all' is too valuable to be killed just because
> of region_info.
> 
> Instead, I'm a little more inclined to go for Brian Foster's
> suggestion (from another branch of this thread):
> 
>   "Alternatively, --all only prints what it can get,
>   and --help warns there _may_ be additional data not
>   printed by --all (and to use ‘mtdinfo /dev/mtd<N>’)."
> 
> Only problem I see is that this provides a little bit of
> inconsistency, where `mtdinfo --all' isn't quite all the info.
> But with an addition help message as described by Mr. Foster,
> I don't think this would be much problem.

 Thanks.  I've always presumed ‘--all’ means “all mtd devices”,
 not “all info from mtd”, so this seeming-inconsistency didn't
 even occur to me!  If you want “all info from mtd”, then just
 do something like:
────────────────────────────────────────────────────────────────
# for m in /dev/mtd[0-9]*; do mtdinfo $m; done
mtd0
Name:                           physmap-flash.0
Type:                           nor
Eraseblock size:                131072 bytes, 128.0 KiB
Amount of eraseblocks:          256 (33554432 bytes, 32.0 MiB)
Minimum input/output unit size: 1 byte
Sub-page size:                  1 byte
Additional erase regions:       0
Character device major/minor:   90:0
Bad blocks are allowed:         false
Device is writable:             true
Eraseblock region 0:  offset: 0x27f0004 size: 0xde numblocks: 0

mtdinfo: error!: "/dev/mtd1" does not correspond to any existing MTD device
mtdinfo: error!: "/dev/mtd2" does not correspond to any existing MTD device
mtdinfo: error!: "/dev/mtd3" does not correspond to any existing MTD device
mtdinfo: error!: "/dev/mtd4" does not correspond to any existing MTD device
mtdinfo: error!: "/dev/mtd5" does not correspond to any existing MTD device
mtdinfo: error!: "/dev/mtd6" does not correspond to any existing MTD device
mtdinfo: error!: "/dev/mtd7" does not correspond to any existing MTD device
# 
────────────────────────────────────────────────────────────────
 Simple.  I don't mind the errors shown above, so I haven't
 bothered to work out how to suppress them (without loosing
 other, presumably important, error messages).

cheers!
	-blf-
-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Mike Frysinger]

On Mon, Aug 8, 2011 at 19:19, Brian Norris wrote:
> "Alternatively, --all only prints what it can get,
> and --help warns there _may_ be additional data not
> printed by --all (and to use ‘mtdinfo /dev/mtd<N>’)."

that's really the only other option of the three.
1: add region_info to sysfs
2: add a warning --all that it could be incomplete
3: have --all use the legacy lookup func to get /dev/mtd#

i dont have a preference between any of these 3.  obviously removing
region_info support is not an option.
-mike

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Norris]

On Tue, Aug 9, 2011 at 10:34 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> 1: add region_info to sysfs
> 2: add a warning --all that it could be incomplete
> 3: have --all use the legacy lookup func to get /dev/mtd#
>
> i dont have a preference between any of these 3.  obviously removing
> region_info support is not an option.

OK, then I'll vote for option 2 and send a modification to my last
patch (to include a help message). Then if someone ever makes the time
to do option 1, it can fit nicely into option 2, since newer kernels
would be able to give complete info and older ones would not.

Brian

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Artem Bityutskiy]

On Tue, 2011-08-09 at 13:34 -0400, Mike Frysinger wrote:
> On Mon, Aug 8, 2011 at 19:19, Brian Norris wrote:
> > "Alternatively, --all only prints what it can get,
> > and --help warns there _may_ be additional data not
> > printed by --all (and to use ‘mtdinfo /dev/mtd<N>’)."
> 
> that's really the only other option of the three.
> 1: add region_info to sysfs
> 2: add a warning --all that it could be incomplete
> 3: have --all use the legacy lookup func to get /dev/mtd#

How about: make a quick fix and release mtd-utils-1.4.6, then just kill
the -a option. I'd vote for eliminating all the code which has to
guess/assume/find the device node names - I think they always have to be
passed by the user instead. We can do the elimination the usual way -
first add the warning, then kill.

This is also a good way to go because mtd stuff does not have enough
people for excellent maintenance, and keeping it simpler should be
helpful.

Thoughts?

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Norris]

Hi Artem,

On Sun, Aug 14, 2011 at 9:11 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> How about: make a quick fix and release mtd-utils-1.4.6, then just kill
> the -a option. I'd vote for eliminating all the code which has to
> guess/assume/find the device node names - I think they always have to be
> passed by the user instead. We can do the elimination the usual way -
> first add the warning, then kill.

Well, I have a little reservation on killing `-a', since it's useful.
In light of this, you just pushed some of my bug reporting
documentation to the website that includes references to `mtdinfo -a',
since it contains a lot more info than `cat /proc/mtd'. But we can
just modify the website again, if necessary.

> This is also a good way to go because mtd stuff does not have enough
> people for excellent maintenance, and keeping it simpler should be
> helpful.

I agree that simpler is better (usually). So if we decide that will be
simplest, then that's fine.

Is there a problem with my choice of option 2 from Mike's list?

> > 2: add a warning --all that it could be incomplete

See my patch that implements it:
http://lists.infradead.org/pipermail/linux-mtd/2011-August/037393.html

This way, we do not rely on guessing any paths; we simply use the
sysfs information that is exposed, among other purposes, for cases
cases like this, right? And then (as I stated on another message in
this thread) we could even support the "region info" on newer kernels,
if somebody ever does the work to export them. I think this solution
involves no extra work right now, does not remove valuable features,
does not make dirty assumptions about node names, and can take
advantage of new sysfs features in the future. The main downside I see
is the potential (documented) difference in information printed by
`mtdinfo --all' vs. `mtdinfo /dev/mtdX'.

Brian

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Artem Bityutskiy]

On Mon, 2011-08-15 at 16:13 -0700, Brian Norris wrote:
> This way, we do not rely on guessing any paths; we simply use the
> sysfs information that is exposed, among other purposes, for cases
> cases like this, right? And then (as I stated on another message in
> this thread) we could even support the "region info" on newer kernels,
> if somebody ever does the work to export them. I think this solution
> involves no extra work right now, does not remove valuable features,
> does not make dirty assumptions about node names, and can take
> advantage of new sysfs features in the future. The main downside I see
> is the potential (documented) difference in information printed by
> `mtdinfo --all' vs. `mtdinfo /dev/mtdX'.

Yes, fair enough, just pushed your fix and cleanups, thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Foster]

On Monday 15 August 2011 06:11:27 Artem Bityutskiy wrote:
> On Tue, 2011-08-09 at 13:34 -0400, Mike Frysinger wrote:
> > On Mon, Aug 8, 2011 at 19:19, Brian Norris wrote:
> > > "Alternatively, --all only prints what it can get,
> > > and --help warns there _may_ be additional data not
> > > printed by --all (and to use ‘mtdinfo /dev/mtd<N>’)."
> > 
> > that's really the only other option of the three.
> > 1: add region_info to sysfs
> > 2: add a warning --all that it could be incomplete
> > 3: have --all use the legacy lookup func to get /dev/mtd#
> 
> How about: make a quick fix and release mtd-utils-1.4.6, then just kill
> the -a option.  [...]

 You should decide what ‘-a’ means:  “all information”
 or “all devices”.  If “all information”, then either a
 bunch of work needs to be done, or else ‘-a’ removed.

 But if “all devices”  — which happens to be both what
 I think it means and how I use it —  then it is very
 useful, at least in my situation where I'm bringing up
 a new SoC and board.  It's a nice summary of what I'm
 usually interested in; if it didn't exist, I'd “have”
 to script it.  And in either case, if and when I do
 need all the information about _a_ device, then I do
 ‘mtdinfo /dev/mtd<N>’.

cheers!
	-blf-
-- 
Brian FOSTER
Principal MTS, Software

Maxim Integrated Products (Microcontroller BU), formerly Innova Card
ZI Athélia IV - Le Forum, Bât. A
Quartier Roumagoua
13600 La Ciotat - France
Phone  : +33 (0)4 42 98 15 35
Fax    : +33 (0)4 42 08 33 19
Email  : brian.foster@maxim-ic.com
Web    : http://www.maxim-ic.com/

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Norris]

On Tue, Aug 16, 2011 at 12:31 AM, Brian Foster
<brian.foster@maxim-ic.com> wrote:
>  You should decide what ‘-a’ means:  “all information”
>  or “all devices”.  If “all information”, then either a
>  bunch of work needs to be done, or else ‘-a’ removed.
>
>  But if “all devices”  — which happens to be both what
>  I think it means and how I use it —  then it is very
>  useful, at least in my situation where I'm bringing up
>  a new SoC and board.  It's a nice summary of what I'm
>  usually interested in; if it didn't exist, I'd “have”
>  to script it.  And in either case, if and when I do
>  need all the information about _a_ device, then I do
>  ‘mtdinfo /dev/mtd<N>’.

I think we're going with "all devices" then, with less emphasis on
100% completeness of information. For one, it reflects the current
code, and the help message has stated this its purpose for a while:

"-a, --all        print information about all MTD devices"

Thanks for the contributions.

Brian

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Foster]

On Saturday 06 August 2011 02:09:04 Brian Norris wrote:
> On Thu, Aug 4, 2011 at 3:41 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > On Thu, Aug 4, 2011 at 10:46, Brian Norris wrote:
>[...]
> >> Anyway, since I don't have these types of flash readily
> >> available, I don't even use these options :) And apparently,
> >> one of the biggest contributors for mtd-utils, Mike Frysinger,
> >> doesn't use region_info either...
> > 
> > i dont think any of the flashes (parallel nor, spi nor,
> > nand, etc...) included region_info.  that's why the func
> > print_region_info() sets up a dummy region_info struct that
> > describes the entire flash before calling print_region_map().
> 
> I don't know much about this, but what's the feature for
> if no one uses it?

 I'm also uncertain why Flash manufacturers have
 this “feature”, but some certainly do.  Below's
 the output of ‘mtd_debug info /dev/mtd0’ for
 two different NOR-Flash chips (on different
 reference systems).  In both cases, ‘/dev/mtd0’
 is the entire chip (an important point since
 it looks like partitions of a chip always(?)
 have zero regions?):
────────────────────────────────────────────────
# mtd_debug info /dev/mtd0    #(1) Flash from original bug report
mtd.type = MTD_NORFLASH
mtd.flags = MTD_CAP_NORFLASH
mtd.size = 33554432 (32M)
mtd.erasesize = 131072 (128K)
mtd.writesize = 1
mtd.oobsize = 0
regions = 1

region[0].offset = 0x00000000
region[0].erasesize = 131072 (128K)
region[0].numblocks = 256
region[0].regionindex = 0

# mtd_debug info /dev/mtd0    #(2) Different Flash on different system
mtd.type = MTD_NORFLASH
mtd.flags = MTD_CAP_NORFLASH
mtd.size = 4194304 (4M)
mtd.erasesize = 65536 (64K)
mtd.writesize = 1
mtd.oobsize = 0
regions = 3

region[0].offset = 0x00000000
region[0].erasesize = 8192 (8K)
region[0].numblocks = 8
region[0].regionindex = 0
region[1].offset = 0x00010000
region[1].erasesize = 65536 (64K)
region[1].numblocks = 62
region[1].regionindex = 1
region[2].offset = 0x003f0000
region[2].erasesize = 8192 (8K)
region[2].numblocks = 8
region[2].regionindex = 2
────────────────────────────────────────────────

 Our Linux systems are not explicitly taking
 advantage of multiple eraseblock regions.
 I do not know whether or not other software
 uses them.  (And yes, case ‘(2)’ really is
 only 4MiB — that chip is almost useless for
 the Linux systems.... ;-\   That system also
 has a 2nd NOR-Flash (32MiB), but I don't have
 at-hand the ‘mtd_debug info’ for it....)


cheers!
	-blf-
-- 
Brian FOSTER
Principal MTS, Software

Maxim Integrated Products (Microcontroller BU), formerly Innova Card
ZI Athélia IV - Le Forum, Bât. A
Quartier Roumagoua
13600 La Ciotat - France
Phone  : +33 (0)4 42 98 15 35
Fax    : +33 (0)4 42 08 33 19
Email  : brian.foster@maxim-ic.com
Web    : http://www.maxim-ic.com/

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Foster]

On Monday 08 August 2011 10:10:42 Brian Foster wrote:
>[...]         (And yes, case ‘(2)’ really is
>  only 4MiB — that chip is almost useless for
>  the Linux systems.... ;-\   That system also
>  has a 2nd NOR-Flash (32MiB), but I don't have
>  at-hand the ‘mtd_debug info’ for it....)

 Have it now ....
────────────────────────────────────────────────
# mtd_debug info /dev/mtd1    #(3) 2nd Flash on the 2nd system
mtd.type = MTD_NORFLASH
mtd.flags = MTD_WRITEABLE | MTD_BIT_WRITEABLE | MTD_POWERUP_LOCK
mtd.size = 33423360 (31M)
mtd.erasesize = 131072 (128K)
mtd.writesize = 1
mtd.oobsize = 0
regions = 2

region[0].offset = 0x00000000
region[0].erasesize = 32768 (32K)
region[0].numblocks = 4
region[0].regionindex = 0
region[1].offset = 0x00020000
region[1].erasesize = 131072 (128K)
region[1].numblocks = 255
region[1].regionindex = 1
────────────────────────────────────────────────

 Upshot is I've got three different cases:
 Case (1) has one region, case (2) has three regions,
 and case (3) has two regions.  All three, as predicted,
 cause the ‘mtdinfo -a’ open(2)-NULL bug.  And, AFAIK,
 all three NOR chips use CFI.  Some quick searching
 suggests CFI _requires_ at least one region.

 It also appears to be the case MTD reports a non-0
 number of regions only(?) for not-partitioned Flashes;
 that is, when ‘/dev/mtd<N>’ is the entire chip (not a
 partition of a chip).  This is based on observation,
 not a reading of the code nor the documentation, so
 could very easily be wrong!  But, if CFI does require
 at least one region and I'm not too badly wrong, this
 may explain why others have problems reproducing the
 issue:  The tests use partitions, not whole-chips?

cheers!
	-blf-
-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Foster]

On Thursday 04 August 2011 19:46:10 Brian Norris wrote:
> On Tue, Jul 26, 2011 at 12:21 AM, Brian Foster <brian.foster@maxim-ic.com> wrote:
> >  The list was easy to find; what I could not find was
> >  any indication of how/where bugs should be reported.
> 
> Hmm, it seems like maybe the closest info is hidden away at:
> http://www.linux-mtd.infradead.org/doc/ubifs.html#L_how_send_bugreport
> But much of this refers to UBIFS-specific stuff.  Perhaps the
> site should be updated.

 Indeed.  I don't recall finding that (but as you say,
 this is not a UBIFS issue per se).  May I suggest an
 entry in the FAQ?

>[ ... ] 
> Anyway, since I don't have these types of flash [with region_info]
> readily available, I don't even use these options :)  And apparently,
> one of the biggest contributors for mtd-utils, Mike Frysinger,
> doesn't use region_info either...

 I've no idea why all(? I think!) of our embedded systems
 reference designs _do_ use such Flashes.  The designer's
 on vacation now (this is August, and it is France, hence
 just about everybody's on vacation .... !) or I'd ask if
 there's any reason.

cheers!
	-blf-
-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

Re: [BUG] mtdinfo -a: Tries to open NULL pointer for NOR with Eraseblock Regions

[Brian Norris]

On Fri, Aug 5, 2011 at 12:24 AM, Brian Foster <brian.foster@maxim-ic.com> wrote:
>  May I suggest an
>  entry in the FAQ?

Wrote one here...I'll send the patch Monday.

Brian