[PATCH 1/2] MTD: UBI: speed up init by moving status messages to debugfs

[PATCH 1/2] MTD: UBI: speed up init by moving status messages to debugfs

[Rajeev Kumar]

This patch simply moves the verbose status messages associated with
UBI's fastmap feature to debugfs, thereby decreasing UBI
initialization time from 97 mS to 16 mS. Note that the first time
fastmap is invoked, building the fastmap took 146 mS. In order to
reproduce the test conditions, build with CONFIG_MTD_UBI_FASTMAP=y and
CONFIG_DYNAMIC_DEBUG=y and append the following to bootargs:

initcall_debug ubi.mtd=0 ubi.fm_autoconvert=1

ubi.mtd=0 is needed at every boot if you want ubi to be
autoloaded. ubi.fm_autoconvert switch is needed only once, when
fastmap is created.

How to restore the verbose output for testing:
-----------------------------------------------
root:~# ubidetach -p /dev/mtd0
root:~# mount -t debugfs none /mnt/dyndbg
root:~# echo 'file build.c +p' > /mnt/dyndbg/dynamic_debug/control
root:~# echo 'file fastmap.c +p' > /mnt/dyndbg/dynamic_debug/control
root:~# ubiattach -p /dev/mtd0
root:~# mount -t ubifs /dev/ubi0_0 /mnt/card
-----------------------------------------------
This patch is tested against 3.14 kernel and only build test is performed
against current upstream master branch.

Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
---
 drivers/mtd/ubi/build.c   |   20 ++++++++++----------
 drivers/mtd/ubi/fastmap.c |    6 +++---
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index ef36182..59229c4 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -949,8 +949,8 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 		ubi->fm_disabled = 1;
 	}
 
-	ubi_msg(ubi, "default fastmap pool size: %d", ubi->fm_pool.max_size);
-	ubi_msg(ubi, "default fastmap WL pool size: %d",
+	dbg_gen("default fastmap pool size: %d", ubi->fm_pool.max_size);
+	dbg_gen("default fastmap WL pool size: %d",
 		ubi->fm_wl_pool.max_size);
 #else
 	ubi->fm_disabled = 1;
@@ -1008,23 +1008,23 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 		goto out_debugfs;
 	}
 
-	ubi_msg(ubi, "attached mtd%d (name \"%s\", size %llu MiB)",
+	dbg_gen("attached mtd%d (name \"%s\", size %llu MiB)",
 		mtd->index, mtd->name, ubi->flash_size >> 20);
-	ubi_msg(ubi, "PEB size: %d bytes (%d KiB), LEB size: %d bytes",
+	dbg_gen("PEB size: %d bytes (%d KiB), LEB size: %d bytes",
 		ubi->peb_size, ubi->peb_size >> 10, ubi->leb_size);
-	ubi_msg(ubi, "min./max. I/O unit sizes: %d/%d, sub-page size %d",
+	dbg_gen("min./max. I/O unit sizes: %d/%d, sub-page size %d",
 		ubi->min_io_size, ubi->max_write_size, ubi->hdrs_min_io_size);
-	ubi_msg(ubi, "VID header offset: %d (aligned %d), data offset: %d",
+	dbg_gen("VID header offset: %d (aligned %d), data offset: %d",
 		ubi->vid_hdr_offset, ubi->vid_hdr_aloffset, ubi->leb_start);
-	ubi_msg(ubi, "good PEBs: %d, bad PEBs: %d, corrupted PEBs: %d",
+	dbg_gen("good PEBs: %d, bad PEBs: %d, corrupted PEBs: %d",
 		ubi->good_peb_count, ubi->bad_peb_count, ubi->corr_peb_count);
-	ubi_msg(ubi, "user volume: %d, internal volumes: %d, max. volumes count: %d",
+	dbg_gen("user volume: %d, internal volumes: %d, max. volumes count: %d",
 		ubi->vol_count - UBI_INT_VOL_COUNT, UBI_INT_VOL_COUNT,
 		ubi->vtbl_slots);
-	ubi_msg(ubi, "max/mean erase counter: %d/%d, WL threshold: %d, image sequence number: %u",
+	dbg_gen("max/mean erase counter: %d/%d, WL threshold: %d, image sequence number: %u",
 		ubi->max_ec, ubi->mean_ec, CONFIG_MTD_UBI_WL_THRESHOLD,
 		ubi->image_seq);
-	ubi_msg(ubi, "available PEBs: %d, total reserved PEBs: %d, PEBs reserved for bad PEB handling: %d",
+	dbg_gen("available PEBs: %d, total reserved PEBs: %d, PEBs reserved for bad PEB handling: %d",
 		ubi->avail_pebs, ubi->rsvd_pebs, ubi->beb_rsvd_pebs);
 
 	/*
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 990898b..285a65a 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1053,9 +1053,9 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	ubi->fm = fm;
 	ubi->fm_pool.max_size = ubi->fm->max_pool_size;
 	ubi->fm_wl_pool.max_size = ubi->fm->max_wl_pool_size;
-	ubi_msg(ubi, "attached by fastmap");
-	ubi_msg(ubi, "fastmap pool size: %d", ubi->fm_pool.max_size);
-	ubi_msg(ubi, "fastmap WL pool size: %d",
+	dbg_bld("attached by fastmap");
+	dbg_bld("fastmap pool size: %d", ubi->fm_pool.max_size);
+	dbg_bld("fastmap WL pool size: %d",
 		ubi->fm_wl_pool.max_size);
 	ubi->fm_disabled = 0;
 	ubi->fast_attach = 1;
-- 
1.7.9.5

[PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Rajeev Kumar]

If the master mtd does not have any slave mtd partitions,
and its numeraseregions is one(only has one erease block), and
we attach the master mtd with : ubiattach -m 0 -d 0

We will meet the error:
-------------------------------------------------------
root ~$ ubiattach /dev/ubi_ctrl -m 0 -d 0
UBI: attaching mtd0 to ubi0
UBI error: io_init: multiple regions, not implemented
ubiattach: error!: cannot attach mtd0
           error 22 (Invalid argument)
-------------------------------------------------------

In fact, if there is only one "erase block", we should not
prevent the attach.

This patch is tested against 3.14 kernel and only build test is
performed against current upstream master branch.

Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
---
 drivers/mtd/ubi/build.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 59229c4..562421b 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -628,7 +628,7 @@ static int io_init(struct ubi_device *ubi, int max_beb_per1024)
 	dbg_gen("sizeof(struct ubi_ainf_peb) %zu", sizeof(struct ubi_ainf_peb));
 	dbg_gen("sizeof(struct ubi_wl_entry) %zu", sizeof(struct ubi_wl_entry));
 
-	if (ubi->mtd->numeraseregions != 0) {
+	if (ubi->mtd->numeraseregions > 1) {
 		/*
 		 * Some flashes have several erase regions. Different regions
 		 * may have different eraseblock size and other
-- 
1.7.9.5

Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Richard Weinberger]

Rajeev,

Am 25.07.2016 um 11:46 schrieb Rajeev Kumar:
> If the master mtd does not have any slave mtd partitions,
> and its numeraseregions is one(only has one erease block), and
> we attach the master mtd with : ubiattach -m 0 -d 0
> 
> We will meet the error:
> -------------------------------------------------------
> root ~$ ubiattach /dev/ubi_ctrl -m 0 -d 0
> UBI: attaching mtd0 to ubi0
> UBI error: io_init: multiple regions, not implemented
> ubiattach: error!: cannot attach mtd0
>            error 22 (Invalid argument)
> -------------------------------------------------------
> 
> In fact, if there is only one "erase block", we should not
> prevent the attach.
> 
> This patch is tested against 3.14 kernel and only build test is
> performed against current upstream master branch.

The more interesting question is, why is ->numeraseregions not 0?

The comment in the header says:
        /* Data for variable erase regions. If numeraseregions is zero,
         * it means that the whole device has erasesize as given above.
         */

So, if your MTD erase regions with the same size, it should be 0.

IIRC we had such a discussion already on linux-mtd and it was not clear
whether numeraseregions of 0 and 1 are equal or not.

Thanks,
//richard

Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Rajeev Kumar]

Richard

On 07/25/2016 04:01 PM, Richard Weinberger wrote:
> Rajeev,
>
> Am 25.07.2016 um 11:46 schrieb Rajeev Kumar:
>> If the master mtd does not have any slave mtd partitions,
>> and its numeraseregions is one(only has one erease block), and
>> we attach the master mtd with : ubiattach -m 0 -d 0
>>
>> We will meet the error:
>> -------------------------------------------------------
>> root ~$ ubiattach /dev/ubi_ctrl -m 0 -d 0
>> UBI: attaching mtd0 to ubi0
>> UBI error: io_init: multiple regions, not implemented
>> ubiattach: error!: cannot attach mtd0
>>             error 22 (Invalid argument)
>> -------------------------------------------------------
>>
>> In fact, if there is only one "erase block", we should not
>> prevent the attach.
>>
>> This patch is tested against 3.14 kernel and only build test is
>> performed against current upstream master branch.
>
> The more interesting question is, why is ->numeraseregions not 0?
>
> The comment in the header says:
>          /* Data for variable erase regions. If numeraseregions is zero,
>           * it means that the whole device has erasesize as given above.
>           */
>
> So, if your MTD erase regions with the same size, it should be 0.
>
> IIRC we had such a discussion already on linux-mtd and it was not clear
> whether numeraseregions of 0 and 1 are equal or not.
>

Could you please pass the link
Thanks in advance

~Rajeev
> Thanks,
> //richard
>

Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Richard Weinberger]

Rajeev,

Am 25.07.2016 um 13:16 schrieb Rajeev Kumar:
>> So, if your MTD erase regions with the same size, it should be 0.
>>
>> IIRC we had such a discussion already on linux-mtd and it was not clear
>> whether numeraseregions of 0 and 1 are equal or not.
>>
> 
> Could you please pass the link
> Thanks in advance

http://lmgtfy.com/?q=numeraseregions+ubi+linux-mtd :-)

*scnr*,
//richard

Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Brian Norris]

(Artem, any opinions, since you had the most opinion last time this came
up?)

On Mon, Jul 25, 2016 at 12:31:39PM +0200, Richard Weinberger wrote:
> Am 25.07.2016 um 11:46 schrieb Rajeev Kumar:
> > If the master mtd does not have any slave mtd partitions,
> > and its numeraseregions is one(only has one erease block), and
> > we attach the master mtd with : ubiattach -m 0 -d 0
> > 
> > We will meet the error:
> > -------------------------------------------------------
> > root ~$ ubiattach /dev/ubi_ctrl -m 0 -d 0
> > UBI: attaching mtd0 to ubi0
> > UBI error: io_init: multiple regions, not implemented
> > ubiattach: error!: cannot attach mtd0
> >            error 22 (Invalid argument)
> > -------------------------------------------------------
> > 
> > In fact, if there is only one "erase block", we should not
> > prevent the attach.
> > 
> > This patch is tested against 3.14 kernel and only build test is
> > performed against current upstream master branch.
> 
> The more interesting question is, why is ->numeraseregions not 0?
> 
> The comment in the header says:
>         /* Data for variable erase regions. If numeraseregions is zero,
>          * it means that the whole device has erasesize as given above.
>          */
> 
> So, if your MTD erase regions with the same size, it should be 0.
> 
> IIRC we had such a discussion already on linux-mtd and it was not clear
> whether numeraseregions of 0 and 1 are equal or not.

I think 0 and 1 are essentially equal. But there's some potential room
for error (e.g., if the driver doesn't configure mtd->erasesize ==
mtd->eraseregions[0].erasesize, and similar). Also, I see some
problematic code like this in cfi_cmdset_0001.c:

	mtd->numeraseregions = cfi->cfiq->NumEraseRegions * cfi->numchips;

So, if there are two chips, but both have a single erase region, with
the same erasesize, we'll still end up with ->numeraseregions == 2. We
can't hack all MTD users to start searching the eraseregions[] array to
see if the device is actually uniform, even if it reports
numeraseregions > 0.

I'm inclined, then, to outlaw numeraseregions == 1 (to make it simpler for MTD
users to handle), and put code either in drivers or in mtdcore.c to be slightly
more intelligent (e.g., if the driver left numeraseregions == 1, then just do
some sanity checking and re-set numeraseregions to 0). It might be good to move
code like this from cfi_cmdset_0001.c into mtdcore.c at the same time too:

	if (offset != devsize) {
		/* Argh */
		printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize);
		goto setup_err;
	}

BTW, Rajeev, what devices are you testing? Just curious.

Regards,
Brian

Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Artem Bityutskiy]

On Fri, 2016-07-29 at 11:24 -0700, Brian Norris wrote:
> (Artem, any opinions, since you had the most opinion last time this
> came
> up?)

Hi Brian, 

well, I do not have strong opinion. That UBI translates to "I do not
know how to deal with multiple regions", nothing else.

I think this was my understanding of numeraseregions

1. numaraseregions=0: no regions, just uniform flash.

2. numeraseregions=1: basically same as above, but friendlier to the
region-aware software. Indeed, if I care about regions, I do not want
to handle the special case of numerasereions=0, I want a common case
with numerasereionts>0.

Or to put it this way, from the drivers' writer POW:

numaraseregions=0 - do not know anything about regions, do not care.
Also pre-regions dirivers, old stuff

numeraseregions=1 - I want my driver to be ideal. Non-aware SW won't
look to the regions stuff, the aware SW will work nicely.

So my take is: ban numeraseregions=1 if you feel like, but I recommend
to just document the 0 (don't care/legacy) and 1 (same as 0, but care)
and allow for both.

Artem.

RE: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Chugh, Sanjeev]

Hello Brian, Artem,

Are you suggesting that this change be documented instead. Note that we are using NOR flash devices for our testing as you asked.

Can I request you to please clarify on acceptance of this patch ?

Thanks
Sanjeev

>-----Original Message-----
>From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
>Sent: Thursday, August 04, 2016 12:01 PM
>To: Brian Norris; Richard Weinberger
>Cc: Rajeev Kumar; dwmw2@infradead.org; linux-mtd@lists.infradead.org;
>stable@vger.kernel.org
>Subject: Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd-
>>numeraseregions is 1
>
>On Fri, 2016-07-29 at 11:24 -0700, Brian Norris wrote:
>> (Artem, any opinions, since you had the most opinion last time this
>> came
>> up?)
>
>Hi Brian,
>
>well, I do not have strong opinion. That UBI translates to "I do not know how to
>deal with multiple regions", nothing else.
>
>I think this was my understanding of numeraseregions
>
>1. numaraseregions=0: no regions, just uniform flash.
>
>2. numeraseregions=1: basically same as above, but friendlier to the region-
>aware software. Indeed, if I care about regions, I do not want to handle the
>special case of numerasereions=0, I want a common case with
>numerasereionts>0.
>
>Or to put it this way, from the drivers' writer POW:
>
>numaraseregions=0 - do not know anything about regions, do not care.
>Also pre-regions dirivers, old stuff
>
>numeraseregions=1 - I want my driver to be ideal. Non-aware SW won't look to
>the regions stuff, the aware SW will work nicely.
>
>So my take is: ban numeraseregions=1 if you feel like, but I recommend to just
>document the 0 (don't care/legacy) and 1 (same as 0, but care) and allow for
>both.
>
>Artem.

RE: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Chugh, Sanjeev]

Just to clarify, I meant since you were suggesting that value of numeraseregions either 0 or 1 is essentially equal. 

Are you suggesting that you don't like the change and better clarify this in documentation about this ? 

-Sanjeev

>-----Original Message-----
>From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of
>Chugh, Sanjeev
>Sent: Friday, September 23, 2016 4:51 PM
>To: dedekind1@gmail.com; Brian Norris; Richard Weinberger
>Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org;
>stable@vger.kernel.org
>Subject: RE: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd-
>>numeraseregions is 1
>
>Hello Brian, Artem,
>
>Are you suggesting that this change be documented instead. Note that we are
>using NOR flash devices for our testing as you asked.
>
>Can I request you to please clarify on acceptance of this patch ?
>
>Thanks
>Sanjeev
>
>>-----Original Message-----
>>From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
>>Sent: Thursday, August 04, 2016 12:01 PM
>>To: Brian Norris; Richard Weinberger
>>Cc: Rajeev Kumar; dwmw2@infradead.org; linux-mtd@lists.infradead.org;
>>stable@vger.kernel.org
>>Subject: Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd-
>>>numeraseregions is 1
>>
>>On Fri, 2016-07-29 at 11:24 -0700, Brian Norris wrote:
>>> (Artem, any opinions, since you had the most opinion last time this
>>> came
>>> up?)
>>
>>Hi Brian,
>>
>>well, I do not have strong opinion. That UBI translates to "I do not know how to
>>deal with multiple regions", nothing else.
>>
>>I think this was my understanding of numeraseregions
>>
>>1. numaraseregions=0: no regions, just uniform flash.
>>
>>2. numeraseregions=1: basically same as above, but friendlier to the region-
>>aware software. Indeed, if I care about regions, I do not want to handle the
>>special case of numerasereions=0, I want a common case with
>>numerasereionts>0.
>>
>>Or to put it this way, from the drivers' writer POW:
>>
>>numaraseregions=0 - do not know anything about regions, do not care.
>>Also pre-regions dirivers, old stuff
>>
>>numeraseregions=1 - I want my driver to be ideal. Non-aware SW won't look to
>>the regions stuff, the aware SW will work nicely.
>>
>>So my take is: ban numeraseregions=1 if you feel like, but I recommend to just
>>document the 0 (don't care/legacy) and 1 (same as 0, but care) and allow for
>>both.
>>
>>Artem.
>______________________________________________________
>Linux MTD discussion mailing list
>http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Chugh, Sanjeev]

>-----Original Message-----
>From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of
>Chugh, Sanjeev
>Sent: Friday, September 23, 2016 4:51 PM
>To: dedekind1@gmail.com; Brian Norris; Richard Weinberger
>Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org;
>stable@vger.kernel.org
>Subject: RE: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd-
>>numeraseregions is 1
>
>Hello Brian, Artem,
>
>Are you suggesting that this change be documented instead. Note that we are
>using NOR flash devices for our testing as you asked.
>
>Can I request you to please clarify on acceptance of this patch ?
>
>Thanks
>Sanjeev
>
>>-----Original Message-----
>>From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
>>Sent: Thursday, August 04, 2016 12:01 PM
>>To: Brian Norris; Richard Weinberger
>>Cc: Rajeev Kumar; dwmw2@infradead.org; linux-mtd@lists.infradead.org;
>>stable@vger.kernel.org
>>Subject: Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd-
>>>numeraseregions is 1
>>
>>On Fri, 2016-07-29 at 11:24 -0700, Brian Norris wrote:
>>> (Artem, any opinions, since you had the most opinion last time this
>>> came
>>> up?)
>>
>>Hi Brian,
>>
>>well, I do not have strong opinion. That UBI translates to "I do not know how to
>>deal with multiple regions", nothing else.
>>
>>I think this was my understanding of numeraseregions
>>
>>1. numaraseregions=0: no regions, just uniform flash.
>>
>>2. numeraseregions=1: basically same as above, but friendlier to the region-
>>aware software. Indeed, if I care about regions, I do not want to handle the
>>special case of numerasereions=0, I want a common case with
>>numerasereionts>0.
>>
>>Or to put it this way, from the drivers' writer POW:
>>
>>numaraseregions=0 - do not know anything about regions, do not care.
>>Also pre-regions dirivers, old stuff
>>
>>numeraseregions=1 - I want my driver to be ideal. Non-aware SW won't look to
>>the regions stuff, the aware SW will work nicely.
>>
>>So my take is: ban numeraseregions=1 if you feel like, but I recommend to just
>>document the 0 (don't care/legacy) and 1 (same as 0, but care) and allow for
>>both.
>>
>>Artem.
>______________________________________________________
>Linux MTD discussion mailing list
>http://lists.infradead.org/mailman/listinfo/linux-mtd/

Hello Brian, Artem, etc. al.

I would really appreciate if you clarify what's your opinion on the patch. I would like to close the thread if it's not going to be accepted.

Or I would try making further changes but I need inputs.

Thanks in advance.

-Sanjeev

Re: [PATCH 1/2] MTD: UBI: speed up init by moving status messages to debugfs

[Richard Weinberger]

Rajeev,

Am 25.07.2016 um 11:46 schrieb Rajeev Kumar:
> This patch simply moves the verbose status messages associated with
> UBI's fastmap feature to debugfs, thereby decreasing UBI
> initialization time from 97 mS to 16 mS. Note that the first time
> fastmap is invoked, building the fastmap took 146 mS. In order to
> reproduce the test conditions, build with CONFIG_MTD_UBI_FASTMAP=y and
> CONFIG_DYNAMIC_DEBUG=y and append the following to bootargs:
> 
> initcall_debug ubi.mtd=0 ubi.fm_autoconvert=1
> 
> ubi.mtd=0 is needed at every boot if you want ubi to be
> autoloaded. ubi.fm_autoconvert switch is needed only once, when
> fastmap is created.

So, you're working on a system which has to boot as fast as possible
and writing kernel logs hurts the performance. (Slow UART?)
Why do you change logging only in UBI (Fastmap)? Many other subsystems
print during boot and would also hurt the performance.
In such a situation I'd assume that changing the kernel log level or
using the quiet kernel parameter would help more.

Thanks,
//richard

Re: [PATCH 1/2] MTD: UBI: speed up init by moving status messages to debugfs

[Rajeev Kumar]

Richard

On 07/25/2016 04:01 PM, Richard Weinberger wrote:
> Rajeev,
>
> Am 25.07.2016 um 11:46 schrieb Rajeev Kumar:
>> This patch simply moves the verbose status messages associated with
>> UBI's fastmap feature to debugfs, thereby decreasing UBI
>> initialization time from 97 mS to 16 mS. Note that the first time
>> fastmap is invoked, building the fastmap took 146 mS. In order to
>> reproduce the test conditions, build with CONFIG_MTD_UBI_FASTMAP=y and
>> CONFIG_DYNAMIC_DEBUG=y and append the following to bootargs:
>>
>> initcall_debug ubi.mtd=0 ubi.fm_autoconvert=1
>>
>> ubi.mtd=0 is needed at every boot if you want ubi to be
>> autoloaded. ubi.fm_autoconvert switch is needed only once, when
>> fastmap is created.
>
> So, you're working on a system which has to boot as fast as possible
> and writing kernel logs hurts the performance. (Slow UART?)
> Why do you change logging only in UBI (Fastmap)? Many other subsystems
> print during boot and would also hurt the performance.
> In such a situation I'd assume that changing the kernel log level or
> using the quiet kernel parameter would help more.
>

Agreed, but the question is do we really need these all values for 
UBI(Fastmap) on console every time system is booted ?

Thanks
~Rajeev

> Thanks,
> //richard
>

Re: [PATCH 1/2] MTD: UBI: speed up init by moving status messages to debugfs

[Richard Weinberger]

Rajeev,

Am 25.07.2016 um 13:23 schrieb Rajeev Kumar:
> Richard
> 
> On 07/25/2016 04:01 PM, Richard Weinberger wrote:
>> Rajeev,
>>
>> Am 25.07.2016 um 11:46 schrieb Rajeev Kumar:
>>> This patch simply moves the verbose status messages associated with
>>> UBI's fastmap feature to debugfs, thereby decreasing UBI
>>> initialization time from 97 mS to 16 mS. Note that the first time
>>> fastmap is invoked, building the fastmap took 146 mS. In order to
>>> reproduce the test conditions, build with CONFIG_MTD_UBI_FASTMAP=y and
>>> CONFIG_DYNAMIC_DEBUG=y and append the following to bootargs:
>>>
>>> initcall_debug ubi.mtd=0 ubi.fm_autoconvert=1
>>>
>>> ubi.mtd=0 is needed at every boot if you want ubi to be
>>> autoloaded. ubi.fm_autoconvert switch is needed only once, when
>>> fastmap is created.
>>
>> So, you're working on a system which has to boot as fast as possible
>> and writing kernel logs hurts the performance. (Slow UART?)
>> Why do you change logging only in UBI (Fastmap)? Many other subsystems
>> print during boot and would also hurt the performance.
>> In such a situation I'd assume that changing the kernel log level or
>> using the quiet kernel parameter would help more.
>>
> 
> Agreed, but the question is do we really need these all values for UBI(Fastmap) on console every time system is booted ?

These days I'd not print all of them. But we do already and I know setups which parse dmesg and grep for some of these
values. Either to see whether attach worked or just to gather debug infos for QA.
I don't want to risk breaking existing stuff.
Since your problem can be solved perfectly fine by using loglevels I think it is better to stay on the
safe side and keep them as-is.

Thanks,
//richard

Re: [PATCH 1/2] MTD: UBI: speed up init by moving status messages to debugfs

[Greg KH]

On Mon, Jul 25, 2016 at 01:32:30PM +0200, Richard Weinberger wrote:
> These days I'd not print all of them. But we do already and I know
> setups which parse dmesg and grep for some of these values.

That's horrid and should not be tolerated, please never grep the kernel
log for anything.

> Either to see whether attach worked or just to gather debug infos for
> QA.
> I don't want to risk breaking existing stuff.

We do not claim that kernel log messages are an "ABI" that can not be
changed or broken at all.

thanks,

greg k-h

Re: [PATCH 1/2] MTD: UBI: speed up init by moving status messages to debugfs

[Richard Weinberger]

Am 25.07.2016 um 18:47 schrieb Greg KH:
> On Mon, Jul 25, 2016 at 01:32:30PM +0200, Richard Weinberger wrote:
>> These days I'd not print all of them. But we do already and I know
>> setups which parse dmesg and grep for some of these values.
> 
> That's horrid and should not be tolerated, please never grep the kernel
> log for anything.
> 
>> Either to see whether attach worked or just to gather debug infos for
>> QA.
>> I don't want to risk breaking existing stuff.
> 
> We do not claim that kernel log messages are an "ABI" that can not be
> changed or broken at all.

I know, but today I'm less sadistic than usual and allow users to keep their
stuff.
Rajeev's issue is not related to UBI and can be solved in a generic and sane
way using loglevels. So there is no need to change the existing logging.

Thanks,
//richard