Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
Hi Hu, On 12/29/2014 5:14 AM, hujianyang wrote: On 2014/11/3 21:58, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. The R/O block driver messages were replaced by pr_* since ubi_device structure is not used by it. Amended a bit by Artem. Signed-off-by: Tanya Brokhman Signed-off-by: Artem Bityutskiy --- Changes from V1: Hi Artem and Tanya, Do you think the similar effort is worth to be done on ubifs-level? We already did this for UBIFS as well and have been working with this patch for some time now. Its in our todo to share it, just got pulled into some urgent staff unfortunately. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
Hi Hu, On 12/29/2014 5:14 AM, hujianyang wrote: On 2014/11/3 21:58, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. The R/O block driver messages were replaced by pr_* since ubi_device structure is not used by it. Amended a bit by Artem. Signed-off-by: Tanya Brokhman tlin...@codeaurora.org Signed-off-by: Artem Bityutskiy artem.bityuts...@linux.intel.com --- Changes from V1: Hi Artem and Tanya, Do you think the similar effort is worth to be done on ubifs-level? We already did this for UBIFS as well and have been working with this patch for some time now. Its in our todo to share it, just got pulled into some urgent staff unfortunately. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 3/6] UBI: Fastmap: Notify user in case of an ubi_update_fastmap() failure
On 12/8/2014 11:11 AM, Richard Weinberger wrote:
Hi!
Am 08.12.2014 um 07:58 schrieb Tanya Brokhman:
On 12/7/2014 4:22 PM, Richard Weinberger wrote:
Am 07.12.2014 um 14:59 schrieb Tanya Brokhman:
Hi Richard,
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
If ubi_update_fastmap() fails notify the user.
This is not a hard error as ubi_update_fastmap() makes sure that upon failure
the current on-flash fastmap will no be used upon next UBI attach.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 523d8a4..7821342 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -657,7 +657,11 @@ again:
* refill the WL pool synchronous. */
if (pool->used == pool->size || wl_pool->used == wl_pool->size) {
spin_unlock(>wl_lock);
-ubi_update_fastmap(ubi);
+ret = ubi_update_fastmap(ubi);
+if (ret) {
+ubi_msg(ubi, "Unable to write a new fastmap: %i", ret);
+return -ENOSPC;
Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed?
Its possible that the fm_pools were refilled correctly, and the actual
fastmap_write failed, so there
is nothing preventing the user to get peb allocated and continue working. You
invalidate the fastmap, so if powercut occurs a full scan will be performed. So
its possible to
allocate from fm_pools (although fastmap is not valid on disc) and try writing
fastmap again when the pools filled up.
I'm for the ubi_msg but against "return -ENOSPC;"
Maybe the case you've described is powercut safe, but there can be other unsafe
cases.
Let's stay on the safe side and be paranoid, it does not hurt.
If fastmap has proven stable we can start with tricky optimizations.
I'm sorry that I'm being petty here but the commit msg states that you "notify the
user in case of update fastamap failure". It says nothing about you failing
ubi_wl_get_peb as
well. And this is a major change. At least divide this into 2 patches (so I can
disagree to the function failing and agree to the msg to user :) )
With user I meant users of that function.
I still don't like it.
Leaving this one for Artem... sorry
Thanks,
//richard
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 2/6] UBI: Fastmap: Don't allocate new ubi_wl_entry objects
On 12/8/2014 11:14 AM, Richard Weinberger wrote: Am 08.12.2014 um 09:36 schrieb Tanya Brokhman: On 11/30/2014 1:35 PM, Richard Weinberger wrote: There is no need to allocate new ones every time, we can reuse the existing ones. This makes the code cleaner and more easy to follow. Signed-off-by: Richard Weinberger --- drivers/mtd/ubi/fastmap.c | 31 +-- drivers/mtd/ubi/wl.c | 11 +++ 2 files changed, 12 insertions(+), 30 deletions(-) diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c index db3defd..9507702 100644 --- a/drivers/mtd/ubi/fastmap.c +++ b/drivers/mtd/ubi/fastmap.c @@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi) } new_fm->used_blocks = ubi->fm_size / ubi->leb_size; Not related to this patch, but looking at this function it got me thinking: why do we need to re-calculate new_fm->used_blocks (and check calculated value) each time? fm_size doesn't changed at runtime. leb_size sure does not, so fm->used_blocks can be ubi device parameter and calculated & tested only once, and not each time we write fastmap. Correct? If you dig deeper into the patch set you'll notice that the fastmap size can change. :-) oh, ok. sorry, still in the middle of the review. Thanks, //richard Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 2/6] UBI: Fastmap: Don't allocate new ubi_wl_entry objects
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
There is no need to allocate new ones every time, we can reuse
the existing ones.
This makes the code cleaner and more easy to follow.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/fastmap.c | 31 +--
drivers/mtd/ubi/wl.c | 11 +++
2 files changed, 12 insertions(+), 30 deletions(-)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index db3defd..9507702 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
}
new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
Not related to this patch, but looking at this function it got me
thinking: why do we need to re-calculate new_fm->used_blocks (and check
calculated value) each time? fm_size doesn't changed at runtime.
leb_size sure does not, so fm->used_blocks can be ubi device parameter
and calculated & tested only once, and not each time we write fastmap.
Correct?
-
- for (i = 0; i < new_fm->used_blocks; i++) {
- new_fm->e[i] = kmem_cache_alloc(ubi_wl_entry_slab, GFP_KERNEL);
- if (!new_fm->e[i]) {
- while (i--)
- kfree(new_fm->e[i]);
-
- kfree(new_fm);
- mutex_unlock(>fm_mutex);
- return -ENOMEM;
- }
- }
-
old_fm = ubi->fm;
ubi->fm = NULL;
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 2/6] UBI: Fastmap: Don't allocate new ubi_wl_entry objects
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
There is no need to allocate new ones every time, we can reuse
the existing ones.
This makes the code cleaner and more easy to follow.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/fastmap.c | 31 +--
drivers/mtd/ubi/wl.c | 11 +++
2 files changed, 12 insertions(+), 30 deletions(-)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index db3defd..9507702 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
}
new_fm-used_blocks = ubi-fm_size / ubi-leb_size;
Not related to this patch, but looking at this function it got me
thinking: why do we need to re-calculate new_fm-used_blocks (and check
calculated value) each time? fm_size doesn't changed at runtime.
leb_size sure does not, so fm-used_blocks can be ubi device parameter
and calculated tested only once, and not each time we write fastmap.
Correct?
-
- for (i = 0; i new_fm-used_blocks; i++) {
- new_fm-e[i] = kmem_cache_alloc(ubi_wl_entry_slab, GFP_KERNEL);
- if (!new_fm-e[i]) {
- while (i--)
- kfree(new_fm-e[i]);
-
- kfree(new_fm);
- mutex_unlock(ubi-fm_mutex);
- return -ENOMEM;
- }
- }
-
old_fm = ubi-fm;
ubi-fm = NULL;
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 2/6] UBI: Fastmap: Don't allocate new ubi_wl_entry objects
On 12/8/2014 11:14 AM, Richard Weinberger wrote: Am 08.12.2014 um 09:36 schrieb Tanya Brokhman: On 11/30/2014 1:35 PM, Richard Weinberger wrote: There is no need to allocate new ones every time, we can reuse the existing ones. This makes the code cleaner and more easy to follow. Signed-off-by: Richard Weinberger rich...@nod.at --- drivers/mtd/ubi/fastmap.c | 31 +-- drivers/mtd/ubi/wl.c | 11 +++ 2 files changed, 12 insertions(+), 30 deletions(-) diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c index db3defd..9507702 100644 --- a/drivers/mtd/ubi/fastmap.c +++ b/drivers/mtd/ubi/fastmap.c @@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi) } new_fm-used_blocks = ubi-fm_size / ubi-leb_size; Not related to this patch, but looking at this function it got me thinking: why do we need to re-calculate new_fm-used_blocks (and check calculated value) each time? fm_size doesn't changed at runtime. leb_size sure does not, so fm-used_blocks can be ubi device parameter and calculated tested only once, and not each time we write fastmap. Correct? If you dig deeper into the patch set you'll notice that the fastmap size can change. :-) oh, ok. sorry, still in the middle of the review. Thanks, //richard Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 3/6] UBI: Fastmap: Notify user in case of an ubi_update_fastmap() failure
On 12/8/2014 11:11 AM, Richard Weinberger wrote:
Hi!
Am 08.12.2014 um 07:58 schrieb Tanya Brokhman:
On 12/7/2014 4:22 PM, Richard Weinberger wrote:
Am 07.12.2014 um 14:59 schrieb Tanya Brokhman:
Hi Richard,
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
If ubi_update_fastmap() fails notify the user.
This is not a hard error as ubi_update_fastmap() makes sure that upon failure
the current on-flash fastmap will no be used upon next UBI attach.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 523d8a4..7821342 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -657,7 +657,11 @@ again:
* refill the WL pool synchronous. */
if (pool-used == pool-size || wl_pool-used == wl_pool-size) {
spin_unlock(ubi-wl_lock);
-ubi_update_fastmap(ubi);
+ret = ubi_update_fastmap(ubi);
+if (ret) {
+ubi_msg(ubi, Unable to write a new fastmap: %i, ret);
+return -ENOSPC;
Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed?
Its possible that the fm_pools were refilled correctly, and the actual
fastmap_write failed, so there
is nothing preventing the user to get peb allocated and continue working. You
invalidate the fastmap, so if powercut occurs a full scan will be performed. So
its possible to
allocate from fm_pools (although fastmap is not valid on disc) and try writing
fastmap again when the pools filled up.
I'm for the ubi_msg but against return -ENOSPC;
Maybe the case you've described is powercut safe, but there can be other unsafe
cases.
Let's stay on the safe side and be paranoid, it does not hurt.
If fastmap has proven stable we can start with tricky optimizations.
I'm sorry that I'm being petty here but the commit msg states that you notify the
user in case of update fastamap failure. It says nothing about you failing
ubi_wl_get_peb as
well. And this is a major change. At least divide this into 2 patches (so I can
disagree to the function failing and agree to the msg to user :) )
With user I meant users of that function.
I still don't like it.
Leaving this one for Artem... sorry
Thanks,
//richard
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 3/6] UBI: Fastmap: Notify user in case of an ubi_update_fastmap() failure
On 12/7/2014 4:22 PM, Richard Weinberger wrote:
Am 07.12.2014 um 14:59 schrieb Tanya Brokhman:
Hi Richard,
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
If ubi_update_fastmap() fails notify the user.
This is not a hard error as ubi_update_fastmap() makes sure that upon failure
the current on-flash fastmap will no be used upon next UBI attach.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 523d8a4..7821342 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -657,7 +657,11 @@ again:
* refill the WL pool synchronous. */
if (pool->used == pool->size || wl_pool->used == wl_pool->size) {
spin_unlock(>wl_lock);
-ubi_update_fastmap(ubi);
+ret = ubi_update_fastmap(ubi);
+if (ret) {
+ubi_msg(ubi, "Unable to write a new fastmap: %i", ret);
+return -ENOSPC;
Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed?
Its possible that the fm_pools were refilled correctly, and the actual
fastmap_write failed, so there
is nothing preventing the user to get peb allocated and continue working. You
invalidate the fastmap, so if powercut occurs a full scan will be performed. So
its possible to
allocate from fm_pools (although fastmap is not valid on disc) and try writing
fastmap again when the pools filled up.
I'm for the ubi_msg but against "return -ENOSPC;"
Maybe the case you've described is powercut safe, but there can be other unsafe
cases.
Let's stay on the safe side and be paranoid, it does not hurt.
If fastmap has proven stable we can start with tricky optimizations.
I'm sorry that I'm being petty here but the commit msg states that you
"notify the user in case of update fastamap failure". It says nothing
about you failing ubi_wl_get_peb as well. And this is a major change. At
least divide this into 2 patches (so I can disagree to the function
failing and agree to the msg to user :) )
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 5/6] UBI: Fastmap: Fix fastmap usage in ubi_volume_notify()
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
There is no need to switch to ro mode if ubi_update_fastmap() fails.
Also get rid of the ifdef.
Signed-off-by: Richard Weinberger
Reviewed-by: Tanya Brokhman
---
drivers/mtd/ubi/build.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 3405be4..3152331 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -154,23 +154,22 @@ static struct device_attribute dev_mtd_num =
*/
int ubi_volume_notify(struct ubi_device *ubi, struct ubi_volume *vol, int
ntype)
{
+ int ret;
struct ubi_notification nt;
ubi_do_get_device_info(ubi, );
ubi_do_get_volume_info(ubi, vol, );
-#ifdef CONFIG_MTD_UBI_FASTMAP
switch (ntype) {
case UBI_VOLUME_ADDED:
case UBI_VOLUME_REMOVED:
case UBI_VOLUME_RESIZED:
case UBI_VOLUME_RENAMED:
- if (ubi_update_fastmap(ubi)) {
- ubi_err(ubi, "Unable to update fastmap!");
- ubi_ro_mode(ubi);
- }
+ ret = ubi_update_fastmap(ubi);
+ if (ret)
+ ubi_msg(ubi, "Unable to write a new fastmap: %i", ret);
}
-#endif
+
return blocking_notifier_call_chain(_notifiers, ntype, );
}
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 4/6] UBI: Fastmap: Wrap fastmap specific function in a ifdef
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
...such that we can implement NOP variants of some functions.
This will help to reduce fastmap specific ifdefs in other c files.
Signed-off-by: Richard Weinberger
Reviewed-by: Tanya Brokhman
---
drivers/mtd/ubi/ubi.h | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index d672412..6fadf34 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -864,10 +864,14 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
int pnum, const struct ubi_vid_hdr *vid_hdr);
/* fastmap.c */
+#ifdef CONFIG_MTD_UBI_FASTMAP
size_t ubi_calc_fm_size(struct ubi_device *ubi);
int ubi_update_fastmap(struct ubi_device *ubi);
int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
int fm_anchor);
+#else
+static inline int ubi_update_fastmap(struct ubi_device *ubi) { return 0; }
+#endif
/* block.c */
#ifdef CONFIG_MTD_UBI_BLOCK
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 3/6] UBI: Fastmap: Notify user in case of an ubi_update_fastmap() failure
Hi Richard,
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
If ubi_update_fastmap() fails notify the user.
This is not a hard error as ubi_update_fastmap() makes sure that upon failure
the current on-flash fastmap will no be used upon next UBI attach.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 523d8a4..7821342 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -657,7 +657,11 @@ again:
* refill the WL pool synchronous. */
if (pool->used == pool->size || wl_pool->used == wl_pool->size) {
spin_unlock(>wl_lock);
- ubi_update_fastmap(ubi);
+ ret = ubi_update_fastmap(ubi);
+ if (ret) {
+ ubi_msg(ubi, "Unable to write a new fastmap: %i", ret);
+ return -ENOSPC;
Why do you fail the whole function (ubi_wl_get_peb) if fastmap update
failed? Its possible that the fm_pools were refilled correctly, and the
actual fastmap_write failed, so there is nothing preventing the user to
get peb allocated and continue working. You invalidate the fastmap, so
if powercut occurs a full scan will be performed. So its possible to
allocate from fm_pools (although fastmap is not valid on disc) and try
writing fastmap again when the pools filled up.
I'm for the ubi_msg but against "return -ENOSPC;"
+ }
spin_lock(>wl_lock);
}
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 2/6] UBI: Fastmap: Don't allocate new ubi_wl_entry objects
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
There is no need to allocate new ones every time, we can reuse
the existing ones.
This makes the code cleaner and more easy to follow.
Signed-off-by: Richard Weinberger
Reviewed-by: Tanya Brokhman
---
drivers/mtd/ubi/fastmap.c | 31 +--
drivers/mtd/ubi/wl.c | 11 +++
2 files changed, 12 insertions(+), 30 deletions(-)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index db3defd..9507702 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
}
new_fm->used_blocks = ubi->fm_size / ubi->leb_size;
-
- for (i = 0; i < new_fm->used_blocks; i++) {
- new_fm->e[i] = kmem_cache_alloc(ubi_wl_entry_slab, GFP_KERNEL);
- if (!new_fm->e[i]) {
- while (i--)
- kfree(new_fm->e[i]);
-
- kfree(new_fm);
- mutex_unlock(>fm_mutex);
- return -ENOMEM;
- }
- }
-
old_fm = ubi->fm;
ubi->fm = NULL;
@@ -1494,12 +1481,9 @@ int ubi_update_fastmap(struct ubi_device *ubi)
ubi_err(ubi, "could not erase old fastmap PEB");
goto err;
}
-
- new_fm->e[i]->pnum = old_fm->e[i]->pnum;
- new_fm->e[i]->ec = old_fm->e[i]->ec;
+ new_fm->e[i] = old_fm->e[i];
} else {
- new_fm->e[i]->pnum = tmp_e->pnum;
- new_fm->e[i]->ec = tmp_e->ec;
+ new_fm->e[i] = tmp_e;
if (old_fm)
ubi_wl_put_fm_peb(ubi, old_fm->e[i], i,
@@ -1524,16 +1508,13 @@ int ubi_update_fastmap(struct ubi_device *ubi)
i, 0);
goto err;
}
-
- new_fm->e[0]->pnum = old_fm->e[0]->pnum;
+ new_fm->e[0] = old_fm->e[0];
new_fm->e[0]->ec = ret;
} else {
/* we've got a new anchor PEB, return the old one */
ubi_wl_put_fm_peb(ubi, old_fm->e[0], 0,
old_fm->to_be_tortured[0]);
-
- new_fm->e[0]->pnum = tmp_e->pnum;
- new_fm->e[0]->ec = tmp_e->ec;
+ new_fm->e[0] = tmp_e;
}
} else {
if (!tmp_e) {
@@ -1546,9 +1527,7 @@ int ubi_update_fastmap(struct ubi_device *ubi)
ret = -ENOSPC;
goto err;
}
-
- new_fm->e[0]->pnum = tmp_e->pnum;
- new_fm->e[0]->ec = tmp_e->ec;
+ new_fm->e[0] = tmp_e;
}
down_write(>work_sem);
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 47b215f..523d8a4 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1014,9 +1014,6 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct
ubi_wl_entry *fm_e,
e = fm_e;
ubi_assert(e->ec >= 0);
ubi->lookuptbl[pnum] = e;
- } else {
- e->ec = fm_e->ec;
- kfree(fm_e);
}
spin_unlock(>wl_lock);
@@ -2008,9 +2005,15 @@ int ubi_wl_init(struct ubi_device *ubi, struct
ubi_attach_info *ai)
dbg_wl("found %i PEBs", found_pebs);
- if (ubi->fm)
+ if (ubi->fm) {
ubi_assert(ubi->good_peb_count == \
found_pebs + ubi->fm->used_blocks);
+
+ for (i = 0; i < ubi->fm->used_blocks; i++) {
+ e = ubi->fm->e[i];
+ ubi->lookuptbl[e->pnum] = e;
+ }
+ }
else
ubi_assert(ubi->good_peb_count == found_pebs);
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 1/6] UBI: Fastmap: Fix memory leaks while closing the WL sub-system
On 12/7/2014 1:34 PM, Richard Weinberger wrote:
Am 07.12.2014 um 12:32 schrieb Tanya Brokhman:
On 12/7/2014 11:54 AM, Richard Weinberger wrote:
Am 07.12.2014 um 09:13 schrieb Tanya Brokhman:
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
Add a ubi_fastmap_close() to free all resources used by fastmap
at WL shutdown.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c2822f7..47b215f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
}
}
+static void ubi_fastmap_close(struct ubi_device *ubi)
+{
+#ifdef CONFIG_MTD_UBI_FASTMAP
+int i;
+
+flush_work(>fm_work);
+return_unused_pool_pebs(ubi, >fm_pool);
+return_unused_pool_pebs(ubi, >fm_wl_pool);
+
+if (ubi->fm) {
+for (i = 0; i < ubi->fm->used_blocks; i++)
+kfree(ubi->fm->e[i]);
+}
+kfree(ubi->fm);
kfree(ubi->fm_buf)?
No this is not a typo, I kfree() here ubi->fm by design.
What am I missing? :)
I think you missed freeing ubi->fm_buf, before (not instead) you free ubi->fm :)
No. fm_buf is vfree()d upon detach time.
you're right, found it at ubi_detach_mtd_dev() several lines after
calling ubi_wl_close(). But if you're creating a fastmap-close dedicated
function, fm_buf should be freed in it as it is fastmap related.
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 1/6] UBI: Fastmap: Fix memory leaks while closing the WL sub-system
On 12/7/2014 11:54 AM, Richard Weinberger wrote:
Am 07.12.2014 um 09:13 schrieb Tanya Brokhman:
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
Add a ubi_fastmap_close() to free all resources used by fastmap
at WL shutdown.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c2822f7..47b215f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
}
}
+static void ubi_fastmap_close(struct ubi_device *ubi)
+{
+#ifdef CONFIG_MTD_UBI_FASTMAP
+int i;
+
+flush_work(>fm_work);
+return_unused_pool_pebs(ubi, >fm_pool);
+return_unused_pool_pebs(ubi, >fm_wl_pool);
+
+if (ubi->fm) {
+for (i = 0; i < ubi->fm->used_blocks; i++)
+kfree(ubi->fm->e[i]);
+}
+kfree(ubi->fm);
kfree(ubi->fm_buf)?
No this is not a typo, I kfree() here ubi->fm by design.
What am I missing? :)
I think you missed freeing ubi->fm_buf, before (not instead) you free
ubi->fm :)
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 1/6] UBI: Fastmap: Fix memory leaks while closing the WL sub-system
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
Add a ubi_fastmap_close() to free all resources used by fastmap
at WL shutdown.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c2822f7..47b215f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
}
}
+static void ubi_fastmap_close(struct ubi_device *ubi)
+{
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ int i;
+
+ flush_work(>fm_work);
+ return_unused_pool_pebs(ubi, >fm_pool);
+ return_unused_pool_pebs(ubi, >fm_wl_pool);
+
+ if (ubi->fm) {
+ for (i = 0; i < ubi->fm->used_blocks; i++)
+ kfree(ubi->fm->e[i]);
+ }
+ kfree(ubi->fm);
kfree(ubi->fm_buf)?
+#endif
+}
+
/**
* ubi_wl_close - close the wear-leveling sub-system.
* @ubi: UBI device description object
@@ -2071,9 +2088,7 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl("close the WL sub-system");
-#ifdef CONFIG_MTD_UBI_FASTMAP
- flush_work(>fm_work);
-#endif
+ ubi_fastmap_close(ubi);
shutdown_work(ubi);
protection_queue_destroy(ubi);
tree_destroy(>used);
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 1/6] UBI: Fastmap: Fix memory leaks while closing the WL sub-system
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
Add a ubi_fastmap_close() to free all resources used by fastmap
at WL shutdown.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c2822f7..47b215f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
}
}
+static void ubi_fastmap_close(struct ubi_device *ubi)
+{
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ int i;
+
+ flush_work(ubi-fm_work);
+ return_unused_pool_pebs(ubi, ubi-fm_pool);
+ return_unused_pool_pebs(ubi, ubi-fm_wl_pool);
+
+ if (ubi-fm) {
+ for (i = 0; i ubi-fm-used_blocks; i++)
+ kfree(ubi-fm-e[i]);
+ }
+ kfree(ubi-fm);
kfree(ubi-fm_buf)?
+#endif
+}
+
/**
* ubi_wl_close - close the wear-leveling sub-system.
* @ubi: UBI device description object
@@ -2071,9 +2088,7 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl(close the WL sub-system);
-#ifdef CONFIG_MTD_UBI_FASTMAP
- flush_work(ubi-fm_work);
-#endif
+ ubi_fastmap_close(ubi);
shutdown_work(ubi);
protection_queue_destroy(ubi);
tree_destroy(ubi-used);
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 1/6] UBI: Fastmap: Fix memory leaks while closing the WL sub-system
On 12/7/2014 11:54 AM, Richard Weinberger wrote:
Am 07.12.2014 um 09:13 schrieb Tanya Brokhman:
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
Add a ubi_fastmap_close() to free all resources used by fastmap
at WL shutdown.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c2822f7..47b215f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
}
}
+static void ubi_fastmap_close(struct ubi_device *ubi)
+{
+#ifdef CONFIG_MTD_UBI_FASTMAP
+int i;
+
+flush_work(ubi-fm_work);
+return_unused_pool_pebs(ubi, ubi-fm_pool);
+return_unused_pool_pebs(ubi, ubi-fm_wl_pool);
+
+if (ubi-fm) {
+for (i = 0; i ubi-fm-used_blocks; i++)
+kfree(ubi-fm-e[i]);
+}
+kfree(ubi-fm);
kfree(ubi-fm_buf)?
No this is not a typo, I kfree() here ubi-fm by design.
What am I missing? :)
I think you missed freeing ubi-fm_buf, before (not instead) you free
ubi-fm :)
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 1/6] UBI: Fastmap: Fix memory leaks while closing the WL sub-system
On 12/7/2014 1:34 PM, Richard Weinberger wrote:
Am 07.12.2014 um 12:32 schrieb Tanya Brokhman:
On 12/7/2014 11:54 AM, Richard Weinberger wrote:
Am 07.12.2014 um 09:13 schrieb Tanya Brokhman:
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
Add a ubi_fastmap_close() to free all resources used by fastmap
at WL shutdown.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c2822f7..47b215f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2064,6 +2064,23 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
}
}
+static void ubi_fastmap_close(struct ubi_device *ubi)
+{
+#ifdef CONFIG_MTD_UBI_FASTMAP
+int i;
+
+flush_work(ubi-fm_work);
+return_unused_pool_pebs(ubi, ubi-fm_pool);
+return_unused_pool_pebs(ubi, ubi-fm_wl_pool);
+
+if (ubi-fm) {
+for (i = 0; i ubi-fm-used_blocks; i++)
+kfree(ubi-fm-e[i]);
+}
+kfree(ubi-fm);
kfree(ubi-fm_buf)?
No this is not a typo, I kfree() here ubi-fm by design.
What am I missing? :)
I think you missed freeing ubi-fm_buf, before (not instead) you free ubi-fm :)
No. fm_buf is vfree()d upon detach time.
you're right, found it at ubi_detach_mtd_dev() several lines after
calling ubi_wl_close(). But if you're creating a fastmap-close dedicated
function, fm_buf should be freed in it as it is fastmap related.
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 2/6] UBI: Fastmap: Don't allocate new ubi_wl_entry objects
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
There is no need to allocate new ones every time, we can reuse
the existing ones.
This makes the code cleaner and more easy to follow.
Signed-off-by: Richard Weinberger rich...@nod.at
Reviewed-by: Tanya Brokhman tlin...@codeaurora.org
---
drivers/mtd/ubi/fastmap.c | 31 +--
drivers/mtd/ubi/wl.c | 11 +++
2 files changed, 12 insertions(+), 30 deletions(-)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index db3defd..9507702 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1446,19 +1446,6 @@ int ubi_update_fastmap(struct ubi_device *ubi)
}
new_fm-used_blocks = ubi-fm_size / ubi-leb_size;
-
- for (i = 0; i new_fm-used_blocks; i++) {
- new_fm-e[i] = kmem_cache_alloc(ubi_wl_entry_slab, GFP_KERNEL);
- if (!new_fm-e[i]) {
- while (i--)
- kfree(new_fm-e[i]);
-
- kfree(new_fm);
- mutex_unlock(ubi-fm_mutex);
- return -ENOMEM;
- }
- }
-
old_fm = ubi-fm;
ubi-fm = NULL;
@@ -1494,12 +1481,9 @@ int ubi_update_fastmap(struct ubi_device *ubi)
ubi_err(ubi, could not erase old fastmap PEB);
goto err;
}
-
- new_fm-e[i]-pnum = old_fm-e[i]-pnum;
- new_fm-e[i]-ec = old_fm-e[i]-ec;
+ new_fm-e[i] = old_fm-e[i];
} else {
- new_fm-e[i]-pnum = tmp_e-pnum;
- new_fm-e[i]-ec = tmp_e-ec;
+ new_fm-e[i] = tmp_e;
if (old_fm)
ubi_wl_put_fm_peb(ubi, old_fm-e[i], i,
@@ -1524,16 +1508,13 @@ int ubi_update_fastmap(struct ubi_device *ubi)
i, 0);
goto err;
}
-
- new_fm-e[0]-pnum = old_fm-e[0]-pnum;
+ new_fm-e[0] = old_fm-e[0];
new_fm-e[0]-ec = ret;
} else {
/* we've got a new anchor PEB, return the old one */
ubi_wl_put_fm_peb(ubi, old_fm-e[0], 0,
old_fm-to_be_tortured[0]);
-
- new_fm-e[0]-pnum = tmp_e-pnum;
- new_fm-e[0]-ec = tmp_e-ec;
+ new_fm-e[0] = tmp_e;
}
} else {
if (!tmp_e) {
@@ -1546,9 +1527,7 @@ int ubi_update_fastmap(struct ubi_device *ubi)
ret = -ENOSPC;
goto err;
}
-
- new_fm-e[0]-pnum = tmp_e-pnum;
- new_fm-e[0]-ec = tmp_e-ec;
+ new_fm-e[0] = tmp_e;
}
down_write(ubi-work_sem);
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 47b215f..523d8a4 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1014,9 +1014,6 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct
ubi_wl_entry *fm_e,
e = fm_e;
ubi_assert(e-ec = 0);
ubi-lookuptbl[pnum] = e;
- } else {
- e-ec = fm_e-ec;
- kfree(fm_e);
}
spin_unlock(ubi-wl_lock);
@@ -2008,9 +2005,15 @@ int ubi_wl_init(struct ubi_device *ubi, struct
ubi_attach_info *ai)
dbg_wl(found %i PEBs, found_pebs);
- if (ubi-fm)
+ if (ubi-fm) {
ubi_assert(ubi-good_peb_count == \
found_pebs + ubi-fm-used_blocks);
+
+ for (i = 0; i ubi-fm-used_blocks; i++) {
+ e = ubi-fm-e[i];
+ ubi-lookuptbl[e-pnum] = e;
+ }
+ }
else
ubi_assert(ubi-good_peb_count == found_pebs);
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 3/6] UBI: Fastmap: Notify user in case of an ubi_update_fastmap() failure
Hi Richard,
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
If ubi_update_fastmap() fails notify the user.
This is not a hard error as ubi_update_fastmap() makes sure that upon failure
the current on-flash fastmap will no be used upon next UBI attach.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 523d8a4..7821342 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -657,7 +657,11 @@ again:
* refill the WL pool synchronous. */
if (pool-used == pool-size || wl_pool-used == wl_pool-size) {
spin_unlock(ubi-wl_lock);
- ubi_update_fastmap(ubi);
+ ret = ubi_update_fastmap(ubi);
+ if (ret) {
+ ubi_msg(ubi, Unable to write a new fastmap: %i, ret);
+ return -ENOSPC;
Why do you fail the whole function (ubi_wl_get_peb) if fastmap update
failed? Its possible that the fm_pools were refilled correctly, and the
actual fastmap_write failed, so there is nothing preventing the user to
get peb allocated and continue working. You invalidate the fastmap, so
if powercut occurs a full scan will be performed. So its possible to
allocate from fm_pools (although fastmap is not valid on disc) and try
writing fastmap again when the pools filled up.
I'm for the ubi_msg but against return -ENOSPC;
+ }
spin_lock(ubi-wl_lock);
}
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 4/6] UBI: Fastmap: Wrap fastmap specific function in a ifdef
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
...such that we can implement NOP variants of some functions.
This will help to reduce fastmap specific ifdefs in other c files.
Signed-off-by: Richard Weinberger rich...@nod.at
Reviewed-by: Tanya Brokhman tlin...@codeaurora.org
---
drivers/mtd/ubi/ubi.h | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index d672412..6fadf34 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -864,10 +864,14 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
int pnum, const struct ubi_vid_hdr *vid_hdr);
/* fastmap.c */
+#ifdef CONFIG_MTD_UBI_FASTMAP
size_t ubi_calc_fm_size(struct ubi_device *ubi);
int ubi_update_fastmap(struct ubi_device *ubi);
int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
int fm_anchor);
+#else
+static inline int ubi_update_fastmap(struct ubi_device *ubi) { return 0; }
+#endif
/* block.c */
#ifdef CONFIG_MTD_UBI_BLOCK
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 5/6] UBI: Fastmap: Fix fastmap usage in ubi_volume_notify()
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
There is no need to switch to ro mode if ubi_update_fastmap() fails.
Also get rid of the ifdef.
Signed-off-by: Richard Weinberger rich...@nod.at
Reviewed-by: Tanya Brokhman tlin...@codeaurora.org
---
drivers/mtd/ubi/build.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 3405be4..3152331 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -154,23 +154,22 @@ static struct device_attribute dev_mtd_num =
*/
int ubi_volume_notify(struct ubi_device *ubi, struct ubi_volume *vol, int
ntype)
{
+ int ret;
struct ubi_notification nt;
ubi_do_get_device_info(ubi, nt.di);
ubi_do_get_volume_info(ubi, vol, nt.vi);
-#ifdef CONFIG_MTD_UBI_FASTMAP
switch (ntype) {
case UBI_VOLUME_ADDED:
case UBI_VOLUME_REMOVED:
case UBI_VOLUME_RESIZED:
case UBI_VOLUME_RENAMED:
- if (ubi_update_fastmap(ubi)) {
- ubi_err(ubi, Unable to update fastmap!);
- ubi_ro_mode(ubi);
- }
+ ret = ubi_update_fastmap(ubi);
+ if (ret)
+ ubi_msg(ubi, Unable to write a new fastmap: %i, ret);
}
-#endif
+
return blocking_notifier_call_chain(ubi_notifiers, ntype, nt);
}
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 3/6] UBI: Fastmap: Notify user in case of an ubi_update_fastmap() failure
On 12/7/2014 4:22 PM, Richard Weinberger wrote:
Am 07.12.2014 um 14:59 schrieb Tanya Brokhman:
Hi Richard,
On 11/30/2014 1:35 PM, Richard Weinberger wrote:
If ubi_update_fastmap() fails notify the user.
This is not a hard error as ubi_update_fastmap() makes sure that upon failure
the current on-flash fastmap will no be used upon next UBI attach.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 523d8a4..7821342 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -657,7 +657,11 @@ again:
* refill the WL pool synchronous. */
if (pool-used == pool-size || wl_pool-used == wl_pool-size) {
spin_unlock(ubi-wl_lock);
-ubi_update_fastmap(ubi);
+ret = ubi_update_fastmap(ubi);
+if (ret) {
+ubi_msg(ubi, Unable to write a new fastmap: %i, ret);
+return -ENOSPC;
Why do you fail the whole function (ubi_wl_get_peb) if fastmap update failed?
Its possible that the fm_pools were refilled correctly, and the actual
fastmap_write failed, so there
is nothing preventing the user to get peb allocated and continue working. You
invalidate the fastmap, so if powercut occurs a full scan will be performed. So
its possible to
allocate from fm_pools (although fastmap is not valid on disc) and try writing
fastmap again when the pools filled up.
I'm for the ubi_msg but against return -ENOSPC;
Maybe the case you've described is powercut safe, but there can be other unsafe
cases.
Let's stay on the safe side and be paranoid, it does not hurt.
If fastmap has proven stable we can start with tricky optimizations.
I'm sorry that I'm being petty here but the commit msg states that you
notify the user in case of update fastamap failure. It says nothing
about you failing ubi_wl_get_peb as well. And this is a major change. At
least divide this into 2 patches (so I can disagree to the function
failing and agree to the msg to user :) )
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 6/6] UBI: Fastmap: Make ubi_refill_pools() fair
Hi Richard
On 12/5/2014 10:56 PM, Richard Weinberger wrote:
-/**
- * refill_wl_user_pool - refills all the fastmap pool used by ubi_wl_get_peb.
- * @ubi: UBI device description object
- */
-static void refill_wl_user_pool(struct ubi_device *ubi)
-{
-struct ubi_fm_pool *pool = >fm_pool;
+pool->pebs[pool->size] = e->pnum;
+pool->size++;
+} else
+enough++;
-return_unused_pool_pebs(ubi, pool);
+if (wl_pool->size < wl_pool->max_size) {
+if (!ubi->free.rb_node ||
+ (ubi->free_count - ubi->beb_rsvd_pebs < 5))
+break;
-for (pool->size = 0; pool->size < pool->max_size; pool->size++) {
-pool->pebs[pool->size] = __wl_get_peb(ubi);
-if (pool->pebs[pool->size] < 0)
+e = find_wl_entry(ubi, >free, WL_FREE_MAX_DIFF);
+self_check_in_wl_tree(ubi, e, >free);
+rb_erase(>u.rb, >free);
+ubi->free_count--;
why don't you use wl_get_peb() here?
Because wl_get_peb() is not equivalent to the above code.
We want a PEB to be used for wear-leveling not for "end users" like UBIFS.
sorry, my mistake. I meant wl_get_wle() (the new function). the only
diff between wl_get_wle() and the above is that you use find_wl_entry()
and wl_get_wle() uses find_mean_wl_entry() and takes the anchor into
consideration. So I;m trying to understand why wl_get_wle() isn't good here?
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 4/6] UBI: Fastmap: Fix races in ubi_wl_get_peb()
On 12/5/2014 11:08 PM, Richard Weinberger wrote:
spin_unlock(>wl_lock);
+if (retried) {
+ubi_err(ubi, "Unable to get a free PEB from user WL pool");
+ret = -ENOSPC;
+goto out;
+}
+retried = 1;
Why did you decide to retry in this function? and why only 1 retry attempt? I'm
not against it, trying to understand the logic.
Because failing immediately with -ENOSPC is not nice.
Why not? this is what was done before
The behavior from before was not good.
If we return here a -ENOSPC it is not because we ran out of free PEBs, it is
because the pool contains
no free PEBs and needs refilling.
As between refilling the pool and requesting a fresh PEB from it another thread could
"steal" all PEBs
we retry.
I think what I really bothers me in this case is that you don't sleep, you
branch immediately to retry again, so the chances that there will be context
switch and free pebs appear
aren't that high.
I'm used to functions using some sort of "retry" logic to sleep before retrying. Of
course sleeping isn't a good idea here. That's why the "retry" bugs me a bit.
You mean a cond_resched()?
This retry-logic is common pattern in UBI. For exmaple see ubi_wl_put_peb().
you're right. didn't pay much attention to ubi_wl_put_peb() before.
don't like it there either :)
perhaps we can rethink this later for both cases.
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 4/6] UBI: Fastmap: Fix races in ubi_wl_get_peb()
On 12/5/2014 11:08 PM, Richard Weinberger wrote:
spin_unlock(ubi-wl_lock);
+if (retried) {
+ubi_err(ubi, Unable to get a free PEB from user WL pool);
+ret = -ENOSPC;
+goto out;
+}
+retried = 1;
Why did you decide to retry in this function? and why only 1 retry attempt? I'm
not against it, trying to understand the logic.
Because failing immediately with -ENOSPC is not nice.
Why not? this is what was done before
The behavior from before was not good.
If we return here a -ENOSPC it is not because we ran out of free PEBs, it is
because the pool contains
no free PEBs and needs refilling.
As between refilling the pool and requesting a fresh PEB from it another thread could
steal all PEBs
we retry.
I think what I really bothers me in this case is that you don't sleep, you
branch immediately to retry again, so the chances that there will be context
switch and free pebs appear
aren't that high.
I'm used to functions using some sort of retry logic to sleep before retrying. Of
course sleeping isn't a good idea here. That's why the retry bugs me a bit.
You mean a cond_resched()?
This retry-logic is common pattern in UBI. For exmaple see ubi_wl_put_peb().
you're right. didn't pay much attention to ubi_wl_put_peb() before.
don't like it there either :)
perhaps we can rethink this later for both cases.
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 6/6] UBI: Fastmap: Make ubi_refill_pools() fair
Hi Richard
On 12/5/2014 10:56 PM, Richard Weinberger wrote:
-/**
- * refill_wl_user_pool - refills all the fastmap pool used by ubi_wl_get_peb.
- * @ubi: UBI device description object
- */
-static void refill_wl_user_pool(struct ubi_device *ubi)
-{
-struct ubi_fm_pool *pool = ubi-fm_pool;
+pool-pebs[pool-size] = e-pnum;
+pool-size++;
+} else
+enough++;
-return_unused_pool_pebs(ubi, pool);
+if (wl_pool-size wl_pool-max_size) {
+if (!ubi-free.rb_node ||
+ (ubi-free_count - ubi-beb_rsvd_pebs 5))
+break;
-for (pool-size = 0; pool-size pool-max_size; pool-size++) {
-pool-pebs[pool-size] = __wl_get_peb(ubi);
-if (pool-pebs[pool-size] 0)
+e = find_wl_entry(ubi, ubi-free, WL_FREE_MAX_DIFF);
+self_check_in_wl_tree(ubi, e, ubi-free);
+rb_erase(e-u.rb, ubi-free);
+ubi-free_count--;
why don't you use wl_get_peb() here?
Because wl_get_peb() is not equivalent to the above code.
We want a PEB to be used for wear-leveling not for end users like UBIFS.
sorry, my mistake. I meant wl_get_wle() (the new function). the only
diff between wl_get_wle() and the above is that you use find_wl_entry()
and wl_get_wle() uses find_mean_wl_entry() and takes the anchor into
consideration. So I;m trying to understand why wl_get_wle() isn't good here?
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 6/6] UBI: Fastmap: Make ubi_refill_pools() fair
Hi Richard,
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
Currently ubi_refill_pools() first fills the first and then
the second one.
If only very few free PEBs are available the second pool can get
zero PEBs.
Change ubi_refill_pools() to distribute free PEBs fair between
all pools.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 77 +++-
1 file changed, 40 insertions(+), 37 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index f028b68..c2822f7 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -583,59 +583,62 @@ static void return_unused_pool_pebs(struct ubi_device
*ubi,
}
/**
- * refill_wl_pool - refills all the fastmap pool used by the
- * WL sub-system.
+ * ubi_refill_pools - refills all fastmap PEB pools.
* @ubi: UBI device description object
*/
-static void refill_wl_pool(struct ubi_device *ubi)
+void ubi_refill_pools(struct ubi_device *ubi)
{
+ struct ubi_fm_pool *wl_pool = >fm_wl_pool;
+ struct ubi_fm_pool *pool = >fm_pool;
struct ubi_wl_entry *e;
- struct ubi_fm_pool *pool = >fm_wl_pool;
+ int enough;
+ spin_lock(>wl_lock);
+
+ return_unused_pool_pebs(ubi, wl_pool);
return_unused_pool_pebs(ubi, pool);
- for (pool->size = 0; pool->size < pool->max_size; pool->size++) {
- if (!ubi->free.rb_node ||
- (ubi->free_count - ubi->beb_rsvd_pebs < 5))
- break;
+ wl_pool->size = 0;
+ pool->size = 0;
- e = find_wl_entry(ubi, >free, WL_FREE_MAX_DIFF);
- self_check_in_wl_tree(ubi, e, >free);
- rb_erase(>u.rb, >free);
- ubi->free_count--;
+ for (;;) {
You loop for max(pool->max_size, wl_pool->max_size) itterations. IMO,
the code will be more clear if you use for(i=0; imax_size,
wl_pool->max_size); i++) instead of "int enough".
This is just coding style preference of course. I personally don't like
for(;;) that much Just a suggestion. :)
+ enough = 0;
+ if (pool->size < pool->max_size) {
+ if (!ubi->free.rb_node ||
+ (ubi->free_count - ubi->beb_rsvd_pebs < 5))
+ break;
- pool->pebs[pool->size] = e->pnum;
- }
- pool->used = 0;
-}
+ e = wl_get_wle(ubi);
+ if (!e)
+ break;
-/**
- * refill_wl_user_pool - refills all the fastmap pool used by ubi_wl_get_peb.
- * @ubi: UBI device description object
- */
-static void refill_wl_user_pool(struct ubi_device *ubi)
-{
- struct ubi_fm_pool *pool = >fm_pool;
+ pool->pebs[pool->size] = e->pnum;
+ pool->size++;
+ } else
+ enough++;
- return_unused_pool_pebs(ubi, pool);
+ if (wl_pool->size < wl_pool->max_size) {
+ if (!ubi->free.rb_node ||
+ (ubi->free_count - ubi->beb_rsvd_pebs < 5))
+ break;
- for (pool->size = 0; pool->size < pool->max_size; pool->size++) {
- pool->pebs[pool->size] = __wl_get_peb(ubi);
- if (pool->pebs[pool->size] < 0)
+ e = find_wl_entry(ubi, >free, WL_FREE_MAX_DIFF);
+ self_check_in_wl_tree(ubi, e, >free);
+ rb_erase(>u.rb, >free);
+ ubi->free_count--;
why don't you use wl_get_peb() here?
Other then that - I agree with the patch. So if you want to keep it as
is, I'll add Reviewed-by.
+
+ wl_pool->pebs[wl_pool->size] = e->pnum;
+ wl_pool->size++;
+ } else
+ enough++;
+
+ if (enough == 2)
break;
}
+
+ wl_pool->used = 0;
pool->used = 0;
-}
-/**
- * ubi_refill_pools - refills all fastmap PEB pools.
- * @ubi: UBI device description object
- */
-void ubi_refill_pools(struct ubi_device *ubi)
-{
- spin_lock(>wl_lock);
- refill_wl_pool(ubi);
- refill_wl_user_pool(ubi);
spin_unlock(>wl_lock);
}
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 5/6] UBI: Split __wl_get_peb()
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
Make it two functions, wl_get_wle() and wl_get_peb().
wl_get_peb() works exactly like __wl_get_peb() but wl_get_wle()
does not call produce_free_peb().
While refilling the fastmap user pool we cannot release ubi->wl_lock
as produce_free_peb() does.
Hence the fastmap logic uses now wl_get_wle().
hmmm... confused... I don't see fastmap code changed
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 61
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 7730b97..f028b68 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -499,13 +499,46 @@ out:
#endif
/**
- * __wl_get_peb - get a physical eraseblock.
+ * wl_get_wle - get a mean wl entry to be used by wl_get_peb() or
+ * refill_wl_user_pool().
+ * @ubi: UBI device description object
+ *
+ * This function returns a a wear leveling entry in case of success and
If you upload a new version, you have a double "a" here: "returns a a
wear leveling"
+ * NULL in case of failure.
+ */
+static struct ubi_wl_entry *wl_get_wle(struct ubi_device *ubi)
+{
+ struct ubi_wl_entry *e;
+
+ e = find_mean_wl_entry(ubi, >free);
+ if (!e) {
+ ubi_err(ubi, "no free eraseblocks");
+ return NULL;
+ }
+
+ self_check_in_wl_tree(ubi, e, >free);
+
+ /*
+* Move the physical eraseblock to the protection queue where it will
+* be protected from being moved for some time.
+*/
I don't think this comment is valid anymore
+ rb_erase(>u.rb, >free);
+ ubi->free_count--;
+ dbg_wl("PEB %d EC %d", e->pnum, e->ec);
+
+ return e;
+}
+
+/**
+ * wl_get_peb - get a physical eraseblock.
* @ubi: UBI device description object
*
* This function returns a physical eraseblock in case of success and a
* negative error code in case of failure.
+ * It is the low level component of ubi_wl_get_peb() in the non-fastmap
+ * case.
*/
-static int __wl_get_peb(struct ubi_device *ubi)
+static int wl_get_peb(struct ubi_device *ubi)
{
int err;
struct ubi_wl_entry *e;
@@ -524,27 +557,9 @@ retry:
goto retry;
}
- e = find_mean_wl_entry(ubi, >free);
- if (!e) {
- ubi_err(ubi, "no free eraseblocks");
- return -ENOSPC;
- }
-
- self_check_in_wl_tree(ubi, e, >free);
-
- /*
-* Move the physical eraseblock to the protection queue where it will
-* be protected from being moved for some time.
-*/
- rb_erase(>u.rb, >free);
- ubi->free_count--;
- dbg_wl("PEB %d EC %d", e->pnum, e->ec);
-#ifndef CONFIG_MTD_UBI_FASTMAP
- /* We have to enqueue e only if fastmap is disabled,
-* is fastmap enabled prot_queue_add() will be called by
-* ubi_wl_get_peb() after removing e from the pool. */
+ e = wl_get_wle(ubi);
prot_queue_add(ubi, e);
-#endif
+
return e->pnum;
}
@@ -704,7 +719,7 @@ int ubi_wl_get_peb(struct ubi_device *ubi)
int peb, err;
spin_lock(>wl_lock);
- peb = __wl_get_peb(ubi);
+ peb = wl_get_peb(ubi);
spin_unlock(>wl_lock);
if (peb < 0)
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 4/6] UBI: Fastmap: Fix races in ubi_wl_get_peb()
Hi Richard
On 12/5/2014 3:20 PM, Richard Weinberger wrote:
Tanya,
Am 05.12.2014 um 14:09 schrieb Tanya Brokhman:
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
ubi_wl_get_peb() has two problems, it reads the pool
size and usage counters without any protection.
While reading one value would be perfectly fine it reads multiple
values and compares them. This is racy and can lead to incorrect
pool handling.
Furthermore ubi_update_fastmap() is called without wl_lock held,
before incrementing the used counter it needs to be checked again.
I didn't see where you fixed the ubi_update_fastmap issue you just mentioned.
This is exactly what you're questioning below.
We have to recheck as the pool counter could have changed.
Oh, I understood the commit msg a bit differently, but now I see that it
was my mistake. thanks!
It could happen that another thread consumed all PEBs from the
pool and the counter goes beyond ->size.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/ubi.h | 3 ++-
drivers/mtd/ubi/wl.c | 34 +++---
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 04c4c05..d672412 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -439,7 +439,8 @@ struct ubi_debug_info {
* @pq_head: protection queue head
* @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl,
@move_from,
* @move_to, @move_to_put @erase_pending, @wl_scheduled, @works,
- * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields
+ * @erroneous, @erroneous_peb_count, @fm_work_scheduled, @fm_pool,
+ * and @fm_wl_pool fields
* @move_mutex: serializes eraseblock moves
* @work_sem: used to wait for all the scheduled works to finish and prevent
* new works from being submitted
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index cb2e571..7730b97 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -629,24 +629,36 @@ void ubi_refill_pools(struct ubi_device *ubi)
*/
int ubi_wl_get_peb(struct ubi_device *ubi)
{
-int ret;
+int ret, retried = 0;
struct ubi_fm_pool *pool = >fm_pool;
struct ubi_fm_pool *wl_pool = >fm_wl_pool;
-if (!pool->size || !wl_pool->size || pool->used == pool->size ||
-wl_pool->used == wl_pool->size)
+again:
+spin_lock(>wl_lock);
+/* We check here also for the WL pool because at this point we can
+ * refill the WL pool synchronous. */
+if (pool->used == pool->size || wl_pool->used == wl_pool->size) {
+spin_unlock(>wl_lock);
ubi_update_fastmap(ubi);
-
-/* we got not a single free PEB */
-if (!pool->size)
-ret = -ENOSPC;
-else {
spin_lock(>wl_lock);
-ret = pool->pebs[pool->used++];
-prot_queue_add(ubi, ubi->lookuptbl[ret]);
+}
+
+if (pool->used == pool->size) {
Im confused about this "if" condition. You just tested pool->used == pool->size in the previous
"if". If in the previous if pool->used != pool->size and wl_pool->used !=
wl_pool->size, you didn't enter, the lock is still held so pool->used != pool->size still. If
in the previos "if" wl_pool->used == wl_pool->size was true nd tou released the lock,
ubi_update_fastmap(ubi) was called, which refills the pools. So again, if pools were
refilled pool->used would be 0 here and pool->size > 0.
So in both cases I don't see how at this point pool->used == pool->size could
ever be true?
If we enter the "if (pool->used == pool->size || wl_pool->used == wl_pool->size)
{" branch we unlock wl_lock and call ubi_update_fastmap().
Another thread can enter ubi_wl_get_peb() and alter the pool counter. So we
have to recheck the counter after taking wl_lock again.
hmmm... ok. Perhaps a comment could be added in the code to explain this
case in a few words?
spin_unlock(>wl_lock);
+if (retried) {
+ubi_err(ubi, "Unable to get a free PEB from user WL pool");
+ret = -ENOSPC;
+goto out;
+}
+retried = 1;
Why did you decide to retry in this function? and why only 1 retry attempt? I'm
not against it, trying to understand the logic.
Because failing immediately with -ENOSPC is not nice.
Why not? this is what was done before
I think what I really bothers me in this case is that you don't sleep,
you branch immediately to retry again, so the chances that there will be
context switch and free pebs appear aren't that high.
I'm used to functions using some sort of "retry" logic to sleep before
retrying. Of course sleeping isn't a good idea here. That's why the
"retry" bugs me a bit.
Before we do that I'll give UBI a second chance to produce a free PEB.
Re: [PATCH 4/6] UBI: Fastmap: Fix races in ubi_wl_get_peb()
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
ubi_wl_get_peb() has two problems, it reads the pool
size and usage counters without any protection.
While reading one value would be perfectly fine it reads multiple
values and compares them. This is racy and can lead to incorrect
pool handling.
Furthermore ubi_update_fastmap() is called without wl_lock held,
before incrementing the used counter it needs to be checked again.
I didn't see where you fixed the ubi_update_fastmap issue you just
mentioned.
It could happen that another thread consumed all PEBs from the
pool and the counter goes beyond ->size.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/ubi.h | 3 ++-
drivers/mtd/ubi/wl.c | 34 +++---
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 04c4c05..d672412 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -439,7 +439,8 @@ struct ubi_debug_info {
* @pq_head: protection queue head
* @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from,
* @move_to, @move_to_put @erase_pending, @wl_scheduled, @works,
- * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields
+ * @erroneous, @erroneous_peb_count, @fm_work_scheduled, @fm_pool,
+ * and @fm_wl_pool fields
* @move_mutex: serializes eraseblock moves
* @work_sem: used to wait for all the scheduled works to finish and prevent
* new works from being submitted
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index cb2e571..7730b97 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -629,24 +629,36 @@ void ubi_refill_pools(struct ubi_device *ubi)
*/
int ubi_wl_get_peb(struct ubi_device *ubi)
{
- int ret;
+ int ret, retried = 0;
struct ubi_fm_pool *pool = >fm_pool;
struct ubi_fm_pool *wl_pool = >fm_wl_pool;
- if (!pool->size || !wl_pool->size || pool->used == pool->size ||
- wl_pool->used == wl_pool->size)
+again:
+ spin_lock(>wl_lock);
+ /* We check here also for the WL pool because at this point we can
+* refill the WL pool synchronous. */
+ if (pool->used == pool->size || wl_pool->used == wl_pool->size) {
+ spin_unlock(>wl_lock);
ubi_update_fastmap(ubi);
-
- /* we got not a single free PEB */
- if (!pool->size)
- ret = -ENOSPC;
- else {
spin_lock(>wl_lock);
- ret = pool->pebs[pool->used++];
- prot_queue_add(ubi, ubi->lookuptbl[ret]);
+ }
+
+ if (pool->used == pool->size) {
Im confused about this "if" condition. You just tested pool->used ==
pool->size in the previous "if". If in the previous if pool->used !=
pool->size and wl_pool->used != wl_pool->size, you didn't enter, the
lock is still held so pool->used != pool->size still. If in the previos
"if" wl_pool->used == wl_pool->size was true nd tou released the lock,
ubi_update_fastmap(ubi) was called, which refills the pools. So again,
if pools were refilled pool->used would be 0 here and pool->size > 0.
So in both cases I don't see how at this point pool->used == pool->size
could ever be true?
spin_unlock(>wl_lock);
+ if (retried) {
+ ubi_err(ubi, "Unable to get a free PEB from user WL
pool");
+ ret = -ENOSPC;
+ goto out;
+ }
+ retried = 1;
Why did you decide to retry in this function? and why only 1 retry
attempt? I'm not against it, trying to understand the logic.
+ goto again;
}
+ ubi_assert(pool->used < pool->size);
+ ret = pool->pebs[pool->used++];
+ prot_queue_add(ubi, ubi->lookuptbl[ret]);
+ spin_unlock(>wl_lock);
+out:
return ret;
}
@@ -659,7 +671,7 @@ static struct ubi_wl_entry *get_peb_for_wl(struct
ubi_device *ubi)
struct ubi_fm_pool *pool = >fm_wl_pool;
int pnum;
- if (pool->used == pool->size || !pool->size) {
+ if (pool->used == pool->size) {
/* We cannot update the fastmap here because this
* function is called in atomic context.
* Let's fail here and refill/update it as soon as possible. */
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 4/6] UBI: Fastmap: Fix races in ubi_wl_get_peb()
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
ubi_wl_get_peb() has two problems, it reads the pool
size and usage counters without any protection.
While reading one value would be perfectly fine it reads multiple
values and compares them. This is racy and can lead to incorrect
pool handling.
Furthermore ubi_update_fastmap() is called without wl_lock held,
before incrementing the used counter it needs to be checked again.
I didn't see where you fixed the ubi_update_fastmap issue you just
mentioned.
It could happen that another thread consumed all PEBs from the
pool and the counter goes beyond -size.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/ubi.h | 3 ++-
drivers/mtd/ubi/wl.c | 34 +++---
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 04c4c05..d672412 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -439,7 +439,8 @@ struct ubi_debug_info {
* @pq_head: protection queue head
* @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from,
* @move_to, @move_to_put @erase_pending, @wl_scheduled, @works,
- * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields
+ * @erroneous, @erroneous_peb_count, @fm_work_scheduled, @fm_pool,
+ * and @fm_wl_pool fields
* @move_mutex: serializes eraseblock moves
* @work_sem: used to wait for all the scheduled works to finish and prevent
* new works from being submitted
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index cb2e571..7730b97 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -629,24 +629,36 @@ void ubi_refill_pools(struct ubi_device *ubi)
*/
int ubi_wl_get_peb(struct ubi_device *ubi)
{
- int ret;
+ int ret, retried = 0;
struct ubi_fm_pool *pool = ubi-fm_pool;
struct ubi_fm_pool *wl_pool = ubi-fm_wl_pool;
- if (!pool-size || !wl_pool-size || pool-used == pool-size ||
- wl_pool-used == wl_pool-size)
+again:
+ spin_lock(ubi-wl_lock);
+ /* We check here also for the WL pool because at this point we can
+* refill the WL pool synchronous. */
+ if (pool-used == pool-size || wl_pool-used == wl_pool-size) {
+ spin_unlock(ubi-wl_lock);
ubi_update_fastmap(ubi);
-
- /* we got not a single free PEB */
- if (!pool-size)
- ret = -ENOSPC;
- else {
spin_lock(ubi-wl_lock);
- ret = pool-pebs[pool-used++];
- prot_queue_add(ubi, ubi-lookuptbl[ret]);
+ }
+
+ if (pool-used == pool-size) {
Im confused about this if condition. You just tested pool-used ==
pool-size in the previous if. If in the previous if pool-used !=
pool-size and wl_pool-used != wl_pool-size, you didn't enter, the
lock is still held so pool-used != pool-size still. If in the previos
if wl_pool-used == wl_pool-size was true nd tou released the lock,
ubi_update_fastmap(ubi) was called, which refills the pools. So again,
if pools were refilled pool-used would be 0 here and pool-size 0.
So in both cases I don't see how at this point pool-used == pool-size
could ever be true?
spin_unlock(ubi-wl_lock);
+ if (retried) {
+ ubi_err(ubi, Unable to get a free PEB from user WL
pool);
+ ret = -ENOSPC;
+ goto out;
+ }
+ retried = 1;
Why did you decide to retry in this function? and why only 1 retry
attempt? I'm not against it, trying to understand the logic.
+ goto again;
}
+ ubi_assert(pool-used pool-size);
+ ret = pool-pebs[pool-used++];
+ prot_queue_add(ubi, ubi-lookuptbl[ret]);
+ spin_unlock(ubi-wl_lock);
+out:
return ret;
}
@@ -659,7 +671,7 @@ static struct ubi_wl_entry *get_peb_for_wl(struct
ubi_device *ubi)
struct ubi_fm_pool *pool = ubi-fm_wl_pool;
int pnum;
- if (pool-used == pool-size || !pool-size) {
+ if (pool-used == pool-size) {
/* We cannot update the fastmap here because this
* function is called in atomic context.
* Let's fail here and refill/update it as soon as possible. */
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 4/6] UBI: Fastmap: Fix races in ubi_wl_get_peb()
Hi Richard
On 12/5/2014 3:20 PM, Richard Weinberger wrote:
Tanya,
Am 05.12.2014 um 14:09 schrieb Tanya Brokhman:
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
ubi_wl_get_peb() has two problems, it reads the pool
size and usage counters without any protection.
While reading one value would be perfectly fine it reads multiple
values and compares them. This is racy and can lead to incorrect
pool handling.
Furthermore ubi_update_fastmap() is called without wl_lock held,
before incrementing the used counter it needs to be checked again.
I didn't see where you fixed the ubi_update_fastmap issue you just mentioned.
This is exactly what you're questioning below.
We have to recheck as the pool counter could have changed.
Oh, I understood the commit msg a bit differently, but now I see that it
was my mistake. thanks!
It could happen that another thread consumed all PEBs from the
pool and the counter goes beyond -size.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/ubi.h | 3 ++-
drivers/mtd/ubi/wl.c | 34 +++---
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 04c4c05..d672412 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -439,7 +439,8 @@ struct ubi_debug_info {
* @pq_head: protection queue head
* @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl,
@move_from,
* @move_to, @move_to_put @erase_pending, @wl_scheduled, @works,
- * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields
+ * @erroneous, @erroneous_peb_count, @fm_work_scheduled, @fm_pool,
+ * and @fm_wl_pool fields
* @move_mutex: serializes eraseblock moves
* @work_sem: used to wait for all the scheduled works to finish and prevent
* new works from being submitted
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index cb2e571..7730b97 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -629,24 +629,36 @@ void ubi_refill_pools(struct ubi_device *ubi)
*/
int ubi_wl_get_peb(struct ubi_device *ubi)
{
-int ret;
+int ret, retried = 0;
struct ubi_fm_pool *pool = ubi-fm_pool;
struct ubi_fm_pool *wl_pool = ubi-fm_wl_pool;
-if (!pool-size || !wl_pool-size || pool-used == pool-size ||
-wl_pool-used == wl_pool-size)
+again:
+spin_lock(ubi-wl_lock);
+/* We check here also for the WL pool because at this point we can
+ * refill the WL pool synchronous. */
+if (pool-used == pool-size || wl_pool-used == wl_pool-size) {
+spin_unlock(ubi-wl_lock);
ubi_update_fastmap(ubi);
-
-/* we got not a single free PEB */
-if (!pool-size)
-ret = -ENOSPC;
-else {
spin_lock(ubi-wl_lock);
-ret = pool-pebs[pool-used++];
-prot_queue_add(ubi, ubi-lookuptbl[ret]);
+}
+
+if (pool-used == pool-size) {
Im confused about this if condition. You just tested pool-used == pool-size in the previous
if. If in the previous if pool-used != pool-size and wl_pool-used !=
wl_pool-size, you didn't enter, the lock is still held so pool-used != pool-size still. If
in the previos if wl_pool-used == wl_pool-size was true nd tou released the lock,
ubi_update_fastmap(ubi) was called, which refills the pools. So again, if pools were
refilled pool-used would be 0 here and pool-size 0.
So in both cases I don't see how at this point pool-used == pool-size could
ever be true?
If we enter the if (pool-used == pool-size || wl_pool-used == wl_pool-size)
{ branch we unlock wl_lock and call ubi_update_fastmap().
Another thread can enter ubi_wl_get_peb() and alter the pool counter. So we
have to recheck the counter after taking wl_lock again.
hmmm... ok. Perhaps a comment could be added in the code to explain this
case in a few words?
spin_unlock(ubi-wl_lock);
+if (retried) {
+ubi_err(ubi, Unable to get a free PEB from user WL pool);
+ret = -ENOSPC;
+goto out;
+}
+retried = 1;
Why did you decide to retry in this function? and why only 1 retry attempt? I'm
not against it, trying to understand the logic.
Because failing immediately with -ENOSPC is not nice.
Why not? this is what was done before
I think what I really bothers me in this case is that you don't sleep,
you branch immediately to retry again, so the chances that there will be
context switch and free pebs appear aren't that high.
I'm used to functions using some sort of retry logic to sleep before
retrying. Of course sleeping isn't a good idea here. That's why the
retry bugs me a bit.
Before we do that I'll give UBI a second chance to produce a free PEB.
Thanks,
//richard
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation
Re: [PATCH 5/6] UBI: Split __wl_get_peb()
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
Make it two functions, wl_get_wle() and wl_get_peb().
wl_get_peb() works exactly like __wl_get_peb() but wl_get_wle()
does not call produce_free_peb().
While refilling the fastmap user pool we cannot release ubi-wl_lock
as produce_free_peb() does.
Hence the fastmap logic uses now wl_get_wle().
hmmm... confused... I don't see fastmap code changed
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 61
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 7730b97..f028b68 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -499,13 +499,46 @@ out:
#endif
/**
- * __wl_get_peb - get a physical eraseblock.
+ * wl_get_wle - get a mean wl entry to be used by wl_get_peb() or
+ * refill_wl_user_pool().
+ * @ubi: UBI device description object
+ *
+ * This function returns a a wear leveling entry in case of success and
If you upload a new version, you have a double a here: returns a a
wear leveling
+ * NULL in case of failure.
+ */
+static struct ubi_wl_entry *wl_get_wle(struct ubi_device *ubi)
+{
+ struct ubi_wl_entry *e;
+
+ e = find_mean_wl_entry(ubi, ubi-free);
+ if (!e) {
+ ubi_err(ubi, no free eraseblocks);
+ return NULL;
+ }
+
+ self_check_in_wl_tree(ubi, e, ubi-free);
+
+ /*
+* Move the physical eraseblock to the protection queue where it will
+* be protected from being moved for some time.
+*/
I don't think this comment is valid anymore
+ rb_erase(e-u.rb, ubi-free);
+ ubi-free_count--;
+ dbg_wl(PEB %d EC %d, e-pnum, e-ec);
+
+ return e;
+}
+
+/**
+ * wl_get_peb - get a physical eraseblock.
* @ubi: UBI device description object
*
* This function returns a physical eraseblock in case of success and a
* negative error code in case of failure.
+ * It is the low level component of ubi_wl_get_peb() in the non-fastmap
+ * case.
*/
-static int __wl_get_peb(struct ubi_device *ubi)
+static int wl_get_peb(struct ubi_device *ubi)
{
int err;
struct ubi_wl_entry *e;
@@ -524,27 +557,9 @@ retry:
goto retry;
}
- e = find_mean_wl_entry(ubi, ubi-free);
- if (!e) {
- ubi_err(ubi, no free eraseblocks);
- return -ENOSPC;
- }
-
- self_check_in_wl_tree(ubi, e, ubi-free);
-
- /*
-* Move the physical eraseblock to the protection queue where it will
-* be protected from being moved for some time.
-*/
- rb_erase(e-u.rb, ubi-free);
- ubi-free_count--;
- dbg_wl(PEB %d EC %d, e-pnum, e-ec);
-#ifndef CONFIG_MTD_UBI_FASTMAP
- /* We have to enqueue e only if fastmap is disabled,
-* is fastmap enabled prot_queue_add() will be called by
-* ubi_wl_get_peb() after removing e from the pool. */
+ e = wl_get_wle(ubi);
prot_queue_add(ubi, e);
-#endif
+
return e-pnum;
}
@@ -704,7 +719,7 @@ int ubi_wl_get_peb(struct ubi_device *ubi)
int peb, err;
spin_lock(ubi-wl_lock);
- peb = __wl_get_peb(ubi);
+ peb = wl_get_peb(ubi);
spin_unlock(ubi-wl_lock);
if (peb 0)
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 6/6] UBI: Fastmap: Make ubi_refill_pools() fair
Hi Richard,
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
Currently ubi_refill_pools() first fills the first and then
the second one.
If only very few free PEBs are available the second pool can get
zero PEBs.
Change ubi_refill_pools() to distribute free PEBs fair between
all pools.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 77 +++-
1 file changed, 40 insertions(+), 37 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index f028b68..c2822f7 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -583,59 +583,62 @@ static void return_unused_pool_pebs(struct ubi_device
*ubi,
}
/**
- * refill_wl_pool - refills all the fastmap pool used by the
- * WL sub-system.
+ * ubi_refill_pools - refills all fastmap PEB pools.
* @ubi: UBI device description object
*/
-static void refill_wl_pool(struct ubi_device *ubi)
+void ubi_refill_pools(struct ubi_device *ubi)
{
+ struct ubi_fm_pool *wl_pool = ubi-fm_wl_pool;
+ struct ubi_fm_pool *pool = ubi-fm_pool;
struct ubi_wl_entry *e;
- struct ubi_fm_pool *pool = ubi-fm_wl_pool;
+ int enough;
+ spin_lock(ubi-wl_lock);
+
+ return_unused_pool_pebs(ubi, wl_pool);
return_unused_pool_pebs(ubi, pool);
- for (pool-size = 0; pool-size pool-max_size; pool-size++) {
- if (!ubi-free.rb_node ||
- (ubi-free_count - ubi-beb_rsvd_pebs 5))
- break;
+ wl_pool-size = 0;
+ pool-size = 0;
- e = find_wl_entry(ubi, ubi-free, WL_FREE_MAX_DIFF);
- self_check_in_wl_tree(ubi, e, ubi-free);
- rb_erase(e-u.rb, ubi-free);
- ubi-free_count--;
+ for (;;) {
You loop for max(pool-max_size, wl_pool-max_size) itterations. IMO,
the code will be more clear if you use for(i=0; imax(pool-max_size,
wl_pool-max_size); i++) instead of int enough.
This is just coding style preference of course. I personally don't like
for(;;) that much Just a suggestion. :)
+ enough = 0;
+ if (pool-size pool-max_size) {
+ if (!ubi-free.rb_node ||
+ (ubi-free_count - ubi-beb_rsvd_pebs 5))
+ break;
- pool-pebs[pool-size] = e-pnum;
- }
- pool-used = 0;
-}
+ e = wl_get_wle(ubi);
+ if (!e)
+ break;
-/**
- * refill_wl_user_pool - refills all the fastmap pool used by ubi_wl_get_peb.
- * @ubi: UBI device description object
- */
-static void refill_wl_user_pool(struct ubi_device *ubi)
-{
- struct ubi_fm_pool *pool = ubi-fm_pool;
+ pool-pebs[pool-size] = e-pnum;
+ pool-size++;
+ } else
+ enough++;
- return_unused_pool_pebs(ubi, pool);
+ if (wl_pool-size wl_pool-max_size) {
+ if (!ubi-free.rb_node ||
+ (ubi-free_count - ubi-beb_rsvd_pebs 5))
+ break;
- for (pool-size = 0; pool-size pool-max_size; pool-size++) {
- pool-pebs[pool-size] = __wl_get_peb(ubi);
- if (pool-pebs[pool-size] 0)
+ e = find_wl_entry(ubi, ubi-free, WL_FREE_MAX_DIFF);
+ self_check_in_wl_tree(ubi, e, ubi-free);
+ rb_erase(e-u.rb, ubi-free);
+ ubi-free_count--;
why don't you use wl_get_peb() here?
Other then that - I agree with the patch. So if you want to keep it as
is, I'll add Reviewed-by.
+
+ wl_pool-pebs[wl_pool-size] = e-pnum;
+ wl_pool-size++;
+ } else
+ enough++;
+
+ if (enough == 2)
break;
}
+
+ wl_pool-used = 0;
pool-used = 0;
-}
-/**
- * ubi_refill_pools - refills all fastmap PEB pools.
- * @ubi: UBI device description object
- */
-void ubi_refill_pools(struct ubi_device *ubi)
-{
- spin_lock(ubi-wl_lock);
- refill_wl_pool(ubi);
- refill_wl_user_pool(ubi);
spin_unlock(ubi-wl_lock);
}
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 3/6] UBI: Fastmap: Ensure that all fastmap work is done upon WL shutdown
On 11/27/2014 5:38 PM, Artem Bityutskiy wrote:
On Mon, 2014-11-24 at 14:20 +0100, Richard Weinberger wrote:
...otherwise the deferred work might run after datastructures
got freed and corrupt memory.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 7f135df..cb2e571 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl("close the WL sub-system");
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ flush_work(>fm_work);
+#endif
If you are using the work infrastructure implemented in wl.c, then
fastmap work should be no different to any other work. And we do flush
all works in 'shutdown_work()'. The fastmap work should be flushed there
too.
I tend to agree with Artem. Could you please move it to shutdown_work()?
I'm asking because I do want to pick up all the bug fixes patches, but
not going to pick up the "cleaning up" patches at this point.
I think we discussed this already - there should be one single queue of
works, managed by the same set of functions, all flushed in the same
place, one-by-one...
Obviously, there is some misunderstanding. This looks like lack of
separation and misuse of layering. I am missing explanations why I am
wrong...
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 2/6] UBI: Fastmap: Ensure that only one fastmap work is scheduled
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
If the WL pool runs out of PEBs we schedule a fastmap write
to refill it as soon as possible.
Ensure that only one at a time is scheduled otherwise we might end in
a fastmap write storm because writing the fastmap can schedule another
write if bitflips are detected.
Reviewed-by: Tanya Brokhman
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/ubi.h | 4 +++-
drivers/mtd/ubi/wl.c | 8 +++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index f80ffab..04c4c05 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -427,6 +427,7 @@ struct ubi_debug_info {
* @fm_size: fastmap size in bytes
* @fm_sem: allows ubi_update_fastmap() to block EBA table changes
* @fm_work: fastmap work queue
+ * @fm_work_scheduled: non-zero if fastmap work was scheduled
*
* @used: RB-tree of used physical eraseblocks
* @erroneous: RB-tree of erroneous used physical eraseblocks
@@ -438,7 +439,7 @@ struct ubi_debug_info {
* @pq_head: protection queue head
* @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from,
* @move_to, @move_to_put @erase_pending, @wl_scheduled, @works,
- * @erroneous, and @erroneous_peb_count fields
+ * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields
* @move_mutex: serializes eraseblock moves
* @work_sem: used to wait for all the scheduled works to finish and prevent
* new works from being submitted
@@ -533,6 +534,7 @@ struct ubi_device {
void *fm_buf;
size_t fm_size;
struct work_struct fm_work;
+ int fm_work_scheduled;
/* Wear-leveling sub-system's stuff */
struct rb_root used;
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 834f6fe..7f135df 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -149,6 +149,9 @@ static void update_fastmap_work_fn(struct work_struct *wrk)
{
struct ubi_device *ubi = container_of(wrk, struct ubi_device, fm_work);
ubi_update_fastmap(ubi);
+ spin_lock(>wl_lock);
+ ubi->fm_work_scheduled = 0;
+ spin_unlock(>wl_lock);
}
/**
@@ -660,7 +663,10 @@ static struct ubi_wl_entry *get_peb_for_wl(struct
ubi_device *ubi)
/* We cannot update the fastmap here because this
* function is called in atomic context.
* Let's fail here and refill/update it as soon as possible. */
- schedule_work(>fm_work);
+ if (!ubi->fm_work_scheduled) {
+ ubi->fm_work_scheduled = 1;
+ schedule_work(>fm_work);
+ }
return NULL;
} else {
pnum = pool->pebs[pool->used++];
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 02/35] UBI: Fix stale pointers in ubi->lookuptbl
On 12/4/2014 1:04 PM, Richard Weinberger wrote: Tanya, Am 04.12.2014 um 12:00 schrieb Tanya Brokhman: Hi Richard On 10/29/2014 2:45 PM, Richard Weinberger wrote: In some error paths the WL sub-system gives up on a PEB and frees it's ubi_wl_entry struct but does not set the entry in ubi->lookuptbl to NULL. Fastmap can stumble over such a stale pointer as it uses ubi->lookuptbl to find all PEBs. Fix this by setting the pointers to free'd ubi_wl_entry to NULL. There are 2 more places: tree_destroy() and protection_queue_destroy() where ubi_wl_entry is released. Both functions used on power down so all should be good as is, just wanted to make sure you didn't add ubi->lookuptbl[e2->pnum] = NULL there on purpose. I fear you're looking at the old patch series. Please have a look at: "[PATCH 3/6] UBI: Fix stale pointers in ubi->lookuptbl" sent on 30.11.2014. Thanks, //richard oh, you're right. Started from the oldest. now I see you released v2. Sorry. So I should be looking at "Fastmap update v2 (pile 1-7)", right? Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 02/35] UBI: Fix stale pointers in ubi->lookuptbl
Hi Richard On 10/29/2014 2:45 PM, Richard Weinberger wrote: In some error paths the WL sub-system gives up on a PEB and frees it's ubi_wl_entry struct but does not set the entry in ubi->lookuptbl to NULL. Fastmap can stumble over such a stale pointer as it uses ubi->lookuptbl to find all PEBs. Fix this by setting the pointers to free'd ubi_wl_entry to NULL. There are 2 more places: tree_destroy() and protection_queue_destroy() where ubi_wl_entry is released. Both functions used on power down so all should be good as is, just wanted to make sure you didn't add ubi->lookuptbl[e2->pnum] = NULL there on purpose. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 01/35] UBI: Add initial support for fastmap self checks
On 10/29/2014 2:45 PM, Richard Weinberger wrote:
Using this debugfs knob fastmap self checks can be controlled.
Reviewed-by: Tanya Brokhman
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/debug.c | 11 +++
drivers/mtd/ubi/debug.h | 5 +
drivers/mtd/ubi/ubi.h | 4
3 files changed, 20 insertions(+)
diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index 63cb1d7..ea9be20 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -275,6 +275,8 @@ static ssize_t dfs_file_read(struct file *file, char __user
*user_buf,
val = d->chk_gen;
else if (dent == d->dfs_chk_io)
val = d->chk_io;
+ else if (dent == d->dfs_chk_fastmap)
+ val = d->chk_fastmap;
else if (dent == d->dfs_disable_bgt)
val = d->disable_bgt;
else if (dent == d->dfs_emulate_bitflips)
@@ -336,6 +338,8 @@ static ssize_t dfs_file_write(struct file *file, const char
__user *user_buf,
d->chk_gen = val;
else if (dent == d->dfs_chk_io)
d->chk_io = val;
+ else if (dent == d->dfs_chk_fastmap)
+ d->chk_fastmap = val;
else if (dent == d->dfs_disable_bgt)
d->disable_bgt = val;
else if (dent == d->dfs_emulate_bitflips)
@@ -406,6 +410,13 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi)
goto out_remove;
d->dfs_chk_io = dent;
+ fname = "chk_fastmap";
+ dent = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, (void *)ubi_num,
+ _fops);
+ if (IS_ERR_OR_NULL(dent))
+ goto out_remove;
+ d->dfs_chk_fastmap = dent;
+
fname = "tst_disable_bgt";
dent = debugfs_create_file(fname, S_IWUSR, d->dfs_dir, (void *)ubi_num,
_fops);
diff --git a/drivers/mtd/ubi/debug.h b/drivers/mtd/ubi/debug.h
index cba89fc..e99ef37 100644
--- a/drivers/mtd/ubi/debug.h
+++ b/drivers/mtd/ubi/debug.h
@@ -127,4 +127,9 @@ static inline int ubi_dbg_chk_gen(const struct ubi_device
*ubi)
{
return ubi->dbg.chk_gen;
}
+
+static inline int ubi_dbg_chk_fastmap(const struct ubi_device *ubi)
+{
+ return ubi->dbg.chk_fastmap;
+}
#endif /* !__UBI_DEBUG_H__ */
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 320fc38..f80a726 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -352,6 +352,7 @@ struct ubi_wl_entry;
*
* @chk_gen: if UBI general extra checks are enabled
* @chk_io: if UBI I/O extra checks are enabled
+ * @chk_fastmap: if UBI fastmap extra checks are enabled
* @disable_bgt: disable the background task for testing purposes
* @emulate_bitflips: emulate bit-flips for testing purposes
* @emulate_io_failures: emulate write/erase failures for testing purposes
@@ -359,6 +360,7 @@ struct ubi_wl_entry;
* @dfs_dir: direntry object of the UBI device debugfs directory
* @dfs_chk_gen: debugfs knob to enable UBI general extra checks
* @dfs_chk_io: debugfs knob to enable UBI I/O extra checks
+ * @dfs_chk_fastmap: debugfs knob to enable UBI fastmap extra checks
* @dfs_disable_bgt: debugfs knob to disable the background task
* @dfs_emulate_bitflips: debugfs knob to emulate bit-flips
* @dfs_emulate_io_failures: debugfs knob to emulate write/erase failures
@@ -366,6 +368,7 @@ struct ubi_wl_entry;
struct ubi_debug_info {
unsigned int chk_gen:1;
unsigned int chk_io:1;
+ unsigned int chk_fastmap:1;
unsigned int disable_bgt:1;
unsigned int emulate_bitflips:1;
unsigned int emulate_io_failures:1;
@@ -373,6 +376,7 @@ struct ubi_debug_info {
struct dentry *dfs_dir;
struct dentry *dfs_chk_gen;
struct dentry *dfs_chk_io;
+ struct dentry *dfs_chk_fastmap;
struct dentry *dfs_disable_bgt;
struct dentry *dfs_emulate_bitflips;
struct dentry *dfs_emulate_io_failures;
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 01/35] UBI: Add initial support for fastmap self checks
On 10/29/2014 2:45 PM, Richard Weinberger wrote:
Using this debugfs knob fastmap self checks can be controlled.
Reviewed-by: Tanya Brokhman tlin...@codeaurora.org
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/debug.c | 11 +++
drivers/mtd/ubi/debug.h | 5 +
drivers/mtd/ubi/ubi.h | 4
3 files changed, 20 insertions(+)
diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index 63cb1d7..ea9be20 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -275,6 +275,8 @@ static ssize_t dfs_file_read(struct file *file, char __user
*user_buf,
val = d-chk_gen;
else if (dent == d-dfs_chk_io)
val = d-chk_io;
+ else if (dent == d-dfs_chk_fastmap)
+ val = d-chk_fastmap;
else if (dent == d-dfs_disable_bgt)
val = d-disable_bgt;
else if (dent == d-dfs_emulate_bitflips)
@@ -336,6 +338,8 @@ static ssize_t dfs_file_write(struct file *file, const char
__user *user_buf,
d-chk_gen = val;
else if (dent == d-dfs_chk_io)
d-chk_io = val;
+ else if (dent == d-dfs_chk_fastmap)
+ d-chk_fastmap = val;
else if (dent == d-dfs_disable_bgt)
d-disable_bgt = val;
else if (dent == d-dfs_emulate_bitflips)
@@ -406,6 +410,13 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi)
goto out_remove;
d-dfs_chk_io = dent;
+ fname = chk_fastmap;
+ dent = debugfs_create_file(fname, S_IWUSR, d-dfs_dir, (void *)ubi_num,
+ dfs_fops);
+ if (IS_ERR_OR_NULL(dent))
+ goto out_remove;
+ d-dfs_chk_fastmap = dent;
+
fname = tst_disable_bgt;
dent = debugfs_create_file(fname, S_IWUSR, d-dfs_dir, (void *)ubi_num,
dfs_fops);
diff --git a/drivers/mtd/ubi/debug.h b/drivers/mtd/ubi/debug.h
index cba89fc..e99ef37 100644
--- a/drivers/mtd/ubi/debug.h
+++ b/drivers/mtd/ubi/debug.h
@@ -127,4 +127,9 @@ static inline int ubi_dbg_chk_gen(const struct ubi_device
*ubi)
{
return ubi-dbg.chk_gen;
}
+
+static inline int ubi_dbg_chk_fastmap(const struct ubi_device *ubi)
+{
+ return ubi-dbg.chk_fastmap;
+}
#endif /* !__UBI_DEBUG_H__ */
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 320fc38..f80a726 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -352,6 +352,7 @@ struct ubi_wl_entry;
*
* @chk_gen: if UBI general extra checks are enabled
* @chk_io: if UBI I/O extra checks are enabled
+ * @chk_fastmap: if UBI fastmap extra checks are enabled
* @disable_bgt: disable the background task for testing purposes
* @emulate_bitflips: emulate bit-flips for testing purposes
* @emulate_io_failures: emulate write/erase failures for testing purposes
@@ -359,6 +360,7 @@ struct ubi_wl_entry;
* @dfs_dir: direntry object of the UBI device debugfs directory
* @dfs_chk_gen: debugfs knob to enable UBI general extra checks
* @dfs_chk_io: debugfs knob to enable UBI I/O extra checks
+ * @dfs_chk_fastmap: debugfs knob to enable UBI fastmap extra checks
* @dfs_disable_bgt: debugfs knob to disable the background task
* @dfs_emulate_bitflips: debugfs knob to emulate bit-flips
* @dfs_emulate_io_failures: debugfs knob to emulate write/erase failures
@@ -366,6 +368,7 @@ struct ubi_wl_entry;
struct ubi_debug_info {
unsigned int chk_gen:1;
unsigned int chk_io:1;
+ unsigned int chk_fastmap:1;
unsigned int disable_bgt:1;
unsigned int emulate_bitflips:1;
unsigned int emulate_io_failures:1;
@@ -373,6 +376,7 @@ struct ubi_debug_info {
struct dentry *dfs_dir;
struct dentry *dfs_chk_gen;
struct dentry *dfs_chk_io;
+ struct dentry *dfs_chk_fastmap;
struct dentry *dfs_disable_bgt;
struct dentry *dfs_emulate_bitflips;
struct dentry *dfs_emulate_io_failures;
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 02/35] UBI: Fix stale pointers in ubi-lookuptbl
Hi Richard On 10/29/2014 2:45 PM, Richard Weinberger wrote: In some error paths the WL sub-system gives up on a PEB and frees it's ubi_wl_entry struct but does not set the entry in ubi-lookuptbl to NULL. Fastmap can stumble over such a stale pointer as it uses ubi-lookuptbl to find all PEBs. Fix this by setting the pointers to free'd ubi_wl_entry to NULL. There are 2 more places: tree_destroy() and protection_queue_destroy() where ubi_wl_entry is released. Both functions used on power down so all should be good as is, just wanted to make sure you didn't add ubi-lookuptbl[e2-pnum] = NULL there on purpose. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 02/35] UBI: Fix stale pointers in ubi-lookuptbl
On 12/4/2014 1:04 PM, Richard Weinberger wrote: Tanya, Am 04.12.2014 um 12:00 schrieb Tanya Brokhman: Hi Richard On 10/29/2014 2:45 PM, Richard Weinberger wrote: In some error paths the WL sub-system gives up on a PEB and frees it's ubi_wl_entry struct but does not set the entry in ubi-lookuptbl to NULL. Fastmap can stumble over such a stale pointer as it uses ubi-lookuptbl to find all PEBs. Fix this by setting the pointers to free'd ubi_wl_entry to NULL. There are 2 more places: tree_destroy() and protection_queue_destroy() where ubi_wl_entry is released. Both functions used on power down so all should be good as is, just wanted to make sure you didn't add ubi-lookuptbl[e2-pnum] = NULL there on purpose. I fear you're looking at the old patch series. Please have a look at: [PATCH 3/6] UBI: Fix stale pointers in ubi-lookuptbl sent on 30.11.2014. Thanks, //richard oh, you're right. Started from the oldest. now I see you released v2. Sorry. So I should be looking at Fastmap update v2 (pile 1-7), right? Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 2/6] UBI: Fastmap: Ensure that only one fastmap work is scheduled
On 11/24/2014 3:20 PM, Richard Weinberger wrote:
If the WL pool runs out of PEBs we schedule a fastmap write
to refill it as soon as possible.
Ensure that only one at a time is scheduled otherwise we might end in
a fastmap write storm because writing the fastmap can schedule another
write if bitflips are detected.
Reviewed-by: Tanya Brokhman tlin...@codeaurora.org
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/ubi.h | 4 +++-
drivers/mtd/ubi/wl.c | 8 +++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index f80ffab..04c4c05 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -427,6 +427,7 @@ struct ubi_debug_info {
* @fm_size: fastmap size in bytes
* @fm_sem: allows ubi_update_fastmap() to block EBA table changes
* @fm_work: fastmap work queue
+ * @fm_work_scheduled: non-zero if fastmap work was scheduled
*
* @used: RB-tree of used physical eraseblocks
* @erroneous: RB-tree of erroneous used physical eraseblocks
@@ -438,7 +439,7 @@ struct ubi_debug_info {
* @pq_head: protection queue head
* @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from,
* @move_to, @move_to_put @erase_pending, @wl_scheduled, @works,
- * @erroneous, and @erroneous_peb_count fields
+ * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields
* @move_mutex: serializes eraseblock moves
* @work_sem: used to wait for all the scheduled works to finish and prevent
* new works from being submitted
@@ -533,6 +534,7 @@ struct ubi_device {
void *fm_buf;
size_t fm_size;
struct work_struct fm_work;
+ int fm_work_scheduled;
/* Wear-leveling sub-system's stuff */
struct rb_root used;
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 834f6fe..7f135df 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -149,6 +149,9 @@ static void update_fastmap_work_fn(struct work_struct *wrk)
{
struct ubi_device *ubi = container_of(wrk, struct ubi_device, fm_work);
ubi_update_fastmap(ubi);
+ spin_lock(ubi-wl_lock);
+ ubi-fm_work_scheduled = 0;
+ spin_unlock(ubi-wl_lock);
}
/**
@@ -660,7 +663,10 @@ static struct ubi_wl_entry *get_peb_for_wl(struct
ubi_device *ubi)
/* We cannot update the fastmap here because this
* function is called in atomic context.
* Let's fail here and refill/update it as soon as possible. */
- schedule_work(ubi-fm_work);
+ if (!ubi-fm_work_scheduled) {
+ ubi-fm_work_scheduled = 1;
+ schedule_work(ubi-fm_work);
+ }
return NULL;
} else {
pnum = pool-pebs[pool-used++];
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH 3/6] UBI: Fastmap: Ensure that all fastmap work is done upon WL shutdown
On 11/27/2014 5:38 PM, Artem Bityutskiy wrote:
On Mon, 2014-11-24 at 14:20 +0100, Richard Weinberger wrote:
...otherwise the deferred work might run after datastructures
got freed and corrupt memory.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 7f135df..cb2e571 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl(close the WL sub-system);
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ flush_work(ubi-fm_work);
+#endif
If you are using the work infrastructure implemented in wl.c, then
fastmap work should be no different to any other work. And we do flush
all works in 'shutdown_work()'. The fastmap work should be flushed there
too.
I tend to agree with Artem. Could you please move it to shutdown_work()?
I'm asking because I do want to pick up all the bug fixes patches, but
not going to pick up the cleaning up patches at this point.
I think we discussed this already - there should be one single queue of
works, managed by the same set of functions, all flushed in the same
place, one-by-one...
Obviously, there is some misunderstanding. This looks like lack of
separation and misuse of layering. I am missing explanations why I am
wrong...
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
--
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/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 11/10/2014 7:57 PM, Joe Perches wrote: On Mon, 2014-11-10 at 18:37 +0100, Richard Weinberger wrote: Am 03.11.2014 um 14:58 schrieb Tanya Brokhman: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. Adding "error" and "warning" to the message logs is duplicative to the KERN_ logging information. Changes from V5: - Added ptr verification @ ubi_err/ubi_msg/ubi_warn Removed extra printing of ubi number Removed new messages. Did you all ever look at what I posted? I did. Its not my place to re-post your change in my patch. I personally prefer it the way I've done it but it's just a matter of opinion and personal preference. Artem took this one in... https://lkml.org/lkml/2014/10/14/280 smaller code, consistent prefixing, consistent with typical kernel style, etc. While testing I noticed that the log output looks quite different. e.g. [ 26.564111] UBI-0: ubi_attach_mtd_dev:default fastmap pool size: 256 [ 26.565438] UBI-0: ubi_attach_mtd_dev:default fastmap WL pool size: 128 If __func__ is really desired, it's generally better to put a space after the "%s:", __func__ so there's visual separation between the prefixes and the content. I agree that emitting function names isn't particularly useful. I have to disagree here, but again - this is personal preference. (and Richard, do please remember to trim your posts, you sent > 100KB of unnecessary quoted stuff) Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 11/10/2014 7:57 PM, Joe Perches wrote: On Mon, 2014-11-10 at 18:37 +0100, Richard Weinberger wrote: Am 03.11.2014 um 14:58 schrieb Tanya Brokhman: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. Adding error and warning to the message logs is duplicative to the KERN_LEVEL logging information. Changes from V5: - Added ptr verification @ ubi_err/ubi_msg/ubi_warn Removed extra printing of ubi number Removed new messages. Did you all ever look at what I posted? I did. Its not my place to re-post your change in my patch. I personally prefer it the way I've done it but it's just a matter of opinion and personal preference. Artem took this one in... https://lkml.org/lkml/2014/10/14/280 smaller code, consistent prefixing, consistent with typical kernel style, etc. While testing I noticed that the log output looks quite different. e.g. [ 26.564111] UBI-0: ubi_attach_mtd_dev:default fastmap pool size: 256 [ 26.565438] UBI-0: ubi_attach_mtd_dev:default fastmap WL pool size: 128 If __func__ is really desired, it's generally better to put a space after the %s:, __func__ so there's visual separation between the prefixes and the content. I agree that emitting function names isn't particularly useful. I have to disagree here, but again - this is personal preference. (and Richard, do please remember to trim your posts, you sent 100KB of unnecessary quoted stuff) Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH] UBI: Extend UBI layer debug/messaging capabilities - cosmetics
On 11/10/2014 2:18 PM, Artem Bityutskiy wrote:
On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote:
/* Normal UBI messages */
#define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \
-ubi->ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* UBI warning messages */
#define ubi_warn(ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \
- ubi->ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* UBI error messages */
#define ubi_err(ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \
- ubi->ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
Why did you make these changes? It is preferable to not add another 'if'
statement to this macro to handle one or 2 cases - much bloat, little
gain.
Could we please avoid this?
I just wanted to be on the safe side and prevent this macro being called
with ubi=NULL that may crash the system. If you still prefer the "if"
removed will do.
- if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1)) {
- ubi_warn(ubi, "Can't get peb for fastmap:anchor=%d, free_cnt=%d,
reserved=%d",
-anchor, ubi->free_count, ubi->beb_rsvd_pebs);
+ if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1))
goto out;
The warning looks pretty poor, so I do not mind to remove it, but I
thought your patch is about adding a parameter, but you mix different
kinds of things there. Please, be stricter to the similar UBIFS patch
which you was going to send.
Now I'm confused. I added this msg as part of the patch you already
pushed to your branch but later you requested NOT to add additional msgs
and if required add it in a different patch. So this was added by me and
now removed by me - as per your request.
- if (kthread_should_stop()) {
- ubi_msg(ubi, "background thread \"%s\" should stop, PID
%d",
- ubi->bgt_name, task_pid_nr(current));
+ if (kthread_should_stop())
break;
- }
How about just turning this into a debug message, not removing?
Same here. Removing this because *you* requested it.
Quoting you from V5:
"Yes, please, remove these messages or turn them into debugging messages.
And yes, these should have been added in a separate patch."
Artem.
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH] UBI: Extend UBI layer debug/messaging capabilities - cosmetics
On 11/10/2014 2:18 PM, Artem Bityutskiy wrote:
On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote:
/* Normal UBI messages */
#define ubi_msg(ubi, fmt, ...) pr_notice(UBI-%d: %s: fmt \n, \
-ubi-ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi-ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* UBI warning messages */
#define ubi_warn(ubi, fmt, ...) pr_warn(UBI-%d warning: %s: fmt \n, \
- ubi-ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi-ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* UBI error messages */
#define ubi_err(ubi, fmt, ...) pr_err(UBI-%d error: %s: fmt \n, \
- ubi-ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi-ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
Why did you make these changes? It is preferable to not add another 'if'
statement to this macro to handle one or 2 cases - much bloat, little
gain.
Could we please avoid this?
I just wanted to be on the safe side and prevent this macro being called
with ubi=NULL that may crash the system. If you still prefer the if
removed will do.
- if (!ubi-free.rb_node || (ubi-free_count - ubi-beb_rsvd_pebs 1)) {
- ubi_warn(ubi, Can't get peb for fastmap:anchor=%d, free_cnt=%d,
reserved=%d,
-anchor, ubi-free_count, ubi-beb_rsvd_pebs);
+ if (!ubi-free.rb_node || (ubi-free_count - ubi-beb_rsvd_pebs 1))
goto out;
The warning looks pretty poor, so I do not mind to remove it, but I
thought your patch is about adding a parameter, but you mix different
kinds of things there. Please, be stricter to the similar UBIFS patch
which you was going to send.
Now I'm confused. I added this msg as part of the patch you already
pushed to your branch but later you requested NOT to add additional msgs
and if required add it in a different patch. So this was added by me and
now removed by me - as per your request.
- if (kthread_should_stop()) {
- ubi_msg(ubi, background thread \%s\ should stop, PID
%d,
- ubi-bgt_name, task_pid_nr(current));
+ if (kthread_should_stop())
break;
- }
How about just turning this into a debug message, not removing?
Same here. Removing this because *you* requested it.
Quoting you from V5:
Yes, please, remove these messages or turn them into debugging messages.
And yes, these should have been added in a separate patch.
Artem.
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
[PATCH] UBI: Extend UBI layer debug/messaging capabilities - cosmetics
Some cosmetic fixes to the patch "UBI: Extend UBI layer debug/messaging
capabilities".
Signed-off-by: Tanya Brokhman
---
Changes from original patch:
- Added ptr verification @ ubi_err/ubi_msg/ubi_warn
Removed extra printing of ubi number
Removed new messages.
drivers/mtd/ubi/build.c | 6 +++---
drivers/mtd/ubi/cdev.c | 9 -
drivers/mtd/ubi/io.c| 3 +--
drivers/mtd/ubi/ubi.h | 9 ++---
drivers/mtd/ubi/vtbl.c | 7 +++
drivers/mtd/ubi/wl.c| 10 ++
6 files changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 3405be4..ba01a8d 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -923,7 +923,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
/* Make sure ubi_num is not busy */
if (ubi_devices[ubi_num]) {
- ubi_err(ubi, "ubi%d already exists", ubi_num);
+ ubi_err(ubi, "already exists");
return -EEXIST;
}
}
@@ -973,7 +973,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
mutex_init(>fm_mutex);
init_rwsem(>fm_sem);
- ubi_msg(ubi, "attaching mtd%d to ubi%d", mtd->index, ubi_num);
+ ubi_msg(ubi, "attaching mtd%d", mtd->index);
err = io_init(ubi, max_beb_per1024);
if (err)
@@ -1428,7 +1428,7 @@ static int __init ubi_mtd_param_parse(const char *val,
struct kernel_param *kp)
}
if (len == 0) {
- pr_err("UBI warning: empty 'mtd=' parameter - ignored\n");
+ pr_warn("UBI warning: empty 'mtd=' parameter - ignored\n");
return 0;
}
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 3410ea81..bbef168 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -48,14 +48,13 @@
/**
* get_exclusive - get exclusive access to an UBI volume.
- * @ubi: UBI device description object
* @desc: volume descriptor
*
* This function changes UBI volume open mode to "exclusive". Returns previous
* mode value (positive integer) in case of success and a negative error code
* in case of failure.
*/
-static int get_exclusive(struct ubi_device *ubi, struct ubi_volume_desc *desc)
+static int get_exclusive(struct ubi_volume_desc *desc)
{
int users, err;
struct ubi_volume *vol = desc->vol;
@@ -64,7 +63,7 @@ static int get_exclusive(struct ubi_device *ubi, struct
ubi_volume_desc *desc)
users = vol->readers + vol->writers + vol->exclusive;
ubi_assert(users > 0);
if (users > 1) {
- ubi_err(ubi, "%d users for volume %d", users, vol->vol_id);
+ ubi_err(vol->ubi, "%d users for volume %d", users, vol->vol_id);
err = -EBUSY;
} else {
vol->readers = vol->writers = 0;
@@ -421,7 +420,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int
cmd,
break;
}
- err = get_exclusive(ubi, desc);
+ err = get_exclusive(desc);
if (err < 0)
break;
@@ -457,7 +456,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int
cmd,
req.bytes < 0 || req.lnum >= vol->usable_leb_size)
break;
- err = get_exclusive(ubi, desc);
+ err = get_exclusive(desc);
if (err < 0)
break;
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 396aaa5..ed0bcb3 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -1419,8 +1419,7 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int
pnum, int offset, int len)
fail:
ubi_err(ubi, "self-check failed for PEB %d", pnum);
- ubi_msg(ubi, "hex dump of the %d-%d region",
-offset, offset + len);
+ ubi_msg(ubi, "hex dump of the %d-%d region", offset, offset + len);
print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 32, 1, buf, len, 1);
err = -EINVAL;
error:
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index f80ffab..88c2e9f 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -51,13 +51,16 @@
/* Normal UBI messages */
#define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \
-ubi->ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* UBI warning messages */
#define ubi_warn(ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \
-
[PATCH] UBI: Extend UBI layer debug/messaging capabilities - cosmetics
Some cosmetic fixes to the patch UBI: Extend UBI layer debug/messaging
capabilities.
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
---
Changes from original patch:
- Added ptr verification @ ubi_err/ubi_msg/ubi_warn
Removed extra printing of ubi number
Removed new messages.
drivers/mtd/ubi/build.c | 6 +++---
drivers/mtd/ubi/cdev.c | 9 -
drivers/mtd/ubi/io.c| 3 +--
drivers/mtd/ubi/ubi.h | 9 ++---
drivers/mtd/ubi/vtbl.c | 7 +++
drivers/mtd/ubi/wl.c| 10 ++
6 files changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 3405be4..ba01a8d 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -923,7 +923,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
/* Make sure ubi_num is not busy */
if (ubi_devices[ubi_num]) {
- ubi_err(ubi, ubi%d already exists, ubi_num);
+ ubi_err(ubi, already exists);
return -EEXIST;
}
}
@@ -973,7 +973,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
mutex_init(ubi-fm_mutex);
init_rwsem(ubi-fm_sem);
- ubi_msg(ubi, attaching mtd%d to ubi%d, mtd-index, ubi_num);
+ ubi_msg(ubi, attaching mtd%d, mtd-index);
err = io_init(ubi, max_beb_per1024);
if (err)
@@ -1428,7 +1428,7 @@ static int __init ubi_mtd_param_parse(const char *val,
struct kernel_param *kp)
}
if (len == 0) {
- pr_err(UBI warning: empty 'mtd=' parameter - ignored\n);
+ pr_warn(UBI warning: empty 'mtd=' parameter - ignored\n);
return 0;
}
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 3410ea81..bbef168 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -48,14 +48,13 @@
/**
* get_exclusive - get exclusive access to an UBI volume.
- * @ubi: UBI device description object
* @desc: volume descriptor
*
* This function changes UBI volume open mode to exclusive. Returns previous
* mode value (positive integer) in case of success and a negative error code
* in case of failure.
*/
-static int get_exclusive(struct ubi_device *ubi, struct ubi_volume_desc *desc)
+static int get_exclusive(struct ubi_volume_desc *desc)
{
int users, err;
struct ubi_volume *vol = desc-vol;
@@ -64,7 +63,7 @@ static int get_exclusive(struct ubi_device *ubi, struct
ubi_volume_desc *desc)
users = vol-readers + vol-writers + vol-exclusive;
ubi_assert(users 0);
if (users 1) {
- ubi_err(ubi, %d users for volume %d, users, vol-vol_id);
+ ubi_err(vol-ubi, %d users for volume %d, users, vol-vol_id);
err = -EBUSY;
} else {
vol-readers = vol-writers = 0;
@@ -421,7 +420,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int
cmd,
break;
}
- err = get_exclusive(ubi, desc);
+ err = get_exclusive(desc);
if (err 0)
break;
@@ -457,7 +456,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int
cmd,
req.bytes 0 || req.lnum = vol-usable_leb_size)
break;
- err = get_exclusive(ubi, desc);
+ err = get_exclusive(desc);
if (err 0)
break;
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 396aaa5..ed0bcb3 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -1419,8 +1419,7 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int
pnum, int offset, int len)
fail:
ubi_err(ubi, self-check failed for PEB %d, pnum);
- ubi_msg(ubi, hex dump of the %d-%d region,
-offset, offset + len);
+ ubi_msg(ubi, hex dump of the %d-%d region, offset, offset + len);
print_hex_dump(KERN_DEBUG, , DUMP_PREFIX_OFFSET, 32, 1, buf, len, 1);
err = -EINVAL;
error:
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index f80ffab..88c2e9f 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -51,13 +51,16 @@
/* Normal UBI messages */
#define ubi_msg(ubi, fmt, ...) pr_notice(UBI-%d: %s: fmt \n, \
-ubi-ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi-ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* UBI warning messages */
#define ubi_warn(ubi, fmt, ...) pr_warn(UBI-%d warning: %s: fmt \n, \
- ubi-ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi-ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* UBI error messages */
#define ubi_err(ubi, fmt, ...) pr_err(UBI-%d error: %s: fmt \n
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 11/7/2014 12:08 PM, hujianyang wrote: On 2014/11/5 23:35, Artem Bityutskiy wrote: On Mon, 2014-11-03 at 15:58 +0200, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. The R/O block driver messages were replaced by pr_* since ubi_device structure is not used by it. Amended a bit by Artem. Signed-off-by: Tanya Brokhman Signed-off-by: Artem Bityutskiy Oh, the entire patch-set again... this means we need to review it again! Please, have a mercy! :-) It is huge! Please, send a small patch against the one which I pushed already. Thanks! Oh :( and I did that on purpose For some reason I thought you would revert that one and push a new and updated one. Ok, I'll upload a short version. That means I can't add a Reviewed-by on this huge one... ... and then you can add Reviewed-by! :) What a pity~! ^.^ Artem. __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 11/7/2014 12:08 PM, hujianyang wrote: On 2014/11/5 23:35, Artem Bityutskiy wrote: On Mon, 2014-11-03 at 15:58 +0200, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. The R/O block driver messages were replaced by pr_* since ubi_device structure is not used by it. Amended a bit by Artem. Signed-off-by: Tanya Brokhman tlin...@codeaurora.org Signed-off-by: Artem Bityutskiy artem.bityuts...@linux.intel.com Oh, the entire patch-set again... this means we need to review it again! Please, have a mercy! :-) It is huge! Please, send a small patch against the one which I pushed already. Thanks! Oh :( and I did that on purpose For some reason I thought you would revert that one and push a new and updated one. Ok, I'll upload a short version. That means I can't add a Reviewed-by on this huge one... ... and then you can add Reviewed-by! :) What a pity~! ^.^ Artem. __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
[PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
The R/O block driver messages were replaced by pr_* since
ubi_device structure is not used by it.
Amended a bit by Artem.
Signed-off-by: Tanya Brokhman
Signed-off-by: Artem Bityutskiy
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Changes from V3:
- Compilation fix of block.c
- All ubi_* messages in block.c were replaced with pr_*
Changes from V4:
- Addressed comments on block.c
Changes from V5:
- Added ptr verification @ ubi_err/ubi_msg/ubi_warn
Removed extra printing of ubi number
Removed new messages.
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
drivers/mtd/ubi/attach.c | 126 ---
drivers/mtd/ubi/block.c | 41 +++--
drivers/mtd/ubi/build.c | 124 --
drivers/mtd/ubi/cdev.c| 29 -
drivers/mtd/ubi/debug.c | 10 ++--
drivers/mtd/ubi/eba.c | 53 +
drivers/mtd/ubi/fastmap.c | 96 ++---
drivers/mtd/ubi/io.c | 149 +++---
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 16 +++--
drivers/mtd/ubi/vmt.c | 69 +++--
drivers/mtd/ubi/vtbl.c| 43 ++---
drivers/mtd/ubi/wl.c | 60 ++-
14 files changed, 427 insertions(+), 399 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..9d2e16f 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr->vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av->vol_id) {
- ubi_err("inconsistent vol_id");
+ ubi_err(ubi, "inconsistent vol_id");
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err("inconsistent vol_type");
+ ubi_err(ubi, "inconsistent vol_type");
goto bad;
}
if (used_ebs != av->used_ebs) {
- ubi_err("inconsistent used_ebs");
+ ubi_err(ubi, "inconsistent used_ebs");
goto bad;
}
if (data_pad != av->data_pad) {
- ubi_err("inconsistent data_pad");
+ ubi_err(ubi, "inconsistent data_pad");
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err("inconsistent VID header at PEB %d", pnum);
+ ubi_err(ubi, "inconsistent VID header at PEB %d", pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
-
[PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
The R/O block driver messages were replaced by pr_* since
ubi_device structure is not used by it.
Amended a bit by Artem.
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
Signed-off-by: Artem Bityutskiy artem.bityuts...@linux.intel.com
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Changes from V3:
- Compilation fix of block.c
- All ubi_* messages in block.c were replaced with pr_*
Changes from V4:
- Addressed comments on block.c
Changes from V5:
- Added ptr verification @ ubi_err/ubi_msg/ubi_warn
Removed extra printing of ubi number
Removed new messages.
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
drivers/mtd/ubi/attach.c | 126 ---
drivers/mtd/ubi/block.c | 41 +++--
drivers/mtd/ubi/build.c | 124 --
drivers/mtd/ubi/cdev.c| 29 -
drivers/mtd/ubi/debug.c | 10 ++--
drivers/mtd/ubi/eba.c | 53 +
drivers/mtd/ubi/fastmap.c | 96 ++---
drivers/mtd/ubi/io.c | 149 +++---
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 16 +++--
drivers/mtd/ubi/vmt.c | 69 +++--
drivers/mtd/ubi/vtbl.c| 43 ++---
drivers/mtd/ubi/wl.c | 60 ++-
14 files changed, 427 insertions(+), 399 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..9d2e16f 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr-vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av-vol_id) {
- ubi_err(inconsistent vol_id);
+ ubi_err(ubi, inconsistent vol_id);
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err(inconsistent vol_type);
+ ubi_err(ubi, inconsistent vol_type);
goto bad;
}
if (used_ebs != av-used_ebs) {
- ubi_err(inconsistent used_ebs);
+ ubi_err(ubi, inconsistent used_ebs);
goto bad;
}
if (data_pad != av-data_pad) {
- ubi_err(inconsistent data_pad);
+ ubi_err(ubi, inconsistent data_pad);
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err(inconsistent VID header at PEB %d, pnum);
+ ubi_err(ubi, inconsistent VID header at PEB %d, pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
- ubi_err(unsupported on-flash UBI format);
+ ubi_err(ubi, unsupported
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 11/3/2014 5:08 AM, hujianyang wrote:
Hi Tanya,
On 2014/11/3 1:14, Tanya Brokhman wrote:
This patch add 'struct ubi_device *' for 3 functions. We can get 'ubi_device'
from
'ubi_volume'. So I think it's because when we call these functions, the '->ubi'
pointer of 'ubi_volume' is not initialized, am I right? This patch use
'vol->ubi'
to indicate a 'struct ubi_device *' pointer in some places, I think you are sure
of using them.
1. for validate_vid_hdr() we don;t have a ubi_volume yet since its part of the
attach process so we need struct ubi_device
2. for get_exclusive() - you're right. Will fetch dev number from the volume
3. for check_av() - you're right. fixed
I'm not sure if 'ubi_volume->ubi' is initialized when we call some kinds of
ubi_err() to print error messages. The reference to a null pointer, we perform
'ubi->ubi_num' in ubi_err(), may crash the kernel. So you should be careful
of these situations not only in above cases but also in other places in your
patch.
We have the parameter 'ubi_num' for log in some functions like
'ubi_attach_mtd_dev'
before. This patch remove 'ubi_num' in upper changes but keep it in other
changes.
Do we have a discussed rule to deal with this situation? It's not a big problem~
I removed it because it made no sense printing it twice:
"ubi-0: attached mtd-0 (...) to ubi0"?
so I shortned the message:
"ubi-0: attched mtd..."
All the info is still there
Same for other messages that printed ubi number.
Could we remove some the 'ubi_num'? I think there are no need to print it
twice in other places, like:
@@ -921,7 +923,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
/* Make sure ubi_num is not busy */
if (ubi_devices[ubi_num]) {
- ubi_err("ubi%d already exists", ubi_num);
+ ubi_err(ubi, "ubi%d already exists", ubi_num);
return -EEXIST;
}
}
and
@@ -970,7 +974,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
mutex_init(>fm_mutex);
init_rwsem(>fm_sem);
- ubi_msg("attaching mtd%d to ubi%d", mtd->index, ubi_num);
+ ubi_msg(ubi, "attaching mtd%d to ubi%d", mtd->index, ubi_num);
yes, already removed
@@ -1415,8 +1418,9 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int
pnum, int offset, int len)
return 0;
fail:
-ubi_err("self-check failed for PEB %d", pnum);
-ubi_msg("hex dump of the %d-%d region", offset, offset + len);
+ubi_err(ubi, "self-check failed for PEB %d", pnum);
+ubi_msg(ubi, "hex dump of the %d-%d region",
+ offset, offset + len);
print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 32, 1, buf, len, 1);
err = -EINVAL;
error:
Artem, I know you have tried to align the message code in different lines, maybe
you can check if you lose this one.
hmmm... not sure I understand what is wrong here
Turn
+ubi_msg(ubi, "hex dump of the %d-%d region",
+ offset, offset + len);
to
+ubi_msg(ubi, "hex dump of the %d-%d region",
+offset, offset + len);
Actually, I just made it all in one line thanks!
Maybe like this. The next line aligns to the message in first line, not a big
problem.
By the way, I use space in this example, it's wrong. Tab is right.
Thanks!
Hu
Will upload new version soon. Just running some tests.
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
bi-0: attached mtd-0 (...) to ubi0"?
so I shortned the message:
"ubi-0: attched mtd..."
All the info is still there
Same for other messages that printed ubi number.
@@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
int failures = 0;
struct ubi_device *ubi = u;
- ubi_msg("background thread \"%s\" started, PID %d",
+ ubi_msg(ubi, "background thread \"%s\" started, PID %d",
ubi->bgt_name, task_pid_nr(current));
set_freezable();
for (;;) {
int err;
- if (kthread_should_stop())
+ if (kthread_should_stop()) {
+ ubi_msg(ubi, "background thread \"%s\" should stop, PID
%d",
+ ubi->bgt_name, task_pid_nr(current));
break;
+ }
if (try_to_freeze())
continue;
@@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
int failures = 0;
struct ubi_device *ubi = u;
- ubi_msg("background thread \"%s\" started, PID %d",
+ ubi_msg(ubi, "background thread \"%s\" started, PID %d",
ubi->bgt_name, task_pid_nr(current));
set_freezable();
for (;;) {
int err;
- if (kthread_should_stop())
+ if (kthread_should_stop()) {
+ ubi_msg(ubi, "background thread \"%s\" should stop, PID
%d",
+ ubi->bgt_name, task_pid_nr(current));
break;
+ }
if (try_to_freeze())
continue;
Here are two new adding messages. Maybe a separate patch is better? Just a
suggestion.
Done.
@@ -1415,8 +1418,9 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int
pnum, int offset, int len)
return 0;
fail:
- ubi_err("self-check failed for PEB %d", pnum);
- ubi_msg("hex dump of the %d-%d region", offset, offset + len);
+ ubi_err(ubi, "self-check failed for PEB %d", pnum);
+ ubi_msg(ubi, "hex dump of the %d-%d region",
+offset, offset + len);
print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 32, 1, buf, len, 1);
err = -EINVAL;
error:
Artem, I know you have tried to align the message code in different lines, maybe
you can check if you lose this one.
hmmm... not sure I understand what is wrong here
Thanks~!
Hu
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
,
+ ubi_msg(ubi, background thread \%s\ started, PID %d,
ubi-bgt_name, task_pid_nr(current));
set_freezable();
for (;;) {
int err;
- if (kthread_should_stop())
+ if (kthread_should_stop()) {
+ ubi_msg(ubi, background thread \%s\ should stop, PID
%d,
+ ubi-bgt_name, task_pid_nr(current));
break;
+ }
if (try_to_freeze())
continue;
@@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
int failures = 0;
struct ubi_device *ubi = u;
- ubi_msg(background thread \%s\ started, PID %d,
+ ubi_msg(ubi, background thread \%s\ started, PID %d,
ubi-bgt_name, task_pid_nr(current));
set_freezable();
for (;;) {
int err;
- if (kthread_should_stop())
+ if (kthread_should_stop()) {
+ ubi_msg(ubi, background thread \%s\ should stop, PID
%d,
+ ubi-bgt_name, task_pid_nr(current));
break;
+ }
if (try_to_freeze())
continue;
Here are two new adding messages. Maybe a separate patch is better? Just a
suggestion.
Done.
@@ -1415,8 +1418,9 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int
pnum, int offset, int len)
return 0;
fail:
- ubi_err(self-check failed for PEB %d, pnum);
- ubi_msg(hex dump of the %d-%d region, offset, offset + len);
+ ubi_err(ubi, self-check failed for PEB %d, pnum);
+ ubi_msg(ubi, hex dump of the %d-%d region,
+offset, offset + len);
print_hex_dump(KERN_DEBUG, , DUMP_PREFIX_OFFSET, 32, 1, buf, len, 1);
err = -EINVAL;
error:
Artem, I know you have tried to align the message code in different lines, maybe
you can check if you lose this one.
hmmm... not sure I understand what is wrong here
Thanks~!
Hu
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 11/3/2014 5:08 AM, hujianyang wrote:
Hi Tanya,
On 2014/11/3 1:14, Tanya Brokhman wrote:
This patch add 'struct ubi_device *' for 3 functions. We can get 'ubi_device'
from
'ubi_volume'. So I think it's because when we call these functions, the '-ubi'
pointer of 'ubi_volume' is not initialized, am I right? This patch use
'vol-ubi'
to indicate a 'struct ubi_device *' pointer in some places, I think you are sure
of using them.
1. for validate_vid_hdr() we don;t have a ubi_volume yet since its part of the
attach process so we need struct ubi_device
2. for get_exclusive() - you're right. Will fetch dev number from the volume
3. for check_av() - you're right. fixed
I'm not sure if 'ubi_volume-ubi' is initialized when we call some kinds of
ubi_err() to print error messages. The reference to a null pointer, we perform
'ubi-ubi_num' in ubi_err(), may crash the kernel. So you should be careful
of these situations not only in above cases but also in other places in your
patch.
We have the parameter 'ubi_num' for log in some functions like
'ubi_attach_mtd_dev'
before. This patch remove 'ubi_num' in upper changes but keep it in other
changes.
Do we have a discussed rule to deal with this situation? It's not a big problem~
I removed it because it made no sense printing it twice:
ubi-0: attached mtd-0 (...) to ubi0?
so I shortned the message:
ubi-0: attched mtd...
All the info is still there
Same for other messages that printed ubi number.
Could we remove some the 'ubi_num'? I think there are no need to print it
twice in other places, like:
@@ -921,7 +923,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
/* Make sure ubi_num is not busy */
if (ubi_devices[ubi_num]) {
- ubi_err(ubi%d already exists, ubi_num);
+ ubi_err(ubi, ubi%d already exists, ubi_num);
return -EEXIST;
}
}
and
@@ -970,7 +974,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
mutex_init(ubi-fm_mutex);
init_rwsem(ubi-fm_sem);
- ubi_msg(attaching mtd%d to ubi%d, mtd-index, ubi_num);
+ ubi_msg(ubi, attaching mtd%d to ubi%d, mtd-index, ubi_num);
yes, already removed
@@ -1415,8 +1418,9 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int
pnum, int offset, int len)
return 0;
fail:
-ubi_err(self-check failed for PEB %d, pnum);
-ubi_msg(hex dump of the %d-%d region, offset, offset + len);
+ubi_err(ubi, self-check failed for PEB %d, pnum);
+ubi_msg(ubi, hex dump of the %d-%d region,
+ offset, offset + len);
print_hex_dump(KERN_DEBUG, , DUMP_PREFIX_OFFSET, 32, 1, buf, len, 1);
err = -EINVAL;
error:
Artem, I know you have tried to align the message code in different lines, maybe
you can check if you lose this one.
hmmm... not sure I understand what is wrong here
Turn
+ubi_msg(ubi, hex dump of the %d-%d region,
+ offset, offset + len);
to
+ubi_msg(ubi, hex dump of the %d-%d region,
+offset, offset + len);
Actually, I just made it all in one line thanks!
Maybe like this. The next line aligns to the message in first line, not a big
problem.
By the way, I use space in this example, it's wrong. Tab is right.
Thanks!
Hu
Will upload new version soon. Just running some tests.
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/30/2014 10:40 AM, Artem Bityutskiy wrote: Yeah, lets' correct this too. Thanks for ferview Hujianyang! Tanya, would you send a follow-up patch these? Yes, sure. Sorry, was pulled into some urgent stuff. Will post a new patch early next week. Artem. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/30/2014 10:40 AM, Artem Bityutskiy wrote: Yeah, lets' correct this too. Thanks for ferview Hujianyang! Tanya, would you send a follow-up patch these? Yes, sure. Sorry, was pulled into some urgent stuff. Will post a new patch early next week. Artem. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
[RFC/PATCH/RESEND 2/5 v2] mtd: ubi: Fill read disturb statistics
Fill in eraseblock statistics needed for read disturb decision making:
1. read counter: incremented at each read operation
reset at each erase operation
Saved only as part of the meta data in RAM
On attach: if fastmap data is missing a default value is assigned
2. last erase time stamp: updated at each erase operation.
Saved as part of PEB OOB info on flash
On attach: if fastmap data is missing retrieved from PEB EC Header
on NAND.
If fastmap data is corrupted a default value is assigned.
The above parameters are saved as part of the fastmap data.
Signed-off-by: Dolev Raviv
Signed-off-by: Tanya Brokhman
---
drivers/mtd/ubi/attach.c | 137 +++---
drivers/mtd/ubi/debug.c | 11
drivers/mtd/ubi/fastmap.c | 118 ++-
drivers/mtd/ubi/io.c | 28 ++
drivers/mtd/ubi/ubi.h | 24 +++-
drivers/mtd/ubi/vtbl.c| 6 +-
drivers/mtd/ubi/wl.c | 47
7 files changed, 322 insertions(+), 49 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..e102cac 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1,5 +1,8 @@
/*
* Copyright (c) International Business Machines Corp., 2006
+ * Copyright (c) 2014, Linux Foundation. All rights reserved.
+ * Linux Foundation chooses to take subject only to the GPLv2
+ * license terms, and distributes only under these terms.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -87,8 +90,21 @@
#include
#include
#include
+#include
#include "ubi.h"
+#define set_aeb_default_values(aeb, ai)\
+ do {\
+ if (aeb->ec == UBI_UNKNOWN) { \
+ aeb->ec = ai->mean_ec; \
+ if (ai->mean_last_erase_time) \
+ aeb->last_erase_time = \
+ ai->mean_last_erase_time; \
+ else \
+ aeb->last_erase_time = UBI_DT_THRESHOLD / 2; \
+ } \
+ } while (0)
+
static int self_check_ai(struct ubi_device *ubi, struct ubi_attach_info *ai);
/* Temporary variables used during scanning */
@@ -102,6 +118,9 @@ static struct ubi_vid_hdr *vidh;
* @vol_id: the last used volume id for the PEB
* @lnum: the last used LEB number for the PEB
* @ec: erase counter of the physical eraseblock
+ * @last_erase_time: last erase time stamp (%UBI_UNKNOWN if it
+ * is unknown)
+ * @rc: read counter (%UBI_UNKNOWN if it is unknown)
* @to_head: if not zero, add to the head of the list
* @list: the list to add to
*
@@ -117,7 +136,8 @@ static struct ubi_vid_hdr *vidh;
* failure.
*/
static int add_to_list(struct ubi_attach_info *ai, int pnum, int vol_id,
- int lnum, int ec, int to_head, struct list_head *list)
+ int lnum, int ec, long last_erase_time, long rc,
+ int to_head, struct list_head *list)
{
struct ubi_ainf_peb *aeb;
@@ -139,6 +159,9 @@ static int add_to_list(struct ubi_attach_info *ai, int
pnum, int vol_id,
aeb->vol_id = vol_id;
aeb->lnum = lnum;
aeb->ec = ec;
+ aeb->rc = rc;
+ aeb->last_erase_time = last_erase_time;
+
if (to_head)
list_add(>u.list, list);
else
@@ -151,13 +174,17 @@ static int add_to_list(struct ubi_attach_info *ai, int
pnum, int vol_id,
* @ai: attaching information
* @pnum: physical eraseblock number to add
* @ec: erase counter of the physical eraseblock
+ * @last_erase_time: last erase time stamp (%UBI_UNKNOWN if it
+ *is unknown)
+ * @rc: read counter (%UBI_UNKNOWN if it is unknown)
*
* This function allocates a 'struct ubi_ainf_peb' object for a corrupted
* physical eraseblock @pnum and adds it to the 'corr' list. The corruption
* was presumably not caused by a power cut. Returns zero in case of success
* and a negative error code in case of failure.
*/
-static int add_corrupted(struct ubi_attach_info *ai, int pnum, int ec)
+static int add_corrupted(struct ubi_attach_info *ai, int pnum,
+int ec, long rc, long last_erase_time)
{
struct ubi_ainf_peb *aeb;
@@ -170,6 +197,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
ai->corr_peb_count += 1;
aeb->pnum = pnum;
aeb->ec = ec;
+ aeb->rc = rc;
+ aeb->last_erase_time = last_erase_time;
list_add(>u.list, >corr);
return 0;
}
@@ -434,6 +463,9 @@ out_free_vidh:
* @ai: attaching information
* @pnum: the physical eraseblock number
* @ec: e
[RFC/PATCH/RESEND 4/5 v2] mtd: ubi: Read threshold verification
One of the criteria to scrub an eraseblock due to read disturb issue is
if that eraseblock was read from more times then a pre-defined threshold.
This is verified at each LEB read according to the read counter
parameter of the read PEB.
An eraseblock that is found needs scrubbing is added to the ubi->scrub
list.
Signed-off-by: Tanya Brokhman
---
drivers/mtd/ubi/eba.c | 7 ++-
drivers/mtd/ubi/wl.c | 56 +--
2 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 0e11671d..1c7b5f9 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -1,5 +1,8 @@
/*
* Copyright (c) International Business Machines Corp., 2006
+ * Copyright (c) 2014, Linux Foundation. All rights reserved.
+ * Linux Foundation chooses to take subject only to the GPLv2
+ * license terms, and distributes only under these terms.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -466,8 +469,10 @@ retry:
goto out_unlock;
}
}
+ if (ubi->lookuptbl[pnum]->rc >= ubi->rd_threshold)
+ scrub = 1;
- if (scrub)
+ if (scrub && !ubi_in_wl_tree(ubi->lookuptbl[pnum], >scrub))
err = ubi_wl_scrub_peb(ubi, pnum);
leb_read_unlock(ubi, vol_id, lnum);
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 64c7dfd..a5d754f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -449,7 +449,7 @@ static struct ubi_wl_entry *find_anchor_wl_entry(struct
rb_root *root)
return victim;
}
-static int anchor_pebs_avalible(struct rb_root *root)
+static int anchor_pebs_avalible(struct rb_root *root, int rd_threshold)
{
struct rb_node *p;
struct ubi_wl_entry *e;
@@ -560,6 +560,12 @@ static void return_unused_pool_pebs(struct ubi_device *ubi,
for (i = pool->used; i < pool->size; i++) {
e = ubi->lookuptbl[pool->pebs[i]];
+ /* If given PEB pending to be scrubbed - remove it */
+ if (ubi_in_wl_tree(e, >scrub)) {
+ ubi_err("PEB %d was pending scrubb", e->pnum);
+ self_check_in_wl_tree(ubi, e, >scrub);
+ rb_erase(>u.rb, >scrub);
+ }
wl_tree_add(e, >free);
ubi->free_count++;
}
@@ -919,6 +925,13 @@ static int schedule_erase(struct ubi_device *ubi, struct
ubi_wl_entry *e,
wl_wrk->lnum = lnum;
wl_wrk->torture = torture;
+ /* If given PEB pending to be scrubbed - remove it */
+ if (ubi_in_wl_tree(e, >scrub)) {
+ self_check_in_wl_tree(ubi, e, >scrub);
+ rb_erase(>u.rb, >scrub);
+ ubi_msg("PEB %d was pending scrubb",
+ e->pnum);
+ }
schedule_ubi_work(ubi, wl_wrk);
return 0;
}
@@ -948,6 +961,14 @@ static int do_sync_erase(struct ubi_device *ubi, struct
ubi_wl_entry *e,
wl_wrk->lnum = lnum;
wl_wrk->torture = torture;
+ /* If given PEB pending to be scrubbed - remove it */
+ if (ubi_in_wl_tree(e, >scrub)) {
+ self_check_in_wl_tree(ubi, e, >scrub);
+ rb_erase(>u.rb, >scrub);
+ ubi_msg("PEB %d was pending scrubb",
+ e->pnum);
+ }
+
return erase_worker(ubi, wl_wrk, 0);
}
@@ -1052,7 +1073,7 @@ static int wear_leveling_worker(struct ubi_device *ubi,
struct ubi_work *wrk,
#ifdef CONFIG_MTD_UBI_FASTMAP
/* Check whether we need to produce an anchor PEB */
if (!anchor)
- anchor = !anchor_pebs_avalible(>free);
+ anchor = !anchor_pebs_avalible(>free, ubi->rd_threshold);
if (anchor) {
e1 = find_anchor_wl_entry(>used);
@@ -1613,6 +1634,8 @@ retry:
} else if (ubi_in_wl_tree(e, >scrub)) {
self_check_in_wl_tree(ubi, e, >scrub);
rb_erase(>u.rb, >scrub);
+ ubi_msg("PEB %d was pending scrubb",
+ e->pnum);
} else if (ubi_in_wl_tree(e, >erroneous)) {
self_check_in_wl_tree(ubi, e, >erroneous);
rb_erase(>u.rb, >erroneous);
@@ -1656,8 +1679,6 @@ int ubi_wl_scrub_peb(struct ubi_device *ubi, int pnum)
{
struct ubi_wl_entry *e;
- ubi_msg("schedule PEB %d for scrubbing", pnum);
-
retry:
spin_lock(>wl_lock);
e = ubi->lookuptbl[pnum];
@@ -1695,6 +1716,7 @@ retry:
}
}
+ ubi_msg("schedule PEB %d for scrubbing", p
[RFC/PATCH/RESEND 3/5 v2] mtd: ubi: Make in_wl_tree function public
Make the in_wl_tree function public to be used outside of wl.c.
Rename it to ubi_in_wl_tree.
Signed-off-by: Tanya Brokhman
---
drivers/mtd/ubi/ubi.h | 1 +
drivers/mtd/ubi/wl.c | 18 +-
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index e4c97ad..ed04de2 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -872,6 +872,7 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct
ubi_wl_entry *used_e,
int ubi_is_erase_work(struct ubi_work *wrk);
void ubi_refill_pools(struct ubi_device *ubi);
int ubi_ensure_anchor_pebs(struct ubi_device *ubi);
+int ubi_in_wl_tree(struct ubi_wl_entry *e, struct rb_root *root);
/* io.c */
int ubi_io_read(const struct ubi_device *ubi, void *buf, int pnum, int offset,
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 2b4e6fe..64c7dfd 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -291,14 +291,14 @@ static int produce_free_peb(struct ubi_device *ubi)
}
/**
- * in_wl_tree - check if wear-leveling entry is present in a WL RB-tree.
+ * ubi_in_wl_tree - check if wear-leveling entry is present in a WL RB-tree.
* @e: the wear-leveling entry to check
* @root: the root of the tree
*
* This function returns non-zero if @e is in the @root RB-tree and zero if it
* is not.
*/
-static int in_wl_tree(struct ubi_wl_entry *e, struct rb_root *root)
+int ubi_in_wl_tree(struct ubi_wl_entry *e, struct rb_root *root)
{
struct rb_node *p;
@@ -1607,13 +1607,13 @@ retry:
spin_unlock(>wl_lock);
return 0;
} else {
- if (in_wl_tree(e, >used)) {
+ if (ubi_in_wl_tree(e, >used)) {
self_check_in_wl_tree(ubi, e, >used);
rb_erase(>u.rb, >used);
- } else if (in_wl_tree(e, >scrub)) {
+ } else if (ubi_in_wl_tree(e, >scrub)) {
self_check_in_wl_tree(ubi, e, >scrub);
rb_erase(>u.rb, >scrub);
- } else if (in_wl_tree(e, >erroneous)) {
+ } else if (ubi_in_wl_tree(e, >erroneous)) {
self_check_in_wl_tree(ubi, e, >erroneous);
rb_erase(>u.rb, >erroneous);
ubi->erroneous_peb_count -= 1;
@@ -1661,8 +1661,8 @@ int ubi_wl_scrub_peb(struct ubi_device *ubi, int pnum)
retry:
spin_lock(>wl_lock);
e = ubi->lookuptbl[pnum];
- if (e == ubi->move_from || in_wl_tree(e, >scrub) ||
- in_wl_tree(e, >erroneous)) {
+ if (e == ubi->move_from || ubi_in_wl_tree(e, >scrub) ||
+ ubi_in_wl_tree(e, >erroneous)) {
spin_unlock(>wl_lock);
return 0;
}
@@ -1680,7 +1680,7 @@ retry:
goto retry;
}
- if (in_wl_tree(e, >used)) {
+ if (ubi_in_wl_tree(e, >used)) {
self_check_in_wl_tree(ubi, e, >used);
rb_erase(>u.rb, >used);
} else {
@@ -2150,7 +2150,7 @@ static int self_check_in_wl_tree(const struct ubi_device
*ubi,
if (!ubi_dbg_chk_gen(ubi))
return 0;
- if (in_wl_tree(e, root))
+ if (ubi_in_wl_tree(e, root))
return 0;
ubi_err("self-check failed for PEB %d, EC %d, RB-tree %p ",
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
[RFC/PATCH/RESEND 5/5 v2] mtd: ubi: Add sysfs entry to force all pebs' scan
A given eraseblock can be read many times or not at all.
We do not have the account of such eraseblocks that have
not been accessed since a pre-defined threshold interval.
By means of scanning the entire flash device it is possible to identify
all such eraseblocks.
Add a sysfs entry to force scan on all the PEBs on demand basis.
The sysfs entry would be available under /sys/class/ubi/ubiX/peb_scan
- echo 1 to this entry would trigger the scan, ignored if in progress
- On reading returns the scan status (1 = Scan executing, 0 = Scan not
executing)
Signed-off-by: Pratibhasagar V
Signed-off-by: Tanya Brokhman
---
drivers/mtd/ubi/build.c | 32 +--
drivers/mtd/ubi/ubi.h | 3 +
drivers/mtd/ubi/wl.c| 143 +++-
3 files changed, 171 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 34fe23a..a7464f8 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -154,6 +154,9 @@ static struct device_attribute dev_dt_threshold =
static struct device_attribute dev_rd_threshold =
__ATTR(rd_threshold, (S_IWUSR | S_IRUGO), dev_attribute_show,
dev_attribute_store);
+static struct device_attribute dev_mtd_trigger_scan =
+ __ATTR(peb_scan, (S_IWUSR | S_IRUGO),
+ dev_attribute_show, dev_attribute_store);
/**
* ubi_volume_notify - send a volume change notification.
@@ -395,6 +398,8 @@ static ssize_t dev_attribute_show(struct device *dev,
ret = sprintf(buf, "%d\n", ubi->dt_threshold);
else if (attr == _rd_threshold)
ret = sprintf(buf, "%d\n", ubi->rd_threshold);
+ else if (attr == _mtd_trigger_scan)
+ ret = sprintf(buf, "%d\n", ubi->scan_in_progress);
else
ret = -EINVAL;
@@ -406,7 +411,7 @@ static ssize_t dev_attribute_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- int value;
+ int value, ret;
struct ubi_device *ubi;
ubi = container_of(dev, struct ubi_device, dev);
@@ -414,8 +419,11 @@ static ssize_t dev_attribute_store(struct device *dev,
if (!ubi)
return -ENODEV;
- if (kstrtos32(buf, 10, ))
- return -EINVAL;
+ ret = count;
+ if (kstrtos32(buf, 10, )) {
+ ret = -EINVAL;
+ goto out;
+ }
/* Consider triggering full scan if threshods change */
else if (attr == _dt_threshold) {
if (value < UBI_MAX_DT_THRESHOLD)
@@ -429,9 +437,21 @@ static ssize_t dev_attribute_store(struct device *dev,
else
pr_err("Max supported threshold value is %d",
UBI_MAX_READCOUNTER);
+ } else if (attr == _mtd_trigger_scan) {
+ if (value != 1) {
+ pr_err("Invalid input. Echo 1 to start trigger");
+ goto out;
+ }
+ if (!ubi->lookuptbl) {
+ pr_err("lookuptbl is null");
+ goto out;
+ }
+ ret = ubi_wl_scan_all(ubi);
}
- return count;
+out:
+ ubi_put_device(ubi);
+ return ret;
}
static void dev_release(struct device *dev)
@@ -500,6 +520,9 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int *ref)
if (err)
return err;
err = device_create_file(>dev, _rd_threshold);
+ if (err)
+ return err;
+ err = device_create_file(>dev, _mtd_trigger_scan);
return err;
}
@@ -509,6 +532,7 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int *ref)
*/
static void ubi_sysfs_close(struct ubi_device *ubi)
{
+ device_remove_file(>dev, _mtd_trigger_scan);
device_remove_file(>dev, _mtd_num);
device_remove_file(>dev, _dt_threshold);
device_remove_file(>dev, _rd_threshold);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index ed04de2..1079517 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -491,6 +491,7 @@ struct ubi_debug_info {
* for more info
* @dt_threshold: data retention threshold. See UBI_DT_THRESHOLD
* for more info
+ * @scan_in_progress: true if scanning of device PEBs is in progress
*
* @flash_size: underlying MTD device size (in bytes)
* @peb_count: count of physical eraseblocks on the MTD device
@@ -595,6 +596,7 @@ struct ubi_device {
char bgt_name[sizeof(UBI_BGT_NAME_PATTERN)+2];
int rd_threshold;
int dt_threshold;
+ bool scan_in_progress;
/* I/O sub-system's stuff */
@@ -873,6 +875,7 @@ int ubi_is_erase_work(struct ubi_work *wrk);
void ubi_refill_pools(stru
[RFC/PATCH 1/5 v2] mtd: ubi: Read disturb infrastructure
The need for performing read disturb is determined according to new statistics collected per eraseblock: - read counter: incremented at each read operation reset at each erase - last erase time stamp: updated at each erase This patch adds the infrastructure for the above statistics Signed-off-by: Tanya Brokhman --- Changes from V1: - Documentation file was added Documentation/mtd/ubi/ubi-read-disturb.txt | 145 + drivers/mtd/ubi/build.c| 57 drivers/mtd/ubi/fastmap.c | 14 ++- drivers/mtd/ubi/ubi-media.h| 32 ++- drivers/mtd/ubi/ubi.h | 34 +++ drivers/mtd/ubi/wl.c | 6 ++ 6 files changed, 280 insertions(+), 8 deletions(-) create mode 100644 Documentation/mtd/ubi/ubi-read-disturb.txt diff --git a/Documentation/mtd/ubi/ubi-read-disturb.txt b/Documentation/mtd/ubi/ubi-read-disturb.txt new file mode 100644 index 000..4d3efef --- /dev/null +++ b/Documentation/mtd/ubi/ubi-read-disturb.txt @@ -0,0 +1,145 @@ + +1. Introduction +=== +Raw NAND flash memories are one of the most common storage devices in present +day embedded systems. The most common devices in which one can find raw NAND +flash cards in are mobile phones. +One of the limitations of the NAND devices is the method used to read NAND +flash memory may cause bit-flips on the surrounding cells and result in +uncorrectable ECC errors. This is known as the read disturb or data retention +failure. +Today’s Linux NAND drivers implementation doesn’t address the read disturb and +the data retention limitations of the NAND devices. + + +2. The problem +== +There are two characteristics of the raw NAND that are not addressed by the +NAND driver at the moment: + +2.1 Read Disturb + +The method used to read NAND flash memory can cause nearby cells in the same +memory block to change their value over time (become programmed). This +phenomenon is known as read disturb. The threshold number of reads that leads +to this issue is generally in the hundreds of thousands between intervening +erase operations. When reading continuously from one cell, that cell will not +fail but rather one of the surrounding cells may fail on a subsequent read. If +read disturb is not addressed, there is a high possibility of data loss - if +the errors are too numerous to correct. + +2.2 Data Retention +-- +Another NAND flash limitation is Data Retention (of rarely accessed blocks). +The ability of the NAND device to remain in its programmed state decreases over +time. + +To date these issues could be overlooked since the possibility of their +occurrence in today’s NAND devices is very low. With the evolution of NAND +devices and the requirement for a “long life” NAND flash, read disturb and data +retention can no longer be ignored otherwise there will be data loss over time. + + +3. The Solution +=== +Handling both of the described above types of blocks (read disturb and data +retention) is done by means of scrubbing. Scrubbing in essence is: +- Copy the data from block X to new block Y +- Erase block X + +3.1 Handling Read disturb blocks + +3.1.1 Identification +In order to identify potential read-disturb blocks, a read counter is +maintained per each PEB. The read counter is incremented as part of each read +operation, and is reset in every erase operation. +In each read operation the read counter is verified. This counter is also +verified at initiation phase, when attaching UBI to an MTD device. + +3.1.2 Saving on NAND +Due to the physical characteristics of the NAND flash memory, write operations +can only be performed on an erased block. Due to this, the read counter can’t +be saved as part of the meta-data that is saved on flash per each erase block, +and therefore can exist only in RAM. Once we power off the device, the read +counter will no longer be valid. In order to overcome this issue and to save +the read counter’s value through reboots of the system, it is saved as part of +the fastmap data on the flash. + +3.1.3 Error recovery +It is possible that the fastmap data won’t be valid on boot up - for example if +a sudden power cut occurred. In such case a default value will be assigned to +each PEB. The default value for the read counter will be assigned as follows: +- Free erase blocks: It’s safe to assume that the read counter for free + blocks was 0 prior to the power off since a block is marked as “free” + after it was erased. Such blocks will be assigned read counter 0. +- Allocated erase blocks: We can make no assumptions on the amount of + reads performed on allocated data blocks. To be on the safe side the + default read counter assigned to these blocks is the + read_disturb_threshold/2. + +3.1.4 Enhancements to Fastmap (work in progress
[RFC/PATCH 1/5 v2] mtd: ubi: Read disturb infrastructure
The need for performing read disturb is determined according to new statistics collected per eraseblock: - read counter: incremented at each read operation reset at each erase - last erase time stamp: updated at each erase This patch adds the infrastructure for the above statistics Signed-off-by: Tanya Brokhman tlin...@codeaurora.org --- Changes from V1: - Documentation file was added Documentation/mtd/ubi/ubi-read-disturb.txt | 145 + drivers/mtd/ubi/build.c| 57 drivers/mtd/ubi/fastmap.c | 14 ++- drivers/mtd/ubi/ubi-media.h| 32 ++- drivers/mtd/ubi/ubi.h | 34 +++ drivers/mtd/ubi/wl.c | 6 ++ 6 files changed, 280 insertions(+), 8 deletions(-) create mode 100644 Documentation/mtd/ubi/ubi-read-disturb.txt diff --git a/Documentation/mtd/ubi/ubi-read-disturb.txt b/Documentation/mtd/ubi/ubi-read-disturb.txt new file mode 100644 index 000..4d3efef --- /dev/null +++ b/Documentation/mtd/ubi/ubi-read-disturb.txt @@ -0,0 +1,145 @@ + +1. Introduction +=== +Raw NAND flash memories are one of the most common storage devices in present +day embedded systems. The most common devices in which one can find raw NAND +flash cards in are mobile phones. +One of the limitations of the NAND devices is the method used to read NAND +flash memory may cause bit-flips on the surrounding cells and result in +uncorrectable ECC errors. This is known as the read disturb or data retention +failure. +Today’s Linux NAND drivers implementation doesn’t address the read disturb and +the data retention limitations of the NAND devices. + + +2. The problem +== +There are two characteristics of the raw NAND that are not addressed by the +NAND driver at the moment: + +2.1 Read Disturb + +The method used to read NAND flash memory can cause nearby cells in the same +memory block to change their value over time (become programmed). This +phenomenon is known as read disturb. The threshold number of reads that leads +to this issue is generally in the hundreds of thousands between intervening +erase operations. When reading continuously from one cell, that cell will not +fail but rather one of the surrounding cells may fail on a subsequent read. If +read disturb is not addressed, there is a high possibility of data loss - if +the errors are too numerous to correct. + +2.2 Data Retention +-- +Another NAND flash limitation is Data Retention (of rarely accessed blocks). +The ability of the NAND device to remain in its programmed state decreases over +time. + +To date these issues could be overlooked since the possibility of their +occurrence in today’s NAND devices is very low. With the evolution of NAND +devices and the requirement for a “long life” NAND flash, read disturb and data +retention can no longer be ignored otherwise there will be data loss over time. + + +3. The Solution +=== +Handling both of the described above types of blocks (read disturb and data +retention) is done by means of scrubbing. Scrubbing in essence is: +- Copy the data from block X to new block Y +- Erase block X + +3.1 Handling Read disturb blocks + +3.1.1 Identification +In order to identify potential read-disturb blocks, a read counter is +maintained per each PEB. The read counter is incremented as part of each read +operation, and is reset in every erase operation. +In each read operation the read counter is verified. This counter is also +verified at initiation phase, when attaching UBI to an MTD device. + +3.1.2 Saving on NAND +Due to the physical characteristics of the NAND flash memory, write operations +can only be performed on an erased block. Due to this, the read counter can’t +be saved as part of the meta-data that is saved on flash per each erase block, +and therefore can exist only in RAM. Once we power off the device, the read +counter will no longer be valid. In order to overcome this issue and to save +the read counter’s value through reboots of the system, it is saved as part of +the fastmap data on the flash. + +3.1.3 Error recovery +It is possible that the fastmap data won’t be valid on boot up - for example if +a sudden power cut occurred. In such case a default value will be assigned to +each PEB. The default value for the read counter will be assigned as follows: +- Free erase blocks: It’s safe to assume that the read counter for free + blocks was 0 prior to the power off since a block is marked as “free” + after it was erased. Such blocks will be assigned read counter 0. +- Allocated erase blocks: We can make no assumptions on the amount of + reads performed on allocated data blocks. To be on the safe side the + default read counter assigned to these blocks is the + read_disturb_threshold/2. + +3.1.4 Enhancements to Fastmap
[RFC/PATCH/RESEND 4/5 v2] mtd: ubi: Read threshold verification
One of the criteria to scrub an eraseblock due to read disturb issue is
if that eraseblock was read from more times then a pre-defined threshold.
This is verified at each LEB read according to the read counter
parameter of the read PEB.
An eraseblock that is found needs scrubbing is added to the ubi-scrub
list.
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
---
drivers/mtd/ubi/eba.c | 7 ++-
drivers/mtd/ubi/wl.c | 56 +--
2 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 0e11671d..1c7b5f9 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -1,5 +1,8 @@
/*
* Copyright (c) International Business Machines Corp., 2006
+ * Copyright (c) 2014, Linux Foundation. All rights reserved.
+ * Linux Foundation chooses to take subject only to the GPLv2
+ * license terms, and distributes only under these terms.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -466,8 +469,10 @@ retry:
goto out_unlock;
}
}
+ if (ubi-lookuptbl[pnum]-rc = ubi-rd_threshold)
+ scrub = 1;
- if (scrub)
+ if (scrub !ubi_in_wl_tree(ubi-lookuptbl[pnum], ubi-scrub))
err = ubi_wl_scrub_peb(ubi, pnum);
leb_read_unlock(ubi, vol_id, lnum);
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 64c7dfd..a5d754f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -449,7 +449,7 @@ static struct ubi_wl_entry *find_anchor_wl_entry(struct
rb_root *root)
return victim;
}
-static int anchor_pebs_avalible(struct rb_root *root)
+static int anchor_pebs_avalible(struct rb_root *root, int rd_threshold)
{
struct rb_node *p;
struct ubi_wl_entry *e;
@@ -560,6 +560,12 @@ static void return_unused_pool_pebs(struct ubi_device *ubi,
for (i = pool-used; i pool-size; i++) {
e = ubi-lookuptbl[pool-pebs[i]];
+ /* If given PEB pending to be scrubbed - remove it */
+ if (ubi_in_wl_tree(e, ubi-scrub)) {
+ ubi_err(PEB %d was pending scrubb, e-pnum);
+ self_check_in_wl_tree(ubi, e, ubi-scrub);
+ rb_erase(e-u.rb, ubi-scrub);
+ }
wl_tree_add(e, ubi-free);
ubi-free_count++;
}
@@ -919,6 +925,13 @@ static int schedule_erase(struct ubi_device *ubi, struct
ubi_wl_entry *e,
wl_wrk-lnum = lnum;
wl_wrk-torture = torture;
+ /* If given PEB pending to be scrubbed - remove it */
+ if (ubi_in_wl_tree(e, ubi-scrub)) {
+ self_check_in_wl_tree(ubi, e, ubi-scrub);
+ rb_erase(e-u.rb, ubi-scrub);
+ ubi_msg(PEB %d was pending scrubb,
+ e-pnum);
+ }
schedule_ubi_work(ubi, wl_wrk);
return 0;
}
@@ -948,6 +961,14 @@ static int do_sync_erase(struct ubi_device *ubi, struct
ubi_wl_entry *e,
wl_wrk-lnum = lnum;
wl_wrk-torture = torture;
+ /* If given PEB pending to be scrubbed - remove it */
+ if (ubi_in_wl_tree(e, ubi-scrub)) {
+ self_check_in_wl_tree(ubi, e, ubi-scrub);
+ rb_erase(e-u.rb, ubi-scrub);
+ ubi_msg(PEB %d was pending scrubb,
+ e-pnum);
+ }
+
return erase_worker(ubi, wl_wrk, 0);
}
@@ -1052,7 +1073,7 @@ static int wear_leveling_worker(struct ubi_device *ubi,
struct ubi_work *wrk,
#ifdef CONFIG_MTD_UBI_FASTMAP
/* Check whether we need to produce an anchor PEB */
if (!anchor)
- anchor = !anchor_pebs_avalible(ubi-free);
+ anchor = !anchor_pebs_avalible(ubi-free, ubi-rd_threshold);
if (anchor) {
e1 = find_anchor_wl_entry(ubi-used);
@@ -1613,6 +1634,8 @@ retry:
} else if (ubi_in_wl_tree(e, ubi-scrub)) {
self_check_in_wl_tree(ubi, e, ubi-scrub);
rb_erase(e-u.rb, ubi-scrub);
+ ubi_msg(PEB %d was pending scrubb,
+ e-pnum);
} else if (ubi_in_wl_tree(e, ubi-erroneous)) {
self_check_in_wl_tree(ubi, e, ubi-erroneous);
rb_erase(e-u.rb, ubi-erroneous);
@@ -1656,8 +1679,6 @@ int ubi_wl_scrub_peb(struct ubi_device *ubi, int pnum)
{
struct ubi_wl_entry *e;
- ubi_msg(schedule PEB %d for scrubbing, pnum);
-
retry:
spin_lock(ubi-wl_lock);
e = ubi-lookuptbl[pnum];
@@ -1695,6 +1716,7 @@ retry:
}
}
+ ubi_msg(schedule PEB %d for scrubbing, pnum);
wl_tree_add(e, ubi-scrub);
spin_unlock(ubi-wl_lock);
@@ -1888,7 +1910,9 @@ int ubi_wl_init(struct ubi_device *ubi, struct
[RFC/PATCH/RESEND 3/5 v2] mtd: ubi: Make in_wl_tree function public
Make the in_wl_tree function public to be used outside of wl.c.
Rename it to ubi_in_wl_tree.
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
---
drivers/mtd/ubi/ubi.h | 1 +
drivers/mtd/ubi/wl.c | 18 +-
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index e4c97ad..ed04de2 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -872,6 +872,7 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct
ubi_wl_entry *used_e,
int ubi_is_erase_work(struct ubi_work *wrk);
void ubi_refill_pools(struct ubi_device *ubi);
int ubi_ensure_anchor_pebs(struct ubi_device *ubi);
+int ubi_in_wl_tree(struct ubi_wl_entry *e, struct rb_root *root);
/* io.c */
int ubi_io_read(const struct ubi_device *ubi, void *buf, int pnum, int offset,
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 2b4e6fe..64c7dfd 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -291,14 +291,14 @@ static int produce_free_peb(struct ubi_device *ubi)
}
/**
- * in_wl_tree - check if wear-leveling entry is present in a WL RB-tree.
+ * ubi_in_wl_tree - check if wear-leveling entry is present in a WL RB-tree.
* @e: the wear-leveling entry to check
* @root: the root of the tree
*
* This function returns non-zero if @e is in the @root RB-tree and zero if it
* is not.
*/
-static int in_wl_tree(struct ubi_wl_entry *e, struct rb_root *root)
+int ubi_in_wl_tree(struct ubi_wl_entry *e, struct rb_root *root)
{
struct rb_node *p;
@@ -1607,13 +1607,13 @@ retry:
spin_unlock(ubi-wl_lock);
return 0;
} else {
- if (in_wl_tree(e, ubi-used)) {
+ if (ubi_in_wl_tree(e, ubi-used)) {
self_check_in_wl_tree(ubi, e, ubi-used);
rb_erase(e-u.rb, ubi-used);
- } else if (in_wl_tree(e, ubi-scrub)) {
+ } else if (ubi_in_wl_tree(e, ubi-scrub)) {
self_check_in_wl_tree(ubi, e, ubi-scrub);
rb_erase(e-u.rb, ubi-scrub);
- } else if (in_wl_tree(e, ubi-erroneous)) {
+ } else if (ubi_in_wl_tree(e, ubi-erroneous)) {
self_check_in_wl_tree(ubi, e, ubi-erroneous);
rb_erase(e-u.rb, ubi-erroneous);
ubi-erroneous_peb_count -= 1;
@@ -1661,8 +1661,8 @@ int ubi_wl_scrub_peb(struct ubi_device *ubi, int pnum)
retry:
spin_lock(ubi-wl_lock);
e = ubi-lookuptbl[pnum];
- if (e == ubi-move_from || in_wl_tree(e, ubi-scrub) ||
- in_wl_tree(e, ubi-erroneous)) {
+ if (e == ubi-move_from || ubi_in_wl_tree(e, ubi-scrub) ||
+ ubi_in_wl_tree(e, ubi-erroneous)) {
spin_unlock(ubi-wl_lock);
return 0;
}
@@ -1680,7 +1680,7 @@ retry:
goto retry;
}
- if (in_wl_tree(e, ubi-used)) {
+ if (ubi_in_wl_tree(e, ubi-used)) {
self_check_in_wl_tree(ubi, e, ubi-used);
rb_erase(e-u.rb, ubi-used);
} else {
@@ -2150,7 +2150,7 @@ static int self_check_in_wl_tree(const struct ubi_device
*ubi,
if (!ubi_dbg_chk_gen(ubi))
return 0;
- if (in_wl_tree(e, root))
+ if (ubi_in_wl_tree(e, root))
return 0;
ubi_err(self-check failed for PEB %d, EC %d, RB-tree %p ,
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/
[RFC/PATCH/RESEND 5/5 v2] mtd: ubi: Add sysfs entry to force all pebs' scan
A given eraseblock can be read many times or not at all.
We do not have the account of such eraseblocks that have
not been accessed since a pre-defined threshold interval.
By means of scanning the entire flash device it is possible to identify
all such eraseblocks.
Add a sysfs entry to force scan on all the PEBs on demand basis.
The sysfs entry would be available under /sys/class/ubi/ubiX/peb_scan
- echo 1 to this entry would trigger the scan, ignored if in progress
- On reading returns the scan status (1 = Scan executing, 0 = Scan not
executing)
Signed-off-by: Pratibhasagar V prati...@codeaurora.org
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
---
drivers/mtd/ubi/build.c | 32 +--
drivers/mtd/ubi/ubi.h | 3 +
drivers/mtd/ubi/wl.c| 143 +++-
3 files changed, 171 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 34fe23a..a7464f8 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -154,6 +154,9 @@ static struct device_attribute dev_dt_threshold =
static struct device_attribute dev_rd_threshold =
__ATTR(rd_threshold, (S_IWUSR | S_IRUGO), dev_attribute_show,
dev_attribute_store);
+static struct device_attribute dev_mtd_trigger_scan =
+ __ATTR(peb_scan, (S_IWUSR | S_IRUGO),
+ dev_attribute_show, dev_attribute_store);
/**
* ubi_volume_notify - send a volume change notification.
@@ -395,6 +398,8 @@ static ssize_t dev_attribute_show(struct device *dev,
ret = sprintf(buf, %d\n, ubi-dt_threshold);
else if (attr == dev_rd_threshold)
ret = sprintf(buf, %d\n, ubi-rd_threshold);
+ else if (attr == dev_mtd_trigger_scan)
+ ret = sprintf(buf, %d\n, ubi-scan_in_progress);
else
ret = -EINVAL;
@@ -406,7 +411,7 @@ static ssize_t dev_attribute_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- int value;
+ int value, ret;
struct ubi_device *ubi;
ubi = container_of(dev, struct ubi_device, dev);
@@ -414,8 +419,11 @@ static ssize_t dev_attribute_store(struct device *dev,
if (!ubi)
return -ENODEV;
- if (kstrtos32(buf, 10, value))
- return -EINVAL;
+ ret = count;
+ if (kstrtos32(buf, 10, value)) {
+ ret = -EINVAL;
+ goto out;
+ }
/* Consider triggering full scan if threshods change */
else if (attr == dev_dt_threshold) {
if (value UBI_MAX_DT_THRESHOLD)
@@ -429,9 +437,21 @@ static ssize_t dev_attribute_store(struct device *dev,
else
pr_err(Max supported threshold value is %d,
UBI_MAX_READCOUNTER);
+ } else if (attr == dev_mtd_trigger_scan) {
+ if (value != 1) {
+ pr_err(Invalid input. Echo 1 to start trigger);
+ goto out;
+ }
+ if (!ubi-lookuptbl) {
+ pr_err(lookuptbl is null);
+ goto out;
+ }
+ ret = ubi_wl_scan_all(ubi);
}
- return count;
+out:
+ ubi_put_device(ubi);
+ return ret;
}
static void dev_release(struct device *dev)
@@ -500,6 +520,9 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int *ref)
if (err)
return err;
err = device_create_file(ubi-dev, dev_rd_threshold);
+ if (err)
+ return err;
+ err = device_create_file(ubi-dev, dev_mtd_trigger_scan);
return err;
}
@@ -509,6 +532,7 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int *ref)
*/
static void ubi_sysfs_close(struct ubi_device *ubi)
{
+ device_remove_file(ubi-dev, dev_mtd_trigger_scan);
device_remove_file(ubi-dev, dev_mtd_num);
device_remove_file(ubi-dev, dev_dt_threshold);
device_remove_file(ubi-dev, dev_rd_threshold);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index ed04de2..1079517 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -491,6 +491,7 @@ struct ubi_debug_info {
* for more info
* @dt_threshold: data retention threshold. See UBI_DT_THRESHOLD
* for more info
+ * @scan_in_progress: true if scanning of device PEBs is in progress
*
* @flash_size: underlying MTD device size (in bytes)
* @peb_count: count of physical eraseblocks on the MTD device
@@ -595,6 +596,7 @@ struct ubi_device {
char bgt_name[sizeof(UBI_BGT_NAME_PATTERN)+2];
int rd_threshold;
int dt_threshold;
+ bool scan_in_progress;
/* I/O sub-system's stuff */
@@ -873,6 +875,7 @@ int ubi_is_erase_work(struct ubi_work *wrk);
void ubi_refill_pools(struct
[RFC/PATCH/RESEND 2/5 v2] mtd: ubi: Fill read disturb statistics
Fill in eraseblock statistics needed for read disturb decision making:
1. read counter: incremented at each read operation
reset at each erase operation
Saved only as part of the meta data in RAM
On attach: if fastmap data is missing a default value is assigned
2. last erase time stamp: updated at each erase operation.
Saved as part of PEB OOB info on flash
On attach: if fastmap data is missing retrieved from PEB EC Header
on NAND.
If fastmap data is corrupted a default value is assigned.
The above parameters are saved as part of the fastmap data.
Signed-off-by: Dolev Raviv dra...@codeaurora.org
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
---
drivers/mtd/ubi/attach.c | 137 +++---
drivers/mtd/ubi/debug.c | 11
drivers/mtd/ubi/fastmap.c | 118 ++-
drivers/mtd/ubi/io.c | 28 ++
drivers/mtd/ubi/ubi.h | 24 +++-
drivers/mtd/ubi/vtbl.c| 6 +-
drivers/mtd/ubi/wl.c | 47
7 files changed, 322 insertions(+), 49 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..e102cac 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1,5 +1,8 @@
/*
* Copyright (c) International Business Machines Corp., 2006
+ * Copyright (c) 2014, Linux Foundation. All rights reserved.
+ * Linux Foundation chooses to take subject only to the GPLv2
+ * license terms, and distributes only under these terms.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -87,8 +90,21 @@
#include linux/crc32.h
#include linux/math64.h
#include linux/random.h
+#include linux/time.h
#include ubi.h
+#define set_aeb_default_values(aeb, ai)\
+ do {\
+ if (aeb-ec == UBI_UNKNOWN) { \
+ aeb-ec = ai-mean_ec; \
+ if (ai-mean_last_erase_time) \
+ aeb-last_erase_time = \
+ ai-mean_last_erase_time; \
+ else \
+ aeb-last_erase_time = UBI_DT_THRESHOLD / 2; \
+ } \
+ } while (0)
+
static int self_check_ai(struct ubi_device *ubi, struct ubi_attach_info *ai);
/* Temporary variables used during scanning */
@@ -102,6 +118,9 @@ static struct ubi_vid_hdr *vidh;
* @vol_id: the last used volume id for the PEB
* @lnum: the last used LEB number for the PEB
* @ec: erase counter of the physical eraseblock
+ * @last_erase_time: last erase time stamp (%UBI_UNKNOWN if it
+ * is unknown)
+ * @rc: read counter (%UBI_UNKNOWN if it is unknown)
* @to_head: if not zero, add to the head of the list
* @list: the list to add to
*
@@ -117,7 +136,8 @@ static struct ubi_vid_hdr *vidh;
* failure.
*/
static int add_to_list(struct ubi_attach_info *ai, int pnum, int vol_id,
- int lnum, int ec, int to_head, struct list_head *list)
+ int lnum, int ec, long last_erase_time, long rc,
+ int to_head, struct list_head *list)
{
struct ubi_ainf_peb *aeb;
@@ -139,6 +159,9 @@ static int add_to_list(struct ubi_attach_info *ai, int
pnum, int vol_id,
aeb-vol_id = vol_id;
aeb-lnum = lnum;
aeb-ec = ec;
+ aeb-rc = rc;
+ aeb-last_erase_time = last_erase_time;
+
if (to_head)
list_add(aeb-u.list, list);
else
@@ -151,13 +174,17 @@ static int add_to_list(struct ubi_attach_info *ai, int
pnum, int vol_id,
* @ai: attaching information
* @pnum: physical eraseblock number to add
* @ec: erase counter of the physical eraseblock
+ * @last_erase_time: last erase time stamp (%UBI_UNKNOWN if it
+ *is unknown)
+ * @rc: read counter (%UBI_UNKNOWN if it is unknown)
*
* This function allocates a 'struct ubi_ainf_peb' object for a corrupted
* physical eraseblock @pnum and adds it to the 'corr' list. The corruption
* was presumably not caused by a power cut. Returns zero in case of success
* and a negative error code in case of failure.
*/
-static int add_corrupted(struct ubi_attach_info *ai, int pnum, int ec)
+static int add_corrupted(struct ubi_attach_info *ai, int pnum,
+int ec, long rc, long last_erase_time)
{
struct ubi_ainf_peb *aeb;
@@ -170,6 +197,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
ai-corr_peb_count += 1;
aeb-pnum = pnum;
aeb-ec = ec;
+ aeb-rc = rc;
+ aeb-last_erase_time = last_erase_time;
list_add(aeb-u.list, ai-corr);
return 0;
}
@@ -434,6 +463,9 @@ out_free_vidh:
* @ai: attaching information
* @pnum: the physical eraseblock number
* @ec: erase
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/21/2014 10:56 AM, Artem Bityutskiy wrote: On Mon, 2014-10-20 at 19:57 +0300, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. The R/O block driver messages were replaced by pr_* since ubi_device structure is not used by it. Signed-off-by: Tanya Brokhman Pushed to linux-ubifs.git, thanks. You did not get indentations right, though, so I amended your patch. Also, some lines were split unnecessarily. My amendments are in the diff below. Please, take this into account for the similar UBIFS patch which you was going to send. Thank you! Thanks Artem and sorry for the extra work for you. I was trying to find the balance between pleasing checkpatch and your request to have the message in the same line :) Will be extra careful about the indentation next time (guess my editor needs re-configuration). Sorry about that. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/21/2014 10:56 AM, Artem Bityutskiy wrote: On Mon, 2014-10-20 at 19:57 +0300, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. The R/O block driver messages were replaced by pr_* since ubi_device structure is not used by it. Signed-off-by: Tanya Brokhman tlin...@codeaurora.org Pushed to linux-ubifs.git, thanks. You did not get indentations right, though, so I amended your patch. Also, some lines were split unnecessarily. My amendments are in the diff below. Please, take this into account for the similar UBIFS patch which you was going to send. Thank you! Thanks Artem and sorry for the extra work for you. I was trying to find the balance between pleasing checkpatch and your request to have the message in the same line :) Will be extra careful about the indentation next time (guess my editor needs re-configuration). Sorry about that. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
[PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
The R/O block driver messages were replaced by pr_* since
ubi_device structure is not used by it.
Signed-off-by: Tanya Brokhman
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Changes from V3:
- Compilation fix of block.c
- All ubi_* messages in block.c were replaced with pr_*
Changes from V4:
- Addressed comments on block.c
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
drivers/mtd/ubi/attach.c | 126 +++---
drivers/mtd/ubi/block.c | 33 +-
drivers/mtd/ubi/build.c | 122 -
drivers/mtd/ubi/cdev.c| 36 +--
drivers/mtd/ubi/debug.c | 8 +--
drivers/mtd/ubi/eba.c | 54 +
drivers/mtd/ubi/fastmap.c | 96 ++---
drivers/mtd/ubi/io.c | 150 --
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 13 ++--
drivers/mtd/ubi/vmt.c | 69 +++--
drivers/mtd/ubi/vtbl.c| 49 ---
drivers/mtd/ubi/wl.c | 70 --
14 files changed, 437 insertions(+), 399 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..3da5df1 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr->vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av->vol_id) {
- ubi_err("inconsistent vol_id");
+ ubi_err(ubi, "inconsistent vol_id");
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err("inconsistent vol_type");
+ ubi_err(ubi, "inconsistent vol_type");
goto bad;
}
if (used_ebs != av->used_ebs) {
- ubi_err("inconsistent used_ebs");
+ ubi_err(ubi, "inconsistent used_ebs");
goto bad;
}
if (data_pad != av->data_pad) {
- ubi_err("inconsistent data_pad");
+ ubi_err(ubi, "inconsistent data_pad");
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err("inconsistent VID header at PEB %d", pnum);
+ ubi_err(ubi, "inconsistent VID header at PEB %d", pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
- ubi_err("unsupported on-flash UBI format");
+ ubi_err(ubi, "unsupported on-flash UBI format");
return -EINVAL;
}
@@ -377,7 +379,7
Re: [PATCH v4] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/20/2014 4:51 PM, Artem Bityutskiy wrote: On Tue, 2014-10-14 at 09:05 -0700, Joe Perches wrote: It's pretty trivial when all the lines are already being touched. OK, but then the same change should done in UBIFS, because it's ubifs_msg() and so on macros are consistent with UBI macros. So I think if this is done, then it is done separately for both UBI and UBIFS. Artem. We have similar patch for ubifs as well, one that adds ubi number to all ubifs messages. We just didn't get to sharing it yet. I prefer not adding the "\n" to my patch unless Artem insists on it. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH v4] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/20/2014 4:51 PM, Artem Bityutskiy wrote: On Tue, 2014-10-14 at 09:05 -0700, Joe Perches wrote: It's pretty trivial when all the lines are already being touched. OK, but then the same change should done in UBIFS, because it's ubifs_msg() and so on macros are consistent with UBI macros. So I think if this is done, then it is done separately for both UBI and UBIFS. Artem. We have similar patch for ubifs as well, one that adds ubi number to all ubifs messages. We just didn't get to sharing it yet. I prefer not adding the \n to my patch unless Artem insists on it. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
[PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
The R/O block driver messages were replaced by pr_* since
ubi_device structure is not used by it.
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Changes from V3:
- Compilation fix of block.c
- All ubi_* messages in block.c were replaced with pr_*
Changes from V4:
- Addressed comments on block.c
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
drivers/mtd/ubi/attach.c | 126 +++---
drivers/mtd/ubi/block.c | 33 +-
drivers/mtd/ubi/build.c | 122 -
drivers/mtd/ubi/cdev.c| 36 +--
drivers/mtd/ubi/debug.c | 8 +--
drivers/mtd/ubi/eba.c | 54 +
drivers/mtd/ubi/fastmap.c | 96 ++---
drivers/mtd/ubi/io.c | 150 --
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 13 ++--
drivers/mtd/ubi/vmt.c | 69 +++--
drivers/mtd/ubi/vtbl.c| 49 ---
drivers/mtd/ubi/wl.c | 70 --
14 files changed, 437 insertions(+), 399 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..3da5df1 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr-vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av-vol_id) {
- ubi_err(inconsistent vol_id);
+ ubi_err(ubi, inconsistent vol_id);
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err(inconsistent vol_type);
+ ubi_err(ubi, inconsistent vol_type);
goto bad;
}
if (used_ebs != av-used_ebs) {
- ubi_err(inconsistent used_ebs);
+ ubi_err(ubi, inconsistent used_ebs);
goto bad;
}
if (data_pad != av-data_pad) {
- ubi_err(inconsistent data_pad);
+ ubi_err(ubi, inconsistent data_pad);
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err(inconsistent VID header at PEB %d, pnum);
+ ubi_err(ubi, inconsistent VID header at PEB %d, pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
- ubi_err(unsupported on-flash UBI format);
+ ubi_err(ubi, unsupported on-flash UBI format);
return -EINVAL;
}
@@ -377,7 +379,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
if (err == UBI_IO_BITFLIPS
Re: [PATCH v4] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/14/2014 5:13 PM, Tanya Brokhman wrote:
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
The R/O block driver messages were replaced by pr_* since
ubi_device structure is not used by it.
Signed-off-by: Tanya Brokhman
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Changes from V3:
- Compilation fix of block.c
- All ubi_* messages in block.c were replaced with pr_*
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
Forgot to mention that I compiled based on linux-next branch of
git://git.infradead.org/linux-ubifs.git
as requested.
drivers/mtd/ubi/attach.c | 126 +++
drivers/mtd/ubi/block.c | 25
drivers/mtd/ubi/build.c | 122 -
drivers/mtd/ubi/cdev.c| 36 +--
drivers/mtd/ubi/debug.c | 8 +--
drivers/mtd/ubi/eba.c | 57 +-
drivers/mtd/ubi/fastmap.c | 100 +++
drivers/mtd/ubi/io.c | 149 +++---
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 13 ++--
drivers/mtd/ubi/vmt.c | 68 +++--
drivers/mtd/ubi/vtbl.c| 48 ---
drivers/mtd/ubi/wl.c | 70 --
14 files changed, 426 insertions(+), 406 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..2a44ceb 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr->vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av->vol_id) {
- ubi_err("inconsistent vol_id");
+ ubi_err(ubi, "inconsistent vol_id");
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err("inconsistent vol_type");
+ ubi_err(ubi, "inconsistent vol_type");
goto bad;
}
if (used_ebs != av->used_ebs) {
- ubi_err("inconsistent used_ebs");
+ ubi_err(ubi, "inconsistent used_ebs");
goto bad;
}
if (data_pad != av->data_pad) {
- ubi_err("inconsistent data_pad");
+ ubi_err(ubi, "inconsistent data_pad");
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err("inconsistent VID header at PEB %d", pnum);
+ ubi_err(ubi, "inconsistent VID header at PEB %d", pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
- ubi_err("unsupported on-flash UBI format");
+
[PATCH v4] mtd: ubi: Extend UBI layer debug/messaging capabilities
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
The R/O block driver messages were replaced by pr_* since
ubi_device structure is not used by it.
Signed-off-by: Tanya Brokhman
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Changes from V3:
- Compilation fix of block.c
- All ubi_* messages in block.c were replaced with pr_*
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
drivers/mtd/ubi/attach.c | 126 +++
drivers/mtd/ubi/block.c | 25
drivers/mtd/ubi/build.c | 122 -
drivers/mtd/ubi/cdev.c| 36 +--
drivers/mtd/ubi/debug.c | 8 +--
drivers/mtd/ubi/eba.c | 57 +-
drivers/mtd/ubi/fastmap.c | 100 +++
drivers/mtd/ubi/io.c | 149 +++---
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 13 ++--
drivers/mtd/ubi/vmt.c | 68 +++--
drivers/mtd/ubi/vtbl.c| 48 ---
drivers/mtd/ubi/wl.c | 70 --
14 files changed, 426 insertions(+), 406 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..2a44ceb 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr->vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av->vol_id) {
- ubi_err("inconsistent vol_id");
+ ubi_err(ubi, "inconsistent vol_id");
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err("inconsistent vol_type");
+ ubi_err(ubi, "inconsistent vol_type");
goto bad;
}
if (used_ebs != av->used_ebs) {
- ubi_err("inconsistent used_ebs");
+ ubi_err(ubi, "inconsistent used_ebs");
goto bad;
}
if (data_pad != av->data_pad) {
- ubi_err("inconsistent data_pad");
+ ubi_err(ubi, "inconsistent data_pad");
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err("inconsistent VID header at PEB %d", pnum);
+ ubi_err(ubi, "inconsistent VID header at PEB %d", pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
- ubi_err("unsupported on-flash UBI format");
+ ubi_err(ubi, "unsupported on-flash UBI format");
return -EINVAL;
}
@@ -377,7 +379,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb
Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue
On 10/14/2014 4:02 PM, Artem Bityutskiy wrote: On Tue, 2014-10-14 at 15:21 +0300, Tanya Brokhman wrote: Hi Artem/Richard I think your discussion here stopped being relevant to this specific patch but went on to the fastmap feature design in general :) This patch fixes a real bug in the current implementation of the feature. What you're discussing requires a re-writing and re-design of the feature. Perhaps this one can be merged and will be "fixed" later on when you agree on how you would like FM to access WL data structures in general? First of all, "re-writing and re-design of the feature" is an overstatement. So far this is on the "cleaning things up" side of the spectrum, closer to the "re-factoring" area. WRT "merge the fix now and improve later" - this is a good argument for an "inside a company" discussion, where the primary TTM is the driving factor. For the community TTM is a good thing, but quality comes first. Now, if this was about a regression, one could apply time pressure on the maintainer. But we are talking about a problem which was there from day 0. It is completely normal for the maintainer to push back various hot-fixes for the problem and request some reasonable re-factoring first. This is what I do. This is very very usual thing in the Linux community. So far I did not ask anything huge and unreasonable, I think. Just cleaner inter-subsystem APIs, less of the "fastmap uses the other subsystems' internals" kind of things. -- Artem. Ok, accepted. It was just a suggestion. I'm all for quality coming first, even if you were asking for something "huge". Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue
On 10/14/2014 1:23 PM, Artem Bityutskiy wrote: On Mon, 2014-10-13 at 23:04 +0200, Richard Weinberger wrote: Am 13.10.2014 um 17:23 schrieb Artem Bityutskiy: Well, used and free are RB-trees, looking them up is slow. This is true but we'd have to look it up in multiple trees and the protection queue... Right. 2 RB-trees, and one list. The list is empty most of the time, or contains one element. So we'd look-up 2 RB-trees most of the time. Very rarely we'd need to look at the list containing very few elements. Not that bad, I think. ubi_update_fastmap() takes ubi->wl_lock anyway to block any changes in the free, used, etc. trees to make sure that the to be taken state snapshot is consistent. I think this is fine. But there is a price - memory consumption. We do not want to pay it just for making the inter-subsystems boundaries better, there ought to be a better reason. Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would cost 256KiB of RAM. Is 128KiB PEB size still realistic on modern NANDs? Even if, 256KiB are not much and the kernel consumes this additionally with every new release. Right, but the point is that bigger systems use eMMC and wouldn't bother with raw flash. Raw flash trending down the smaller systems, where a hundred of kilobytes matters. But I can understand your concerns. Thanks, yes, my point is to be careful about the RAM we consume, and try to avoid growing this data structure, an only do it if we have to. Okay, I'll try harder to make you happy. Well, making me happy is not the point of course :-) Thanks! -- Artem __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ Hi Artem/Richard I think your discussion here stopped being relevant to this specific patch but went on to the fastmap feature design in general :) This patch fixes a real bug in the current implementation of the feature. What you're discussing requires a re-writing and re-design of the feature. Perhaps this one can be merged and will be "fixed" later on when you agree on how you would like FM to access WL data structures in general? Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH v3] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/13/2014 6:59 PM, Artem Bityutskiy wrote: On Mon, 2014-10-13 at 18:37 +0300, Artem Bityutskiy wrote: On Mon, 2014-10-06 at 14:01 +0300, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. Looks good to me, pushed to the master branch of the linux-ubifs.git tree. Later, when the merge window is closed, I'll merge this patch to the linux-next branch too. Tanya, sorry, I was not careful enough, I merged it and tested against the Linuses head, it is fine. But it does not apply the the linux-ubifs.git tree. There are conflicts. But more importantly, you did not get the 'block.c' right. There we use the same printing macros, but we do not have 'struct ubi_info' there at all. Please, enable the R/O block layer feature and try to compile, it'll fail. The block driver is in 'drivers/mtd/ubi', but it is kind of a separate driver - it does not access the internal UBI data structures. I guess the solution would be to just use pr_* functions there instead. CCing Ezequiel. Please, submit a patch against the 'linux-next' branch of this tree: git://git.infradead.org/linux-ubifs.git Artem. Ok, will do ASAP. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH v3] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/13/2014 6:59 PM, Artem Bityutskiy wrote: On Mon, 2014-10-13 at 18:37 +0300, Artem Bityutskiy wrote: On Mon, 2014-10-06 at 14:01 +0300, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. Looks good to me, pushed to the master branch of the linux-ubifs.git tree. Later, when the merge window is closed, I'll merge this patch to the linux-next branch too. Tanya, sorry, I was not careful enough, I merged it and tested against the Linuses head, it is fine. But it does not apply the the linux-ubifs.git tree. There are conflicts. But more importantly, you did not get the 'block.c' right. There we use the same printing macros, but we do not have 'struct ubi_info' there at all. Please, enable the R/O block layer feature and try to compile, it'll fail. The block driver is in 'drivers/mtd/ubi', but it is kind of a separate driver - it does not access the internal UBI data structures. I guess the solution would be to just use pr_* functions there instead. CCing Ezequiel. Please, submit a patch against the 'linux-next' branch of this tree: git://git.infradead.org/linux-ubifs.git Artem. Ok, will do ASAP. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue
On 10/14/2014 1:23 PM, Artem Bityutskiy wrote: On Mon, 2014-10-13 at 23:04 +0200, Richard Weinberger wrote: Am 13.10.2014 um 17:23 schrieb Artem Bityutskiy: Well, used and free are RB-trees, looking them up is slow. This is true but we'd have to look it up in multiple trees and the protection queue... Right. 2 RB-trees, and one list. The list is empty most of the time, or contains one element. So we'd look-up 2 RB-trees most of the time. Very rarely we'd need to look at the list containing very few elements. Not that bad, I think. ubi_update_fastmap() takes ubi-wl_lock anyway to block any changes in the free, used, etc. trees to make sure that the to be taken state snapshot is consistent. I think this is fine. But there is a price - memory consumption. We do not want to pay it just for making the inter-subsystems boundaries better, there ought to be a better reason. Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would cost 256KiB of RAM. Is 128KiB PEB size still realistic on modern NANDs? Even if, 256KiB are not much and the kernel consumes this additionally with every new release. Right, but the point is that bigger systems use eMMC and wouldn't bother with raw flash. Raw flash trending down the smaller systems, where a hundred of kilobytes matters. But I can understand your concerns. Thanks, yes, my point is to be careful about the RAM we consume, and try to avoid growing this data structure, an only do it if we have to. Okay, I'll try harder to make you happy. Well, making me happy is not the point of course :-) Thanks! -- Artem __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ Hi Artem/Richard I think your discussion here stopped being relevant to this specific patch but went on to the fastmap feature design in general :) This patch fixes a real bug in the current implementation of the feature. What you're discussing requires a re-writing and re-design of the feature. Perhaps this one can be merged and will be fixed later on when you agree on how you would like FM to access WL data structures in general? Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue
On 10/14/2014 4:02 PM, Artem Bityutskiy wrote: On Tue, 2014-10-14 at 15:21 +0300, Tanya Brokhman wrote: Hi Artem/Richard I think your discussion here stopped being relevant to this specific patch but went on to the fastmap feature design in general :) This patch fixes a real bug in the current implementation of the feature. What you're discussing requires a re-writing and re-design of the feature. Perhaps this one can be merged and will be fixed later on when you agree on how you would like FM to access WL data structures in general? First of all, re-writing and re-design of the feature is an overstatement. So far this is on the cleaning things up side of the spectrum, closer to the re-factoring area. WRT merge the fix now and improve later - this is a good argument for an inside a company discussion, where the primary TTM is the driving factor. For the community TTM is a good thing, but quality comes first. Now, if this was about a regression, one could apply time pressure on the maintainer. But we are talking about a problem which was there from day 0. It is completely normal for the maintainer to push back various hot-fixes for the problem and request some reasonable re-factoring first. This is what I do. This is very very usual thing in the Linux community. So far I did not ask anything huge and unreasonable, I think. Just cleaner inter-subsystem APIs, less of the fastmap uses the other subsystems' internals kind of things. -- Artem. Ok, accepted. It was just a suggestion. I'm all for quality coming first, even if you were asking for something huge. Thanks, Tanya Brokhman -- Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/
[PATCH v4] mtd: ubi: Extend UBI layer debug/messaging capabilities
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
The R/O block driver messages were replaced by pr_* since
ubi_device structure is not used by it.
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Changes from V3:
- Compilation fix of block.c
- All ubi_* messages in block.c were replaced with pr_*
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
drivers/mtd/ubi/attach.c | 126 +++
drivers/mtd/ubi/block.c | 25
drivers/mtd/ubi/build.c | 122 -
drivers/mtd/ubi/cdev.c| 36 +--
drivers/mtd/ubi/debug.c | 8 +--
drivers/mtd/ubi/eba.c | 57 +-
drivers/mtd/ubi/fastmap.c | 100 +++
drivers/mtd/ubi/io.c | 149 +++---
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 13 ++--
drivers/mtd/ubi/vmt.c | 68 +++--
drivers/mtd/ubi/vtbl.c| 48 ---
drivers/mtd/ubi/wl.c | 70 --
14 files changed, 426 insertions(+), 406 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..2a44ceb 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr-vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av-vol_id) {
- ubi_err(inconsistent vol_id);
+ ubi_err(ubi, inconsistent vol_id);
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err(inconsistent vol_type);
+ ubi_err(ubi, inconsistent vol_type);
goto bad;
}
if (used_ebs != av-used_ebs) {
- ubi_err(inconsistent used_ebs);
+ ubi_err(ubi, inconsistent used_ebs);
goto bad;
}
if (data_pad != av-data_pad) {
- ubi_err(inconsistent data_pad);
+ ubi_err(ubi, inconsistent data_pad);
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err(inconsistent VID header at PEB %d, pnum);
+ ubi_err(ubi, inconsistent VID header at PEB %d, pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
- ubi_err(unsupported on-flash UBI format);
+ ubi_err(ubi, unsupported on-flash UBI format);
return -EINVAL;
}
@@ -377,7 +379,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
if (err == UBI_IO_BITFLIPS)
bitflips = 1;
else
Re: [PATCH v4] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 10/14/2014 5:13 PM, Tanya Brokhman wrote:
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
The R/O block driver messages were replaced by pr_* since
ubi_device structure is not used by it.
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Changes from V3:
- Compilation fix of block.c
- All ubi_* messages in block.c were replaced with pr_*
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
Forgot to mention that I compiled based on linux-next branch of
git://git.infradead.org/linux-ubifs.git
as requested.
drivers/mtd/ubi/attach.c | 126 +++
drivers/mtd/ubi/block.c | 25
drivers/mtd/ubi/build.c | 122 -
drivers/mtd/ubi/cdev.c| 36 +--
drivers/mtd/ubi/debug.c | 8 +--
drivers/mtd/ubi/eba.c | 57 +-
drivers/mtd/ubi/fastmap.c | 100 +++
drivers/mtd/ubi/io.c | 149 +++---
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 13 ++--
drivers/mtd/ubi/vmt.c | 68 +++--
drivers/mtd/ubi/vtbl.c| 48 ---
drivers/mtd/ubi/wl.c | 70 --
14 files changed, 426 insertions(+), 406 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..2a44ceb 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr-vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av-vol_id) {
- ubi_err(inconsistent vol_id);
+ ubi_err(ubi, inconsistent vol_id);
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err(inconsistent vol_type);
+ ubi_err(ubi, inconsistent vol_type);
goto bad;
}
if (used_ebs != av-used_ebs) {
- ubi_err(inconsistent used_ebs);
+ ubi_err(ubi, inconsistent used_ebs);
goto bad;
}
if (data_pad != av-data_pad) {
- ubi_err(inconsistent data_pad);
+ ubi_err(ubi, inconsistent data_pad);
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err(inconsistent VID header at PEB %d, pnum);
+ ubi_err(ubi, inconsistent VID header at PEB %d, pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
- ubi_err(unsupported on-flash UBI format);
+ ubi_err(ubi, unsupported on-flash UBI format);
return -EINVAL;
}
@@ -377,7 +379,7 @@ int ubi_compare_lebs
[PATCH v3] mtd: ubi: Extend UBI layer debug/messaging capabilities
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
Signed-off-by: Tanya Brokhman
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
drivers/mtd/ubi/attach.c | 126 +++
drivers/mtd/ubi/block.c | 26
drivers/mtd/ubi/build.c | 122 -
drivers/mtd/ubi/cdev.c| 36 +--
drivers/mtd/ubi/debug.c | 8 +--
drivers/mtd/ubi/eba.c | 57 +-
drivers/mtd/ubi/fastmap.c | 100 +++
drivers/mtd/ubi/io.c | 149 +++---
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 13 ++--
drivers/mtd/ubi/vmt.c | 68 +++--
drivers/mtd/ubi/vtbl.c| 48 ---
drivers/mtd/ubi/wl.c | 72 --
14 files changed, 427 insertions(+), 408 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..2a44ceb 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr->vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av->vol_id) {
- ubi_err("inconsistent vol_id");
+ ubi_err(ubi, "inconsistent vol_id");
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err("inconsistent vol_type");
+ ubi_err(ubi, "inconsistent vol_type");
goto bad;
}
if (used_ebs != av->used_ebs) {
- ubi_err("inconsistent used_ebs");
+ ubi_err(ubi, "inconsistent used_ebs");
goto bad;
}
if (data_pad != av->data_pad) {
- ubi_err("inconsistent data_pad");
+ ubi_err(ubi, "inconsistent data_pad");
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err("inconsistent VID header at PEB %d", pnum);
+ ubi_err(ubi, "inconsistent VID header at PEB %d", pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
- ubi_err("unsupported on-flash UBI format");
+ ubi_err(ubi, "unsupported on-flash UBI format");
return -EINVAL;
}
@@ -377,7 +379,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
if (err == UBI_IO_BITFLIPS)
bitflips = 1;
else {
- ubi_err("VID of PEB %d header is bad, b
[PATCH v3] mtd: ubi: Extend UBI layer debug/messaging capabilities
If there is more then one UBI device mounted, there is no way to
distinguish between messages from different UBI devices.
Add device number to all ubi layer message types.
Signed-off-by: Tanya Brokhman tlin...@codeaurora.org
---
Changes from V1:
- Compilation error fixed
- macros were updated to receive the ubi structure as
parameter and not just ubi_number
- Places in code, where ubi-messaging macros were used, but
ubi struct is not present yet (init phase), were updated
to just use pr_err or similar
Changes from V2:
- multi line messages layout, as requested by Artem
note that due to these changes checkpatch fails
Note for reviewers:
The updated macros are all in ubi.h. All other file changes are just
technical changes for compilation (usage of the defined macros)
drivers/mtd/ubi/attach.c | 126 +++
drivers/mtd/ubi/block.c | 26
drivers/mtd/ubi/build.c | 122 -
drivers/mtd/ubi/cdev.c| 36 +--
drivers/mtd/ubi/debug.c | 8 +--
drivers/mtd/ubi/eba.c | 57 +-
drivers/mtd/ubi/fastmap.c | 100 +++
drivers/mtd/ubi/io.c | 149 +++---
drivers/mtd/ubi/kapi.c| 6 +-
drivers/mtd/ubi/misc.c| 4 +-
drivers/mtd/ubi/ubi.h | 13 ++--
drivers/mtd/ubi/vmt.c | 68 +++--
drivers/mtd/ubi/vtbl.c| 48 ---
drivers/mtd/ubi/wl.c | 72 --
14 files changed, 427 insertions(+), 408 deletions(-)
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 6f27d9a..2a44ceb 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -188,7 +189,8 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
* information in the VID header is consistent to the information in other VID
* headers of the same volume.
*/
-static int validate_vid_hdr(const struct ubi_vid_hdr *vid_hdr,
+static int validate_vid_hdr(const struct ubi_device *ubi,
+ const struct ubi_vid_hdr *vid_hdr,
const struct ubi_ainf_volume *av, int pnum)
{
int vol_type = vid_hdr-vol_type;
@@ -206,7 +208,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
*/
if (vol_id != av-vol_id) {
- ubi_err(inconsistent vol_id);
+ ubi_err(ubi, inconsistent vol_id);
goto bad;
}
@@ -216,17 +218,17 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
av_vol_type = UBI_VID_DYNAMIC;
if (vol_type != av_vol_type) {
- ubi_err(inconsistent vol_type);
+ ubi_err(ubi, inconsistent vol_type);
goto bad;
}
if (used_ebs != av-used_ebs) {
- ubi_err(inconsistent used_ebs);
+ ubi_err(ubi, inconsistent used_ebs);
goto bad;
}
if (data_pad != av-data_pad) {
- ubi_err(inconsistent data_pad);
+ ubi_err(ubi, inconsistent data_pad);
goto bad;
}
}
@@ -234,7 +236,7 @@ static int validate_vid_hdr(const struct ubi_vid_hdr
*vid_hdr,
return 0;
bad:
- ubi_err(inconsistent VID header at PEB %d, pnum);
+ ubi_err(ubi, inconsistent VID header at PEB %d, pnum);
ubi_dump_vid_hdr(vid_hdr);
ubi_dump_av(av);
return -EINVAL;
@@ -336,7 +338,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
* support these images anymore. Well, those images still work,
* but only if no unclean reboots happened.
*/
- ubi_err(unsupported on-flash UBI format);
+ ubi_err(ubi, unsupported on-flash UBI format);
return -EINVAL;
}
@@ -377,7 +379,7 @@ int ubi_compare_lebs(struct ubi_device *ubi, const struct
ubi_ainf_peb *aeb,
if (err == UBI_IO_BITFLIPS)
bitflips = 1;
else {
- ubi_err(VID of PEB %d header is bad, but it
was OK earlier, err %d,
+ ubi_err(ubi, VID of PEB %d header is bad, but
it was OK earlier, err %d
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
On 9/30/2014 10:44 AM, Richard Weinberger wrote: Am 30.09.2014 09:39, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote: Am 30.09.2014 08:45, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: + spin_lock(>wl_lock); + ubi->fm_work_scheduled = 0; + spin_unlock(>wl_lock); Andrew Morton once said me that if I am protecting an integer change like this with a spinlock, I have a problem in my locking design. He was right for my particular case. Integer is changes atomic. The only other thing spinlock adds are the barriers. I've added the spinlock to have a barrier in any case. Examples of any? You mean a case where the compiler would reorder code and the barrier is needed? I don't have one, but I'm not that creative as a modern C compiler. If you say that no barrier is needed I'll trust you. :-) we just implemented the same thing :) It's being tested Why not use atomic_t fm_work_scheduled and save the spin_lock? Thanks, //richard __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- 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/
Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue
On 10/2/2014 4:32 PM, Richard Weinberger wrote:
Am 02.10.2014 15:28, schrieb Tanya Brokhman:
Hi Richard
On 9/30/2014 1:20 AM, Richard Weinberger wrote:
Fastmap can miss a PEB if it is in the protection queue
and not jet in the used tree.
Treat every protected PEB as used.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/fastmap.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 2b0d8d6..2853a69 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1195,6 +1195,19 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
fm_pos += sizeof(*fec);
ubi_assert(fm_pos <= ubi->fm_size);
}
+
+for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
+list_for_each_entry(wl_e, >pq[i], u.list) {
why not list_for_each_entry_safe?
Because we don't delete elements from this list while iterating over it.
+fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
+
+fec->pnum = cpu_to_be32(wl_e->pnum);
+fec->ec = cpu_to_be32(wl_e->ec);
+
+used_peb_count++;
+fm_pos += sizeof(*fec);
+ubi_assert(fm_pos <= ubi->fm_size);
Is fm_size ok with this addition or does it needs updating as well?
It is okay. The fastmap size calculation reserves enough space for all possible
PEBs.
Thanks,
//richard
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Reviewed-by: Tanya Brokhman
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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/
Re: [RFC/PATCH 1/5] mtd: ubi: Read disturb infrastructure
On 10/2/2014 4:36 PM, Richard Weinberger wrote:
Am 02.10.2014 14:50, schrieb Tanya Brokhman:
Hi Richard,
Sorry it took me some time to answer, got per-occupied with some urgent staff.
On 9/28/2014 1:54 PM, Richard Weinberger wrote:
Am 28.09.2014 12:46, schrieb Tanya Brokhman:
On 9/28/2014 11:54 AM, Richard Weinberger wrote:
Am 28.09.2014 10:48, schrieb Tanya Brokhman:
@@ -424,6 +440,8 @@ struct ubi_fm_sb {
__be32 used_blocks;
__be32 block_loc[UBI_FM_MAX_BLOCKS];
__be32 block_ec[UBI_FM_MAX_BLOCKS];
+__be32 block_rc[UBI_FM_MAX_BLOCKS];
+__be64 block_let[UBI_FM_MAX_BLOCKS];
Doesn't this break the fastmap on-disk layout?
What do you mean "break"? I verified fastmap feature is working. the whole
read-disturb depends on it so I tested this thoroughly.
Did you write a fastmap with your changes applied and then an attach using a
fastmap implementation *without*
you changes?
I bet it will not work because the disk layout is now different.
you're right, it wont work. I did a set of attach/detach tests to verify
fastmap, but of course with my changes.
Linux is not the only user of fastmap. We need to be very careful here.
Could you please elaborate here? I'm not sure I understand the use case you're
referring to.
Consider the case where you have a board with a fastmap enabled bootloader and
a Linux OS.
The bootloader does a fastmap attach and boots the kernel from UBI and the
kernel it self has the rootfs
on UBI too. If you install a new kernel with your changes applied it will write
the fastmap in a different
format and the bootloader will fail badly. In worst case the board bricks, in
best case the bootloader can fall back
to scanning mode but it will be slow and the customer unhappy.
Ok, I understand the problem now. I wanted to discuss a possible solution
before implementing it:
We have a "fastmap version" in fm_sb. At the moment UBI_FM_FMT_VERSION = 1 and
any other is not supported. We can use that; Add another fm version
(UBI_FM_FMT_VERSION_RD = 2) and
then decide according to it. Meaning, if during attach process we find fm
superblock we check it's version, if it's != UBI_FM_FMT_VERSION_RD, we fall
back to full scan. The next
fastmap will be written with the new layout (and new version number) so second
boot will attach from fastmap without any issues.
BTW: I think I've found a way such that your change will not break anything.
Keep UBI_FM_FMT_VERSION=1, but claim one field in ubi_fm_sb to indicate a
fastmap subversion or extension.
Create new data structures which carry all the information you need and place
them at the end of the fastmap.
An old implementation will not evaluate ubi_fm_sb->extension and therefore will
not use the additional info
you've placed at the end of the fastmap.
A new implementation will evaluate ubi_fm_sb->extension and notice that this fastmap
carries the "read disturb infrastructure"
extension info at it's end and can use it...
Not nice, not perfect but could work. 8-)
Agree, it will work, but seems a bit ugly to me You really think it
will be better than add a new fm_version? I agree that breaking fm
layout is dangerous but it seems to me like the correct way to implement
this requirement. Saving all read-disturb data in "extensions" feels
like a hack.
That said, you're have much more experienced with ubi then I do, so
I'll do as you see best.
Thanks,
//richard
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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/
Re: [PATCH 2/4] UBI: Fastmap: Calc fastmap size correctly
On 9/30/2014 1:20 AM, Richard Weinberger wrote:
We need to add fm_sb too.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/fastmap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 0431b46..2b0d8d6 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -24,7 +24,8 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
{
size_t size;
- size = sizeof(struct ubi_fm_hdr) + \
+ size = sizeof(struct ubi_fm_sb) + \
+ sizeof(struct ubi_fm_hdr) + \
sizeof(struct ubi_fm_scan_pool) + \
sizeof(struct ubi_fm_scan_pool) + \
(ubi->peb_count * sizeof(struct ubi_fm_ec)) + \
Reviewed-by: Tanya Brokhman
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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/
Re: [RFC/PATCH 1/5] mtd: ubi: Read disturb infrastructure
On 10/2/2014 4:24 PM, Richard Weinberger wrote: Am 02.10.2014 14:50, schrieb Tanya Brokhman: Consider the case where you have a board with a fastmap enabled bootloader and a Linux OS. The bootloader does a fastmap attach and boots the kernel from UBI and the kernel it self has the rootfs on UBI too. If you install a new kernel with your changes applied it will write the fastmap in a different format and the bootloader will fail badly. In worst case the board bricks, in best case the bootloader can fall back to scanning mode but it will be slow and the customer unhappy. Ok, I understand the problem now. I wanted to discuss a possible solution before implementing it: We have a "fastmap version" in fm_sb. At the moment UBI_FM_FMT_VERSION = 1 and any other is not supported. We can use that; Add another fm version (UBI_FM_FMT_VERSION_RD = 2) and then decide according to it. Meaning, if during attach process we find fm superblock we check it's version, if it's != UBI_FM_FMT_VERSION_RD, we fall back to full scan. The next fastmap will be written with the new layout (and new version number) so second boot will attach from fastmap without any issues. Yes, if we change the fastmap on-disk layout we need to change UBI_FM_FMT_VERSION. Then other fastmap implementations will notice the change and can hopefully recover. Implementations which do not evaluate UBI_FM_FMT_VERSION deserve breaking. ;-) good. will work on the fix and upload a new set when ready That said, I'll not block a layout change but we have to be sure that it is *really* needed. In order to support read-disturb, I think its really needed. There is no other way to save read counter per PEB but in fastmap. I'm currently heavily working on fastmap and my local queue with fastmap fixes keeps growing. If I find a horror bug which needs a fastmap layout change I want to change the layout only once, not twice. How do you test all of your fastmap fixes? Some of them are not easy to reproduce (the pq saving for example). Besides heavy stability testing, I was testing my changes manually by a lot of dbg prints in the code and analyzing the logs manually. Not the optimal way Thanks, //richard __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ Thanks, Tanya Brokhman -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- 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/
Re: [PATCH 3/4] UBI: Fastmap: Care about the protection queue
Hi Richard
On 9/30/2014 1:20 AM, Richard Weinberger wrote:
Fastmap can miss a PEB if it is in the protection queue
and not jet in the used tree.
Treat every protected PEB as used.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/fastmap.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 2b0d8d6..2853a69 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1195,6 +1195,19 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
fm_pos += sizeof(*fec);
ubi_assert(fm_pos <= ubi->fm_size);
}
+
+ for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
+ list_for_each_entry(wl_e, >pq[i], u.list) {
why not list_for_each_entry_safe?
+ fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
+
+ fec->pnum = cpu_to_be32(wl_e->pnum);
+ fec->ec = cpu_to_be32(wl_e->ec);
+
+ used_peb_count++;
+ fm_pos += sizeof(*fec);
+ ubi_assert(fm_pos <= ubi->fm_size);
Is fm_size ok with this addition or does it needs updating as well?
+ }
+ }
fmh->used_peb_count = cpu_to_be32(used_peb_count);
for (node = rb_first(>scrub); node; node = rb_next(node)) {
Thanks
Tanya Brokhman
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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/
Re: [PATCH 2/4] UBI: Fastmap: Calc fastmap size correctly
On 9/30/2014 1:20 AM, Richard Weinberger wrote:
We need to add fm_sb too.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/fastmap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 0431b46..2b0d8d6 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -24,7 +24,8 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
{
size_t size;
- size = sizeof(struct ubi_fm_hdr) + \
+ size = sizeof(struct ubi_fm_sb) + \
+ sizeof(struct ubi_fm_hdr) + \
sizeof(struct ubi_fm_scan_pool) + \
sizeof(struct ubi_fm_scan_pool) + \
(ubi->peb_count * sizeof(struct ubi_fm_ec)) + \
Not sure what's the proper way doing this (Reviewed-by/Acked-by) but I
agree this patch is required. I would just elaborate a bit more on the
commit message.
Thanks,
Tanya Brokhman
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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/
Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
Hi Richard,
On 9/30/2014 1:20 AM, Richard Weinberger wrote:
...otherwise the deferred work might run after datastructures
got freed and corrupt memory.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 20f491713..dc01b1f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2029,6 +2029,9 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl("close the WL sub-system");
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ flush_work(>fm_work);
flush_work returns bool. It might be useful to print that value for
debugging.
+#endif
cancel_pending(ubi);
protection_queue_destroy(ubi);
tree_destroy(>used);
Thanks
Tanya Brokhman
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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/
Re: [RFC/PATCH 1/5] mtd: ubi: Read disturb infrastructure
Hi Richard,
Sorry it took me some time to answer, got per-occupied with some urgent
staff.
On 9/28/2014 1:54 PM, Richard Weinberger wrote:
Am 28.09.2014 12:46, schrieb Tanya Brokhman:
On 9/28/2014 11:54 AM, Richard Weinberger wrote:
Am 28.09.2014 10:48, schrieb Tanya Brokhman:
@@ -424,6 +440,8 @@ struct ubi_fm_sb {
__be32 used_blocks;
__be32 block_loc[UBI_FM_MAX_BLOCKS];
__be32 block_ec[UBI_FM_MAX_BLOCKS];
+__be32 block_rc[UBI_FM_MAX_BLOCKS];
+__be64 block_let[UBI_FM_MAX_BLOCKS];
Doesn't this break the fastmap on-disk layout?
What do you mean "break"? I verified fastmap feature is working. the whole
read-disturb depends on it so I tested this thoroughly.
Did you write a fastmap with your changes applied and then an attach using a
fastmap implementation *without*
you changes?
I bet it will not work because the disk layout is now different.
you're right, it wont work. I did a set of attach/detach tests to verify
fastmap, but of course with my changes.
Linux is not the only user of fastmap. We need to be very careful here.
Could you please elaborate here? I'm not sure I understand the use case you're
referring to.
Consider the case where you have a board with a fastmap enabled bootloader and
a Linux OS.
The bootloader does a fastmap attach and boots the kernel from UBI and the
kernel it self has the rootfs
on UBI too. If you install a new kernel with your changes applied it will write
the fastmap in a different
format and the bootloader will fail badly. In worst case the board bricks, in
best case the bootloader can fall back
to scanning mode but it will be slow and the customer unhappy.
Ok, I understand the problem now. I wanted to discuss a possible
solution before implementing it:
We have a "fastmap version" in fm_sb. At the moment UBI_FM_FMT_VERSION =
1 and any other is not supported. We can use that; Add another fm
version (UBI_FM_FMT_VERSION_RD = 2) and then decide according to it.
Meaning, if during attach process we find fm superblock we check it's
version, if it's != UBI_FM_FMT_VERSION_RD, we fall back to full scan.
The next fastmap will be written with the new layout (and new version
number) so second boot will attach from fastmap without any issues.
Thanks,
//richard
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Thanks,
Tanya Brokhman
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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/
Re: [RFC/PATCH 1/5] mtd: ubi: Read disturb infrastructure
Hi Richard,
Sorry it took me some time to answer, got per-occupied with some urgent
staff.
On 9/28/2014 1:54 PM, Richard Weinberger wrote:
Am 28.09.2014 12:46, schrieb Tanya Brokhman:
On 9/28/2014 11:54 AM, Richard Weinberger wrote:
Am 28.09.2014 10:48, schrieb Tanya Brokhman:
@@ -424,6 +440,8 @@ struct ubi_fm_sb {
__be32 used_blocks;
__be32 block_loc[UBI_FM_MAX_BLOCKS];
__be32 block_ec[UBI_FM_MAX_BLOCKS];
+__be32 block_rc[UBI_FM_MAX_BLOCKS];
+__be64 block_let[UBI_FM_MAX_BLOCKS];
Doesn't this break the fastmap on-disk layout?
What do you mean break? I verified fastmap feature is working. the whole
read-disturb depends on it so I tested this thoroughly.
Did you write a fastmap with your changes applied and then an attach using a
fastmap implementation *without*
you changes?
I bet it will not work because the disk layout is now different.
you're right, it wont work. I did a set of attach/detach tests to verify
fastmap, but of course with my changes.
Linux is not the only user of fastmap. We need to be very careful here.
Could you please elaborate here? I'm not sure I understand the use case you're
referring to.
Consider the case where you have a board with a fastmap enabled bootloader and
a Linux OS.
The bootloader does a fastmap attach and boots the kernel from UBI and the
kernel it self has the rootfs
on UBI too. If you install a new kernel with your changes applied it will write
the fastmap in a different
format and the bootloader will fail badly. In worst case the board bricks, in
best case the bootloader can fall back
to scanning mode but it will be slow and the customer unhappy.
Ok, I understand the problem now. I wanted to discuss a possible
solution before implementing it:
We have a fastmap version in fm_sb. At the moment UBI_FM_FMT_VERSION =
1 and any other is not supported. We can use that; Add another fm
version (UBI_FM_FMT_VERSION_RD = 2) and then decide according to it.
Meaning, if during attach process we find fm superblock we check it's
version, if it's != UBI_FM_FMT_VERSION_RD, we fall back to full scan.
The next fastmap will be written with the new layout (and new version
number) so second boot will attach from fastmap without any issues.
Thanks,
//richard
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Thanks,
Tanya Brokhman
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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/
