| Message ID | 20180926055803.19071-1-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | lib: add missing files from libfdt | expand |
On Wed, Sep 26, 2018 at 12:57 AM AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > There are two files that are part of dtc/libfdt, but don't appear > in lib. This patch fixes it. > Please note that strtoul() used in fdt_overlay.c is now faked using > kstrtoul() (not simple_strtoul() which is obsolute). > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Frank Rowand <frowand.list@gmail.com> > --- > lib/Makefile | 2 +- > lib/fdt_addresses.c | 3 +++ > lib/fdt_overlay.c | 23 +++++++++++++++++++++++ > 3 files changed, 27 insertions(+), 1 deletion(-) > create mode 100644 lib/fdt_addresses.c > create mode 100644 lib/fdt_overlay.c > > diff --git a/lib/Makefile b/lib/Makefile > index ca3f7ebb900d..444c82413a3c 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -202,7 +202,7 @@ KASAN_SANITIZE_stackdepot.o := n > KCOV_INSTRUMENT_stackdepot.o := n > > libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \ > - fdt_empty_tree.o > + fdt_empty_tree.o fdt_addresses.o fdt_overlay.o Nothing in the kernel needs fdt_overlay.o, so we shouldn't add it until we do. It used to bloat the kernel size, but maybe that changed with using archive files now. > $(foreach file, $(libfdt_files), \ > $(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt)) > lib-$(CONFIG_LIBFDT) += $(libfdt_files) > diff --git a/lib/fdt_addresses.c b/lib/fdt_addresses.c > new file mode 100644 > index 000000000000..241780d09882 > --- /dev/null > +++ b/lib/fdt_addresses.c > @@ -0,0 +1,3 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/libfdt_env.h> > +#include "../scripts/dtc/libfdt/fdt_addresses.c" > diff --git a/lib/fdt_overlay.c b/lib/fdt_overlay.c > new file mode 100644 > index 000000000000..1def7268d313 > --- /dev/null > +++ b/lib/fdt_overlay.c > @@ -0,0 +1,23 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/kernel.h> > +#include <linux/libfdt_env.h> > + > +static unsigned long strtoul(const char *nptr, char **endptr, int base) > +{ > + unsigned long res; > + int ret; > + > + /* > + * FIXME: no way to return a correct value of endptr. > + * The values here would be still good for use in fdt_overlay > + */ > + *endptr = (char *)nptr; endptr could be NULL. > + ret = kstrtoul(nptr, base, &res); kstrtoul doesn't handle conversions if there are non-convertible characters after the number while strtoul (including simple_strtoul) will still do the conversion. We should make sure we don't need that behavior. That's doubtful if callers pass non-NULL endptr. > + if (ret < 0) > + return ULONG_MAX; > + > + (*endptr)++; > + return res; > +} > + > +#include "../scripts/dtc/libfdt/fdt_overlay.c" > -- > 2.18.0 >
Rob, On Fri, Sep 28, 2018 at 07:49:13AM -0500, Rob Herring wrote: > On Wed, Sep 26, 2018 at 12:57 AM AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > There are two files that are part of dtc/libfdt, but don't appear > > in lib. This patch fixes it. > > Please note that strtoul() used in fdt_overlay.c is now faked using > > kstrtoul() (not simple_strtoul() which is obsolute). > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Frank Rowand <frowand.list@gmail.com> > > --- > > lib/Makefile | 2 +- > > lib/fdt_addresses.c | 3 +++ > > lib/fdt_overlay.c | 23 +++++++++++++++++++++++ > > 3 files changed, 27 insertions(+), 1 deletion(-) > > create mode 100644 lib/fdt_addresses.c > > create mode 100644 lib/fdt_overlay.c > > > > diff --git a/lib/Makefile b/lib/Makefile > > index ca3f7ebb900d..444c82413a3c 100644 > > --- a/lib/Makefile > > +++ b/lib/Makefile > > @@ -202,7 +202,7 @@ KASAN_SANITIZE_stackdepot.o := n > > KCOV_INSTRUMENT_stackdepot.o := n > > > > libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \ > > - fdt_empty_tree.o > > + fdt_empty_tree.o fdt_addresses.o fdt_overlay.o > > Nothing in the kernel needs fdt_overlay.o, so we shouldn't add it > until we do. It used to bloat the kernel size, but maybe that changed > with using archive files now. > > > $(foreach file, $(libfdt_files), \ > > $(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt)) > > lib-$(CONFIG_LIBFDT) += $(libfdt_files) > > diff --git a/lib/fdt_addresses.c b/lib/fdt_addresses.c > > new file mode 100644 > > index 000000000000..241780d09882 > > --- /dev/null > > +++ b/lib/fdt_addresses.c > > @@ -0,0 +1,3 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +#include <linux/libfdt_env.h> > > +#include "../scripts/dtc/libfdt/fdt_addresses.c" > > diff --git a/lib/fdt_overlay.c b/lib/fdt_overlay.c > > new file mode 100644 > > index 000000000000..1def7268d313 > > --- /dev/null > > +++ b/lib/fdt_overlay.c > > @@ -0,0 +1,23 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +#include <linux/kernel.h> > > +#include <linux/libfdt_env.h> > > + > > +static unsigned long strtoul(const char *nptr, char **endptr, int base) > > +{ > > + unsigned long res; > > + int ret; > > + > > + /* > > + * FIXME: no way to return a correct value of endptr. > > + * The values here would be still good for use in fdt_overlay > > + */ > > + *endptr = (char *)nptr; > > endptr could be NULL. No, it couldn't at overlay_fixup_phandle(), which is the only caller of strtoul() in fdt_overlay.c, is concerned. > > + ret = kstrtoul(nptr, base, &res); > > kstrtoul doesn't handle conversions if there are non-convertible > characters after the number while strtoul (including simple_strtoul) > will still do the conversion. I don't believe so. > We should make sure we don't need that > behavior. That's doubtful if callers pass non-NULL endptr. > > > + if (ret < 0) > > + return ULONG_MAX; > > + > > + (*endptr)++; > > + return res; > > +} > > + > > +#include "../scripts/dtc/libfdt/fdt_overlay.c" > > -- > > 2.18.0 > > Having said that, I would simply drop 'fdt_overlay' part from my patch as it won't be used anywhere at this stage. Are you happy with this change? Thanks, -Takahiro Akashi
On Tue, Oct 2, 2018 at 1:11 AM AKASHI, Takahiro <takahiro.akashi@linaro.org> wrote: > > Rob, > > On Fri, Sep 28, 2018 at 07:49:13AM -0500, Rob Herring wrote: > > On Wed, Sep 26, 2018 at 12:57 AM AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > There are two files that are part of dtc/libfdt, but don't appear > > > in lib. This patch fixes it. > > > Please note that strtoul() used in fdt_overlay.c is now faked using > > > kstrtoul() (not simple_strtoul() which is obsolute). > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > Cc: Rob Herring <robh+dt@kernel.org> > > > Cc: Frank Rowand <frowand.list@gmail.com> > > > --- > > > lib/Makefile | 2 +- > > > lib/fdt_addresses.c | 3 +++ > > > lib/fdt_overlay.c | 23 +++++++++++++++++++++++ > > > 3 files changed, 27 insertions(+), 1 deletion(-) > > > create mode 100644 lib/fdt_addresses.c > > > create mode 100644 lib/fdt_overlay.c > > > > > > diff --git a/lib/Makefile b/lib/Makefile > > > index ca3f7ebb900d..444c82413a3c 100644 > > > --- a/lib/Makefile > > > +++ b/lib/Makefile > > > @@ -202,7 +202,7 @@ KASAN_SANITIZE_stackdepot.o := n > > > KCOV_INSTRUMENT_stackdepot.o := n > > > > > > libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \ > > > - fdt_empty_tree.o > > > + fdt_empty_tree.o fdt_addresses.o fdt_overlay.o > > > > Nothing in the kernel needs fdt_overlay.o, so we shouldn't add it > > until we do. It used to bloat the kernel size, but maybe that changed > > with using archive files now. > > > > > $(foreach file, $(libfdt_files), \ > > > $(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt)) > > > lib-$(CONFIG_LIBFDT) += $(libfdt_files) > > > diff --git a/lib/fdt_addresses.c b/lib/fdt_addresses.c > > > new file mode 100644 > > > index 000000000000..241780d09882 > > > --- /dev/null > > > +++ b/lib/fdt_addresses.c > > > @@ -0,0 +1,3 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +#include <linux/libfdt_env.h> > > > +#include "../scripts/dtc/libfdt/fdt_addresses.c" > > > diff --git a/lib/fdt_overlay.c b/lib/fdt_overlay.c > > > new file mode 100644 > > > index 000000000000..1def7268d313 > > > --- /dev/null > > > +++ b/lib/fdt_overlay.c > > > @@ -0,0 +1,23 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +#include <linux/kernel.h> > > > +#include <linux/libfdt_env.h> > > > + > > > +static unsigned long strtoul(const char *nptr, char **endptr, int base) > > > +{ > > > + unsigned long res; > > > + int ret; > > > + > > > + /* > > > + * FIXME: no way to return a correct value of endptr. > > > + * The values here would be still good for use in fdt_overlay > > > + */ > > > + *endptr = (char *)nptr; > > > > endptr could be NULL. > > No, it couldn't at overlay_fixup_phandle(), which is the only caller > of strtoul() in fdt_overlay.c, is concerned. The definition of strtoul allows endptr to be NULL, so your implementation should support that. Otherwise, when a new user is added we won't find it until run-time. > > > + ret = kstrtoul(nptr, base, &res); > > > > kstrtoul doesn't handle conversions if there are non-convertible > > characters after the number while strtoul (including simple_strtoul) > > will still do the conversion. > > I don't believe so. The same problem exists here. IMO, we should use simple_strtoul because kstrtoul is not a compatible implementation of strtoul. Or libfdt upstream should add a strtoul wrapper that has the same traits as kstrtoul. > > We should make sure we don't need that > > behavior. That's doubtful if callers pass non-NULL endptr. > > > > > + if (ret < 0) > > > + return ULONG_MAX; > > > + > > > + (*endptr)++; > > > + return res; > > > +} > > > + > > > +#include "../scripts/dtc/libfdt/fdt_overlay.c" > > > -- > > > 2.18.0 > > > > > Having said that, I would simply drop 'fdt_overlay' part from my patch > as it won't be used anywhere at this stage. > Are you happy with this change? That's fine by me. Rob
diff --git a/lib/Makefile b/lib/Makefile index ca3f7ebb900d..444c82413a3c 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -202,7 +202,7 @@ KASAN_SANITIZE_stackdepot.o := n KCOV_INSTRUMENT_stackdepot.o := n libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \ - fdt_empty_tree.o + fdt_empty_tree.o fdt_addresses.o fdt_overlay.o $(foreach file, $(libfdt_files), \ $(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt)) lib-$(CONFIG_LIBFDT) += $(libfdt_files) diff --git a/lib/fdt_addresses.c b/lib/fdt_addresses.c new file mode 100644 index 000000000000..241780d09882 --- /dev/null +++ b/lib/fdt_addresses.c @@ -0,0 +1,3 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/libfdt_env.h> +#include "../scripts/dtc/libfdt/fdt_addresses.c" diff --git a/lib/fdt_overlay.c b/lib/fdt_overlay.c new file mode 100644 index 000000000000..1def7268d313 --- /dev/null +++ b/lib/fdt_overlay.c @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/kernel.h> +#include <linux/libfdt_env.h> + +static unsigned long strtoul(const char *nptr, char **endptr, int base) +{ + unsigned long res; + int ret; + + /* + * FIXME: no way to return a correct value of endptr. + * The values here would be still good for use in fdt_overlay + */ + *endptr = (char *)nptr; + ret = kstrtoul(nptr, base, &res); + if (ret < 0) + return ULONG_MAX; + + (*endptr)++; + return res; +} + +#include "../scripts/dtc/libfdt/fdt_overlay.c"
There are two files that are part of dtc/libfdt, but don't appear in lib. This patch fixes it. Please note that strtoul() used in fdt_overlay.c is now faked using kstrtoul() (not simple_strtoul() which is obsolute). Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Frank Rowand <frowand.list@gmail.com> --- lib/Makefile | 2 +- lib/fdt_addresses.c | 3 +++ lib/fdt_overlay.c | 23 +++++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 lib/fdt_addresses.c create mode 100644 lib/fdt_overlay.c -- 2.18.0