| Message ID | 20220702132832.32187-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] avb: honor CONFIG_SYS_64BIT_LBA | expand |
On Sat, Jul 02, 2022 at 03:28:32PM +0200, Heinrich Schuchardt wrote: > The size of lbaint_t depends on CONFIG_SYS_64BIT_LBA defined in common.h. > common.h should always be included as first include. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > common/avb_verify.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/common/avb_verify.c b/common/avb_verify.c > index 0520a71455..6d11e31687 100644 > --- a/common/avb_verify.c > +++ b/common/avb_verify.c > @@ -4,6 +4,7 @@ > * SPDX-License-Identifier: GPL-2.0+ > */ > > +#include <common.h> > #include <avb_verify.h> > #include <blk.h> > #include <cpu_func.h> Did you find a problem here, by inspection? If so, OK, I'll take this for master. Otherwise, please drop common.h from the file and see what includes it needs directly, given that -next has the CONFIG symbol in question migrated to Kconfig and so solves the overall problem.
On 7/4/22 14:19, Tom Rini wrote: > On Sat, Jul 02, 2022 at 03:28:32PM +0200, Heinrich Schuchardt wrote: > >> The size of lbaint_t depends on CONFIG_SYS_64BIT_LBA defined in common.h. >> common.h should always be included as first include. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> common/avb_verify.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/common/avb_verify.c b/common/avb_verify.c >> index 0520a71455..6d11e31687 100644 >> --- a/common/avb_verify.c >> +++ b/common/avb_verify.c >> @@ -4,6 +4,7 @@ >> * SPDX-License-Identifier: GPL-2.0+ >> */ >> >> +#include <common.h> >> #include <avb_verify.h> >> #include <blk.h> >> #include <cpu_func.h> > > Did you find a problem here, by inspection? If so, OK, I'll take this > for master. Otherwise, please drop common.h from the file and see what > includes it needs directly, given that -next has the CONFIG symbol in > question migrated to Kconfig and so solves the overall problem. > You already merged 054de212cef6 ("disk: honor CONFIG_SYS_64BIT_LBA"). I looked for other uses of blk.h without prior inclusion of common.h. Migration to Kconfig does not move the definition of CONFIG_SYS_64BIT_LBA into blk.h but keeps it in common.h. So the correct size of lbaint_t will still depend on including common.h. Best regards Heinrich
On Mon, Jul 04, 2022 at 08:37:48PM +0200, Heinrich Schuchardt wrote: > > > On 7/4/22 14:19, Tom Rini wrote: > > On Sat, Jul 02, 2022 at 03:28:32PM +0200, Heinrich Schuchardt wrote: > > > > > The size of lbaint_t depends on CONFIG_SYS_64BIT_LBA defined in common.h. > > > common.h should always be included as first include. > > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > > --- > > > common/avb_verify.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/common/avb_verify.c b/common/avb_verify.c > > > index 0520a71455..6d11e31687 100644 > > > --- a/common/avb_verify.c > > > +++ b/common/avb_verify.c > > > @@ -4,6 +4,7 @@ > > > * SPDX-License-Identifier: GPL-2.0+ > > > */ > > > +#include <common.h> > > > #include <avb_verify.h> > > > #include <blk.h> > > > #include <cpu_func.h> > > > > Did you find a problem here, by inspection? If so, OK, I'll take this > > for master. Otherwise, please drop common.h from the file and see what > > includes it needs directly, given that -next has the CONFIG symbol in > > question migrated to Kconfig and so solves the overall problem. > > > > You already merged > 054de212cef6 ("disk: honor CONFIG_SYS_64BIT_LBA"). > I looked for other uses of blk.h without prior inclusion of common.h. > > Migration to Kconfig does not move the definition of CONFIG_SYS_64BIT_LBA > into blk.h but keeps it in common.h. So the correct size of lbaint_t will > still depend on including common.h. Wait, I don't follow, sorry. Can you please reproduce the problem you have, on next? CONFIG_SYS_64BIT_LBA is defined in Kconfig in next, so it will always be defined (or not) before any header is used because of -include. And as a follow-up, nothing should be using <common.h> as it's only including other headers, so files and headers should include what they need directly.
On 7/4/22 20:56, Tom Rini wrote: > On Mon, Jul 04, 2022 at 08:37:48PM +0200, Heinrich Schuchardt wrote: >> >> >> On 7/4/22 14:19, Tom Rini wrote: >>> On Sat, Jul 02, 2022 at 03:28:32PM +0200, Heinrich Schuchardt wrote: >>> >>>> The size of lbaint_t depends on CONFIG_SYS_64BIT_LBA defined in common.h. >>>> common.h should always be included as first include. >>>> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>> --- >>>> common/avb_verify.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/common/avb_verify.c b/common/avb_verify.c >>>> index 0520a71455..6d11e31687 100644 >>>> --- a/common/avb_verify.c >>>> +++ b/common/avb_verify.c >>>> @@ -4,6 +4,7 @@ >>>> * SPDX-License-Identifier: GPL-2.0+ >>>> */ >>>> +#include <common.h> >>>> #include <avb_verify.h> >>>> #include <blk.h> >>>> #include <cpu_func.h> >>> >>> Did you find a problem here, by inspection? If so, OK, I'll take this >>> for master. Otherwise, please drop common.h from the file and see what >>> includes it needs directly, given that -next has the CONFIG symbol in >>> question migrated to Kconfig and so solves the overall problem. >>> >> >> You already merged >> 054de212cef6 ("disk: honor CONFIG_SYS_64BIT_LBA"). >> I looked for other uses of blk.h without prior inclusion of common.h. >> >> Migration to Kconfig does not move the definition of CONFIG_SYS_64BIT_LBA >> into blk.h but keeps it in common.h. So the correct size of lbaint_t will >> still depend on including common.h. > > Wait, I don't follow, sorry. Can you please reproduce the problem you > have, on next? CONFIG_SYS_64BIT_LBA is defined in Kconfig in next, so > it will always be defined (or not) before any header is used because of > -include. GCC is called with -include ./include/linux/kconfig.h which includes generated/autoconf.h defining CONFIG_SYS_64BIT_LBA. Please, drop the patch. Best regards Heinrich > > And as a follow-up, nothing should be using <common.h> as it's only > including other headers, so files and headers should include what they > need directly. >
diff --git a/common/avb_verify.c b/common/avb_verify.c index 0520a71455..6d11e31687 100644 --- a/common/avb_verify.c +++ b/common/avb_verify.c @@ -4,6 +4,7 @@ * SPDX-License-Identifier: GPL-2.0+ */ +#include <common.h> #include <avb_verify.h> #include <blk.h> #include <cpu_func.h>
The size of lbaint_t depends on CONFIG_SYS_64BIT_LBA defined in common.h. common.h should always be included as first include. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- common/avb_verify.c | 1 + 1 file changed, 1 insertion(+)