Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[Arınç ÜNAL]

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ç