Re: [PATCH net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre_config()

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Oct 03, 2024 at 01:56:27AM +0300, Serge Semin wrote:
> 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.

Oh, sod it. Do whatever you bloody well want. I don't care. You're
constantly arguing against me, and I've had enough of this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!