| Message ID | 1479757899-6849-3-git-send-email-zach.brown@ni.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Nov 21, 2016 at 01:51:36PM -0600, Zach Brown wrote: > From: Jeff Westfahl <jeff.westfahl@ni.com> > > Use the MTD function 'max_bad_blocks' to compute the UBI bad_peb_limit, > if the function is implemented for an MTD and doesn't return an error. I'm not exactly a UBI expert here, but it seems reasonable that we should adjust the Kconfig documentation for MTD_UBI_BEB_LIMIT to further emphasize that it's a default, if the value can't be determined by other means. > Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com> > Signed-off-by: Zach Brown <zach.brown@ni.com> > Acked-by: Boris Brezillon <boris.brezillon@free-electron.com> > --- > drivers/mtd/ubi/build.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index 85d54f3..e9940a9 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -584,6 +584,10 @@ static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024) > int limit, device_pebs; > uint64_t device_size; > > + limit = mtd_max_bad_blocks(ubi->mtd, 0, ubi->mtd->size); > + if (limit > 0) > + return limit; Are you sure you want to even override the user-provided max_beb_per1024 value taken from the mtd= line? I'd think if someone went as far as to specify this in the kernel command line, they don't expect it to get overridden. Just my two cents. Brian > + > if (!max_beb_per1024) > return 0; > > -- > 2.7.4 >
On Mon, Nov 21, 2016 at 01:51:36PM -0600, Zach Brown wrote: > From: Jeff Westfahl <jeff.westfahl@ni.com> > > Use the MTD function 'max_bad_blocks' to compute the UBI bad_peb_limit, > if the function is implemented for an MTD and doesn't return an error. > > Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com> > Signed-off-by: Zach Brown <zach.brown@ni.com> > Acked-by: Boris Brezillon <boris.brezillon@free-electron.com> > --- > drivers/mtd/ubi/build.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index 85d54f3..e9940a9 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -584,6 +584,10 @@ static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024) > int limit, device_pebs; > uint64_t device_size; > > + limit = mtd_max_bad_blocks(ubi->mtd, 0, ubi->mtd->size); > + if (limit > 0) I guess we're assuming 0 is an erroneous value? Otherwise, why would mtd_can_have_bb() be true? Brian > + return limit; > + > if (!max_beb_per1024) > return 0; > > -- > 2.7.4 >
Zach, Brian, sorry for the late review. On 22.11.2016 19:42, Brian Norris wrote: > On Mon, Nov 21, 2016 at 01:51:36PM -0600, Zach Brown wrote: >> From: Jeff Westfahl <jeff.westfahl@ni.com> >> >> Use the MTD function 'max_bad_blocks' to compute the UBI bad_peb_limit, >> if the function is implemented for an MTD and doesn't return an error. > > I'm not exactly a UBI expert here, but it seems reasonable that we > should adjust the Kconfig documentation for MTD_UBI_BEB_LIMIT to further > emphasize that it's a default, if the value can't be determined by other > means. > >> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com> >> Signed-off-by: Zach Brown <zach.brown@ni.com> >> Acked-by: Boris Brezillon <boris.brezillon@free-electron.com> >> --- >> drivers/mtd/ubi/build.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c >> index 85d54f3..e9940a9 100644 >> --- a/drivers/mtd/ubi/build.c >> +++ b/drivers/mtd/ubi/build.c >> @@ -584,6 +584,10 @@ static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024) >> int limit, device_pebs; >> uint64_t device_size; >> >> + limit = mtd_max_bad_blocks(ubi->mtd, 0, ubi->mtd->size); >> + if (limit > 0) >> + return limit; > > Are you sure you want to even override the user-provided > max_beb_per1024 value taken from the mtd= line? I'd think if someone > went as far as to specify this in the kernel command line, they don't > expect it to get overridden. Just my two cents. I agree. With this patch applied the limit can be set via Kconfig, cmdline and automatically. IMHO the automatic value via mtd_max_bad_blocks and over-writeable via cmdline and Kconfig. ...and this needs to be documented. :-) I'd go so far and suggest dropping the Kconfig option. Thanks, //richard
On 25.11.2016 14:37, Richard Weinberger wrote: >> Are you sure you want to even override the user-provided >> max_beb_per1024 value taken from the mtd= line? I'd think if someone >> went as far as to specify this in the kernel command line, they don't >> expect it to get overridden. Just my two cents. > > I agree. With this patch applied the limit can be set via Kconfig, cmdline and automatically. > IMHO the automatic value via mtd_max_bad_blocks and over-writeable via cmdline and Kconfig. s/and/should be/ Thanks, //richard
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index 85d54f3..e9940a9 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -584,6 +584,10 @@ static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024) int limit, device_pebs; uint64_t device_size; + limit = mtd_max_bad_blocks(ubi->mtd, 0, ubi->mtd->size); + if (limit > 0) + return limit; + if (!max_beb_per1024) return 0;