Re: [PATCH] ubi: Update mean Erase Counter calculation at run-time

[Richard Weinberger <richard.weinberger@xxxxxxxxx>]

On Thu, Jan 2, 2020 at 1:29 PM Paul HENRYS <paul.henrysd@xxxxxxxxx> wrote:
>
> The mean EC value was only calculated when attaching a UBI device
> but not updated afterwards. A new parameter is added to the struct
> ubi_device to keep track of the sum of the erase counters of all
> eraseblocks for a UBI device while they are erased and potentially
> become bad. The mean EC is then calculated on request when reading
> the "mean_ec" sysfs file of a UBI device.
>
> Signed-off-by: Paul HENRYS <paul.henrysd@xxxxxxxxx>
> ---
>  drivers/mtd/ubi/build.c | 10 ++++++++++
>  drivers/mtd/ubi/ubi.h   |  3 ++-
>  drivers/mtd/ubi/wl.c    |  5 +++++

Please update Documentation/ABI/stable/sysfs-class-ubi too.

>  3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index d636bbe..3291fa4 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -126,6 +126,8 @@ static struct device_attribute dev_volumes_count =
>         __ATTR(volumes_count, S_IRUGO, dev_attribute_show, NULL);
>  static struct device_attribute dev_max_ec =
>         __ATTR(max_ec, S_IRUGO, dev_attribute_show, NULL);
> +static struct device_attribute dev_mean_ec =
> +       __ATTR(mean_ec, S_IRUGO, dev_attribute_show, NULL);
>  static struct device_attribute dev_reserved_for_bad =
>         __ATTR(reserved_for_bad, S_IRUGO, dev_attribute_show, NULL);
>  static struct device_attribute dev_bad_peb_count =
> @@ -364,6 +366,13 @@ static ssize_t dev_attribute_show(struct device *dev,
>                 ret = sprintf(buf, "%d\n", ubi->vol_count - UBI_INT_VOL_COUNT);
>         else if (attr == &dev_max_ec)
>                 ret = sprintf(buf, "%d\n", ubi->max_ec);
> +       else if (attr == &dev_mean_ec) {
> +               /* Calculate mean erase counter */
> +               if (ubi->good_peb_count)
> +                       ubi->mean_ec = div_u64(ubi->ec_sum,
> +                                               ubi->good_peb_count);

Is there a reason why you don't the math in sync_erase()?

> +               ret = sprintf(buf, "%d\n", ubi->mean_ec);
> +       }
>         else if (attr == &dev_reserved_for_bad)
>                 ret = sprintf(buf, "%d\n", ubi->beb_rsvd_pebs);
>         else if (attr == &dev_bad_peb_count)
> @@ -391,6 +400,7 @@ static struct attribute *ubi_dev_attrs[] = {
>         &dev_total_eraseblocks.attr,
>         &dev_volumes_count.attr,
>         &dev_max_ec.attr,
> +       &dev_mean_ec.attr,
>         &dev_reserved_for_bad.attr,
>         &dev_bad_peb_count.attr,
>         &dev_max_vol_count.attr,
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index 9688b41..d796217 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -472,6 +472,7 @@ struct ubi_debug_info {
>   *
>   * @max_ec: current highest erase counter value
>   * @mean_ec: current mean erase counter value
> + * @ec_sum: a temporary variable used when calculating @mean_ec
>   *
>   * @global_sqnum: global sequence number
>   * @ltree_lock: protects the lock tree and @global_sqnum
> @@ -580,8 +581,8 @@ struct ubi_device {
>         struct mutex device_mutex;
>
>         int max_ec;
> -       /* Note, mean_ec is not updated run-time - should be fixed */

Thanks for addressing this. :-)

>         int mean_ec;
> +       uint64_t ec_sum;
>
>         /* EBA sub-system's stuff */
>         unsigned long long global_sqnum;
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 5d77a38..24c47ce 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -442,6 +442,7 @@ static int sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
>         int err;
>         struct ubi_ec_hdr *ec_hdr;
>         unsigned long long ec = e->ec;
> +       unsigned long long erasecycle;

Why do you need a new variable?
We already have everything we need.

-- 
Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/