Re: [PATCH 1/1] powerpc: fix fdt_fixup_liodn_tbl_fman()

[Heinrich Schuchardt]

On 10/13/22 18:12, Marek Behún wrote:
On Wed, 12 Oct 2022 19:13:11 +0200
Heinrich Schuchardt <heinrich.schucha...@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.schucha...@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