Hi Richard,
Sorry for reviewing this late...
On Tue, 10 Jul 2012 18:23:42 +0200 Richard Genoud <[email protected]> wrote:
> -config MTD_UBI_BEB_LIMIT
> - int "Maximum expected bad eraseblocks per 1024 eraseblocks"
> - default 20
> - range 2 256
I see some benefit keeping the config.
For the simplest systems (those having one ubi device) that need a limit
*other* than the default (20 per 1024), they can simply set the config
to their chosen value, as they were used to.
With you approach, these system MUST pass the limit parameter via the
ioctl / module-parameter.
> +static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024)
> +{
> + int device_peb_count;
> + uint64_t device_size;
> + int beb_limit = 0;
> +
> + /* this has already been checked in ioctl */
> + if (max_beb_per1024 <= 0)
> + goto out;
Can you explain how 'max_beb_per1024 <= 0' may happen?
It seems that all of your calls to 'ubi_attach_mtd_dev' pass a positive
max_beb_per1024 value (the default is always set). See your
'ubi_mtd_param_parse' and 'ctrl_cdev_ioctl'. Am I missing something?
Also, since max_beb_per1024 is always set, how one may specify a zero
limit?
Regards,
Shmulik
2012/8/16 Shmulik Ladkani <[email protected]>:
> Hi Richard,
>
> Sorry for reviewing this late...
>
> On Tue, 10 Jul 2012 18:23:42 +0200 Richard Genoud <[email protected]> wrote:
>> -config MTD_UBI_BEB_LIMIT
>> - int "Maximum expected bad eraseblocks per 1024 eraseblocks"
>> - default 20
>> - range 2 256
>
> I see some benefit keeping the config.
>
> For the simplest systems (those having one ubi device) that need a limit
> *other* than the default (20 per 1024), they can simply set the config
> to their chosen value, as they were used to.
>
> With you approach, these system MUST pass the limit parameter via the
> ioctl / module-parameter.
That's right.
We can add a kernel config option to change the max_beb_per1024
default value (actually, this is almost the patch I send first).
But I see something disturbing with that:
It means that an ubi_attach call from userspace, without specifying
max_beb_per1024, won't have the same result, depending of the default
config value the kernel has been compiled with.
(Or maybe this behavior is acceptable).
>> +static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024)
>> +{
>> + int device_peb_count;
>> + uint64_t device_size;
>> + int beb_limit = 0;
>> +
>> + /* this has already been checked in ioctl */
>> + if (max_beb_per1024 <= 0)
>> + goto out;
>
> Can you explain how 'max_beb_per1024 <= 0' may happen?
>
> It seems that all of your calls to 'ubi_attach_mtd_dev' pass a positive
> max_beb_per1024 value (the default is always set). See your
> 'ubi_mtd_param_parse' and 'ctrl_cdev_ioctl'. Am I missing something?
Actually it can. But it's because I made a mistake:
p->max_beb_per1024 = bytes_str_to_int(tokens[2]);
=> I didn't check the return value of this. It can fail, and if it
does the return value is >0.
I'm going to change that.
>
> Also, since max_beb_per1024 is always set, how one may specify a zero
> limit?
You can't.
Do you think we need that ?
0 should be kept for "default value", not to break user-space.
But we can use another value for 0, like 1024 or 2048.
(but honestly, I think this will add complexity for an unlikely configuration.)
>
> Regards,
> Shmulik
Regards,
Richard.
--
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
Hi Richard, Artem,
On Thu, 16 Aug 2012 12:07:01 +0200 Richard Genoud <[email protected]> wrote:
> > With you approach, these system MUST pass the limit parameter via the
> > ioctl / module-parameter.
> That's right.
> We can add a kernel config option to change the max_beb_per1024
> default value (actually, this is almost the patch I send first).
> But I see something disturbing with that:
> It means that an ubi_attach call from userspace, without specifying
> max_beb_per1024, won't have the same result, depending of the default
> config value the kernel has been compiled with.
> (Or maybe this behavior is acceptable).
Well, that was the previous behavior of MTD_UBI_BEB_RESERVE, long before
our patchsets.
I think it is acceptable, given the fact it simplifies the configuration
for most simple systems.
Anyway I'm just pointing out the consequences of your change and try to
suggest other alternatives.
Artem should decide as he's the maintainer.
> > Also, since max_beb_per1024 is always set, how one may specify a zero
> > limit?
> You can't.
> Do you think we need that ?
Well again, originally, prior our patchsets, one *could* set a zero
MTD_UBI_BEB_RESERVE for his system. So we're introducing a change that
affects the possible ways an ubi system can be configured, banning
a configuration that was valid in the past.
Does it make sense to set a zero limit? dunno.
For testing purposes, maybe.
Artem, what do you think? prohibit a zero beb limit?
Regards,
Shmulik
On Thu, 2012-08-16 at 11:57 +0300, Shmulik Ladkani wrote:
>
> For the simplest systems (those having one ubi device) that need a
> limit
> *other* than the default (20 per 1024), they can simply set the config
> to their chosen value, as they were used to.
>
> With you approach, these system MUST pass the limit parameter via the
> ioctl / module-parameter.
Yeah, when you change the default, you usually need to do an extra step.
It does not feel too bad, and I would not keep additional configuration
option for a hypothetical user. If someone suffers, we can add an option
to change the default. But I'd start without it. So, I think it is OK to
remove this.
--
Best Regards,
Artem Bityutskiy
On Thu, 2012-08-16 at 13:42 +0300, Shmulik Ladkani wrote:
>
> Does it make sense to set a zero limit? dunno.
> For testing purposes, maybe.
>
> Artem, what do you think? prohibit a zero beb limit?
We do not have that big user-base. No one uses 0 in the tree, most use
the default. I never heard anyone using 0. I did not use it also. I
think it is OK to have the lower range start from non-zero. But why it
is 2 and not 1 - I am not sure :-)
--
Best Regards,
Artem Bityutskiy
On Thu, 16 Aug 2012 16:28:38 +0300 Artem Bityutskiy <[email protected]> wrote:
> On Thu, 2012-08-16 at 11:57 +0300, Shmulik Ladkani wrote:
> >
> > For the simplest systems (those having one ubi device) that need a
> > limit
> > *other* than the default (20 per 1024), they can simply set the config
> > to their chosen value, as they were used to.
> >
> > With you approach, these system MUST pass the limit parameter via the
> > ioctl / module-parameter.
>
> Yeah, when you change the default, you usually need to do an extra step.
> It does not feel too bad, and I would not keep additional configuration
> option for a hypothetical user. If someone suffers, we can add an option
> to change the default. But I'd start without it. So, I think it is OK to
> remove this.
Yes, but the main drawback I was referring to is those platforms already
setting MTD_UBI_BEB_RESERVE other than the default, by means of kernel
configuration.
(there's one platform known to do so in its defconfig, that's
sam9_l9260_defconfig, which uses 3% instead of the "standard" 2%).
These platforms must now change their usermode code to either pass a
module parameter during the insmod or change their attach ioctl of
their application.
We force these systems to change their usermode because we changed ubi's
default BEB limit to be 20/1024 _hardcoded_ (instead of kernel
configurable as previously was).
Is this ok?
Regards,
Shmulik
2012/8/16 Shmulik Ladkani <[email protected]>:
> On Thu, 16 Aug 2012 16:28:38 +0300 Artem Bityutskiy <[email protected]> wrote:
>> On Thu, 2012-08-16 at 11:57 +0300, Shmulik Ladkani wrote:
>> >
>> > For the simplest systems (those having one ubi device) that need a
>> > limit
>> > *other* than the default (20 per 1024), they can simply set the config
>> > to their chosen value, as they were used to.
>> >
>> > With you approach, these system MUST pass the limit parameter via the
>> > ioctl / module-parameter.
>>
>> Yeah, when you change the default, you usually need to do an extra step.
>> It does not feel too bad, and I would not keep additional configuration
>> option for a hypothetical user. If someone suffers, we can add an option
>> to change the default. But I'd start without it. So, I think it is OK to
>> remove this.
>
> Yes, but the main drawback I was referring to is those platforms already
> setting MTD_UBI_BEB_RESERVE other than the default, by means of kernel
> configuration.
> (there's one platform known to do so in its defconfig, that's
> sam9_l9260_defconfig, which uses 3% instead of the "standard" 2%).
I found the board:
https://www.olimex.com/dev/sam9-L9260.html
and the nand datasheet:
http://www.rockbox.org/wiki/pub/Main/LyrePrototype/K9xxG08UXM.pdf
page 11, we can see that the max_bad_bebper1024 is 25 (100 for 4096)
> These platforms must now change their usermode code to either pass a
> module parameter during the insmod or change their attach ioctl of
> their application.
>
> We force these systems to change their usermode because we changed ubi's
> default BEB limit to be 20/1024 _hardcoded_ (instead of kernel
> configurable as previously was).
>
> Is this ok?
well, I don't know, Artem, you're the maintainer :)
I made a quick patch on this, if you decide to apply it.
Richard
--
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
This patch provides the possibility to adjust the default value for
"maximum expected number of bad blocks per 1024 blocks" from kernel
configuration.
sam9_l9260_defconfig has been adjusted as well. The nand device used by
this card has a NVB of 3996/4096 ie a MAX_BEB_LIMIT of 25/1024
Signed-off-by: Richard Genoud <[email protected]>
---
arch/arm/configs/sam9_l9260_defconfig | 1 +
drivers/mtd/ubi/Kconfig | 29 +++++++++++++++++++++++++++++
drivers/mtd/ubi/ubi.h | 2 +-
3 files changed, 31 insertions(+), 1 deletions(-)
diff --git a/arch/arm/configs/sam9_l9260_defconfig b/arch/arm/configs/sam9_l9260_defconfig
index 47dd71a..2c20c9e 100644
--- a/arch/arm/configs/sam9_l9260_defconfig
+++ b/arch/arm/configs/sam9_l9260_defconfig
@@ -39,6 +39,7 @@ CONFIG_MTD_NAND=y
CONFIG_MTD_NAND_ATMEL=y
CONFIG_MTD_NAND_PLATFORM=y
CONFIG_MTD_UBI=y
+CONFIG_MTD_UBI_DEFAULT_BEB_LIMIT=25
CONFIG_MTD_UBI_GLUEBI=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_BLK_DEV_RAM=y
diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig
index 7a57cc0..30e8370 100644
--- a/drivers/mtd/ubi/Kconfig
+++ b/drivers/mtd/ubi/Kconfig
@@ -27,6 +27,35 @@ config MTD_UBI_WL_THRESHOLD
life-cycle less than 10000, the threshold should be lessened (e.g.,
to 128 or 256, although it does not have to be power of 2).
+config MTD_UBI_DEFAULT_BEB_LIMIT
+ int "Maximum expected bad eraseblock count per 1024 eraseblocks"
+ default 20
+ range 0 256
+ help
+ This option specifies the default value for maximum bad physical
+ eraseblocks UBI expects on the MTD device (per 1024 eraseblocks).
+ It will be used when the maximum bad PEB value is null in the
+ UBI_IOCATT ioctl (e.g. when calling ubiattach) or in the kernel
+ parameter ubi.mtd.
+ If the underlying flash does not admit of bad eraseblocks
+ (e.g. NOR flash), this value is ignored.
+
+ NAND datasheets often specify the minimum and maximum NVM (Number of
+ Valid Blocks) for the flashes' endurance lifetime. The maximum
+ expected bad eraseblocks per 1024 eraseblocks then can be calculated
+ as "1024 * (1 - MinNVB / MaxNVB)", which gives 20 for most NANDs
+ (MaxNVB is basically the total count of eraseblocks on the chip).
+
+ To put it differently, if this value is 20, UBI will try to reserve
+ about 1.9% of physical eraseblocks for bad blocks handling. And that
+ will be 1.9% of eraseblocks on the entire NAND chip, not just the MTD
+ partition UBI attaches. This means that if you have, say, a NAND
+ flash chip that admits a maximum 40 bad eraseblocks, and it is split
+ on two MTD partitions of the same size, UBI will reserve 40
+ eraseblocks when attaching a partition.
+
+ Leave the default value if unsure.
+
config MTD_UBI_GLUEBI
tristate "MTD devices emulation driver (gluebi)"
help
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index f926343..39a1e7e 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -51,7 +51,7 @@
#define UBI_NAME_STR "ubi"
/* Default number of maximum expected bad blocks per 1024 eraseblocks */
-#define MTD_UBI_DEFAULT_BEB_LIMIT 20
+#define MTD_UBI_DEFAULT_BEB_LIMIT CONFIG_MTD_UBI_DEFAULT_BEB_LIMIT
/* Normal UBI messages */
#define ubi_msg(fmt, ...) printk(KERN_NOTICE "UBI: " fmt "\n", ##__VA_ARGS__)
--
1.7.2.5
On Thu, 2012-08-16 at 16:50 +0300, Shmulik Ladkani wrote:
> Yes, but the main drawback I was referring to is those platforms already
> setting MTD_UBI_BEB_RESERVE other than the default, by means of kernel
> configuration.
> (there's one platform known to do so in its defconfig, that's
> sam9_l9260_defconfig, which uses 3% instead of the "standard" 2%).
Yes, I understand this. I think there are few such systems and many of
them are fine to pass the parameter explicitly.
> We force these systems to change their usermode because we changed ubi's
> default BEB limit to be 20/1024 _hardcoded_ (instead of kernel
> configurable as previously was).
Not necessarily, they can come to us and ask to add the kernel option.
> Is this ok?
It is not ideal, but I am willing to take this risk.
--
Best Regards,
Artem Bityutskiy
On Thu, 2012-08-16 at 16:30 +0200, Richard Genoud wrote:
> > (there's one platform known to do so in its defconfig, that's
> > sam9_l9260_defconfig, which uses 3% instead of the "standard" 2%).
> I found the board:
> https://www.olimex.com/dev/sam9-L9260.html
> and the nand datasheet:
> http://www.rockbox.org/wiki/pub/Main/LyrePrototype/K9xxG08UXM.pdf
> page 11, we can see that the max_bad_bebper1024 is 25 (100 for 4096)
OK, thanks for the research!
--
Best Regards,
Artem Bityutskiy
On Thu, 16 Aug 2012 16:33:32 +0300 Artem Bityutskiy <[email protected]> wrote:
> We do not have that big user-base. No one uses 0 in the tree, most use
> the default. I never heard anyone using 0. I did not use it also. I
> think it is OK to have the lower range start from non-zero. But why it
> is 2 and not 1 - I am not sure :-)
Artem,
Note that originally range was 0..25 (percentage).
It was changed to be 2..256 (per 1024) by 07ad322 in linux-ubi.
Seems arbitrary change.
So if you'd like, we can fix to 1..256 (or 0..256) - as you prefer.
Regards,
Shmulik
On Sun, 2012-08-19 at 10:09 +0300, Shmulik Ladkani wrote:
> On Thu, 16 Aug 2012 16:33:32 +0300 Artem Bityutskiy <[email protected]> wrote:
> > We do not have that big user-base. No one uses 0 in the tree, most use
> > the default. I never heard anyone using 0. I did not use it also. I
> > think it is OK to have the lower range start from non-zero. But why it
> > is 2 and not 1 - I am not sure :-)
>
> Artem,
>
> Note that originally range was 0..25 (percentage).
>
> It was changed to be 2..256 (per 1024) by 07ad322 in linux-ubi.
>
> Seems arbitrary change.
> So if you'd like, we can fix to 1..256 (or 0..256) - as you prefer.
Yeah, I wanted to make it 1..256 but forgot, will do now. 0..256 would
need some more work to avoid division by 0.
--
Best Regards,
Artem Bityutskiy
Hi Artem,
2012/8/19 Artem Bityutskiy <[email protected]>:
> Yeah, I wanted to make it 1..256 but forgot, will do now. 0..256 would
> need some more work to avoid division by 0.
Division by 0 is handled in the get_bad_peb_limit() function, I don't
see another dangerous place.
So, I think that we can change back the range to 0..256.
(and if we want to be coherent with user-space, it should be 0..255,
as the range is coded with an u8)
Richard.
--
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
On Mon, 2012-08-20 at 08:55 +0200, Richard Genoud wrote:
> Hi Artem,
> 2012/8/19 Artem Bityutskiy <[email protected]>:
> > Yeah, I wanted to make it 1..256 but forgot, will do now. 0..256 would
> > need some more work to avoid division by 0.
> Division by 0 is handled in the get_bad_peb_limit() function, I don't
> see another dangerous place.
if (mult_frac(limit, 1024, max_beb_per1024) < device_pebs)
will divide by 0 if max_beb_per1024 is 0.
> (and if we want to be coherent with user-space, it should be 0..255,
> as the range is coded with an u8)
I think it should be uint16_t instead, because we are defining ABI here
and we should not assume no one will ever nee values higher than 255.
--
Best Regards,
Artem Bityutskiy
2012/8/20 Artem Bityutskiy <[email protected]>:
> On Mon, 2012-08-20 at 08:55 +0200, Richard Genoud wrote:
>> Hi Artem,
>> 2012/8/19 Artem Bityutskiy <[email protected]>:
>> > Yeah, I wanted to make it 1..256 but forgot, will do now. 0..256 would
>> > need some more work to avoid division by 0.
>> Division by 0 is handled in the get_bad_peb_limit() function, I don't
>> see another dangerous place.
>
> if (mult_frac(limit, 1024, max_beb_per1024) < device_pebs)
>
> will divide by 0 if max_beb_per1024 is 0.
>
just a few lines before, you've got:
/*
* We don't want a division by 0, and having max_beb_per1024 == 0 is ok
*/
if (!max_beb_per1024)
return 0;
from commit abae1f1
(or I'm not looking at the right line ?)
>> (and if we want to be coherent with user-space, it should be 0..255,
>> as the range is coded with an u8)
>
> I think it should be uint16_t instead, because we are defining ABI here
> and we should not assume no one will ever nee values higher than 255.
I agree with you, even if 25% of reserved space for bad block seems
insane, we never know...
I'll update that.
--
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?