All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	"biju.das.au" <biju.das.au@gmail.com>,
	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>,
	Ovidiu Panait <ovidiu.panait.rb@renesas.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH net-next] net: phy: call phy_init_hw() in phy resume path
Date: Sun, 12 Apr 2026 13:55:26 +0100	[thread overview]
Message-ID: <aduWPn0cMWjeIyt-@shell.armlinux.org.uk> (raw)
In-Reply-To: <TY3PR01MB11346F78B929EA7F377BB5B4A86272@TY3PR01MB11346.jpnprd01.prod.outlook.com>

On Sun, Apr 12, 2026 at 12:05:06PM +0000, Biju Das wrote:
> Hi Russell King,
> 
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: 11 April 2026 17:47
> > Subject: Re: [PATCH net-next] net: phy: call phy_init_hw() in phy resume path
> > 
> > On Sat, Apr 11, 2026 at 03:50:13PM +0200, Andrew Lunn wrote:
> > > > So, I question whether any of the functions in this driver actually
> > > > have a valid reason to take phydev->lock - looks to me like a not
> > > > very well written driver.
> > > >
> > > > In cases like this, I don't think we should make things more
> > > > difficult in the core just because we have a lockdep splat when that
> > > > can be avoided by killing off unnecessary locking.
> > >
> > > Agreed. This patchset should cleanup these locks.
> > >
> > > We also need to look at lan937x_dsp_workaround(). I also don't see
> > > what that mutex lock/unlock is protecting. Accessing bank registers
> > > need to be protected, so doing one additional access within that
> > > should not need additional protection.
> > 
> > Looking at access_ereg(), shouldn't it be taking the MDIO bus lock and using the __phy_* accessors
> > anyway because it's writing various registers which determine what is being read via the
> > LAN87XX_EXT_REG_RD_DATA register or the value written via the LAN87XX_EXT_REG_WR_DATA register.
> > 
> > Also, as it has access_ereg_modify_changed(), that entire sequence needs to take the MDIO bus lock to
> > safely do the read-modify-write.
> > 
> > Then there's lan87xx_config_rgmii_delay() which is a large open coded read-modify-write for the
> > PHYACC_ATTR_BANK_MISC, LAN87XX_CTRL_1 register.
> > 
> > To me, this looks like a racy driver, and it also looks like it's using the wrong lock to try and
> > protect hardware accesses.
> 
> OK, will replace it with MDIO bus lock.

Remember that the phy_* accessors will take the MDIO bus lock, so will
need to be changed to their __phy_* counterparts.

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

  reply	other threads:[~2026-04-12 12:55 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-10 14:29 [PATCH net-next] net: phy: call phy_init_hw() in phy resume path Biju
2026-04-10 14:51 ` Russell King (Oracle)
2026-04-10 14:55   ` Russell King (Oracle)
2026-04-10 17:27     ` Biju Das
2026-04-10 23:51       ` Russell King (Oracle)
2026-04-10 15:15   ` Andrew Lunn
2026-04-10 15:22     ` Russell King (Oracle)
2026-04-10 15:38       ` Biju Das
2026-04-10 16:00       ` Andrew Lunn
2026-04-10 16:41     ` Biju Das
2026-04-10 23:49       ` Russell King (Oracle)
2026-04-11 13:50         ` Andrew Lunn
2026-04-11 14:32           ` Biju Das
2026-04-11 16:46           ` Russell King (Oracle)
2026-04-12 12:05             ` Biju Das
2026-04-12 12:55               ` Russell King (Oracle) [this message]
2026-04-10 17:00   ` Florian Fainelli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aduWPn0cMWjeIyt-@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=andrew@lunn.ch \
    --cc=biju.das.au@gmail.com \
    --cc=biju.das.jz@bp.renesas.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=geert+renesas@glider.be \
    --cc=hkallweit1@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=ovidiu.panait.rb@renesas.com \
    --cc=pabeni@redhat.com \
    --cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.