Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 May 2023 15:32:36 +0200
From:   David Epping <david.epping@missinglinkelectronics.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Vladimir Oltean <olteanv@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, UNGLinuxDriver@microchip.com
Subject: Re: [PATCH net v2 0/3] net: phy: mscc: support VSC8501
Message-ID: <20230523133236.GA7185@nucnuc.mle>
References: <20230523090405.10655-1-david.epping@missinglinkelectronics.com>
 <c613298d-53bc-46ef-9cb2-4b385e21ba7b@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c613298d-53bc-46ef-9cb2-4b385e21ba7b@lunn.ch>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, May 23, 2023 at 03:16:51PM +0200, Andrew Lunn wrote:
> > - I left the mutex_lock(&phydev->lock) in the
> >   vsc85xx_update_rgmii_cntl() function, as I'm not sure whether it
> >   is required to repeatedly access phydev->interface and
> >   phy_interface_is_rgmii(phydev) in a consistent way.
> 
> Just adding to Russell comment.
> 
> As a general rule of thumb, if your driver is doing something which no
> other driver is doing, you have to consider if it is correct. A PHY
> driver taking phydev->lock is very unusual. So at minimum you should
> be able to explain why it is needed. And when it comes to locking,
> locking is hard, so you really should understand it.
> 
> Now the mscc is an odd device, because it has multiple PHYs in the
> package, and a number of registers are shared between these PHYs. So
> it does have different locking requirements to most PHYs. However, i
> don't think that is involved here. Those oddities are hidden behind
> phy_base_write() and phy_base_read().
> 
> 	Andrew

Russell, Andrew,

as you stated, locking is hard, and I am not in detail familiar with
the mscc driver and the supported PHYs behavior. Also, I only have
VSC8501, the single PHY chip, and none of the multi PHY chips to test.
And testing these corner cases and race conditions is hard anyways.
Thus my current patch is not touching the locking code at all, and
assumes the current mainline code is correct in that regard.
Because I don't understand all implications, I'm hesitant to change
the existing locking scheme.
Maybe this can be a separate patch? In the current patch set I'm not
making the situation worse (unlike the first one where I added locks
which Russell pointed out).
If you insist and all agree the locks should be removed with this
patch set, I'll update it of course.