| Message ID | 20200615091740.2958303-1-noltari@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 91e81150d38842b58133ce1a5d70c88e8f1cf7c1 |
| Headers | show |
| Series | [v4] mtd: parsers: bcm63xx: simplify CFE detection | expand |
On 6/15/2020 2:17 AM, Álvaro Fernández Rojas wrote: > Instead of trying to parse CFE version string, which is customized by some > vendors, let's just check that "CFE1" was passed on argument 3. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On Mon, 2020-06-15 at 09:17:40 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?= wrote: > Instead of trying to parse CFE version string, which is customized by some > vendors, let's just check that "CFE1" was passed on argument 3. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: > Instead of trying to parse CFE version string, which is customized by some > vendors, let's just check that "CFE1" was passed on argument 3. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> mips:allmodconfig: ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! This is not surprising, since fw_arg3 is not exported. Guenter > --- > v4: shorten conditional compilation part as suggested by Miquèl. > v3: keep COMPILE_TEST compatibility by adding a new function that only checks > fw_arg3 when CONFIG_MIPS is defined. > v2: use CFE_EPTSEAL definition and avoid using an additional function. > > drivers/mtd/parsers/bcm63xxpart.c | 32 ++++++++++++------------------- > 1 file changed, 12 insertions(+), 20 deletions(-) > > diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c > index 78f90c6c18fd..b15bdadaedb5 100644 > --- a/drivers/mtd/parsers/bcm63xxpart.c > +++ b/drivers/mtd/parsers/bcm63xxpart.c > @@ -22,6 +22,11 @@ > #include <linux/mtd/partitions.h> > #include <linux/of.h> > > +#ifdef CONFIG_MIPS > +#include <asm/bootinfo.h> > +#include <asm/fw/cfe/cfe_api.h> > +#endif /* CONFIG_MIPS */ > + > #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ > > #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 > @@ -32,28 +37,15 @@ > #define STR_NULL_TERMINATE(x) \ > do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) > > -static int bcm63xx_detect_cfe(struct mtd_info *master) > +static inline int bcm63xx_detect_cfe(void) > { > - char buf[9]; > - int ret; > - size_t retlen; > + int ret = 0; > > - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, > - (void *)buf); > - buf[retlen] = 0; > +#ifdef CONFIG_MIPS > + ret = (fw_arg3 == CFE_EPTSEAL); > +#endif /* CONFIG_MIPS */ > > - if (ret) > - return ret; > - > - if (strncmp("cfe-v", buf, 5) == 0) > - return 0; > - > - /* very old CFE's do not have the cfe-v string, so check for magic */ > - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, > - (void *)buf); > - buf[retlen] = 0; > - > - return strncmp("CFE1CFE1", buf, 8); > + return ret; > } > > static int bcm63xx_read_nvram(struct mtd_info *master, > @@ -138,7 +130,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, > struct bcm963xx_nvram *nvram = NULL; > int ret; > > - if (bcm63xx_detect_cfe(master)) > + if (!bcm63xx_detect_cfe()) > return -EINVAL; > > nvram = vzalloc(sizeof(*nvram));
On Fri, 14 Aug 2020 at 14:26, Guenter Roeck <linux@roeck-us.net> wrote: > > On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: > > Instead of trying to parse CFE version string, which is customized by some > > vendors, let's just check that "CFE1" was passed on argument 3. > > > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > We still see mips: allmodconfig build failure on Linus tree make -sk KBUILD_BUILD_USER=TuxBuild -C/linux ARCH=mips CROSS_COMPILE=mips-linux-gnu- HOSTCC=gcc CC="sccache mips-linux-gnu-gcc" O=build allmodconfig make -sk KBUILD_BUILD_USER=TuxBuild -C/linux -j16 ARCH=mips CROSS_COMPILE=mips-linux-gnu- HOSTCC=gcc CC="sccache mips-linux-gnu-gcc" O=build > mips:allmodconfig: > > ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> Full build link, https://builds.tuxbuild.com/Sm59_9tjFMpIvT27qf5kNA/build.log > > This is not surprising, since fw_arg3 is not exported. > > Guenter > > > --- > > v4: shorten conditional compilation part as suggested by Miquèl. > > v3: keep COMPILE_TEST compatibility by adding a new function that only checks > > fw_arg3 when CONFIG_MIPS is defined. > > v2: use CFE_EPTSEAL definition and avoid using an additional function. > > > > drivers/mtd/parsers/bcm63xxpart.c | 32 ++++++++++++------------------- > > 1 file changed, 12 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c > > index 78f90c6c18fd..b15bdadaedb5 100644 > > --- a/drivers/mtd/parsers/bcm63xxpart.c > > +++ b/drivers/mtd/parsers/bcm63xxpart.c > > @@ -22,6 +22,11 @@ > > #include <linux/mtd/partitions.h> > > #include <linux/of.h> > > > > +#ifdef CONFIG_MIPS > > +#include <asm/bootinfo.h> > > +#include <asm/fw/cfe/cfe_api.h> > > +#endif /* CONFIG_MIPS */ > > + > > #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ > > > > #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 > > @@ -32,28 +37,15 @@ > > #define STR_NULL_TERMINATE(x) \ > > do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) > > > > -static int bcm63xx_detect_cfe(struct mtd_info *master) > > +static inline int bcm63xx_detect_cfe(void) > > { > > - char buf[9]; > > - int ret; > > - size_t retlen; > > + int ret = 0; > > > > - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, > > - (void *)buf); > > - buf[retlen] = 0; > > +#ifdef CONFIG_MIPS > > + ret = (fw_arg3 == CFE_EPTSEAL); > > +#endif /* CONFIG_MIPS */ > > > > - if (ret) > > - return ret; > > - > > - if (strncmp("cfe-v", buf, 5) == 0) > > - return 0; > > - > > - /* very old CFE's do not have the cfe-v string, so check for magic */ > > - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, > > - (void *)buf); > > - buf[retlen] = 0; > > - > > - return strncmp("CFE1CFE1", buf, 8); > > + return ret; > > } > > > > static int bcm63xx_read_nvram(struct mtd_info *master, > > @@ -138,7 +130,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, > > struct bcm963xx_nvram *nvram = NULL; > > int ret; > > > > - if (bcm63xx_detect_cfe(master)) > > + if (!bcm63xx_detect_cfe()) > > return -EINVAL; > > > > nvram = vzalloc(sizeof(*nvram));
On 9/21/20 8:18 PM, Naresh Kamboju wrote: > On Fri, 14 Aug 2020 at 14:26, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: >>> Instead of trying to parse CFE version string, which is customized by some >>> vendors, let's just check that "CFE1" was passed on argument 3. >>> >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> > > We still see mips: allmodconfig build failure on Linus tree > Yes, same here. Guenter > make -sk KBUILD_BUILD_USER=TuxBuild -C/linux ARCH=mips > CROSS_COMPILE=mips-linux-gnu- HOSTCC=gcc CC="sccache > mips-linux-gnu-gcc" O=build allmodconfig > > make -sk KBUILD_BUILD_USER=TuxBuild -C/linux -j16 ARCH=mips > CROSS_COMPILE=mips-linux-gnu- HOSTCC=gcc CC="sccache > mips-linux-gnu-gcc" O=build > > >> mips:allmodconfig: >> >> ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! > > ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > > Full build link, > https://builds.tuxbuild.com/Sm59_9tjFMpIvT27qf5kNA/build.log > >> >> This is not surprising, since fw_arg3 is not exported. >> >> Guenter >> >>> --- >>> v4: shorten conditional compilation part as suggested by Miquèl. >>> v3: keep COMPILE_TEST compatibility by adding a new function that only checks >>> fw_arg3 when CONFIG_MIPS is defined. >>> v2: use CFE_EPTSEAL definition and avoid using an additional function. >>> >>> drivers/mtd/parsers/bcm63xxpart.c | 32 ++++++++++++------------------- >>> 1 file changed, 12 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c >>> index 78f90c6c18fd..b15bdadaedb5 100644 >>> --- a/drivers/mtd/parsers/bcm63xxpart.c >>> +++ b/drivers/mtd/parsers/bcm63xxpart.c >>> @@ -22,6 +22,11 @@ >>> #include <linux/mtd/partitions.h> >>> #include <linux/of.h> >>> >>> +#ifdef CONFIG_MIPS >>> +#include <asm/bootinfo.h> >>> +#include <asm/fw/cfe/cfe_api.h> >>> +#endif /* CONFIG_MIPS */ >>> + >>> #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ >>> >>> #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 >>> @@ -32,28 +37,15 @@ >>> #define STR_NULL_TERMINATE(x) \ >>> do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) >>> >>> -static int bcm63xx_detect_cfe(struct mtd_info *master) >>> +static inline int bcm63xx_detect_cfe(void) >>> { >>> - char buf[9]; >>> - int ret; >>> - size_t retlen; >>> + int ret = 0; >>> >>> - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, >>> - (void *)buf); >>> - buf[retlen] = 0; >>> +#ifdef CONFIG_MIPS >>> + ret = (fw_arg3 == CFE_EPTSEAL); >>> +#endif /* CONFIG_MIPS */ >>> >>> - if (ret) >>> - return ret; >>> - >>> - if (strncmp("cfe-v", buf, 5) == 0) >>> - return 0; >>> - >>> - /* very old CFE's do not have the cfe-v string, so check for magic */ >>> - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, >>> - (void *)buf); >>> - buf[retlen] = 0; >>> - >>> - return strncmp("CFE1CFE1", buf, 8); >>> + return ret; >>> } >>> >>> static int bcm63xx_read_nvram(struct mtd_info *master, >>> @@ -138,7 +130,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, >>> struct bcm963xx_nvram *nvram = NULL; >>> int ret; >>> >>> - if (bcm63xx_detect_cfe(master)) >>> + if (!bcm63xx_detect_cfe()) >>> return -EINVAL; >>> >>> nvram = vzalloc(sizeof(*nvram));
Hello, Guenter Roeck <linux@roeck-us.net> wrote on Mon, 21 Sep 2020 20:26:19 -0700: > On 9/21/20 8:18 PM, Naresh Kamboju wrote: > > On Fri, 14 Aug 2020 at 14:26, Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: > >>> Instead of trying to parse CFE version string, which is customized by some > >>> vendors, let's just check that "CFE1" was passed on argument 3. > >>> > >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > >>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > >>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > >> > > > > We still see mips: allmodconfig build failure on Linus tree > > > > Yes, same here. Perhaps this check should be done by an exported helper so that we do not blindly export the variable? Alvaro, Jonas, can one of you try to address this issue please? Thanks, Miquèl
On 9/28/2020 7:16 AM, Miquel Raynal wrote: > Hello, > > Guenter Roeck <linux@roeck-us.net> wrote on Mon, 21 Sep 2020 20:26:19 > -0700: > >> On 9/21/20 8:18 PM, Naresh Kamboju wrote: >>> On Fri, 14 Aug 2020 at 14:26, Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: >>>>> Instead of trying to parse CFE version string, which is customized by some >>>>> vendors, let's just check that "CFE1" was passed on argument 3. >>>>> >>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >>>>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >>>> >>> >>> We still see mips: allmodconfig build failure on Linus tree >>> >> >> Yes, same here. > > Perhaps this check should be done by an exported helper so that we do > not blindly export the variable? > > Alvaro, Jonas, can one of you try to address this issue please? We should probably just make the parser 'bool' there is no much point building this as a module, it will not improve compile time coverage, and it will most likely not help with the kernel image size on the platforms that require the parser to be there anyway.
diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c index 78f90c6c18fd..b15bdadaedb5 100644 --- a/drivers/mtd/parsers/bcm63xxpart.c +++ b/drivers/mtd/parsers/bcm63xxpart.c @@ -22,6 +22,11 @@ #include <linux/mtd/partitions.h> #include <linux/of.h> +#ifdef CONFIG_MIPS +#include <asm/bootinfo.h> +#include <asm/fw/cfe/cfe_api.h> +#endif /* CONFIG_MIPS */ + #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 @@ -32,28 +37,15 @@ #define STR_NULL_TERMINATE(x) \ do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) -static int bcm63xx_detect_cfe(struct mtd_info *master) +static inline int bcm63xx_detect_cfe(void) { - char buf[9]; - int ret; - size_t retlen; + int ret = 0; - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, - (void *)buf); - buf[retlen] = 0; +#ifdef CONFIG_MIPS + ret = (fw_arg3 == CFE_EPTSEAL); +#endif /* CONFIG_MIPS */ - if (ret) - return ret; - - if (strncmp("cfe-v", buf, 5) == 0) - return 0; - - /* very old CFE's do not have the cfe-v string, so check for magic */ - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, - (void *)buf); - buf[retlen] = 0; - - return strncmp("CFE1CFE1", buf, 8); + return ret; } static int bcm63xx_read_nvram(struct mtd_info *master, @@ -138,7 +130,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, struct bcm963xx_nvram *nvram = NULL; int ret; - if (bcm63xx_detect_cfe(master)) + if (!bcm63xx_detect_cfe()) return -EINVAL; nvram = vzalloc(sizeof(*nvram));