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, 18 Oct 2022 09:02:19 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Frank <Frank.Sae@motor-comm.com>
Cc:     Peter Geis <pgwipeout@gmail.com>, Andrew Lunn <andrew@lunn.ch>,
        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>, yinghong.zhang@motor-comm.com,
        fei.zhang@motor-comm.com, hua.sun@motor-comm.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v7] net: phy: Add driver for Motorcomm yt8521
 gigabit ethernet phy
Message-ID: <Y05diwTQdqr9za3G@shell.armlinux.org.uk>
References: <20221018011439.1169-1-Frank.Sae@motor-comm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221018011439.1169-1-Frank.Sae@motor-comm.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Tue, Oct 18, 2022 at 09:14:39AM +0800, Frank wrote:
>  Add a driver for the motorcomm yt8521 gigabit ethernet phy. We have verified
>  the driver on StarFive VisionFive development board, which is developed by
>  Shanghai StarFive Technology Co., Ltd.. On the board, yt8521 gigabit ethernet
>  phy works in utp mode, RGMII interface, supports 1000M/100M/10M speeds, and
>  wol(magic package).
> 
> Signed-off-by: Frank <Frank.Sae@motor-comm.com>
> ---
> patch v7:
>  Hi Russell
> 
> > As previously pointed out, dropping the MDIO bus lock in a
> > phy_select_page()..phy_restore_page() region unsafe. I think you need
> > to ensure that yt8521_fiber_config_aneg() is safe to be called under
> > the lock, and I suspect having a version of genphy_config_aneg() which
> > can be called with the lock held would be a better approach.
> 
>  with your suggestion we add yt8521_set_page() which does not hold the lock.

And so you don't understand the locking issues...

If you drop the lock, it means that some other thread (e.g. userspace) can
come in and change the page register under you, which will completely
change which register(s) you are accessing.

Please implement locking properly, don't bodge around with it. I made my
suggestion above specifically after having thought about the issues and
what would be possible to give a correct locking implementation.

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