From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B11F2C433FE for ; Thu, 13 Oct 2022 20:10:37 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 4746684D36; Thu, 13 Oct 2022 22:10:35 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="DmfY0CQB"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id A066F84CA5; Thu, 13 Oct 2022 22:10:33 +0200 (CEST) Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 88FDE84DB2 for ; Thu, 13 Oct 2022 22:10:30 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=kabel@kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 2700EB82078; Thu, 13 Oct 2022 20:10:30 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 05EFBC433D6; Thu, 13 Oct 2022 20:10:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1665691828; bh=btfxkwNtM5cNP1Qxlh43WWG8mJa5osci++GegdgkeEo=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=DmfY0CQBuzq/GexBtOqL8WM6jKRyGr7NfyaFm2BC6fIrsCMDoOnqWxf+JBKTkamfe hyG5j0giMKnS3ZdwnLGD7GOJIn23IiBs49lC9iS2i2pn/SdipvvsoZbkmsA1Y2H8jW xLwhT3eQzSKJFElibECe8RMhXoz5ZUYeZNyxkkbKa/gF/95+UEYSbrErXL2MvHa72Y seBtlc8WfTnyVQ85pa1dTv3BU4jmtD+81Aix1Yz5NfiYG4kyG4mn9uGWPb5wKYUdrI gxuZdV9lnUrVWBrE9q9ZwLwqXFEtta6+F3swSyLz3LPcSd69t7+ynANIIm4GduFMVE Kpsy2rOi9Zgyg== Date: Thu, 13 Oct 2022 22:07:23 +0200 From: Marek =?UTF-8?B?QmVow7pu?= To: Heinrich Schuchardt Cc: Wolfgang Denk , u-boot@lists.denx.de Subject: Re: [PATCH 1/1] powerpc: fix fdt_fixup_liodn_tbl_fman() Message-ID: <20221013220723.43dcccb8@thinkpad> In-Reply-To: References: <20221012171311.42881-1-heinrich.schuchardt@canonical.com> <20221013181236.0c5e94d8@thinkpad> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.34; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Thu, 13 Oct 2022 18:47:29 +0200 Heinrich Schuchardt wrote: > On 10/13/22 18:12, Marek Beh=C3=BAn wrote: > > On Wed, 12 Oct 2022 19:13:11 +0200 > > Heinrich Schuchardt wrote: > > =20 > >> Builiding with GCC 12.2 fails: > >> > >> arch/powerpc/cpu/mpc85xx/liodn.c: In function 'fdt_fixup_liodn_tb= l_fman': > >> arch/powerpc/cpu/mpc85xx/liodn.c:340:35: error: the comparison wi= ll > >> always evaluate as 'false' for the address of 'compat' > >> will never be NULL [-Werror=3Daddress] > >> 340 | if (tbl[i].compat =3D=3D NULL) > >> | > >> > >> Remove the superfluous check. > >> > >> Fixes: 97a8d010e029 ("net/fman: Support both new and legacy FMan Compa= tibles") > >> Signed-off-by: Heinrich Schuchardt > >> --- > >> 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/mpc85= xx/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 =3D 0; i < sz; i++) { > >> int off; > >> =20 > >> - if (tbl[i].compat =3D=3D NULL) > >> - continue; > >> - > >> /* Try the new compatible first. > >> * If the node is missing, try the old. > >> */ =20 > >=20 > > This is the wrong fix, IMO. Instead we should do something like > >=20 > > diff --git a/arch/powerpc/cpu/mpc85xx/liodn.c b/arch/powerpc/cpu/mpc85x= x/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 =3D 0; i < sz; i++) { > > int off; > > =20 > > - if (tbl[i].compat =3D=3D NULL) > > + if (tbl[i].compat[0] =3D=3D NULL) > > continue; > > =20 > > /* Try the new compatible first. > > @@ -345,7 +345,7 @@ static void fdt_fixup_liodn_tbl_fman(void *blob, > > */ > > off =3D 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] !=3D NULL) > > off =3D fdt_node_offset_by_compat_reg(blob, > > tbl[i].compat[1], tbl[i].compat_offset); =20 >=20 > There are two orthogonal changes here: >=20 > * removing a superfluous check. > * adding new ones >=20 > According to your review there seems to be nothing wrong in removing the= =20 > old check. >=20 > But if you think that a check of compat[i] is needed and you prefer to=20 > 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