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