| Message ID | 20221012171311.42881-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted, archived |
| Commit | f17fe71222e5fa0975089772e22b45979f202eff |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] powerpc: fix fdt_fixup_liodn_tbl_fman() | expand |
On Wed, 12 Oct 2022 19:13:11 +0200 Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > Builiding with GCC 12.2 fails: > > arch/powerpc/cpu/mpc85xx/liodn.c: In function 'fdt_fixup_liodn_tbl_fman': > arch/powerpc/cpu/mpc85xx/liodn.c:340:35: error: the comparison will > always evaluate as 'false' for the address of 'compat' > will never be NULL [-Werror=address] > 340 | if (tbl[i].compat == NULL) > | > > Remove the superfluous check. > > Fixes: 97a8d010e029 ("net/fman: Support both new and legacy FMan Compatibles") > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > arch/powerpc/cpu/mpc85xx/liodn.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/arch/powerpc/cpu/mpc85xx/liodn.c b/arch/powerpc/cpu/mpc85xx/liodn.c > index a084002494..2d55916841 100644 > --- a/arch/powerpc/cpu/mpc85xx/liodn.c > +++ b/arch/powerpc/cpu/mpc85xx/liodn.c > @@ -337,9 +337,6 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, > for (i = 0; i < sz; i++) { > int off; > > - if (tbl[i].compat == NULL) > - continue; > - > /* Try the new compatible first. > * If the node is missing, try the old. > */ This is the wrong fix, IMO. Instead we should do something like diff --git a/arch/powerpc/cpu/mpc85xx/liodn.c b/arch/powerpc/cpu/mpc85xx/liodn.c index a084002494..41b7d53ec3 100644 --- a/arch/powerpc/cpu/mpc85xx/liodn.c +++ b/arch/powerpc/cpu/mpc85xx/liodn.c @@ -337,7 +337,7 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, for (i = 0; i < sz; i++) { int off; - if (tbl[i].compat == NULL) + if (tbl[i].compat[0] == NULL) continue; /* Try the new compatible first. @@ -345,7 +345,7 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, */ off = fdt_node_offset_by_compat_reg(blob, tbl[i].compat[0], tbl[i].compat_offset); - if (off < 0) + if (off < 0 && tbl[i].compat[1] != NULL) off = fdt_node_offset_by_compat_reg(blob, tbl[i].compat[1], tbl[i].compat_offset);
On 10/13/22 18:12, Marek Behún wrote: > On Wed, 12 Oct 2022 19:13:11 +0200 > Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > >> Builiding with GCC 12.2 fails: >> >> arch/powerpc/cpu/mpc85xx/liodn.c: In function 'fdt_fixup_liodn_tbl_fman': >> arch/powerpc/cpu/mpc85xx/liodn.c:340:35: error: the comparison will >> always evaluate as 'false' for the address of 'compat' >> will never be NULL [-Werror=address] >> 340 | if (tbl[i].compat == NULL) >> | >> >> Remove the superfluous check. >> >> Fixes: 97a8d010e029 ("net/fman: Support both new and legacy FMan Compatibles") >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> arch/powerpc/cpu/mpc85xx/liodn.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/arch/powerpc/cpu/mpc85xx/liodn.c b/arch/powerpc/cpu/mpc85xx/liodn.c >> index a084002494..2d55916841 100644 >> --- a/arch/powerpc/cpu/mpc85xx/liodn.c >> +++ b/arch/powerpc/cpu/mpc85xx/liodn.c >> @@ -337,9 +337,6 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, >> for (i = 0; i < sz; i++) { >> int off; >> >> - if (tbl[i].compat == NULL) >> - continue; >> - >> /* Try the new compatible first. >> * If the node is missing, try the old. >> */ > > This is the wrong fix, IMO. Instead we should do something like > > diff --git a/arch/powerpc/cpu/mpc85xx/liodn.c b/arch/powerpc/cpu/mpc85xx/liodn.c > index a084002494..41b7d53ec3 100644 > --- a/arch/powerpc/cpu/mpc85xx/liodn.c > +++ b/arch/powerpc/cpu/mpc85xx/liodn.c > @@ -337,7 +337,7 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, > for (i = 0; i < sz; i++) { > int off; > > - if (tbl[i].compat == NULL) > + if (tbl[i].compat[0] == NULL) > continue; > > /* Try the new compatible first. > @@ -345,7 +345,7 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, > */ > off = fdt_node_offset_by_compat_reg(blob, > tbl[i].compat[0], tbl[i].compat_offset); > - if (off < 0) > + if (off < 0 && tbl[i].compat[1] != NULL) > off = fdt_node_offset_by_compat_reg(blob, > tbl[i].compat[1], tbl[i].compat_offset); There are two orthogonal changes here: * removing a superfluous check. * adding new ones According to your review there seems to be nothing wrong in removing the old check. But if you think that a check of compat[i] is needed and you prefer to create a patch combining both changes, please, go ahead. Best regards Heinrich
On Thu, 13 Oct 2022 18:47:29 +0200 Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > On 10/13/22 18:12, Marek Behún wrote: > > On Wed, 12 Oct 2022 19:13:11 +0200 > > Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > >> Builiding with GCC 12.2 fails: > >> > >> arch/powerpc/cpu/mpc85xx/liodn.c: In function 'fdt_fixup_liodn_tbl_fman': > >> arch/powerpc/cpu/mpc85xx/liodn.c:340:35: error: the comparison will > >> always evaluate as 'false' for the address of 'compat' > >> will never be NULL [-Werror=address] > >> 340 | if (tbl[i].compat == NULL) > >> | > >> > >> Remove the superfluous check. > >> > >> Fixes: 97a8d010e029 ("net/fman: Support both new and legacy FMan Compatibles") > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> --- > >> arch/powerpc/cpu/mpc85xx/liodn.c | 3 --- > >> 1 file changed, 3 deletions(-) > >> > >> diff --git a/arch/powerpc/cpu/mpc85xx/liodn.c b/arch/powerpc/cpu/mpc85xx/liodn.c > >> index a084002494..2d55916841 100644 > >> --- a/arch/powerpc/cpu/mpc85xx/liodn.c > >> +++ b/arch/powerpc/cpu/mpc85xx/liodn.c > >> @@ -337,9 +337,6 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, > >> for (i = 0; i < sz; i++) { > >> int off; > >> > >> - if (tbl[i].compat == NULL) > >> - continue; > >> - > >> /* Try the new compatible first. > >> * If the node is missing, try the old. > >> */ > > > > This is the wrong fix, IMO. Instead we should do something like > > > > diff --git a/arch/powerpc/cpu/mpc85xx/liodn.c b/arch/powerpc/cpu/mpc85xx/liodn.c > > index a084002494..41b7d53ec3 100644 > > --- a/arch/powerpc/cpu/mpc85xx/liodn.c > > +++ b/arch/powerpc/cpu/mpc85xx/liodn.c > > @@ -337,7 +337,7 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, > > for (i = 0; i < sz; i++) { > > int off; > > > > - if (tbl[i].compat == NULL) > > + if (tbl[i].compat[0] == NULL) > > continue; > > > > /* Try the new compatible first. > > @@ -345,7 +345,7 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, > > */ > > off = fdt_node_offset_by_compat_reg(blob, > > tbl[i].compat[0], tbl[i].compat_offset); > > - if (off < 0) > > + if (off < 0 && tbl[i].compat[1] != NULL) > > off = fdt_node_offset_by_compat_reg(blob, > > tbl[i].compat[1], tbl[i].compat_offset); > > There are two orthogonal changes here: > > * removing a superfluous check. > * adding new ones > > According to your review there seems to be nothing wrong in removing the > old check. > > But if you think that a check of compat[i] is needed and you prefer to > create a patch combining both changes, please, go ahead. My reasoning is that - we are trying to fix function fdt_fixup_liodn_tbl_fman(), which operates on struct fman_liodn_id_table - just above this function there is function fdt_fixup_liodn_tbl(), which operates on struct liodn_id_table - one of the differences between these 2 structs is that the fman one has char *compat[2], while the non-fman has char *compat member - it seems that the fman version of this function was inspired by the non-fman one, and just copied this check, but since the new struct changed char *compat to char *compat[2], it should also have fixed this check, so that the original idea of what this function should do would be preserved Of course now there is the question whether these checks are needed at all, even in the non-fman function. I looked at the instances of these structs and it seems there are none where compat is NULL... Marek
On Wed, 12 Oct 2022 19:13:11 +0200, Heinrich Schuchardt wrote: > Builiding with GCC 12.2 fails: > > arch/powerpc/cpu/mpc85xx/liodn.c: In function 'fdt_fixup_liodn_tbl_fman': > arch/powerpc/cpu/mpc85xx/liodn.c:340:35: error: the comparison will > always evaluate as 'false' for the address of 'compat' > will never be NULL [-Werror=address] > 340 | if (tbl[i].compat == NULL) > | > > [...] I know there was some discussion of a more optimal or useful set of checks here but since that hasn't been posted and I want to mvoe to gcc-12.2 now, this is applied to u-boot/next.
diff --git a/arch/powerpc/cpu/mpc85xx/liodn.c b/arch/powerpc/cpu/mpc85xx/liodn.c index a084002494..2d55916841 100644 --- a/arch/powerpc/cpu/mpc85xx/liodn.c +++ b/arch/powerpc/cpu/mpc85xx/liodn.c @@ -337,9 +337,6 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, for (i = 0; i < sz; i++) { int off; - if (tbl[i].compat == NULL) - continue; - /* Try the new compatible first. * If the node is missing, try the old. */
Builiding with GCC 12.2 fails: arch/powerpc/cpu/mpc85xx/liodn.c: In function 'fdt_fixup_liodn_tbl_fman': arch/powerpc/cpu/mpc85xx/liodn.c:340:35: error: the comparison will always evaluate as 'false' for the address of 'compat' will never be NULL [-Werror=address] 340 | if (tbl[i].compat == NULL) | Remove the superfluous check. Fixes: 97a8d010e029 ("net/fman: Support both new and legacy FMan Compatibles") Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- arch/powerpc/cpu/mpc85xx/liodn.c | 3 --- 1 file changed, 3 deletions(-)