Hi Richard,
Sorry for the late reply.
On Sun, 29 Mar 2015 14:13:17 +0200
Richard Weinberger <[email protected]> wrote:
> This patch implements bitrot checking for UBI.
> ubi_wl_trigger_bitrot_check() triggers a re-read of every
> PEB. If a bitflip is detected PEBs in use will get scrubbed
> and free ones erased.
As you'll see, I didn't have much to say about the 'UBI bitrot
detection' mechanism, so this review is a collection of
nitpicks :-).
>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> drivers/mtd/ubi/build.c | 39 +++++++++++++
> drivers/mtd/ubi/ubi.h | 4 ++
> drivers/mtd/ubi/wl.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 189 insertions(+)
>
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 9690cf9..f58330b 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -118,6 +118,9 @@ static struct class_attribute ubi_version =
>
> static ssize_t dev_attribute_show(struct device *dev,
> struct device_attribute *attr, char *buf);
> +static ssize_t trigger_bitrot_check(struct device *dev,
> + struct device_attribute *mattr,
> + const char *data, size_t count);
>
> /* UBI device attributes (correspond to files in '/<sysfs>/class/ubi/ubiX') */
> static struct device_attribute dev_eraseblock_size =
> @@ -142,6 +145,8 @@ static struct device_attribute dev_bgt_enabled =
> __ATTR(bgt_enabled, S_IRUGO, dev_attribute_show, NULL);
> static struct device_attribute dev_mtd_num =
> __ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
> +static struct device_attribute dev_trigger_bitrot_check =
> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check);
How about making this attribute a RW one, so that users could check
if there's a bitrot check in progress.
>
> /**
> * ubi_volume_notify - send a volume change notification.
> @@ -334,6 +339,36 @@ int ubi_major2num(int major)
> return ubi_num;
> }
>
> +/* "Store" method for file '/<sysfs>/class/ubi/ubiX/trigger_bitrot_check' */
> +static ssize_t trigger_bitrot_check(struct device *dev,
> + struct device_attribute *mattr,
> + const char *data, size_t count)
> +{
> + struct ubi_device *ubi;
> + int ret;
> +
Maybe that's on purpose, but you do not check the value passed in data
(in your documention you suggest to do an
echo 1 > /sys/class/ubi/ubiX/trigger_bitrot_check).
> + ubi = container_of(dev, struct ubi_device, dev);
> + ubi = ubi_get_device(ubi->ubi_num);
> + if (!ubi) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + if (atomic_inc_return(&ubi->bit_rot_work) != 1) {
> + ret = -EBUSY;
> + goto out_dec;
> + }
> +
> + ubi_wl_trigger_bitrot_check(ubi);
> + ret = count;
> +
> +out_dec:
> + atomic_dec(&ubi->bit_rot_work);
> +out:
> + ubi_put_device(ubi);
> + return ret;
> +}
> +
> /* "Show" method for files in '/<sysfs>/class/ubi/ubiX/' */
> static ssize_t dev_attribute_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> @@ -445,6 +480,9 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int *ref)
> if (err)
> return err;
> err = device_create_file(&ubi->dev, &dev_mtd_num);
> + if (err)
> + return err;
> + err = device_create_file(&ubi->dev, &dev_trigger_bitrot_check);
> return err;
You don't seem to control the return code, so, how about replacing
those 2 lines by:
return device_create_file(&ubi->dev, &dev_trigger_bitrot_check);
> }
[...]
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 9b11db9..784bb52 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -1447,6 +1447,150 @@ static void tree_destroy(struct ubi_device *ubi, struct rb_root *root)
> }
>
> /**
> + * bitrot_check_worker - physical eraseblock bitrot check worker function.
> + * @ubi: UBI device description object
> + * @wl_wrk: the work object
> + * @shutdown: non-zero if the worker has to free memory and exit
> + *
> + * This function reads a physical eraseblock and schedules scrubbing if
> + * bit flips are detected.
> + */
> +static int bitrot_check_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
> + int shutdown)
> +{
> + struct ubi_wl_entry *e = wl_wrk->e;
> + int err;
> +
> + kfree(wl_wrk);
> + if (shutdown) {
> + dbg_wl("cancel bitrot check of PEB %d", e->pnum);
> + wl_entry_destroy(ubi, e);
> + return 0;
> + }
> +
> + mutex_lock(&ubi->buf_mutex);
> + err = ubi_io_read(ubi, ubi->peb_buf, e->pnum, 0, ubi->peb_size);
> + mutex_unlock(&ubi->buf_mutex);
> + if (err == UBI_IO_BITFLIPS) {
> + dbg_wl("found bitflips in PEB %d", e->pnum);
> + spin_lock(&ubi->wl_lock);
> +
> + if (in_pq(ubi, e)) {
> + prot_queue_del(ubi, e->pnum);
> + wl_tree_add(e, &ubi->scrub);
> + spin_unlock(&ubi->wl_lock);
> +
> + err = ensure_wear_leveling(ubi, 1);
> + }
> + else if (in_wl_tree(e, &ubi->used)) {
> + rb_erase(&e->u.rb, &ubi->used);
> + wl_tree_add(e, &ubi->scrub);
> + spin_unlock(&ubi->wl_lock);
> +
> + err = ensure_wear_leveling(ubi, 1);
> + }
> + else if (in_wl_tree(e, &ubi->free)) {
> + rb_erase(&e->u.rb, &ubi->free);
> + spin_unlock(&ubi->wl_lock);
> +
> + wl_wrk = prepare_erase_work(e, -1, -1, 1);
> + if (IS_ERR(wl_wrk)) {
> + err = PTR_ERR(wl_wrk);
> + goto out;
> + }
> +
> + __schedule_ubi_work(ubi, wl_wrk);
> + err = 0;
> + }
> + /*
> + * e is target of a move operation, all we can do is kicking
> + * wear leveling such that we can catch it later or wear
> + * leveling itself scrubbs the PEB.
> + */
> + else if (ubi->move_to == e || ubi->move_from == e) {
> + spin_unlock(&ubi->wl_lock);
> +
> + err = ensure_wear_leveling(ubi, 1);
> + }
> + /*
> + * e is member of a fastmap pool. We are not allowed to
> + * remove it from that pool as the on-flash fastmap data
> + * structure refers to it. Let's schedule a new fastmap write
> + * such that the said PEB can get released.
> + */
> + else {
> + ubi_schedule_fm_work(ubi);
> + spin_unlock(&ubi->wl_lock);
> +
> + err = 0;
> + }
Nitpick, but checkpatch complains about 'else' or 'else if' statements
that are not on the '}' line.
> + }
> + else {
> + /*
> + * Ignore read errors as we return only work related errors.
> + * Read errors will be logged by ubi_io_read().
> + */
> + err = 0;
> + }
Nitpicking again, but you can avoid another level of indentation by
doing the following:
if (err != UBI_IO_BITFLIPS) {
err = 0;
goto out;
}
dbg_wl("found bitflips in PEB %d", e->pnum);
spin_lock(&ubi->wl_lock);
/* ... */
> +
> +out:
> + atomic_dec(&ubi->bit_rot_work);
> + ubi_assert(atomic_read(&ubi->bit_rot_work) >= 0);
How about replacing those two lines by:
ubi_assert(atomic_dec_return(&ubi->bit_rot_work) >= 0);
> + return err;
> +}
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Am 12.04.2015 um 16:12 schrieb Boris Brezillon:
> Hi Richard,
>
> Sorry for the late reply.
>
> On Sun, 29 Mar 2015 14:13:17 +0200
> Richard Weinberger <[email protected]> wrote:
>
>> This patch implements bitrot checking for UBI.
>> ubi_wl_trigger_bitrot_check() triggers a re-read of every
>> PEB. If a bitflip is detected PEBs in use will get scrubbed
>> and free ones erased.
>
> As you'll see, I didn't have much to say about the 'UBI bitrot
> detection' mechanism, so this review is a collection of
> nitpicks :-).
>
>>
>> Signed-off-by: Richard Weinberger <[email protected]>
>> ---
>> drivers/mtd/ubi/build.c | 39 +++++++++++++
>> drivers/mtd/ubi/ubi.h | 4 ++
>> drivers/mtd/ubi/wl.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 189 insertions(+)
>>
>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
>> index 9690cf9..f58330b 100644
>> --- a/drivers/mtd/ubi/build.c
>> +++ b/drivers/mtd/ubi/build.c
>> @@ -118,6 +118,9 @@ static struct class_attribute ubi_version =
>>
>> static ssize_t dev_attribute_show(struct device *dev,
>> struct device_attribute *attr, char *buf);
>> +static ssize_t trigger_bitrot_check(struct device *dev,
>> + struct device_attribute *mattr,
>> + const char *data, size_t count);
>>
>> /* UBI device attributes (correspond to files in '/<sysfs>/class/ubi/ubiX') */
>> static struct device_attribute dev_eraseblock_size =
>> @@ -142,6 +145,8 @@ static struct device_attribute dev_bgt_enabled =
>> __ATTR(bgt_enabled, S_IRUGO, dev_attribute_show, NULL);
>> static struct device_attribute dev_mtd_num =
>> __ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
>> +static struct device_attribute dev_trigger_bitrot_check =
>> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check);
>
> How about making this attribute a RW one, so that users could check
> if there's a bitrot check in progress.
As the check will be initiated only by userspace and writing to the trigger
while a check is running will return anyway a EBUSY I don't really see
a point why userspace would check for it.
>>
>> /**
>> * ubi_volume_notify - send a volume change notification.
>> @@ -334,6 +339,36 @@ int ubi_major2num(int major)
>> return ubi_num;
>> }
>>
>> +/* "Store" method for file '/<sysfs>/class/ubi/ubiX/trigger_bitrot_check' */
>> +static ssize_t trigger_bitrot_check(struct device *dev,
>> + struct device_attribute *mattr,
>> + const char *data, size_t count)
>> +{
>> + struct ubi_device *ubi;
>> + int ret;
>> +
>
> Maybe that's on purpose, but you do not check the value passed in data
> (in your documention you suggest to do an
> echo 1 > /sys/class/ubi/ubiX/trigger_bitrot_check).
Yeah, the example using "1", but why should I limit it to it?
The idea was that any write will trigger a check.
>> + ubi = container_of(dev, struct ubi_device, dev);
>> + ubi = ubi_get_device(ubi->ubi_num);
>> + if (!ubi) {
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> +
>> + if (atomic_inc_return(&ubi->bit_rot_work) != 1) {
>> + ret = -EBUSY;
>> + goto out_dec;
>> + }
>> +
>> + ubi_wl_trigger_bitrot_check(ubi);
>> + ret = count;
>> +
>> +out_dec:
>> + atomic_dec(&ubi->bit_rot_work);
>> +out:
>> + ubi_put_device(ubi);
>> + return ret;
>> +}
>> +
>> /* "Show" method for files in '/<sysfs>/class/ubi/ubiX/' */
>> static ssize_t dev_attribute_show(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> @@ -445,6 +480,9 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int *ref)
>> if (err)
>> return err;
>> err = device_create_file(&ubi->dev, &dev_mtd_num);
>> + if (err)
>> + return err;
>> + err = device_create_file(&ubi->dev, &dev_trigger_bitrot_check);
>> return err;
>
> You don't seem to control the return code, so, how about replacing
> those 2 lines by:
>
> return device_create_file(&ubi->dev, &dev_trigger_bitrot_check);
I did it exactly like the existing code does.
But I can replace the lines...
>> }
>
> [...]
>
>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
>> index 9b11db9..784bb52 100644
>> --- a/drivers/mtd/ubi/wl.c
>> +++ b/drivers/mtd/ubi/wl.c
>> @@ -1447,6 +1447,150 @@ static void tree_destroy(struct ubi_device *ubi, struct rb_root *root)
>> }
>>
>> /**
>> + * bitrot_check_worker - physical eraseblock bitrot check worker function.
>> + * @ubi: UBI device description object
>> + * @wl_wrk: the work object
>> + * @shutdown: non-zero if the worker has to free memory and exit
>> + *
>> + * This function reads a physical eraseblock and schedules scrubbing if
>> + * bit flips are detected.
>> + */
>> +static int bitrot_check_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
>> + int shutdown)
>> +{
>> + struct ubi_wl_entry *e = wl_wrk->e;
>> + int err;
>> +
>> + kfree(wl_wrk);
>> + if (shutdown) {
>> + dbg_wl("cancel bitrot check of PEB %d", e->pnum);
>> + wl_entry_destroy(ubi, e);
>> + return 0;
>> + }
>> +
>> + mutex_lock(&ubi->buf_mutex);
>> + err = ubi_io_read(ubi, ubi->peb_buf, e->pnum, 0, ubi->peb_size);
>> + mutex_unlock(&ubi->buf_mutex);
>> + if (err == UBI_IO_BITFLIPS) {
>> + dbg_wl("found bitflips in PEB %d", e->pnum);
>> + spin_lock(&ubi->wl_lock);
>> +
>> + if (in_pq(ubi, e)) {
>> + prot_queue_del(ubi, e->pnum);
>> + wl_tree_add(e, &ubi->scrub);
>> + spin_unlock(&ubi->wl_lock);
>> +
>> + err = ensure_wear_leveling(ubi, 1);
>> + }
>> + else if (in_wl_tree(e, &ubi->used)) {
>> + rb_erase(&e->u.rb, &ubi->used);
>> + wl_tree_add(e, &ubi->scrub);
>> + spin_unlock(&ubi->wl_lock);
>> +
>> + err = ensure_wear_leveling(ubi, 1);
>> + }
>> + else if (in_wl_tree(e, &ubi->free)) {
>> + rb_erase(&e->u.rb, &ubi->free);
>> + spin_unlock(&ubi->wl_lock);
>> +
>> + wl_wrk = prepare_erase_work(e, -1, -1, 1);
>> + if (IS_ERR(wl_wrk)) {
>> + err = PTR_ERR(wl_wrk);
>> + goto out;
>> + }
>> +
>> + __schedule_ubi_work(ubi, wl_wrk);
>> + err = 0;
>> + }
>> + /*
>> + * e is target of a move operation, all we can do is kicking
>> + * wear leveling such that we can catch it later or wear
>> + * leveling itself scrubbs the PEB.
>> + */
>> + else if (ubi->move_to == e || ubi->move_from == e) {
>> + spin_unlock(&ubi->wl_lock);
>> +
>> + err = ensure_wear_leveling(ubi, 1);
>> + }
>> + /*
>> + * e is member of a fastmap pool. We are not allowed to
>> + * remove it from that pool as the on-flash fastmap data
>> + * structure refers to it. Let's schedule a new fastmap write
>> + * such that the said PEB can get released.
>> + */
>> + else {
>> + ubi_schedule_fm_work(ubi);
>> + spin_unlock(&ubi->wl_lock);
>> +
>> + err = 0;
>> + }
>
> Nitpick, but checkpatch complains about 'else' or 'else if' statements
> that are not on the '}' line.
I like it as is because I can nicely place the comment above the else {.
And checkpatch is not our lawmaker.
>> + }
>> + else {
>> + /*
>> + * Ignore read errors as we return only work related errors.
>> + * Read errors will be logged by ubi_io_read().
>> + */
>> + err = 0;
>> + }
>
> Nitpicking again, but you can avoid another level of indentation by
> doing the following:
>
> if (err != UBI_IO_BITFLIPS) {
> err = 0;
> goto out;
> }
>
> dbg_wl("found bitflips in PEB %d", e->pnum);
> spin_lock(&ubi->wl_lock);
> /* ... */
>
>> +
>> +out:
>> + atomic_dec(&ubi->bit_rot_work);
>> + ubi_assert(atomic_read(&ubi->bit_rot_work) >= 0);
>
> How about replacing those two lines by:
>
> ubi_assert(atomic_dec_return(&ubi->bit_rot_work) >= 0);
True, I always forget how many nice atomic_* helper we have. :)
Thanks,
//richard
On Sun, 12 Apr 2015 18:09:23 +0200
Richard Weinberger <[email protected]> wrote:
> Am 12.04.2015 um 16:12 schrieb Boris Brezillon:
> > Hi Richard,
> >
> > Sorry for the late reply.
> >
> > On Sun, 29 Mar 2015 14:13:17 +0200
> > Richard Weinberger <[email protected]> wrote:
> >
> >> This patch implements bitrot checking for UBI.
> >> ubi_wl_trigger_bitrot_check() triggers a re-read of every
> >> PEB. If a bitflip is detected PEBs in use will get scrubbed
> >> and free ones erased.
> >
> > As you'll see, I didn't have much to say about the 'UBI bitrot
> > detection' mechanism, so this review is a collection of
> > nitpicks :-).
> >
> >>
> >> Signed-off-by: Richard Weinberger <[email protected]>
> >> ---
> >> drivers/mtd/ubi/build.c | 39 +++++++++++++
> >> drivers/mtd/ubi/ubi.h | 4 ++
> >> drivers/mtd/ubi/wl.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 189 insertions(+)
> >>
> >> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> >> index 9690cf9..f58330b 100644
> >> --- a/drivers/mtd/ubi/build.c
> >> +++ b/drivers/mtd/ubi/build.c
> >> @@ -118,6 +118,9 @@ static struct class_attribute ubi_version =
> >>
> >> static ssize_t dev_attribute_show(struct device *dev,
> >> struct device_attribute *attr, char *buf);
> >> +static ssize_t trigger_bitrot_check(struct device *dev,
> >> + struct device_attribute *mattr,
> >> + const char *data, size_t count);
> >>
> >> /* UBI device attributes (correspond to files in '/<sysfs>/class/ubi/ubiX') */
> >> static struct device_attribute dev_eraseblock_size =
> >> @@ -142,6 +145,8 @@ static struct device_attribute dev_bgt_enabled =
> >> __ATTR(bgt_enabled, S_IRUGO, dev_attribute_show, NULL);
> >> static struct device_attribute dev_mtd_num =
> >> __ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
> >> +static struct device_attribute dev_trigger_bitrot_check =
> >> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check);
> >
> > How about making this attribute a RW one, so that users could check
> > if there's a bitrot check in progress.
>
> As the check will be initiated only by userspace and writing to the trigger
> while a check is running will return anyway a EBUSY I don't really see
> a point why userspace would check for it.
Sometime you just want to know whether something is running or not (in
this case the bitrot check) without risking to trigger a new action...
>
> >>
> >> /**
> >> * ubi_volume_notify - send a volume change notification.
> >> @@ -334,6 +339,36 @@ int ubi_major2num(int major)
> >> return ubi_num;
> >> }
> >>
> >> +/* "Store" method for file '/<sysfs>/class/ubi/ubiX/trigger_bitrot_check' */
> >> +static ssize_t trigger_bitrot_check(struct device *dev,
> >> + struct device_attribute *mattr,
> >> + const char *data, size_t count)
> >> +{
> >> + struct ubi_device *ubi;
> >> + int ret;
> >> +
> >
> > Maybe that's on purpose, but you do not check the value passed in data
> > (in your documention you suggest to do an
> > echo 1 > /sys/class/ubi/ubiX/trigger_bitrot_check).
>
> Yeah, the example using "1", but why should I limit it to it?
> The idea was that any write will trigger a check.
Okay.
[...]
> >> + /*
> >> + * e is member of a fastmap pool. We are not allowed to
> >> + * remove it from that pool as the on-flash fastmap data
> >> + * structure refers to it. Let's schedule a new fastmap write
> >> + * such that the said PEB can get released.
> >> + */
> >> + else {
> >> + ubi_schedule_fm_work(ubi);
> >> + spin_unlock(&ubi->wl_lock);
> >> +
> >> + err = 0;
> >> + }
> >
> > Nitpick, but checkpatch complains about 'else' or 'else if' statements
> > that are not on the '}' line.
>
> I like it as is because I can nicely place the comment above the else {.
> And checkpatch is not our lawmaker.
You could put your comment after the braces.
Anyway, you might dislike the coding style imposed by kernel
developers/maintainers, but this is what keeps the kernel code
consistent across the different subsystems.
I agree that some checks done by checkpatch can be a bit restrictive in
some cases (like the 80 characters limit), but I really think the
braces and else[ if] placements should be enforced.
This being said, this is your call to make, so I won't complain about
it anymore ;-).
>
> >> + }
> >> + else {
> >> + /*
> >> + * Ignore read errors as we return only work related errors.
> >> + * Read errors will be logged by ubi_io_read().
> >> + */
> >> + err = 0;
> >> + }
> >
> > Nitpicking again, but you can avoid another level of indentation by
> > doing the following:
> >
> > if (err != UBI_IO_BITFLIPS) {
> > err = 0;
> > goto out;
> > }
> >
> > dbg_wl("found bitflips in PEB %d", e->pnum);
> > spin_lock(&ubi->wl_lock);
> > /* ... */
You didn't answer to that one.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Am 12.04.2015 um 18:43 schrieb Boris Brezillon:
> On Sun, 12 Apr 2015 18:09:23 +0200
> Richard Weinberger <[email protected]> wrote:
>
>> Am 12.04.2015 um 16:12 schrieb Boris Brezillon:
>>> Hi Richard,
>>>
>>> Sorry for the late reply.
>>>
>>> On Sun, 29 Mar 2015 14:13:17 +0200
>>> Richard Weinberger <[email protected]> wrote:
>>>
>>>> This patch implements bitrot checking for UBI.
>>>> ubi_wl_trigger_bitrot_check() triggers a re-read of every
>>>> PEB. If a bitflip is detected PEBs in use will get scrubbed
>>>> and free ones erased.
>>>
>>> As you'll see, I didn't have much to say about the 'UBI bitrot
>>> detection' mechanism, so this review is a collection of
>>> nitpicks :-).
>>>
>>>>
>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>> ---
>>>> drivers/mtd/ubi/build.c | 39 +++++++++++++
>>>> drivers/mtd/ubi/ubi.h | 4 ++
>>>> drivers/mtd/ubi/wl.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 189 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
>>>> index 9690cf9..f58330b 100644
>>>> --- a/drivers/mtd/ubi/build.c
>>>> +++ b/drivers/mtd/ubi/build.c
>>>> @@ -118,6 +118,9 @@ static struct class_attribute ubi_version =
>>>>
>>>> static ssize_t dev_attribute_show(struct device *dev,
>>>> struct device_attribute *attr, char *buf);
>>>> +static ssize_t trigger_bitrot_check(struct device *dev,
>>>> + struct device_attribute *mattr,
>>>> + const char *data, size_t count);
>>>>
>>>> /* UBI device attributes (correspond to files in '/<sysfs>/class/ubi/ubiX') */
>>>> static struct device_attribute dev_eraseblock_size =
>>>> @@ -142,6 +145,8 @@ static struct device_attribute dev_bgt_enabled =
>>>> __ATTR(bgt_enabled, S_IRUGO, dev_attribute_show, NULL);
>>>> static struct device_attribute dev_mtd_num =
>>>> __ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
>>>> +static struct device_attribute dev_trigger_bitrot_check =
>>>> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check);
>>>
>>> How about making this attribute a RW one, so that users could check
>>> if there's a bitrot check in progress.
>>
>> As the check will be initiated only by userspace and writing to the trigger
>> while a check is running will return anyway a EBUSY I don't really see
>> a point why userspace would check for it.
>
> Sometime you just want to know whether something is running or not (in
> this case the bitrot check) without risking to trigger a new action...
Why would they care?
But I can add this feature, no problem.
>>
>>>>
>>>> /**
>>>> * ubi_volume_notify - send a volume change notification.
>>>> @@ -334,6 +339,36 @@ int ubi_major2num(int major)
>>>> return ubi_num;
>>>> }
>>>>
>>>> +/* "Store" method for file '/<sysfs>/class/ubi/ubiX/trigger_bitrot_check' */
>>>> +static ssize_t trigger_bitrot_check(struct device *dev,
>>>> + struct device_attribute *mattr,
>>>> + const char *data, size_t count)
>>>> +{
>>>> + struct ubi_device *ubi;
>>>> + int ret;
>>>> +
>>>
>>> Maybe that's on purpose, but you do not check the value passed in data
>>> (in your documention you suggest to do an
>>> echo 1 > /sys/class/ubi/ubiX/trigger_bitrot_check).
>>
>> Yeah, the example using "1", but why should I limit it to it?
>> The idea was that any write will trigger a check.
>
> Okay.
>
>
> [...]
>
>>>> + /*
>>>> + * e is member of a fastmap pool. We are not allowed to
>>>> + * remove it from that pool as the on-flash fastmap data
>>>> + * structure refers to it. Let's schedule a new fastmap write
>>>> + * such that the said PEB can get released.
>>>> + */
>>>> + else {
>>>> + ubi_schedule_fm_work(ubi);
>>>> + spin_unlock(&ubi->wl_lock);
>>>> +
>>>> + err = 0;
>>>> + }
>>>
>>> Nitpick, but checkpatch complains about 'else' or 'else if' statements
>>> that are not on the '}' line.
>>
>> I like it as is because I can nicely place the comment above the else {.
>> And checkpatch is not our lawmaker.
>
> You could put your comment after the braces.
> Anyway, you might dislike the coding style imposed by kernel
> developers/maintainers, but this is what keeps the kernel code
> consistent across the different subsystems.
> I agree that some checks done by checkpatch can be a bit restrictive in
> some cases (like the 80 characters limit), but I really think the
> braces and else[ if] placements should be enforced.
> This being said, this is your call to make, so I won't complain about
> it anymore ;-).
It is corner case which is not handled by the kernel coding style IMHO.
The sad thing is that checkpatch is not developed by kernel developers.
>>
>>>> + }
>>>> + else {
>>>> + /*
>>>> + * Ignore read errors as we return only work related errors.
>>>> + * Read errors will be logged by ubi_io_read().
>>>> + */
>>>> + err = 0;
>>>> + }
>>>
>>> Nitpicking again, but you can avoid another level of indentation by
>>> doing the following:
>>>
>>> if (err != UBI_IO_BITFLIPS) {
>>> err = 0;
>>> goto out;
>>> }
>>>
>>> dbg_wl("found bitflips in PEB %d", e->pnum);
>>> spin_lock(&ubi->wl_lock);
>>> /* ... */
>
> You didn't answer to that one.
Whoops.
Yeah, that makes sense!
Thanks,
//richard
Il 12/04/2015 18:55, Richard Weinberger ha scritto:
> Am 12.04.2015 um 18:43 schrieb Boris Brezillon:
>> On Sun, 12 Apr 2015 18:09:23 +0200
>> Richard Weinberger <[email protected]> wrote:
>>
>>> Am 12.04.2015 um 16:12 schrieb Boris Brezillon:
>>>> Hi Richard,
>>>>
>>>> Sorry for the late reply.
>>>>
>>>> On Sun, 29 Mar 2015 14:13:17 +0200
>>>> Richard Weinberger <[email protected]> wrote:
>>>>
>>>>> This patch implements bitrot checking for UBI.
>>>>> ubi_wl_trigger_bitrot_check() triggers a re-read of every
>>>>> PEB. If a bitflip is detected PEBs in use will get scrubbed
>>>>> and free ones erased.
>>>>
>>>> As you'll see, I didn't have much to say about the 'UBI bitrot
>>>> detection' mechanism, so this review is a collection of
>>>> nitpicks :-).
>>>>
>>>>>
>>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>>> ---
>>>>> drivers/mtd/ubi/build.c | 39 +++++++++++++
>>>>> drivers/mtd/ubi/ubi.h | 4 ++
>>>>> drivers/mtd/ubi/wl.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 189 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
>>>>> index 9690cf9..f58330b 100644
>>>>> --- a/drivers/mtd/ubi/build.c
>>>>> +++ b/drivers/mtd/ubi/build.c
>>>>> @@ -118,6 +118,9 @@ static struct class_attribute ubi_version =
>>>>>
>>>>> static ssize_t dev_attribute_show(struct device *dev,
>>>>> struct device_attribute *attr, char *buf);
>>>>> +static ssize_t trigger_bitrot_check(struct device *dev,
>>>>> + struct device_attribute *mattr,
>>>>> + const char *data, size_t count);
>>>>>
>>>>> /* UBI device attributes (correspond to files in '/<sysfs>/class/ubi/ubiX') */
>>>>> static struct device_attribute dev_eraseblock_size =
>>>>> @@ -142,6 +145,8 @@ static struct device_attribute dev_bgt_enabled =
>>>>> __ATTR(bgt_enabled, S_IRUGO, dev_attribute_show, NULL);
>>>>> static struct device_attribute dev_mtd_num =
>>>>> __ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
>>>>> +static struct device_attribute dev_trigger_bitrot_check =
>>>>> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check);
>>>>
>>>> How about making this attribute a RW one, so that users could check
>>>> if there's a bitrot check in progress.
>>>
>>> As the check will be initiated only by userspace and writing to the trigger
>>> while a check is running will return anyway a EBUSY I don't really see
>>> a point why userspace would check for it.
>>
>> Sometime you just want to know whether something is running or not (in
>> this case the bitrot check) without risking to trigger a new action...
>
> Why would they care?
I think is always useful to give some additional information in
userspace, from both debugging and diagnostic point of view.
> But I can add this feature, no problem.
Thanks ;-)
May I ask if can be useful to abort the (IMHO quite long running) operation?
I think it can be useful to save power, e.g. when running on batteries:
smart systems will trigger the operation when charging and aborting it
if on batteries (or on low batteries).
What happens if the system need to reboot in the middle of scanning?
Probably nothing at all but I think it's worth asking ;-)
Anyway I think it's better if we can, on runlevel 6, shutdown the
operation in a clean way
To ask a little bit more from the current implementation, can it be
useful expand sysfs entry with the current status (stopped, running,
completed)?
In this way the userspace knows whenever the operation it has triggered,
it completed successfully or something interrupt it (e.g. an internal
error). I will schedule a new operation sooner if I have no evidence
that the last one completed successfully.. WDYT?
But maybe all of this stuff will be implemented inside a daemon with
additional ioctl() (IIRC Richard already is working on this).
>
>>>
>>>>>
>>>>> /**
>>>>> * ubi_volume_notify - send a volume change notification.
>>>>> @@ -334,6 +339,36 @@ int ubi_major2num(int major)
>>>>> return ubi_num;
>>>>> }
>>>>>
>>>>> +/* "Store" method for file '/<sysfs>/class/ubi/ubiX/trigger_bitrot_check' */
>>>>> +static ssize_t trigger_bitrot_check(struct device *dev,
>>>>> + struct device_attribute *mattr,
>>>>> + const char *data, size_t count)
>>>>> +{
>>>>> + struct ubi_device *ubi;
>>>>> + int ret;
>>>>> +
>>>>
>>>> Maybe that's on purpose, but you do not check the value passed in data
>>>> (in your documention you suggest to do an
>>>> echo 1 > /sys/class/ubi/ubiX/trigger_bitrot_check).
>>>
>>> Yeah, the example using "1", but why should I limit it to it?
>>> The idea was that any write will trigger a check.
>>
>> Okay.
See above my reason for having something more that 1 value (or having
more than one sysfs entry ;-) )
Kind Regards,
Andrea SCIAN
--
Andrea SCIAN
DAVE Embedded Systems
Am 12.04.2015 um 22:42 schrieb Andrea Scian:
>
> Il 12/04/2015 18:55, Richard Weinberger ha scritto:
>> Am 12.04.2015 um 18:43 schrieb Boris Brezillon:
>>> On Sun, 12 Apr 2015 18:09:23 +0200
>>> Richard Weinberger <[email protected]> wrote:
>>>
>>>> Am 12.04.2015 um 16:12 schrieb Boris Brezillon:
>>>>> Hi Richard,
>>>>>
>>>>> Sorry for the late reply.
>>>>>
>>>>> On Sun, 29 Mar 2015 14:13:17 +0200
>>>>> Richard Weinberger <[email protected]> wrote:
>>>>>
>>>>>> This patch implements bitrot checking for UBI.
>>>>>> ubi_wl_trigger_bitrot_check() triggers a re-read of every
>>>>>> PEB. If a bitflip is detected PEBs in use will get scrubbed
>>>>>> and free ones erased.
>>>>>
>>>>> As you'll see, I didn't have much to say about the 'UBI bitrot
>>>>> detection' mechanism, so this review is a collection of
>>>>> nitpicks :-).
>>>>>
>>>>>>
>>>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>>>> ---
>>>>>> drivers/mtd/ubi/build.c | 39 +++++++++++++
>>>>>> drivers/mtd/ubi/ubi.h | 4 ++
>>>>>> drivers/mtd/ubi/wl.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> 3 files changed, 189 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
>>>>>> index 9690cf9..f58330b 100644
>>>>>> --- a/drivers/mtd/ubi/build.c
>>>>>> +++ b/drivers/mtd/ubi/build.c
>>>>>> @@ -118,6 +118,9 @@ static struct class_attribute ubi_version =
>>>>>>
>>>>>> static ssize_t dev_attribute_show(struct device *dev,
>>>>>> struct device_attribute *attr, char *buf);
>>>>>> +static ssize_t trigger_bitrot_check(struct device *dev,
>>>>>> + struct device_attribute *mattr,
>>>>>> + const char *data, size_t count);
>>>>>>
>>>>>> /* UBI device attributes (correspond to files in '/<sysfs>/class/ubi/ubiX') */
>>>>>> static struct device_attribute dev_eraseblock_size =
>>>>>> @@ -142,6 +145,8 @@ static struct device_attribute dev_bgt_enabled =
>>>>>> __ATTR(bgt_enabled, S_IRUGO, dev_attribute_show, NULL);
>>>>>> static struct device_attribute dev_mtd_num =
>>>>>> __ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
>>>>>> +static struct device_attribute dev_trigger_bitrot_check =
>>>>>> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check);
>>>>>
>>>>> How about making this attribute a RW one, so that users could check
>>>>> if there's a bitrot check in progress.
>>>>
>>>> As the check will be initiated only by userspace and writing to the trigger
>>>> while a check is running will return anyway a EBUSY I don't really see
>>>> a point why userspace would check for it.
>>>
>>> Sometime you just want to know whether something is running or not (in
>>> this case the bitrot check) without risking to trigger a new action...
>>
>> Why would they care?
>
> I think is always useful to give some additional information in userspace, from both debugging and diagnostic point of view.
The question is, why does userspace care?
Other UBI operations are also not visible...
>> But I can add this feature, no problem.
>
> Thanks ;-)
>
> May I ask if can be useful to abort the (IMHO quite long running) operation?
> I think it can be useful to save power, e.g. when running on batteries: smart systems will trigger the operation when charging and aborting it if on batteries (or on low batteries).
If the system is running on low power mode just don't trigger the run...
Userspace controls it.
> What happens if the system need to reboot in the middle of scanning?
Just reboot, UBI can handle that. Work will be canceled.
> Probably nothing at all but I think it's worth asking ;-)
> Anyway I think it's better if we can, on runlevel 6, shutdown the operation in a clean way
>
> To ask a little bit more from the current implementation, can it be useful expand sysfs entry with the current status (stopped, running, completed)?
> In this way the userspace knows whenever the operation it has triggered, it completed successfully or something interrupt it (e.g. an internal error). I will schedule a new
> operation sooner if I have no evidence that the last one completed successfully.. WDYT?
> But maybe all of this stuff will be implemented inside a daemon with additional ioctl() (IIRC Richard already is working on this).
That's the plan. The interface proposed in that patch series it designed to be a simple replacement for the dd if=/dev/ubiXY of=/dev/null hack.
The next step is adding a more advanced ioctl() interface to support a clever deamon.
Thanks,
//richard
On Sun, 12 Apr 2015 23:01:27 +0200
Richard Weinberger <[email protected]> wrote:
> >>>>>> +static struct device_attribute dev_trigger_bitrot_check =
> >>>>>> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check);
> >>>>>
> >>>>> How about making this attribute a RW one, so that users could check
> >>>>> if there's a bitrot check in progress.
> >>>>
> >>>> As the check will be initiated only by userspace and writing to the trigger
> >>>> while a check is running will return anyway a EBUSY I don't really see
> >>>> a point why userspace would check for it.
> >>>
> >>> Sometime you just want to know whether something is running or not (in
> >>> this case the bitrot check) without risking to trigger a new action...
> >>
> >> Why would they care?
> >
> > I think is always useful to give some additional information in userspace, from both debugging and diagnostic point of view.
>
> The question is, why does userspace care?
> Other UBI operations are also not visible...
Yes, but AFAIK other wear-leveling operations are not directly triggered
by user-space.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Il 12/04/2015 23:01, Richard Weinberger ha scritto:
> Am 12.04.2015 um 22:42 schrieb Andrea Scian:
>> Il 12/04/2015 18:55, Richard Weinberger ha scritto:
>>> Am 12.04.2015 um 18:43 schrieb Boris Brezillon:
>>>> On Sun, 12 Apr 2015 18:09:23 +0200
>>>> Richard Weinberger <[email protected]> wrote:
>>>>
>>>>> Am 12.04.2015 um 16:12 schrieb Boris Brezillon:
>>>>>> Hi Richard,
>>>>>>
>>>>>> Sorry for the late reply.
>>>>>>
>>>>>> On Sun, 29 Mar 2015 14:13:17 +0200
>>>>>> Richard Weinberger <[email protected]> wrote:
>>>>>>
>>>>>>> This patch implements bitrot checking for UBI.
>>>>>>> ubi_wl_trigger_bitrot_check() triggers a re-read of every
>>>>>>> PEB. If a bitflip is detected PEBs in use will get scrubbed
>>>>>>> and free ones erased.
>>>>>> As you'll see, I didn't have much to say about the 'UBI bitrot
>>>>>> detection' mechanism, so this review is a collection of
>>>>>> nitpicks :-).
>>>>>>
>>>>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>>>>> ---
>>>>>>> drivers/mtd/ubi/build.c | 39 +++++++++++++
>>>>>>> drivers/mtd/ubi/ubi.h | 4 ++
>>>>>>> drivers/mtd/ubi/wl.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>> 3 files changed, 189 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
>>>>>>> index 9690cf9..f58330b 100644
>>>>>>> --- a/drivers/mtd/ubi/build.c
>>>>>>> +++ b/drivers/mtd/ubi/build.c
>>>>>>> @@ -118,6 +118,9 @@ static struct class_attribute ubi_version =
>>>>>>>
>>>>>>> static ssize_t dev_attribute_show(struct device *dev,
>>>>>>> struct device_attribute *attr, char *buf);
>>>>>>> +static ssize_t trigger_bitrot_check(struct device *dev,
>>>>>>> + struct device_attribute *mattr,
>>>>>>> + const char *data, size_t count);
>>>>>>>
>>>>>>> /* UBI device attributes (correspond to files in '/<sysfs>/class/ubi/ubiX') */
>>>>>>> static struct device_attribute dev_eraseblock_size =
>>>>>>> @@ -142,6 +145,8 @@ static struct device_attribute dev_bgt_enabled =
>>>>>>> __ATTR(bgt_enabled, S_IRUGO, dev_attribute_show, NULL);
>>>>>>> static struct device_attribute dev_mtd_num =
>>>>>>> __ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
>>>>>>> +static struct device_attribute dev_trigger_bitrot_check =
>>>>>>> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check);
>>>>>> How about making this attribute a RW one, so that users could check
>>>>>> if there's a bitrot check in progress.
>>>>> As the check will be initiated only by userspace and writing to the trigger
>>>>> while a check is running will return anyway a EBUSY I don't really see
>>>>> a point why userspace would check for it.
>>>> Sometime you just want to know whether something is running or not (in
>>>> this case the bitrot check) without risking to trigger a new action...
>>> Why would they care?
>> I think is always useful to give some additional information in userspace, from both debugging and diagnostic point of view.
> The question is, why does userspace care?
Because the userspace trigger it
> Other UBI operations are also not visible...
I don't really know much about UBI internals, but I think not many
operation are triggered from userspace (apart from ubiformat and mount ;-) )
>>> But I can add this feature, no problem.
>> Thanks ;-)
>>
>> May I ask if can be useful to abort the (IMHO quite long running) operation?
>> I think it can be useful to save power, e.g. when running on batteries: smart systems will trigger the operation when charging and aborting it if on batteries (or on low batteries).
> If the system is running on low power mode just don't trigger the run...
> Userspace controls it.
It heavily depends on how long the operation takes, we may have 4 to 32
GiB devices so I think it may take more than just a few minutes to scan
the whole device (and additional time depending on how many scrub
operation are needed).
You may start the operation when power is not an issue (e.g. while
charging) and when it is (e.g. when running on batteries and you need to
save any mAh you have ;-) ) it may be still running for a while..
I know that this is some kind of advance feature, but I would like to
point this out for comments.
>
>> What happens if the system need to reboot in the middle of scanning?
> Just reboot, UBI can handle that. Work will be canceled.
Thanks for the confirm
>
>> Probably nothing at all but I think it's worth asking ;-)
>> Anyway I think it's better if we can, on runlevel 6, shutdown the operation in a clean way
>>
>> To ask a little bit more from the current implementation, can it be useful expand sysfs entry with the current status (stopped, running, completed)?
>> In this way the userspace knows whenever the operation it has triggered, it completed successfully or something interrupt it (e.g. an internal error). I will schedule a new
>> operation sooner if I have no evidence that the last one completed successfully.. WDYT?
>> But maybe all of this stuff will be implemented inside a daemon with additional ioctl() (IIRC Richard already is working on this).
> That's the plan. The interface proposed in that patch series it designed to be a simple replacement for the dd if=/dev/ubiXY of=/dev/null hack.
dd can be stopped if needed and you may also have the progress status :-)
for sure you probably don't need all of the above additional stuff but
it may be useful anyway
Maybe it's better to have an initial working implementation and add some
more (backward compatible?) features later.
> The next step is adding a more advanced ioctl() interface to support a clever deamon.
Kind Regards and thanks for you comments,
--
Andrea SCIAN
DAVE Embedded Systems
Am 12.04.2015 um 23:30 schrieb Boris Brezillon:
> On Sun, 12 Apr 2015 23:01:27 +0200
> Richard Weinberger <[email protected]> wrote:
>
>
>>>>>>>> +static struct device_attribute dev_trigger_bitrot_check =
>>>>>>>> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check);
>>>>>>>
>>>>>>> How about making this attribute a RW one, so that users could check
>>>>>>> if there's a bitrot check in progress.
>>>>>>
>>>>>> As the check will be initiated only by userspace and writing to the trigger
>>>>>> while a check is running will return anyway a EBUSY I don't really see
>>>>>> a point why userspace would check for it.
>>>>>
>>>>> Sometime you just want to know whether something is running or not (in
>>>>> this case the bitrot check) without risking to trigger a new action...
>>>>
>>>> Why would they care?
>>>
>>> I think is always useful to give some additional information in userspace, from both debugging and diagnostic point of view.
>>
>> The question is, why does userspace care?
>> Other UBI operations are also not visible...
>
> Yes, but AFAIK other wear-leveling operations are not directly triggered
> by user-space.
If userspace trigger is, it knows that it was triggered.
Unless you have multiple tools/scripts which want to trigger it,
which is IMHO a problem anyways.
But as stated before, I can add an indicator.
Thanks,
//richard
Am 12.04.2015 um 23:33 schrieb Andrea Scian:
>>> I think is always useful to give some additional information in userspace, from both debugging and diagnostic point of view.
>> The question is, why does userspace care?
>
> Because the userspace trigger it
As written in the previous mail, then userspace knows anyway the state.
>
>> Other UBI operations are also not visible...
>
> I don't really know much about UBI internals, but I think not many operation are triggered from userspace (apart from ubiformat and mount ;-) )
>
>
>>>> But I can add this feature, no problem.
>>> Thanks ;-)
>>>
>>> May I ask if can be useful to abort the (IMHO quite long running) operation?
>>> I think it can be useful to save power, e.g. when running on batteries: smart systems will trigger the operation when charging and aborting it if on batteries (or on low
>>> batteries).
>> If the system is running on low power mode just don't trigger the run...
>> Userspace controls it.
>
> It heavily depends on how long the operation takes, we may have 4 to 32 GiB devices so I think it may take more than just a few minutes to scan the whole device (and additional
> time depending on how many scrub operation are needed).
> You may start the operation when power is not an issue (e.g. while charging) and when it is (e.g. when running on batteries and you need to save any mAh you have ;-) ) it may be
> still running for a while..
> I know that this is some kind of advance feature, but I would like to point this out for comments.
In such a scenario one definitely wants the emerging ioctl() interface.
>>
>>> What happens if the system need to reboot in the middle of scanning?
>> Just reboot, UBI can handle that. Work will be canceled.
>
> Thanks for the confirm
>
>>
>>> Probably nothing at all but I think it's worth asking ;-)
>>> Anyway I think it's better if we can, on runlevel 6, shutdown the operation in a clean way
>>>
>>> To ask a little bit more from the current implementation, can it be useful expand sysfs entry with the current status (stopped, running, completed)?
>>> In this way the userspace knows whenever the operation it has triggered, it completed successfully or something interrupt it (e.g. an internal error). I will schedule a new
>>> operation sooner if I have no evidence that the last one completed successfully.. WDYT?
>>> But maybe all of this stuff will be implemented inside a daemon with additional ioctl() (IIRC Richard already is working on this).
>> That's the plan. The interface proposed in that patch series it designed to be a simple replacement for the dd if=/dev/ubiXY of=/dev/null hack.
>
> dd can be stopped if needed and you may also have the progress status :-)
>
> for sure you probably don't need all of the above additional stuff but it may be useful anyway
> Maybe it's better to have an initial working implementation and add some more (backward compatible?) features later.
I agree.
BTW: Thanks a lot for your comments, Boris and Andrea!
Thanks,
//richard
Hi everyone,
linux-mtd administrative note:
Please don't start conversations based on mailing list "digest" emails.
It's not helpful for others (e.g., it likely breaks threading). Also,
the mailing list filters these types of messages out, presumably based
on the email subject, so many subscribers probably did not see your
email.
So, a recent thread was under the subject:
Re: [PATCH 4/4] UBI: Implement bitrot checking (linux-mtd Digest, Vol 145, Issue 24)
But it should be using:
Re: [PATCH 4/4] UBI: Implement bitrot checking
I've released a few messages for now, but I can't guarantee that such
messages will get through in the future (and certainly not promptly).
Thanks,
Brian