linux-kernel - RE: [PATCH net-next] net: phy: micrel: Add callback for restoring context

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] [day] [month] [year] [list]

Message-ID:
 <TY3PR01MB11346D7C845C06A40407A83528648A@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Thu, 10 Jul 2025 17:43:50 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>, Heiner
 Kallweit <hkallweit1@...il.com>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>
CC: Russell King <linux@...linux.org.uk>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Geert Uytterhoeven <geert+renesas@...der.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>, biju.das.au
	<biju.das.au@...il.com>, "linux-renesas-soc@...r.kernel.org"
	<linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH net-next] net: phy: micrel: Add callback for restoring
 context

Hi Paolo Abeni,

Thanks for the feedback.

> -----Original Message-----
> From: Paolo Abeni <pabeni@...hat.com>
> Sent: 10 July 2025 14:38
> Subject: Re: [PATCH net-next] net: phy: micrel: Add callback for restoring context
> 
> On 7/7/25 4:29 PM, Biju Das wrote:
> > @@ -374,6 +376,7 @@ static struct kszphy_hw_stat kszphy_hw_stats[] = {
> > };
> >
> >  struct kszphy_type {
> > +	void (*resume)(struct phy_device *phydev);
> >  	u32 led_mode_reg;
> >  	u16 interrupt_level_mask;
> >  	u16 cable_diag_reg;
> 
> Why adding another callback? I think you could avoid it using a ksz9131-specific phy_driver->resume.

It is being called from open() as well see [1]

[1]
[    1.934177]  kszphy_resume+0x18/0xc4
[    1.934193]  phy_resume+0x3c/0x74
[    1.934207]  phy_attach_direct+0x1bc/0x398
[    1.934220]  phylink_fwnode_phy_connect+0xb0/0x130
[    1.934237]  __stmmac_open+0xe4/0x50c
[    1.934251]  stmmac_open+0x44/0xd4

> 
> > @@ -444,6 +447,7 @@ struct kszphy_priv {
> >  	bool rmii_ref_clk_sel;
> >  	bool rmii_ref_clk_sel_val;
> >  	bool clk_enable;
> > +	bool is_suspended;
> >  	u64 stats[ARRAY_SIZE(kszphy_hw_stats)];
> >  	struct kszphy_phy_stats phy_stats;
> >  };
> > @@ -491,6 +495,7 @@ static const struct kszphy_type ksz9021_type = {
> > };
> >
> >  static const struct kszphy_type ksz9131_type = {
> > +	.resume = ksz9131_restore_rgmii_delay,
> >  	.interrupt_level_mask	= BIT(14),
> >  	.disable_dll_tx_bit	= BIT(12),
> >  	.disable_dll_rx_bit	= BIT(12),
> > @@ -1387,6 +1392,12 @@ static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
> >  			      txcdll_val);
> >  }
> >
> > +static void ksz9131_restore_rgmii_delay(struct phy_device *phydev) {
> > +	if (phy_interface_is_rgmii(phydev))
> > +		ksz9131_config_rgmii_delay(phydev);
> > +}
> > +
> >  /* Silicon Errata DS80000693B
> >   *
> >   * When LEDs are configured in Individual Mode, LED1 is ON in a
> > no-link @@ -2345,6 +2356,11 @@ static int
> > kszphy_generic_suspend(struct phy_device *phydev)
> >
> >  static int kszphy_suspend(struct phy_device *phydev)  {
> > +	struct kszphy_priv *priv = phydev->priv;
> > +
> > +	if (priv)
> > +		priv->is_suspended = true;
> 
> Under which circumstances `priv` could be NULL? AFAICS it should always not NULL after probe.

On the kszphy_config_init(), there is a check for NULL. That is the reason for adding it here.

Yor right, priv cannot be NULL after probe.


> 
> > +
> >  	/* Disable PHY Interrupts */
> >  	if (phy_interrupt_is_valid(phydev)) {
> >  		phydev->interrupts = PHY_INTERRUPT_DISABLED; @@ -2381,8 +2397,17 @@
> > static void kszphy_parse_led_mode(struct phy_device *phydev)
> >
> >  static int kszphy_resume(struct phy_device *phydev)  {
> > +	struct kszphy_priv *priv = phydev->priv;
> >  	int ret;
> >
> > +	if (priv && priv->is_suspended) {
> 
> I think you can use phydev->suspended instead of adding another flag.

Yes, I can use phydev->suspended as phylink_prepare_resume() calls phy_resume()
with phydev->suspended set.
> 
> Also, can resume be invoked without the phydev being suspended?

Yes, phy_attach_direct() during open() invokes resume.

Cheers,
Biju