Hi Ezequiel,
On Fri, 23 Nov 2012 09:13:29 -0300 Ezequiel Garcia <elezegar...@gmail.com>
wrote:
> /**
> - * ubi_thread - UBI background thread.
> + * ubi_wl_do_work - UBI background wl worker function.
> * @u: the UBI device description object pointer
> */
> -int ubi_thread(void *u)
> +void ubi_wl_do_work(struct work_struct *work)
> {
> + int err;
> int failures = 0;
> - struct ubi_device *ubi = u;
> -
> - ubi_msg("background thread \"%s\" started, PID %d",
> - ubi->bgt_name, task_pid_nr(current));
> -
> - set_freezable();
> - for (;;) {
> - int err;
> -
> - if (kthread_should_stop())
> - break;
> + struct ubi_device *ubi = container_of(work, struct ubi_device, work);
>
> - if (try_to_freeze())
> - continue;
> -
> - spin_lock(&ubi->wl_lock);
> - if (list_empty(&ubi->works) || ubi->ro_mode ||
Originally, 'ubi_thread' did nothing if 'ubi->ro_mode'.
This filtering is missing from 'ubi_wl_do_work' implementation.
How do we guarantee 'ubi_wl_do_work' is never queued when in RO mode?
> - !ubi->thread_enabled || ubi_dbg_is_bgt_disabled(ubi)) {
> - set_current_state(TASK_INTERRUPTIBLE);
> - spin_unlock(&ubi->wl_lock);
> - schedule();
> - continue;
> - }
> - spin_unlock(&ubi->wl_lock);
> + err = do_work(ubi);
> + if (!err)
> + return;
>
> - err = do_work(ubi);
> - if (err) {
> - ubi_err("%s: work failed with error code %d",
> - ubi->bgt_name, err);
> - if (failures++ > WL_MAX_FAILURES) {
> - /*
> - * Too many failures, disable the thread and
> - * switch to read-only mode.
> - */
> - ubi_msg("%s: %d consecutive failures",
> - ubi->bgt_name, WL_MAX_FAILURES);
> - ubi_ro_mode(ubi);
> - ubi->thread_enabled = 0;
> - continue;
> - }
> - } else
> - failures = 0;
> -
> - cond_resched();
> + ubi_err("%s: work failed with error code %d",
> + ubi->wq_name, err);
> + if (failures++ > WL_MAX_FAILURES) {
> + /*
> + * Too many failures, disable the workqueue and
> + * switch to read-only mode.
> + */
This condition will never be met (after your change), since 'failures'
is local to 'ubi_wl_do_work' (per work invocation).
Formerly, 'failures' was local to 'ubi_thread' (per ubi device's
thread), hence it was possible that several 'do_works()' calls have
failed during thread's lifetime, reaching the WL_MAX_FAILURES limit.
If we'd like to preseve the 'failures' semantics, 'failures' should be
an 'ubi_device' property.
One last thing:
Some variables and functions (debug and sysfs) are still named *bgt*,
which is confusing.
Regards,
Shmulik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
