netdev - Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

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] [day] [month] [year] [list]

Message-ID: <20240926140634.kpiwi7xjhkrq3kaq@skbuf>
Date: Thu, 26 Sep 2024 17:06:34 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Jiawen Wu <jiawenwu@...stnetic.com>,
	Jose Abreu <joabreu@...opsys.com>,
	Jose Abreu <Jose.Abreu@...opsys.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Mengyuan Lou <mengyuanlou@...-swift.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static
 configuration reload

On Thu, Sep 26, 2024 at 01:02:35PM +0100, Russell King (Oracle) wrote:
> On Thu, Sep 26, 2024 at 12:16:13AM +0300, Vladimir Oltean wrote:
> > On Wed, Sep 25, 2024 at 08:38:23PM +0100, Russell King (Oracle) wrote:
> > > > There are 2 more changes which I believe should be made in sja1105_set_port_speed():
> > > > - since it isn't called from mac_config() anymore but from mac_link_up()
> > > >   (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN
> > > > - we can trust that phylink will not call mac_link_up() with a speed
> > > >   outside what we provided in mac_capabilities, so we can remove the
> > > >   -EINVAL "default" speed_mbps case, and make this method return void,
> > > >   as it can never truly cause an error
> > > > 
> > > > But I believe these are incremental changes which should be done after
> > > > this patch. I've made a note of them and will create 2 patches on top
> > > > when I have the spare time.
> > > 
> > > ... if we were to make those changes prior to this patch, then the
> > > dev_err() will no longer be there and thus this becomes a non-issue.
> > > So I'd suggest a patch prior to this one to make the changes you state
> > > here, thus eliminating the need for this hunk in this patch.
> > 
> > That sounds good. Are you suggesting you will write up such a patch for v2?
> 
> Actually, the three patches become interdependent.
> 
> Let's say we want to eliminate SPEED_UNKNOWN. Prior to my patch in this
> sub-thread, we have this:
> 
>                 speed_mbps[i] = sja1105_port_speed_to_ethtool(priv,
>                                                               mac[i].speed);
> ...
>                 rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);
> 
> sja1105_port_speed_to_ethtool() can return SPEED_UNKNOWN if
> mac[i].speed is not one of the four encodings. If we can't guarantee
> that it is one of the four encodings, then SPEED_UNKNOWN may be
> passed into sja1105_adjust_port_config().
> 
> Similarly, as for the default case, we can't simply delete that,
> because that'll leave "speed" uninitialised and we'll get a build
> warning without my changes. We could change the default case to
> simply:
> 
> 	default:
> 		return 0;
> 
> but that just looks perverse.
> 
> So, I think rather than trying to do your suggestion before my patch,
> my patch needs to stand as it currently is, and then your suggestion
> must happen after it - otherwise we end up introducing more complexity
> or weirdness.
> 
> Hmm?

Ok, if we're back to my original proposal, I'm implicitly okay with that.