netdev - Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2ad136ed-be3a-407f-bf3c-5faf664b927c@arinc9.com>
Date: Sat, 6 Jan 2024 20:01:25 +0200
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Vladimir Oltean <olteanv@...il.com>, Simon Horman <horms@...nel.org>,
 Daniel Golle <daniel@...rotopia.org>, Landen Chao
 <Landen.Chao@...iatek.com>, DENG Qingfang <dqfext@...il.com>,
 Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
 Florian Fainelli <f.fainelli@...il.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org,
 Frank Wunderlich <frank-w@...lic-files.de>,
 Bartel Eerdekens <bartel.eerdekens@...stell8.be>, mithat.guner@...ont.com,
 erkin.bozoglu@...ont.com
Subject: Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run
 mt7530_setup_port5() if port 5 is disabled

On 1/2/24 13:16, Dan Carpenter wrote:
> On Sun, Dec 17, 2023 at 03:22:27PM +0300, Arınç ÜNAL wrote:
>> On 8.12.2023 21:46, Vladimir Oltean wrote:
>>> Hmm, maybe the problem, all along, was that we let the -ENODEV return
>>> code from of_get_phy_mode() pass through? "interface" will really be
>>> uninitialized in that case. It's not a false positive.
>>>
>>> Instead of:
>>>
>>> 	ret = of_get_phy_mode(mac_np, &interface);
>>> 	if (ret && ret != -ENODEV) {
>>> 		...
>>> 		return ret;
>>> 	}
>>>
>>> it should have been like this, to not complain:
>>>
>>> 	ret = of_get_phy_mode(mac_np, &interface);
>>> 	if (ret) {
>>> 		...
>>> 		return ret;
>>> 	}
>>>
>>
>> I just tried this, smatch still reports "interface" as uninitialised.
>>
>> $ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
>> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
>>
>>    UPD     include/config/kernel.release
>>    UPD     include/generated/utsrelease.h
>>    CHECK   scripts/mod/empty.c
>>    CALL    scripts/checksyscalls.sh
>>    CC      drivers/net/dsa/mt7530.o
>>    CHECK   drivers/net/dsa/mt7530.c
>> drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
>> drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
>> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
>> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
>> drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.
> 
> That's so strange.
> 
> Vladimir was right that I was misreading what he said and also hadn't
> noticed the break.
> 
> For me, his approach silences the warning with or without the cross
> function DB.  Also of_get_phy_mode() initializes interface on all paths
> so checking for -EINVAL doesn't matter as far as this warning is
> concerned.

I must be missing something.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 7f9d63b61168..05ce3face47c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2325,7 +2325,7 @@ mt7530_setup(struct dsa_switch *ds)
  
  			if (phy_node->parent == priv->dev->of_node->parent) {
  				ret = of_get_phy_mode(mac_np, &interface);
-				if (ret && ret != -ENODEV) {
+				if (ret) {
  					of_node_put(mac_np);
  					of_node_put(phy_node);
  					return ret;

$ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
   CHECK   scripts/mod/empty.c
   CALL    scripts/checksyscalls.sh
   DESCEND objtool
   INSTALL libsubcmd_headers
   CC      drivers/net/dsa/mt7530.o
   CHECK   drivers/net/dsa/mt7530.c
drivers/net/dsa/mt7530.c:469 mt7530_pad_clk_setup() error: uninitialized symbol 'ncpo1'.
drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
drivers/net/dsa/mt7530.c:2346 mt7530_setup() error: uninitialized symbol 'interface'.

Just so you know, I intend to remove this whole PHY muxing feature once I
bring changing DSA conduit support to this subdriver. I've got two strong
reasons for this.

- Changing the DSA conduit achieves the same result with the only overhead
   being the DSA header included on every frame.

- There can't be proper dt-bindings for it as the nature of the feature
   shows that it represents an optional way to operate the hardware, it does
   not represent a hardware design. Overall, the implementation is a hack to
   make it work for specific hardware (switch must be connected to gmac1 of
   a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
   of the SoC. It should rather be configurable on userspace. Which will
   never happen as it is specific to this switch and the changing DSA
   conduit feature is the perfect substitute for this.

Let me know if you've got any suggestions that can get rid of the warning
without reworking the whole code block. Otherwise, I'm just going to ignore
it until I get rid of the whole code block.

Arınç