Re: [PATCH v2] UBI: add debugfs file for tracking PEB state

[Zach Brown]

On Wed, Sep 21, 2016 at 01:13:29PM +0200, Richard Weinberger wrote:
> Zach,
>
> On 20.09.2016 22:45, Zach Brown wrote:
> > From: Ben Shelton <ben.shelton@xxxxxx>
> >
> > Add a file under debugfs to allow easy access to the erase count for
> > each physical erase block on an UBI device.  This is useful when
> > debugging data integrity issues with UBIFS on NAND flash devices.
> >
> > Signed-off-by: Ben Shelton <ben.shelton@xxxxxx>
> > Signed-off-by: Zach Brown <zach.brown@xxxxxx>
> > ---
> > v2
> >  * Cast pointer in unsigned long instead of int to avoid build warning
> >  * Use ubi->lookuptbl[] to get erase counter instead of reading from flash
> >
> >
>
> [...]
>
> > +enum block_status {
> > +	BLOCK_STATUS_OK,
> > +	BLOCK_STATUS_BAD_BLOCK,
> > +	BLOCK_STATUS_ERASE_COUNT_BEYOND_MAX
> > +};
>
> Do you plan to add more states?
> In UBI a block can have much more states.
> I'd like to see all states, free, in protection, used, bad, corrupted, scrub, etc...
>

Adding more states sounds like a good idea, but I'm not sure how to get that
information. If I made the in_wl_tree function accessible I could use that to
get the information, but I'd have to check each RB Tree. Do you have a
suggestion?

> AFAIK BLOCK_STATUS_ERASE_COUNT_BEYOND_MAX is also unreachable since UBI aborts before that.
>
Do you mean that the block would've have already been marked as bad? So there's
no point checking?


> What about locking? :-)
> This is racy.
> You need at least wl_lock. Otherwise wl might disappear under you.
> And ->lookuptbl[] can return a NULL object too.
>
Do you know what ->lookuptbl[] returning NULL would signify about the state of
the block? Currently I'm thinking of treating as a bad read status and letting
the show function move on.

> Thanks,
> //richard