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

[Richard Weinberger]

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

AFAIK BLOCK_STATUS_ERASE_COUNT_BEYOND_MAX is also unreachable since UBI aborts before that.

> +static char const *block_status_names[] = {"OK", "marked_bad",
> +					   "erase_count_beyond_max"};
> +
> +enum read_status {
> +	READ_STATUS_OK,
> +	READ_STATUS_ERR_READING_BLOCK,
> +	READ_STATUS_ERR_READING_ERASE_COUNT
> +};
>

READ_STATUS_ERR_READING_ERASE_COUNT is now no longer needed, right?

> +static char const *read_status_names[] = {"OK", "err_reading_block",
> +					  "err_reading_erase_count"};
> +
> +static int eraseblk_count_seq_show(struct seq_file *s, void *iter)
> +{
> +	struct ubi_device *ubi = s->private;
> +	struct ubi_wl_entry *wl;
> +	int *block_number = iter;
> +	int erase_count = -1;
> +	enum block_status b_sts = BLOCK_STATUS_OK;
> +	enum read_status r_sts = READ_STATUS_OK;
> +	int err;
> +
> +	/* If this is the start, print a header */
> +	if (iter == SEQ_START_TOKEN) {
> +		seq_puts(s,
> +			 "physical_block_number\terase_count\tblock_status\tread_status\n");
> +		return 0;
> +	}
> +
> +	err = ubi_io_is_bad(ubi, *block_number);
> +	if (err) {
> +		if (err < 0)
> +			r_sts = READ_STATUS_ERR_READING_BLOCK;
> +		else
> +			b_sts = BLOCK_STATUS_BAD_BLOCK;
> +	} else {
> +		wl = ubi->lookuptbl[*block_number];
> +		erase_count = wl->ec;

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.

Thanks,
//richard