netdev - Re: [PATCH net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre

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: <pm7v7x2ttdkjygakcjjbjae764ezagf4jujn26xnk7driykbu3@hfh4lwpfuowk>
Date: Thu, 3 Oct 2024 01:56:27 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>, 
	Andrew Lunn <andrew@...n.ch>
Cc: 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>, Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH net-next 01/10] net: pcs: xpcs: move PCS reset to
 .pcs_pre_config()

On Wed, Oct 02, 2024 at 12:09:22AM GMT, Andrew Lunn wrote:
> > I'm wondering why we seem to be having a communication issue here.

No communication issue. I just didn't find the discussion over with
all the aspects clarified. That's why I've got back to the topic here.

> > 
> > I'm not sure which part of "keeping the functional changes to a
> > minimum for a cleanup series" you're not understanding. This is
> > one of the basics for kernel development... and given that you're
> > effectively maintaining stmmac, it's something you _should_ know.
> > 
> > So no, I'm going to outright refuse to merge your patch in to this
> > series, because as I see it, it would be wrong to do so. This is
> > a _cleanup_ series, not a functional change series, and what you're
> > proposing _changes_ the _way_ reset happens in this driver beyond
> > the minimum that is required for this cleanup. It's introducing a
> > completely _new_ way of writing to the devices registers to do
> > the reset that's different.
> 
> I have to agree with Russell. Cleanups should be as simple as
> possible, and hopefully obviously correct. They should be low risk.

In general as a thing in itself with no better option to improve the
code logic I agree, it should be kept simple. But since the cleanups
normally land to net-next, and seeing the patch set still implies some
level of the functional change, I don't see much problem with adding a
one more change to simplify the driver logic, decrease the level
of cohesions (by eliminating the PHY-interface passing to the
soft-reset method) and avoid some unneeded change in this patch set.
Yes, my patch adds some amount of functional change, but is that that
a big problem if both this series and my patch (set) are going to land
in net-next anyway, and probably with a little time-lag?

Here what we'll see in the commits-tree if my patch is applied as a
pre-requisite one of this series:

1.0 Serge: net: pcs: xpcs: Drop compat arg from soft-reset method
- 1.1 Russell: net: pcs: xpcs: move PCS reset to .pcs_pre_config()
* This patch won't be needed since the PHY-interface will be no
  longer used for the soft-reset to be performed.
1.2 Russell: net: pcs: xpcs: drop interface argument from internal functions
- 1.3 net: pcs: xpcs: get rid of xpcs_init_iface()
* This patch won't be applicable since the xpcs_init_iface() method
  will be still utilized for the basic dw_xpcs initializations and the
  controller soft-resetting.
...
1.1x Serge: my series rebased onto the Russell' patch set

Here is what we'll see in the git-tree if my patch left omitted in
this patch set:

2.1 Russell: net: pcs: xpcs: move PCS reset to .pcs_pre_config()
2.2 Russell: net: pcs: xpcs: drop interface argument from internal functions
2.3 Russell: net: pcs: xpcs: get rid of xpcs_init_iface()
...
2.1x Serge: net: pcs: xpcs: Drop compat arg from soft-reset method
+ 2.1y Serge: net: pcs: xpcs: Get back xpcs_init_iface()
* Since the PHY-interface is no longer required for the XPCS soft-resetting
  I'll move the basic dw_xpcs initializations to the xpcs_init_iface()
  in order to simplify the driver logic by consolidating the initial
  setups at the early XPCS-setup stage. This will basically mean to
  revert the Russell' patches 2.1 and 2.3.
2.1z Serge: the rest of my series rebased onto the Russell' patch set

> 
> Lets do all the simple cleanups first. Later we can consider more
> invasive and risky changes.

Based on all the considerations above I still think that option 1.
described above looks better since it decreases the changes volume
in general and decreases the number of patches (by three actually),
conserves the changes linearity.

But if my reasoning haven't been persuasive enough anyway, then fine by
me. I'll just add a new patch (as described in 2.1y) to my series.
But please be ready that it will look as a reversion of the Russell'
patches 2.1 and 2.3.

-Serge(y)

> 
> 	Andrew