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

["Marek Behún" <kabel@kernel.org>]

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